diff --git a/.github/workflows/ci-apply.yml b/.github/workflows/ci-apply.yml index 046e97ce..546267a1 100644 --- a/.github/workflows/ci-apply.yml +++ b/.github/workflows/ci-apply.yml @@ -80,9 +80,40 @@ jobs: exit 1 } - gh api "repos/${REPO}/commits/${RUN_HEAD_SHA}/pulls" > pulls.json - [ "$(jq 'length' pulls.json)" = "1" ] \ - || skip "Not exactly one PR associated with ${RUN_HEAD_SHA}; skipping." + # The sha -> PR association is eventually consistent: a run that + # starts seconds after the contributor's push can legitimately see + # an empty list. Poll before giving up. + # + # Both terminal outcomes below FAIL rather than `skip`. `skip` exits + # 0, which paints the whole job green - and a green PR Apply that + # pushed nothing is exactly how an unstamped branch merged to main + # and broke it (PR #479, reverted in #485). `skip` is reserved for + # states that self-heal (head moved -> a new run follows) or are + # none of our business (PR closed, not targeting main). "I could not + # identify the PR" is neither: it means the fixups were dropped on + # the floor, and that must be visible. + for attempt in 1 2 3 4 5; do + gh api "repos/${REPO}/commits/${RUN_HEAD_SHA}/pulls" > pulls.json + N_PULLS="$(jq 'length' pulls.json)" + [ "$N_PULLS" = "0" ] || break + echo "No PR associated with ${RUN_HEAD_SHA} yet (attempt ${attempt}/5); retrying in 10s." + sleep 10 + done + + if [ "$N_PULLS" = "0" ]; then + echo "No PR associated with ${RUN_HEAD_SHA} after 5 attempts." >&2 + echo "Formatting/metadata fixups were NOT applied to the branch." >&2 + exit 1 + fi + + # More than one PR sharing a head sha is the ambiguity HARD RULE 3 + # exists for: pushing would risk writing to the wrong branch. Fail + # closed, but fail loudly - silence here is what bit us before. + if [ "$N_PULLS" != "1" ]; then + echo "${N_PULLS} PRs share head ${RUN_HEAD_SHA}; cannot resolve which one this run belongs to." >&2 + echo "Failing closed: nothing was applied. A maintainer must apply the fixups manually." >&2 + exit 1 + fi PR_NUMBER="$(jq -r '.[0].number' pulls.json)" [[ "$PR_NUMBER" =~ ^[0-9]+$ ]] || skip "Bad PR number; skipping." @@ -231,6 +262,7 @@ jobs: git apply --check "$PATCH" - name: Apply fixups patch and commit + id: fixups if: steps.pr.outputs.proceed == '1' working-directory: pr env: @@ -260,9 +292,11 @@ jobs: if ! git diff --cached --quiet; then git commit -m "[ci] apply-plugin-metadata-and-formatting" git push "https://x-access-token:${GH_TOKEN}@github.com/${FORK}.git" "HEAD:${HEAD_REF}" + echo "pushed=1" >> "$GITHUB_OUTPUT" fi - name: Apply Version Metadata using the TRUSTED script only (never the fork's copy) + id: vermeta if: steps.pr.outputs.proceed == '1' working-directory: pr env: @@ -280,7 +314,52 @@ jobs: if ! git diff --cached --quiet; then git commit -m "[ci] apply-version-metadata" git push "https://x-access-token:${GH_TOKEN}@github.com/${FORK}.git" "HEAD:${HEAD_REF}" + echo "pushed=1" >> "$GITHUB_OUTPUT" + fi + + # THE MERGE GATE. PR Check is not one: it GENERATES the metadata into its + # own workspace and then tests that generated tree, so it goes green on a + # branch whose committed manifests are still empty. That is precisely how + # #479 merged and broke main - Check was green, Apply had silently + # skipped, and nothing anywhere asserted the branch was actually stamped. + # + # The invariant that does hold: a correctly-applied PR is a FIXPOINT - + # re-running the pipeline over it produces no changes. So this reports + # success only when both steps above found nothing left to push. When + # they did push, the new head sha retriggers Check -> Apply, and that run + # sets success on the sha a maintainer would actually merge. + # + # Make `metadata-fixpoint` a required status check on main. Then every + # path that leaves fixups unapplied - a skip, a hard failure, a crash - + # leaves the status unset and the PR unmergeable, instead of green. + - name: Report metadata fixpoint status + if: always() && steps.pr.outputs.proceed == '1' + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + REPO: ${{ github.repository }} + HEAD_SHA: ${{ steps.pr.outputs.head_sha }} + FIXUPS_PUSHED: ${{ steps.fixups.outputs.pushed }} + VERMETA_PUSHED: ${{ steps.vermeta.outputs.pushed }} + FIXUPS_RESULT: ${{ steps.fixups.outcome }} + VERMETA_RESULT: ${{ steps.vermeta.outcome }} + RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} + run: | + set -euo pipefail + if [ "$FIXUPS_RESULT" != "success" ] || [ "$VERMETA_RESULT" != "success" ]; then + STATE="failure" + DESC="Formatting/metadata could not be applied; see the PR Apply run." + elif [ -n "$FIXUPS_PUSHED" ] || [ -n "$VERMETA_PUSHED" ]; then + STATE="failure" + DESC="Formatting/metadata was just applied; waiting for the re-run on the new commit." + else + STATE="success" + DESC="Formatting and generated metadata are up to date on this commit." fi + gh api --method POST "repos/${REPO}/statuses/${HEAD_SHA}" \ + -f state="$STATE" \ + -f context="metadata-fixpoint" \ + -f description="$DESC" \ + -f target_url="$RUN_URL" - name: On mechanical failure, notify the contributor if: failure() && steps.pr.outputs.pr_number != '' diff --git a/.github/workflows/ci-check.yml b/.github/workflows/ci-check.yml index ea4b8d8c..cfd4cc21 100644 --- a/.github/workflows/ci-check.yml +++ b/.github/workflows/ci-check.yml @@ -17,6 +17,15 @@ name: PR Check # DO NOT switch this to `pull_request_target` and do not add `permissions: # contents: write` here "to save a round trip" - that reintroduces the # pwn-request hole this split exists to close. +# +# THIS JOB IS NOT A MERGE GATE. It GENERATES the formatting and metadata into +# its own workspace and then tests THAT tree, so it goes green on a branch +# whose committed manifests are still empty - which is exactly how #479 merged +# and broke main. Do not add a "patch must be empty" check here either: the +# first run on any real PR legitimately produces a patch, and failing would +# stop ci-apply.yml (`conclusion == 'success'`) from ever applying it. The gate +# is the `metadata-fixpoint` commit status that ci-apply.yml sets; make that +# the required check on main. on: pull_request: diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a5dc8d5d..b1fead78 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -7,6 +7,26 @@ name: CI # here unmodified/strict against real, permanent git history (unlike # ci-check.yml, which can't yet resolve a commit sha for a brand-new plugin # version and runs leniently instead). +# +# It must run the SAME metadata pipeline as the PR path (ci-check.yml -> +# ci-apply.yml): autopep8, then auto_apply_plugin_metadata.py, then +# auto_apply_version_metadata.py. Content can reach main without ever going +# through the PR workflows - a direct maintainer push, or a merge of a PR +# whose checks failed - so a gap here is not covered anywhere else. +# +# Concretely, without the plugin-metadata step a push that bumps a plugin's +# `plugman` version lands the new bytes with no matching manifest entry. The +# manifest goes on advertising the OLD version's md5sum, so the in-game +# manager's checksum check rejects every download of that plugin, and +# test_latest_version fails on this and every later run. That is what 8628c97 +# (finder 1.0 -> 4.1) did to main; it stayed broken until PR #485 reverted the +# whole change, which restored the old bytes rather than recording the new +# ones. +# +# The auto-commit steps below push with the default GITHUB_TOKEN, and GitHub +# does not start a workflow run for those pushes (there is no ci.yml run for +# b1be5d5, the "[ci] apply-formatting" commit). So this job sees each push +# exactly once and does not re-enter itself. on: push: @@ -44,6 +64,61 @@ jobs: with: commit_message: "[ci] apply-formatting" + # Resolved BEFORE any of the auto-commit steps below rewrite HEAD, so it + # stays the tip main had when this push arrived. github.event.before is + # absent/zeroed on a branch's first push and dangling after a force + # push, hence the fallbacks; HEAD^ is the pushed head's first parent, + # which for a merge commit is the previous main tip. + - name: Resolve push base + env: + BEFORE_SHA: ${{ github.event.before }} + run: | + set -euo pipefail + BASE="$BEFORE_SHA" + if [ -z "$BASE" ] || ! git cat-file -e "${BASE}^{commit}" 2>/dev/null; then + # 4b825dc.. is git's empty tree - diffs as "everything is new" and + # simply fails to resolve as a manifest ref, which the metadata + # script handles by falling back to origin/main. + BASE="$(git rev-parse HEAD^ 2>/dev/null || echo 4b825dc642cb6eb9a060e54bf8d69288fbee4904)" + fi + echo "$BASE" > "${RUNNER_TEMP}/base_sha.txt" + echo "Push base: $BASE" + + # The counterpart of ci-check.yml's plugin-metadata step. Diffed against + # HEAD (not the push head) so plugin files that only the autopep8 commit + # above touched are included - their bytes changed, so their checksums + # must be recomputed too. + # + # PLUGMAN_BASE_REF is REQUIRED here, for the same reason ci-check.yml + # sets it. It makes "already published" mean the manifest as of the push + # base, rather than the working tree. On the ordinary path - merging a + # PR - the merge brings the version bump AND the manifest entry that + # ci-apply.yml already stamped onto the PR branch, so the working tree + # here ALREADY lists the version being pushed. Comparing against that + # would make the script raise "Version cant be lower or equal" on every + # single merge. Compared against the base, the version is correctly seen + # as new, the entry is found already stamped with a matching md5sum, and + # the step is a no-op. + # + # This step FAILING is a feature: it means a plugin's bytes changed with + # no version bump, which would leave every existing install unable to + # verify its download. + - name: Apply Plugin Metadata (writes null version placeholders) + run: | + set -euo pipefail + BASE="$(cat "${RUNNER_TEMP}/base_sha.txt")" + # --diff-filter=d drops deletions; the script opens each path it is + # handed, so a removed plugin would crash it. + git diff --name-only --diff-filter=d "$BASE" HEAD > "${RUNNER_TEMP}/changed_files.txt" + cat "${RUNNER_TEMP}/changed_files.txt" + PLUGMAN_BASE_REF="$BASE" \ + python test/auto_apply_plugin_metadata.py "$(cat "${RUNNER_TEMP}/changed_files.txt")" + + - name: Commit Plugin Metadata + uses: stefanzweifel/git-auto-commit-action@v7 + with: + commit_message: "[ci] apply-plugin-metadata" + - name: Apply Version Metadata run: | python test/auto_apply_version_metadata.py $(git log --pretty=format:'%h' -n 1) diff --git a/README.md b/README.md index b78af519..d78bcc45 100644 --- a/README.md +++ b/README.md @@ -87,7 +87,14 @@ There are three different ways the plugin manager can be installed: the category directory you feel is the most relevant to the type of plugin you're submitting, [here](plugins). - You also need a `plugman` dict with the plugin metadata in the plugin (see the [example](https://github.com/bombsquad-community/plugin-manager?tab=readme-ov-file#example) below). - The name of the plugin must be in snake_case and matching the file name. - - Must have the plugin_name, description, external_url, authors and version keys. + - Must have the description, external_url, authors and version keys. + - `plugin_name` is optional and defaults to the file name without `.py`. Spell it out only if you + want it documented in the source; it still has to match the file name. + - Values may be module-level constants defined *above* the dict, so a plugin that already keeps + a `__version__` or `__author__` around does not have to repeat itself: + `version=__version__` and `authors=[{"name": __author__[0], ...}]` both work. The plugin is + parsed, never imported, so only plain literals and indexing into them are understood; + expressions like `__file__.split("/")[-1]` are rejected. - Plugin manager will also show and execute the settings icon if your `ba.Plugin` class has methods `has_settings_ui` and `show_settings_ui`; check out the [colorscheme](https://github.com/bombsquad-community/plugin-manager/blob/eb163cf86014b2a057c4a048dcfa3d5b540b7fe1/plugins/utilities/colorscheme.py#L448-L452) plugin for an example. #### Example: @@ -97,15 +104,18 @@ Let's say you wanna submit this new utility-type plugin named as `sample_plugin. # ba_meta require api 9 import babase +__version__ = "1.0.0" +__author__ = ["Loup", "brostos"] + plugman = dict( - plugin_name="sample_plugin", + plugin_name="sample_plugin", # optional, defaults to the file name description="A test plugin for demonstration purposes blah blah.", external_url="https://www.youtube.com/watch?v=dQw4w9WgXcQ", authors=[ - {"name": "Loup", "email": "loupg450@gmail.com", "discord": "loupgarou_"}, - {"name": "brostos", "email": "", "discord": "brostos"} + {"name": __author__[0], "email": "loupg450@gmail.com", "discord": "loupgarou_"}, + {"name": __author__[1], "email": "", "discord": "brostos"} ], - version="1.0.0", + version=__version__, ) # ba_meta export babase.Plugin @@ -127,7 +137,7 @@ guide) once you open a pull request. ### Updating a Plugin - Make a [pull request](../../compare) with whatever changes you'd like to make to an existing plugin, and add a new - version number in your plugin in the plugman dict. + version number in your plugin in the plugman dict, or in the `__version__` it reads from. #### Example @@ -138,16 +148,26 @@ diff --git a/plugins/utilities/sample_plugin.py b/plugins/utilities/sample_plugi index ebb7dcc..da2b312 100644 --- a/plugins/utilities/sample_plugin.py +++ b/plugins/utilities/sample_plugin.py -@@ -9,7 +9,7 @@ - {"name": "Loup", "email": "loupg450@gmail.com", "discord": "loupgarou_"}, - {"name": "brostos", "email": "", "discord": "brostos"} +@@ -1,16 +1,16 @@ + # ba_meta require api 9 + import babase + +-__version__ = "1.0.0" ++__version__ = "1.1.0" + __author__ = ["Loup", "brostos"] + + plugman = dict( + plugin_name="sample_plugin", # optional, defaults to the file name + description="A test plugin for demonstration purposes blah blah.", + external_url="https://www.youtube.com/watch?v=dQw4w9WgXcQ", + authors=[ + {"name": __author__[0], "email": "loupg450@gmail.com", "discord": "loupgarou_"}, + {"name": __author__[1], "email": "", "discord": "brostos"} ], -- version="1.0.0", -+ version="1.1.0", + version=__version__, ) - - # ba_meta export babase.Plugin -@@ -21,4 +21,4 @@ +@@ -23,5 +23,5 @@ + def has_settings_ui(self): return True def show_settings_ui(self, source_widget): diff --git a/test/auto_apply_plugin_metadata.py b/test/auto_apply_plugin_metadata.py index 088e3cc9..42c7bc72 100644 --- a/test/auto_apply_plugin_metadata.py +++ b/test/auto_apply_plugin_metadata.py @@ -1,23 +1,23 @@ import sys import json import ast +import copy import os import hashlib import subprocess -import get_latest from auto_apply_version_metadata import get_comparable_version_tuple_from_string DEBUG = True +# index.json is deliberately absent: plugin manager releases add their own +# "x.y.z": null entry by hand (see CLAUDE.md), this script only ever touches the +# category manifests. MANIFEST_PATHS = { "minigames": "plugins/minigames.json", "utilities": "plugins/utilities.json", "maps": "plugins/maps.json", - "plugman": "index.json", } -print("DOES THIS RUN AUTO APPLY PLUGIN METADATA?") - def debug_print(*args, **kwargs): if DEBUG: @@ -37,16 +37,6 @@ def md5sum_of(path): return hashlib.md5(fin.read()).hexdigest() -def get_latest_version(plugin_name, category) -> str: - try: - if category != "plugman": - return get_latest.get_latest_plugin_version(plugin_name, MANIFEST_PATHS[category]) - return get_latest.get_latest_plugman_version() - - except Exception as e: - raise e - - def read_manifest_at(path, ref): """Load a manifest as it exists at `ref`, or None if it can't be read there.""" try: @@ -87,19 +77,6 @@ def get_published_versions(plugin_name, category): return manifest["plugins"].get(plugin_name, {}).get("versions", {}) -def update_plugman_json(version): - with open("index.json", "r+") as file: - data = json.load(file) - plugman_version = int(get_latest_version("plugin_manager", "plugman").replace(".", "")) - current_version = int(version["version"].replace(".", "")) - - if current_version > plugman_version: - with open("index.json", "r+") as file: - data = json.load(file) - data[current_version] = None - data["versions"] = dict(sorted(data["versions"].items(), reverse=True)) - - def update_plugin_json(plugin_info, category, plugin_path): name = plugin_info["plugin_name"] version = plugin_info["version"] @@ -158,6 +135,125 @@ def update_plugin_json(plugin_info, category, plugin_path): file.truncate() +def resolve_literal(node, constants, where): + """ast.literal_eval, widened to module-level constants and indexing into them. + + Plugins commonly keep a `__version__` / `__author__` dunder next to the code + that uses them and want the plugman dict to reference those rather than + repeat the values. Nothing here imports or executes the plugin, so only + names this module already resolved to a literal are available, and only + indexing (not attribute access or calls) is applied to them. + """ + if isinstance(node, ast.Name): + if node.id not in constants: + raise ValueError( + f"{where}: {node.id} is not a module-level constant defined above " + "plugman. Only names assigned a literal earlier in the file can be " + "referenced, because the plugin is parsed, never imported." + ) + # Copied so an entry like authors=__author__ cannot be mutated later + # through the constants table. + return copy.deepcopy(constants[node.id]) + + if isinstance(node, ast.Subscript): + container = resolve_literal(node.value, constants, where) + index = resolve_literal(node.slice, constants, where) + try: + return container[index] + except (TypeError, KeyError, IndexError) as err: + raise ValueError( + f"{where}: cannot index {container!r} with {index!r}: {err}" + ) from None + + if isinstance(node, (ast.List, ast.Tuple, ast.Set)): + items = [resolve_literal(item, constants, where) for item in node.elts] + if isinstance(node, ast.Tuple): + return tuple(items) + if isinstance(node, ast.Set): + return set(items) + return items + + if isinstance(node, ast.Dict): + if any(key is None for key in node.keys): + raise ValueError(f"{where}: ** unpacking inside plugman is not supported.") + return { + resolve_literal(key, constants, where): resolve_literal(value, constants, where) + for key, value in zip(node.keys, node.values) + } + + try: + return ast.literal_eval(node) + except (ValueError, TypeError, SyntaxError, MemoryError, RecursionError): + raise ValueError( + f"{where}: unsupported expression `{ast.unparse(node)}`. plugman values must " + "be literals, or module-level constants (optionally indexed, e.g. " + "__author__[0]). Method calls such as __file__.split('/') are not evaluated." + ) from None + + +def collect_module_constants(tree, stop_at): + """Module-level names bound to a literal, in the statements before `stop_at`. + + Order matters: a name assigned *after* plugman would raise NameError when the + game imports the plugin, so it must not resolve here either. + """ + constants = {} + for node in tree.body: + if node is stop_at: + break + if isinstance(node, ast.Assign): + targets = node.targets + elif isinstance(node, ast.AnnAssign) and node.value is not None: + targets = [node.target] + else: + continue + try: + value = ast.literal_eval(node.value) + except (ValueError, TypeError, SyntaxError, MemoryError, RecursionError): + continue + for target in targets: + if isinstance(target, ast.Name): + constants[target.id] = value + return constants + + +def build_plugman_info(node, tree, plugin, file_name_no_extension): + """Turn a `plugman = dict(...)` call node into the dict the manifest needs.""" + where = f"{plugin}: plugman" + constants = collect_module_constants(tree, stop_at=node) + + result = {} + for keyword in node.value.keywords: + if keyword.arg is None: + raise ValueError(f"{where}: ** unpacking inside plugman is not supported.") + result[keyword.arg] = resolve_literal(keyword.value, constants, where) + + # plugin_name is optional: the only value the check below would ever accept + # is the file's own stem, so a plugin that omits it simply gets that. + if "plugin_name" in result: + plugin_name = result["plugin_name"] + # some basic validation specific to plugin manager + if not isinstance(plugin_name, str): + raise ValueError(f"{where}: plugin_name must be a string.") + if plugin_name != plugin_name.lower(): + raise ValueError("Plugin name in plugman must be in snakecase.") + if plugin_name != file_name_no_extension: + raise ValueError("Plugin name in plugman does not match the file name.") + else: + result["plugin_name"] = file_name_no_extension + + missing = [key for key in ("description", "external_url", "authors", "version") + if key not in result] + if missing: + raise ValueError(f"{where}: missing required key(s) {', '.join(missing)}.") + if not isinstance(result["version"], str): + raise ValueError( + f"{where}: version must be a string in x.y.z form, got " + f"{result['version']!r}. Quote it (version=\"1.0.0\")." + ) + return result + + def extract_plugman(plugins): for plugin in plugins: if "plugins" + os.sep in plugin and plugin.endswith(".py"): @@ -175,41 +271,50 @@ def extract_plugman(plugins): with open(plugin, "r") as f: tree = ast.parse(f.read()) - for node in ast.walk(tree): + # A changed file that reaches the end of this loop without stamping + # the manifest ships against the PREVIOUS version's md5sum. That + # used to pass here silently and only surface much later as an + # opaque "checksum changed" failure in test_latest_version, so + # every path below either updates a manifest or raises. + handled = False + + # Module level statements in source order, so collect_module_constants() + # can tell what is defined ABOVE plugman. Every plugin in the catalog + # assigns plugman at column 0; one hidden inside a function or an if + # was never picked up meaningfully anyway and now raises below. + for node in tree.body: if isinstance(node, ast.Assign) and len(node.targets) == 1: target = node.targets[0] if isinstance(target, ast.Name) and target.id == "plugman": - if isinstance(node.value, ast.Dict): - # i dont want to support multiple formats for now - # because its harder to parse and maintain - # ill leave this here for now, though not supported - # Standard dictionary format {key: value} - return ast.literal_eval(node.value) - elif ( + if ( isinstance(node.value, ast.Call) and isinstance(node.value.func, ast.Name) and node.value.func.id == "dict" ): # dict() constructor format - result = {} - for kw in node.value.keywords: - if kw.arg == "plugin_name": - plugin_name = ast.literal_eval(kw.value) - # some basic validation specific to plugin manager - if plugin_name != plugin_name.lower(): - raise ValueError( - "Plugin name in plugman must be in snakecase." - ) - if plugin_name != file_name_no_extension: - raise ValueError( - "Plugin name in plugman does not match the file name." - ) - result[kw.arg] = ast.literal_eval(kw.value) - if category: - update_plugin_json(result, category=category, plugin_path=plugin) - else: - update_plugman_json(result) - # raise ValueError("Variable plugman not found in the file or has unsupported format.") + result = build_plugman_info( + node, tree, plugin, file_name_no_extension + ) + update_plugin_json(result, category=category, plugin_path=plugin) + handled = True + else: + # Only the dict() constructor form is parsed. A literal + # {key: value} was previously returned from here, which + # silently abandoned every remaining changed file too. + raise ValueError( + f"{plugin}: plugman must be assigned with the dict() " + "constructor, e.g. plugman = dict(plugin_name=..., " + "version=...). A literal { } dict is not supported." + ) + + if not handled: + raise ValueError( + f"{plugin}: no plugman dict found. Every plugin under plugins/ " + "needs one so its catalog entry and version can be generated, see\n" + "https://github.com/bombsquad-community/plugin-manager#submitting-a-plugin\n" + f'plugin_name defaults to "{file_name_no_extension}" (the file stem). ' + "Bump version on every edit; the manifest entry is derived from it." + ) if __name__ == "__main__": diff --git a/test/test_checks.py b/test/test_checks.py index 5de80109..125ad248 100644 --- a/test/test_checks.py +++ b/test/test_checks.py @@ -135,20 +135,78 @@ def test_changelog_entries(self): class TestPluginMetadata(unittest.TestCase): def setUp(self): + # os.path.isdir() must be given the joined path. Testing the bare name + # resolves it against the repo root, where no such directory exists, so + # this tuple came out empty and every test below passed vacuously. self.category_directories = tuple( - f'{os.path.join("plugins", path)}' - for path in os.listdir("plugins") if os.path.isdir(path) + os.path.join("plugins", path) + for path in sorted(os.listdir("plugins")) + if os.path.isdir(os.path.join("plugins", path)) ) + # Fail loudly if discovery ever goes empty again, rather than reporting + # a pass for a comparison of nothing against nothing. + self.assertTrue(self.category_directories, + "no category directories discovered under plugins/") + self.api_version_regexp = re.compile(b"(?<=ba_meta require api )(.*)") + self.entry_point_regexp = re.compile(b"ba_meta export ") + + def plugin_files(self, category): + return sorted(name for name in os.listdir(category) if name.endswith(".py")) def test_no_duplicates(self): unique_plugins = set() total_plugin_count = 0 for category in self.category_directories: - plugins = os.listdir(category) + plugins = self.plugin_files(category) total_plugin_count += len(plugins) unique_plugins.update(plugins) self.assertEqual(len(unique_plugins), total_plugin_count) + def test_plugin_files_and_manifest_entries_agree(self): + """Catches a plugin whose metadata step produced nothing at all. + + auto_apply_plugin_metadata.py now raises rather than skipping a file it + cannot read a plugman dict out of, but this is the independent check on + the result: a .py with no entry would be invisible in-game, and an entry + with no .py makes the manager offer a download that cannot exist. + """ + for category in self.category_directories: + manifest_file = f"{category}.json" + with self.subTest(category=category): + self.assertTrue(os.path.isfile(manifest_file), + f"{category} has no matching {manifest_file}") + with open(manifest_file, "rb") as fin: + entries = set(json.load(fin)["plugins"]) + files = {name[:-len(".py")] for name in self.plugin_files(category)} + self.assertEqual( + sorted(files - entries), [], + f"plugin file(s) under {category} with no entry in {manifest_file}; " + "bump the version in the plugman dict so CI can generate one" + ) + self.assertEqual( + sorted(entries - files), [], + f"entries in {manifest_file} with no matching .py under {category}" + ) + + def test_plugins_declare_their_ba_meta_directives(self): + for category in self.category_directories: + for name in self.plugin_files(category): + plugin = os.path.join(category, name) + with open(plugin, "rb") as fin: + content = fin.read() + with self.subTest(plugin=plugin): + self.assertIsNotNone( + self.api_version_regexp.search(content), + f"{plugin} declares no '# ba_meta require api '. The version " + "tests read that straight out of the source, so without it they " + "fail with an unhelpful AttributeError instead." + ) + self.assertIsNotNone( + self.entry_point_regexp.search(content), + f"{plugin} declares no '# ba_meta export', so the game would " + "load nothing from it." + ) + class BaseCategoryMetadataTestCases: class BaseTest(unittest.TestCase): diff --git a/test/test_plugman_parsing.py b/test/test_plugman_parsing.py new file mode 100644 index 00000000..6ca15461 --- /dev/null +++ b/test/test_plugman_parsing.py @@ -0,0 +1,390 @@ +"""Unit tests for test/auto_apply_plugin_metadata.py. + +test_checks.py validates the manifests that the pipeline has already produced. +These tests cover the step before that: turning a plugin's `plugman = dict(...)` +block into the metadata written to plugins/.json, including the cases +that must be rejected rather than silently skipped. + +Run from the repo root with the rest of the suite: + + python -m unittest discover -v + python -m unittest test.test_plugman_parsing -v +""" + +import ast +import contextlib +import io +import json +import os +import pathlib +import sys +import tempfile +import unittest + +# auto_apply_plugin_metadata.py is a CI script rather than a package module: it +# imports its siblings by bare name (`from auto_apply_version_metadata import +# ...`), so its own directory must be importable. +TEST_DIR = pathlib.Path(__file__).resolve().parent +REPO_ROOT = TEST_DIR.parent +if str(TEST_DIR) not in sys.path: + sys.path.insert(0, str(TEST_DIR)) + +import auto_apply_plugin_metadata as apm # noqa: E402 (needs the path above) + + +def plugman_node(source): + """Return (plugman assign node, module tree) for a snippet of plugin source.""" + tree = ast.parse(source) + for node in tree.body: + if ( + isinstance(node, ast.Assign) + and len(node.targets) == 1 + and isinstance(node.targets[0], ast.Name) + and node.targets[0].id == "plugman" + ): + return node, tree + raise AssertionError("test snippet has no module-level plugman assignment") + + +def build(source, stem="sample_plugin"): + """Run build_plugman_info() over a snippet, as extract_plugman() would.""" + node, tree = plugman_node(source) + return apm.build_plugman_info(node, tree, f"plugins/utilities/{stem}.py", stem) + + +def resolve(expression, constants=None): + return apm.resolve_literal(ast.parse(expression, mode="eval").body, + constants or {}, "snippet") + + +# The keys every plugman dict needs, so each test only spells out what it is about. +COMMON_KEYS = ( + ' description="A test plugin.",\n' + ' external_url="https://example.invalid/",\n' + ' authors=[{"name": "n", "email": "", "discord": ""}],\n' +) + + +class TestCollectModuleConstants(unittest.TestCase): + def constants(self, source): + node, tree = plugman_node(source) + return apm.collect_module_constants(tree, stop_at=node) + + def test_literal_above_plugman_is_collected(self): + constants = self.constants('__version__ = "1.0.0"\nplugman = dict()\n') + self.assertEqual(constants["__version__"], "1.0.0") + + def test_assignment_below_plugman_is_ignored(self): + # It would raise NameError when the game imports the plugin, so it must + # not resolve here either. + constants = self.constants('plugman = dict()\n__version__ = "1.0.0"\n') + self.assertNotIn("__version__", constants) + + def test_non_literal_assignment_is_skipped(self): + constants = self.constants( + "import os\nCWD = os.getcwd()\nCOUNT = 2\nplugman = dict()\n" + ) + self.assertNotIn("CWD", constants) + self.assertEqual(constants["COUNT"], 2) + + def test_annotated_assignment_is_collected(self): + constants = self.constants('__version__: str = "2.0.0"\nplugman = dict()\n') + self.assertEqual(constants["__version__"], "2.0.0") + + def test_last_assignment_wins(self): + constants = self.constants('V = "1"\nV = "2"\nplugman = dict()\n') + self.assertEqual(constants["V"], "2") + + def test_assignment_inside_a_function_is_not_module_level(self): + constants = self.constants('def f():\n HIDDEN = "x"\nplugman = dict()\n') + self.assertNotIn("HIDDEN", constants) + + +class TestResolveLiteral(unittest.TestCase): + def test_plain_literals_are_unchanged(self): + self.assertEqual(resolve('"x"'), "x") + self.assertEqual(resolve("[1, 2]"), [1, 2]) + self.assertEqual(resolve("(1, 2)"), (1, 2)) + self.assertEqual(resolve('{"a": 1}'), {"a": 1}) + self.assertEqual(resolve("-1"), -1) + self.assertIsNone(resolve("None")) + + def test_name_resolves_to_its_constant(self): + self.assertEqual(resolve("__version__", {"__version__": "1.0.0"}), "1.0.0") + + def test_subscript_by_index(self): + authors = {"__author__": ["Loup", "brostos"]} + self.assertEqual(resolve("__author__[0]", authors), "Loup") + self.assertEqual(resolve("__author__[-1]", authors), "brostos") + + def test_subscript_by_dict_key(self): + self.assertEqual(resolve('META["v"]', {"META": {"v": 7}}), 7) + + def test_constants_resolve_inside_nested_containers(self): + result = resolve('[{"name": __author__[1]}]', {"__author__": ["a", "b"]}) + self.assertEqual(result, [{"name": "b"}]) + + def test_resolved_value_is_a_copy(self): + constants = {"__author__": [{"name": "a"}]} + result = resolve("__author__", constants) + result[0]["name"] = "mutated" + self.assertEqual(constants["__author__"][0]["name"], "a") + + def test_unknown_name_is_rejected(self): + with self.assertRaisesRegex(ValueError, "not a module-level constant"): + resolve("__nope__") + + def test_method_call_is_rejected(self): + with self.assertRaisesRegex(ValueError, "unsupported expression"): + resolve("__file__.split('/')[-1]", {"__file__": "a/b.py"}) + + def test_dict_unpacking_is_rejected(self): + with self.assertRaisesRegex(ValueError, r"\*\* unpacking"): + resolve("{**EXTRA}", {"EXTRA": {"a": 1}}) + + def test_bad_index_is_reported_clearly(self): + with self.assertRaisesRegex(ValueError, "cannot index"): + resolve("__author__[5]", {"__author__": ["only one"]}) + + +class TestBuildPlugmanInfo(unittest.TestCase): + def test_plugin_name_defaults_to_the_file_stem(self): + info = build(f'plugman = dict(\n{COMMON_KEYS} version="1.0.0",\n)\n') + self.assertEqual(info["plugin_name"], "sample_plugin") + + def test_explicit_matching_plugin_name_is_kept(self): + info = build( + 'plugman = dict(\n plugin_name="sample_plugin",\n' + f'{COMMON_KEYS} version="1.0.0",\n)\n' + ) + self.assertEqual(info["plugin_name"], "sample_plugin") + + def test_plugin_name_must_match_the_file_name(self): + with self.assertRaisesRegex(ValueError, "does not match the file name"): + build( + 'plugman = dict(\n plugin_name="something_else",\n' + f'{COMMON_KEYS} version="1.0.0",\n)\n' + ) + + def test_plugin_name_must_be_snakecase(self): + with self.assertRaisesRegex(ValueError, "snakecase"): + build( + 'plugman = dict(\n plugin_name="Sample_Plugin",\n' + f'{COMMON_KEYS} version="1.0.0",\n)\n', + stem="Sample_Plugin", + ) + + def test_missing_required_keys_are_named(self): + with self.assertRaisesRegex( + ValueError, "missing required key\\(s\\) description, external_url, authors" + ): + build('plugman = dict(\n version="1.0.0",\n)\n') + + def test_version_must_be_a_string(self): + # __version__ = 1.0 is the easy mistake once the value moves to a dunder. + with self.assertRaisesRegex(ValueError, "version must be a string"): + build(f'__version__ = 1.0\nplugman = dict(\n{COMMON_KEYS}' + " version=__version__,\n)\n") + + def test_dunders_resolve(self): + info = build( + '__version__ = "1.2.3"\n' + '__author__ = ["Loup", "brostos"]\n' + "plugman = dict(\n" + ' description="A test plugin.",\n' + ' external_url="https://example.invalid/",\n' + " authors=[\n" + ' {"name": __author__[0], "email": "a@b.c", "discord": "loupgarou_"},\n' + ' {"name": __author__[1], "email": "", "discord": "brostos"},\n' + " ],\n" + " version=__version__,\n" + ")\n" + ) + self.assertEqual(info["version"], "1.2.3") + self.assertEqual([author["name"] for author in info["authors"]], + ["Loup", "brostos"]) + + +class TestExtractPlugman(unittest.TestCase): + """End to end over a throwaway tree, which is what CI actually invokes.""" + + def setUp(self): + cwd = os.getcwd() + tmp = tempfile.TemporaryDirectory() + # addCleanup rather than tearDown so the chdir is undone even if setUp + # or a test raises; test_checks.py depends on cwd being the repo root. + self.addCleanup(tmp.cleanup) + self.addCleanup(os.chdir, cwd) + + root = pathlib.Path(tmp.name) + (root / "plugins" / "utilities").mkdir(parents=True) + os.chdir(root) + self.write_manifest({"plugins": {}}) + + # The script prints per-file progress for the CI log; keep it out of the + # test report. Failures still surface, they are raised not printed. + self.enterContext(contextlib.redirect_stdout(io.StringIO())) + + # get_published_versions() shells out to `git show :`. The + # temp tree is not a repository, so every ref fails and it falls back to + # the working tree manifest, which is what these tests seed. + base_ref = os.environ.pop("PLUGMAN_BASE_REF", None) + if base_ref is not None: + self.addCleanup(os.environ.__setitem__, "PLUGMAN_BASE_REF", base_ref) + + def write_manifest(self, content): + pathlib.Path("plugins/utilities.json").write_text( + json.dumps(content, indent=2), encoding="utf-8") + + def manifest(self): + return json.loads( + pathlib.Path("plugins/utilities.json").read_text(encoding="utf-8")) + + def write_plugin(self, stem, source): + path = f"plugins/utilities/{stem}.py" + pathlib.Path(path).write_text(source, encoding="utf-8") + return path + + def test_writes_a_null_placeholder_for_a_new_plugin(self): + path = self.write_plugin( + "sample_plugin", f'plugman = dict(\n{COMMON_KEYS} version="1.0.0",\n)\n') + apm.extract_plugman([path]) + entry = self.manifest()["plugins"]["sample_plugin"] + self.assertEqual(entry["versions"], {"1.0.0": None}) + self.assertEqual(entry["description"], "A test plugin.") + + def test_dunder_metadata_end_to_end(self): + path = self.write_plugin( + "sample_plugin", + '__version__ = "1.0.0"\n' + '__author__ = ["Loup", "brostos"]\n' + "plugman = dict(\n" + ' description="A test plugin.",\n' + ' external_url="https://example.invalid/",\n' + " authors=[\n" + ' {"name": __author__[0], "email": "", "discord": ""},\n' + ' {"name": __author__[1], "email": "", "discord": ""},\n' + " ],\n" + " version=__version__,\n" + ")\n", + ) + apm.extract_plugman([path]) + entry = self.manifest()["plugins"]["sample_plugin"] + self.assertEqual(entry["versions"], {"1.0.0": None}) + self.assertEqual([author["name"] for author in entry["authors"]], + ["Loup", "brostos"]) + + def test_missing_plugman_dict_raises(self): + path = self.write_plugin("sample_plugin", "# ba_meta require api 9\nx = 1\n") + with self.assertRaisesRegex(ValueError, "no plugman dict found"): + apm.extract_plugman([path]) + self.assertEqual(self.manifest()["plugins"], {}) + + def test_literal_dict_form_raises(self): + path = self.write_plugin( + "sample_plugin", 'plugman = {"plugin_name": "sample_plugin"}\n') + with self.assertRaisesRegex(ValueError, "dict\\(\\) constructor"): + apm.extract_plugman([path]) + + def test_paths_outside_plugins_are_ignored(self): + apm.extract_plugman(["plugin_manager.py", ".github/workflows/ci.yml", + "CHANGELOG.md", "index.json"]) + self.assertEqual(self.manifest()["plugins"], {}) + + def test_every_changed_file_is_processed(self): + # A `return` used to abandon the rest of the list partway through. + paths = [ + self.write_plugin( + stem, f'plugman = dict(\n{COMMON_KEYS} version="1.0.0",\n)\n') + for stem in ("plugin_one", "plugin_two") + ] + apm.extract_plugman(paths) + self.assertEqual(sorted(self.manifest()["plugins"]), + ["plugin_one", "plugin_two"]) + + def test_version_must_be_greater_than_the_published_one(self): + self.write_manifest({ + "plugins": { + "sample_plugin": { + "description": "A test plugin.", + "external_url": "https://example.invalid/", + "authors": [{"name": "n", "email": "", "discord": ""}], + "versions": {"1.1.0": {"md5sum": "whatever"}}, + } + } + }) + path = self.write_plugin( + "sample_plugin", f'plugman = dict(\n{COMMON_KEYS} version="1.0.0",\n)\n') + with self.assertRaisesRegex(Exception, "cant be lower or equal"): + apm.extract_plugman([path]) + + def test_newest_version_is_listed_first(self): + self.write_manifest({ + "plugins": { + "sample_plugin": { + "description": "A test plugin.", + "external_url": "https://example.invalid/", + "authors": [{"name": "n", "email": "", "discord": ""}], + "versions": {"1.0.9": {"md5sum": "whatever"}}, + } + } + }) + path = self.write_plugin( + "sample_plugin", f'plugman = dict(\n{COMMON_KEYS} version="1.0.10",\n)\n') + apm.extract_plugman([path]) + versions = self.manifest()["plugins"]["sample_plugin"]["versions"] + self.assertEqual(list(versions), ["1.0.10", "1.0.9"]) + + +class TestVersionKey(unittest.TestCase): + """version_key orders every `versions` block in every manifest.""" + + def test_orders_numerically_not_lexically(self): + self.assertGreater(apm.version_key("1.0.10"), apm.version_key("1.0.9")) + self.assertGreater(apm.version_key("1.10.0"), apm.version_key("1.9.0")) + self.assertGreater(apm.version_key("2.0.0"), apm.version_key("1.99.99")) + + def test_equal_versions_compare_equal(self): + self.assertEqual(apm.version_key("1.2.3"), apm.version_key("1.2.3")) + + def test_sorts_a_versions_block_newest_first(self): + versions = ["1.0.9", "1.0.10", "1.1.0", "1.0.2"] + self.assertEqual( + sorted(versions, key=apm.version_key, reverse=True), + ["1.1.0", "1.0.10", "1.0.9", "1.0.2"], + ) + + def test_non_numeric_versions_are_rejected(self): + for version in ("1.0.0-beta", "v1.0.0", "1.0.x", ""): + with self.subTest(version=version): + with self.assertRaisesRegex(ValueError, "not in x.y.z form"): + apm.version_key(version) + + +class TestCatalogPlugmanBlocks(unittest.TestCase): + """Every plugman block actually in the catalog must still parse.""" + + def test_every_plugman_block_in_the_catalog_parses(self): + checked = 0 + for path in sorted((REPO_ROOT / "plugins").glob("*/*.py")): + tree = ast.parse(path.read_text(encoding="utf-8")) + for node in tree.body: + if not ( + isinstance(node, ast.Assign) + and len(node.targets) == 1 + and isinstance(node.targets[0], ast.Name) + and node.targets[0].id == "plugman" + ): + continue + with self.subTest(plugin=path.name): + self.assertIsInstance(node.value, ast.Call) + self.assertEqual(node.value.func.id, "dict") + info = apm.build_plugman_info(node, tree, str(path), path.stem) + self.assertEqual(info["plugin_name"], path.stem) + self.assertIsInstance(info["version"], str) + checked += 1 + self.assertGreater(checked, 0, "no plugman blocks found in plugins/") + + +if __name__ == "__main__": + unittest.main()