Repository navigation
Refuse to drop types from generated namespace files unless --force (#403); sort namespace contents - #410
Refuse to drop types from generated namespace files unless --force (#403); sort namespace contents#410thirtytwobits wants to merge 5 commits into
Conversation
Fixes #403. With --target-language=py, generating a root namespace that depends on another rewrote the dependency's namespace __init__.py to list only the types that run needed. A complete __init__.py generated into the same outdir by an earlier run was lost, so earlier generated types that reach dependencies through the package (e.g. uavcan.primitive.Unstructured_1_0), and nunavut_support.get_class(), failed with AttributeError. When namespace files are generated (Python by default, or any language with --generate-namespace-types), every namespace a dependency is found in is now read in full: all definitions in the dependency's directory, plus their dependencies. A namespace file then lists its whole namespace no matter which target pulled it in, so sequential generation into one outdir composes. Nested namespaces are not pulled in, and --omit-dependencies behaves as before. Languages without namespace files (C, C++) are unchanged. They generate one file per type, so their output already doesn't depend on which run wrote it, and they keep generating only the dependency types they need. Also make Namespace.get_nested_types() and get_nested_namespaces() return a stable, sorted order. Their order followed the order DSDL files were read in, which varies between runs, so two identical generations produced __init__.py files with imports in different orders. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… namespaces Replaces the approach in the previous commit, which made Python generate every type in a dependency's namespace. That made Python generate types C would not (for my.type.Foo depending on my.othertype.Bar, Python also generated my.othertype.Baz). Python again generates the same types as C: the targets and their dependencies. Instead, generated namespace files (Python __init__.py, HTML index.html) now carry a list of their members between nunavut-namespace-members-begin and nunavut-namespace-members-end. Members are read as versioned DSDL names regardless of whitespace, line breaks, or comment characters, so running a formatter over generated code does not change the list. Before writing anything, generation checks each namespace file it would replace. Replacing is allowed if the new file lists every member of the existing one (the same types, or more), so regenerating is idempotent. Otherwise, or if the existing file has no member list, generation is refused with NamespaceConflictError and nothing is written; nnvg prints the conflicting files and the types they would drop, and exits with 1. With --force (generate_all(force=True)), the namespace files under the output directory that have member lists are removed first, together with the files generated for their members, and then generation proceeds. Other files are not touched, and directories left empty are removed. Languages without namespace files (C, C++) are unaffected. Fixes #403. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ba2de380ee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…arker - Move the new force parameter after code_generator_type and support_generator_type in generate_all, generate_all_for_language and generate_all_from_namespace, so existing positional callers don't bind a generator class to force. - --force cleanup now finds earlier namespace files by their member list instead of by this run's namespace file name, so output generated with a different --namespace-output-stem or extension is removed too. Each member's file is expected to have the extension of the namespace file that lists it. - nnvg: explain why a refusal is logged with logging.error rather than logging.exception (no traceback for an expected outcome). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce2fabbc22
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A namespace file generated with --output-extension=.gen.py is named __init__.gen.py, and its members Foo_1_0.gen.py, but Path.suffix is only .py, so forced cleanup removed the namespace file and left the members. Look for each member's file under the namespace file's last suffix, all of its suffixes, and this run's configured extension. Only files with exactly those names are removed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
SonarCloud python:S3776 flagged remove_generated_namespaces (complexity 20, limit 15). Move the namespace file scan and the member path computation into _find_namespace_files and _member_files. No behavior change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f941fe257a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| member_ref = cast(pydsdl.CompositeType, _MemberRef(member)) | ||
| for extension in extensions: | ||
| yield output_root / IncludeGenerator.make_path(member_ref, language, extension) |
There was a problem hiding this comment.
Preserve prior stropping when locating files to remove
When a forced run changes any path-affecting language configuration, such as Python's stropping suffix or reserved-identifier rules, this reconstructs each old member path using the current language. For example, output previously placed under a stropped class_ namespace is searched for under the newly configured spelling instead; the old namespace marker is deleted, but its generated type files remain. Record the generated member paths in the marker or otherwise avoid applying the current run's stropping rules to earlier output.
Useful? React with 👍 / 👎.
| extensions = {namespace_file.suffix, "".join(namespace_file.suffixes), current_extension} | ||
| for member in members: | ||
| member_ref = cast(pydsdl.CompositeType, _MemberRef(member)) | ||
| for extension in extensions: | ||
| yield output_root / IncludeGenerator.make_path(member_ref, language, extension) |
There was a problem hiding this comment.
Avoid deleting alternate-extension files during cleanup
When an earlier namespace uses a compound extension such as .gen.py, this candidate set includes both .gen.py and its final suffix .py. Consequently, --force deletes an unrelated or hand-maintained Foo_1_0.py alongside the generated Foo_1_0.gen.py whenever both exist, despite promising not to touch other files. Preserve the exact extension or member path in the namespace metadata rather than deleting every plausible suffix variant.
Useful? React with 👍 / 👎.
|
|
Abandoning this change. Six defects were found in review (one in the design, one in the public API, four in #403 will be redone from scratch under a new development process with gates for the problem statement, design, and test plan. The branch is left in place for reference. 🤖 Generated with Claude Code |
Fixes #403. Supersedes #410, which was abandoned. Generating Python into an output directory that already held generated code replaced each package's `__init__.py` with only the current run's types. Types written by earlier runs could then no longer be imported from their package. ## The change Namespace files are now **append-only** for languages that set the new `merge_namespace_files` property. Only Python sets it. - Each namespace file ends with a **manifest** comment. It records the file's namespace, its types, and a digest of how they were named. - A later run adds its types to those recorded and recomputes the newest-minor-version aliases. Generation never deletes a file. - The manifest is read without regard to whitespace, line breaks or comment characters, so formatters don't affect it. Before writing anything, generation **refuses** in four cases (`NamespaceConflictKind`), and writes nothing: | Kind | When | `--force` | |---|---|---| | `NO_MANIFEST` | An existing namespace file has no valid manifest | Writes only this run's types | | `NAMING_CHANGED` | This run would name the recorded types differently | Writes only this run's types | | `CASE_COLLISION` | Two types' files would differ only in letter case | Can't be forced | | `NAMESPACE_COLLISION` | Two namespaces would share one namespace file (case, or stropping such as `import`/`import_`), within a run or across runs | Can't be forced | `nnvg` lists every refused file and exits 1. `--dry-run`, `--list-outputs` and `--list-inputs` report the same refusals. The API raises `NamespaceConflictError`. The guarantees are deliberately weak: - Output is defined only when Nunavut alone modified it. - `clean → a → b → c` within one build is the supported workflow. - Generating across builds without a clean is discouraged. ## Upgrade impact (release note) - **The first run refuses once.** The first run into an output directory written by an older Nunavut refuses with `NO_MANIFEST`. Clean the directory, or pass `--force` once. - **Custom Python namespace templates** must write the manifest with the `namespace_manifest_lines` filter (see the template guide). Otherwise every later run into the same directory refuses. - **Imports in Python namespace files are now sorted** (review finding R13). This also helps reproducibility. - **API:** `generate_all`, `generate_all_for_language`, `generate_all_from_namespace` and `generate_all_from_namespace_with_generators` gain a trailing `force=False` parameter. Existing positional calls are unchanged. - **Same-run namespace collisions are now refused.** They used to silently drop types on `main`. ## Gated process artifacts (all approved on #403) - [Problem statement](#403 (comment)) - [Design](#403 (comment)): includes the input-enumeration and failure-mode tables. - [Test plan](#403 (comment)) - Amendments: - [1](#403 (comment)): a stropping change relocates a namespace. - [2](#403 (comment)): list-only runs, `--no-overwrite`, manifest placement. - [3](#403 (comment)): namespace identity, `NAMESPACE_COLLISION`. The design and test plan comments are updated in place for each amendment. G3 and V1 were shown failing on `main` before the fix. ## Self-review and independent reviews - **Self-review:** the input-enumeration and failure-mode tables were walked against the code, and the diff was checked against the design. - **Independent review 1** (fresh agent: issue, design and diff only) found 14 items: | # | Root cause | Class | Change | |---|---|---|---| | R1, R2 | Existing run modes (`--list-*`, `--no-overwrite`) weren't considered together with the new refusal | Each input combined with each refusal condition | Amendment 2; tests G38, G39 | | R4 | Namespace-level case collisions weren't in the design | Namespace identity (reopened by review 2) | Amendment 2, replaced by Amendment 3 | | R3 | A crash after the manifest was written could leave a complete manifest on a truncated file | Write ordering | Manifest written last; test G42 | | R5–R10 | Test gaps: shared consistency check, dry-run vs real run, split manifest lines, recorded-only keyword and service members, positional calls, rerun hygiene | Tests not derived from every row | Tests added or extended | | R11, R14 | Docs: example messages, links, heading level | Docs | Docs updated | | R12 | The verification noxfile reused its output directory | Generating into a non-clean directory | The noxfile generates into a clean directory | | R13 | Imports are now sorted | Behavior change | Noted above | - **Independent review 2** (fresh agent) found N1–N7: - **N1 and N3 share a root cause with R4.** Namespace identity was never recorded: the R4 fix compared the namespaces of *members*, not the namespace itself. That missed namespaces with no types of their own, and it broke idempotence for `mg.import`/`mg.import_`. - **Class:** two DSDL namespaces whose namespace files are the same file. The instances are case-only names and stropping pairs, within a run and across runs. - **Fix:** Amendment 3. Manifests record their namespace, and `NAMESPACE_COLLISION` can't be forced. Tests U8, G45–G47 and A6. - **N2:** inaccurate messages; each conflict kind now has its own message and advice. - **N4–N7:** - N4: the consistency check now runs after every Python generation, with named exceptions. - N5: `--list-inputs` is tested too. - N6: the slow-marker lookup uses `get_closest_marker`. - N7: G11's cleanup works on Windows. - **Independent review 3** (fresh agent) found **no code defects**, and four low findings, all fixed: - stale docstrings; - test G48 (a stropping collision across runs); - the test plan's list of consistency-check exceptions; - a docs note on stale namespace collisions. - **CI and SonarCloud on this PR** (fixed in 8a78313): - **The main suite failed on Python 3.14+ in CI** (156 tests, `FileNotFoundError` connecting to the forkserver). - **Root cause:** the noxfile's `--basetemp` was nox's tmp directory, which nox also exports as `TMPDIR`. pytest empties a given basetemp the first time a test uses `tmp_path`. On 3.14+, the forkserver (now Linux's default start method) keeps its socket in `TMPDIR`, so that socket was deleted mid-run. - **Why it showed up only now:** no test on `main` uses `tmp_path`. - **Why local runs missed it:** the local main-suite runs were on 3.11 only, at a path too long for a Unix socket, so CPython fell back to `/tmp`. - **Class:** a pytest session whose `--basetemp` is `TMPDIR`. - **Instances:** the `test`, `doctest` and `rstdoctest` sessions. All three now use a `pytest` subdirectory. `verification/python` passes no `--basetemp`, so it isn't affected. - **Verified:** the CI failure was reproduced in CI's container with the old noxfile (78 failed, 215 passed). With the fix, the main suite passes on 3.14 and 3.15. - **SonarCloud S6546** (one `Union` hint) and **S9073** (three composite assertions): every instance in the diff is fixed, and the gate shows 0 new issues. - **Codex** found no issues. - **Maintainer review:** the standalone docs page was removed (b3040a4) because it made a minor consideration a top-level concept. The template guide keeps a short section on writing a namespace template that merges, and `--force`'s help describes the refusals. - **Process fixes**, which belong to the maintainer's process draft (not part of this PR): - enumerate each input together with each refusal condition; - write tests that don't depend on their environment; - write a finding's class and every instance down before changing code; - carry every amendment through every artifact; - run the main suite on the newest supported Python as well as the oldest, from a short native path. The [test plan's suite list](#403 (comment)) is updated to match. - **Defects caught locally, before review:** - G33 assumed `os.access` reflects permission bits, which isn't true as root. - G40 broke on a macOS directory mounted into a Linux container. Both are now environment-independent. ## Suites run Run in `ghcr.io/opencyphal/toxic:tx26.4.1` (linux/arm64) on the final code: | Suite | Result | |---|---| | `nox -s test-3.11` (with `NUNAVUT_RUN_SLOW_TESTS=1`) | 294 passed, including slow G11 and all namespace-merge tests | | `doctest-3.11` / `rstdoctest-3.11` | 379 / 7 passed | | `test-3.15`, `test-3.14` (checkout on the container's own filesystem, at 8a78313) | 293 passed each (G11 is skipped; it runs in the main CI job) | | `doctest-3.15` / `rstdoctest-3.15` | 379 / 7 passed | | `lint`, `docs` | passed | | `verification/python` (`nox`) | 731 passed on each of 3.11, 3.12, 3.13, 3.14 and 3.15, including V1 and V2; `black` passed | **Not verified locally** (CI covers these): - amd64 and native32; - the Windows and macOS compatibility jobs, the only case-insensitive native filesystems; - the C/C++ verification workflows (no C/C++ code changed); - the x86-only fuzz jobs. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>



Fixes #403.
Problem
With
--target-language=py, generating a root namespace that depends on another one rewrote the dependency's namespace__init__.pyso it listed only the types that run needed. A complete__init__.pygenerated into the same output directory by an earlier run was silently lost. This broke more thannunavut_support.get_class(): generated modules reach their dependencies through the package (import uavcan.primitive, thenuavcan.primitive.String_1_0), so code from the earlier run also failed at runtime.Approach
Python generates exactly the types C generates: the targets plus their dependencies, and nothing else. Generation refuses to replace a namespace file if that would drop types from it, unless
--forceis given.(The first commit on this branch generated every type in a dependency's namespace. That was replaced, because it made Python generate types C would not.)
Member list in namespace files
Generated namespace files (Python
__init__.py, HTMLindex.html) now list their members:Members are read as versioned DSDL names (
name.space.Type.major.minor) between the begin and end tokens. Whitespace, line breaks, CRLF line endings and comment characters are ignored, so running a formatter over generated code doesn't change the list. Only namespace files carry it.Checking before writing
For each namespace file this run would replace, the check happens before anything is written:
NamespaceConflictError; nothing is written.nnvgprints the conflicting files and the types each would drop, then exits with status 1. A--dry-runreports the same refusal. For example, generatingregulatedon top ofuavcan:--forcennvg --force, orgenerate_all(force=True):Other files aren't touched, and directories left empty are removed. Read-only generated files (the default
--file-mode) are made writable before deletion, which Windows needs. In themy.thistype.Foo→my.othertype.Barexample,nnvg --force … Baz.1.0.dsdlleaves justBazand its dependencies.Unaffected
ResourceType.ONLY): they write no type or namespace files, so no check is done.Deterministic namespace contents (fixed in this PR)
Namespace.get_nested_types()andget_nested_namespaces()now return a stable order: by name, then version. Their order used to follow the order DSDL files were read in (set iteration and parallel job completion), so two identical generations produced__init__.pyfiles with imports in a different order.Tests
test/gentest_dependency_namespacesuses a fixture wheremy.thistype.Foo.1.0depends onmy.othertype.Bar.1.0, andmy.othertype.Baz.1.0is another type in Bar's namespace. The tests check:{Bar};forceleaves just Baz and keeps unrelated files, while a forced dry run removes nothing;forcereplaces it;nnvgexits with 1 and mentions--force, and--forcesucceeds.Verification
All under
toxic:tx26.4.1:nox -s test-3.11: 229 passed.doctest-3.11: 377 passed, including examples in the new module's docstrings.rstdoctest-3.11andlint(mypy, pylint, black): pass.verification/pythontest-3.11: 729 passed.nnvgCLI: regeneratinguavcansucceeds,regulatedon top is refused, and--forcesucceeds.Related
#411 covers HTML parent namespace pages, which also list nested namespaces. The member list here covers types only, so a run that would drop nested-namespace links from a parent page is not detected.
🤖 Generated with Claude Code