Conversation
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
avengineers/SPLed develop has two commits the useblocks fork lacks: spl-core 8.9 with a refreshed lock (f5ba89e), and the light controller state machine fix (b8beee9). Conflicts, and how they were resolved: - components/light_controller/doc/index.md: upstream nests the blink states in LIGHT_ON, inside a Jinja `{% if config.BLINKING %}` block. This branch turned that block into `{if} var.features.BLINKING` fences, so the fix goes into the BLINKING diagram, without the Jinja markers. - pypeline.yaml: upstream only quoted `python_version: "3.11"`; the fork had already replaced that key with `python_executable: python3`. Kept the fork's. - poetry.lock: upstream's refreshed lock, re-resolved against this branch's pyproject.toml. sphinx-codelinks stays at 1.4.0, the version this branch ran, with typer 0.26.7, because codelinks 1.4.0 caps typer below 0.26.8. - uv.lock, which only the fork keeps: aligned to the same versions. One conflict git could not see: the refreshed lock brings sphinx-needs 8.5.0, which loads needs_from_toml at config-inited priority 10 and resolves the variant data at 11. conf.py selected the variant data file at priority 20, too late, so every build either failed on a missing `build/autoconf.json` or silently read that pointer instead of the cell it was given. conf.py now selects it at priority 10, between the two.
The documentation changes this project needs from spl-core are not in a release yet: optional Jinja raw tags on generated listings, a variant data file per Sphinx build shape, a stable path to the build directory for the generated pages, and KConfig.declared_boolean_symbols(). They live on the feat/configurable-docs-pipeline branch of useblocks/spl-core, five commits on top of spl-core 8.9.0. Pin that commit, so CI and every machine build the same code. In both lock files only spl-core's source changes. Nothing uses the new settings yet; the following commits do.
KConfig omits a promptless boolean from its output when it evaluates to n, so the variant data defaults every declared boolean to false first. Finding them meant reading the kconfiglib instance spl-core keeps privately, behind a fallback for spl-core versions without an accessor. The pinned spl-core has KConfig.declared_boolean_symbols(), so call it and drop the fallback.
…orkarounds
The pinned spl-core can now be told what this project's documentation
needs, so each workaround in conf.py and CMakeLists.txt gives way to one
setting.
Jinja markers. SPL_SOURCE_DOCS_JINJA_RAW_TAGS OFF: clanguru no longer wraps
the generated listings in `{% raw %}`, and conf.py's source-read handler that
blanked the markers again is deleted. conf.py registers no handler at all.
Variant data per build shape. SPL_VARIANT_DATA_FILE_DOCS and _REPORTS name
the cells, and spl-core passes the right one to every Sphinx build as
`-D needs_variant_data_file=`. The fixed-name copies CMake published and
conf.py's fallback that read them are deleted. Nothing in conf.py selects a
cell any more, which is also the only way that works with sphinx-needs 8.5:
it replaces conf.py's values with ubproject.toml's and resolves the variant
data before a config-inited handler at the old priority runs, so the old
selection was silently ignored there. The tests and the VS Code task select a
cell the same way, and it is the same key `ubc check -c` overrides.
Stable report paths. SPL_SPHINX_BINARY_DIR points spl-core at `generated`,
the link tools/variant_data.py maintains, so every generated page is named
`generated/...`. The report sections name their pages directly instead of
globbing `/build/**`, spl-core writes each coverage report next to its page,
and SplBuild finds the report artifacts there. conf.py prunes `build` from
the walk, so each page has exactly one name, and forwards only spl-core's
`generated/` patterns: the `generated/**` rule in ubproject.toml keeps them
out of a docs build, declaratively, for both readers. On Windows without
Developer Mode, `generated` becomes a junction instead of a marker directory,
because the report pages are now read through it.
The tests follow: they drive the fake report tree through `generated`,
check the settings CMake hands spl-core, and assert conf.py registers no
source-read handler. The strip's line-number test is gone with the strip.
VARIANTS.md walks a newcomer through the declarative variant handling in 18 use cases: switching the variant ubCode shows, previewing any variant with Sphinx or ubc, comparing two variants, trying a change on a copy of a variant data file, and writing, extending and checking variant- dependent documentation. Every command in it was run against this branch. It sits at the root, outside both readers' document sets, so neither Sphinx nor ubCode indexes it. README.md and AGENTS.md link to it.
| BlinkOFF --> BlinkON : Blink State == FALSE | ||
| BlinkON --> LIGHT_ON : Reset Blink Counter | ||
| BlinkOFF --> LIGHT_ON : Reset Blink Counter | ||
| state LIGHT_ON { |
There was a problem hiding this comment.
Not sure if we really need to change the state machine here. It's a bit out of scope I think. (The new diagram is easier to read tho)
There was a problem hiding this comment.
This should be ok, it replicated the change in develop...avengineers:SPLed:develop
There was a problem hiding this comment.
I don't think we should introduce uv to the project, since the avengineers repo does not use it (yet). Stefan and I initially committed the uv.lock (because we didn't know better) but I feel like it would be better to remove it before raising the PR in the avengineers repo. @PhilipPartsch also mentioned that we should stick to poetry in this context.
There was a problem hiding this comment.
Yes, that is also what I got from our meeting with MQ and discussions with Philip.
There was a problem hiding this comment.
In SPLed, uv does one thing that Poetry doesn't do reliably: it installs the Python interpreter itself. Resolving, locking and installing the dependencies (dev tools included) is all Poetry's job. uv.lock isn't used by anything. I'll remove uv.lock.
There was a problem hiding this comment.
I compared this PR with #2 and re-checked #2 against @ubmarco's review. Nothing regresses from #2. Every check #2 added is either kept or deliberately replaced, and the replacements for the report-pages guard and the "generated/ and build/ never both live" guard are stronger. The merge resolves its conflicts correctly, and the priority-10 workaround is removed cleanly in 362333b.
Local run with the forked spl-core at 010727d on the path: test_variant_data.py and test_ubproject_config.py 64 passed; test_documentation.py 19 passed, 12 skipped (the ubc tests, as ubc was not installed). Not verified: anything on Windows, a real CMake build against the fork (coverage links end to end), and sphinx-needs 8.5.0 (the local run used 8.2.0; from reading its source, 8.5.0 still lets -D override the TOML).
Findings, most important first:
1. [Medium] The spl-core pin only exists on an open fork branch
010727d can only be reached through feat/configurable-docs-pipeline and refs/pull/4/head on useblocks/spl-core. If that PR is rebased and the branch deleted, the commit eventually disappears and poetry install fails in CI and on every fresh checkout. The fork also still calls itself 8.9.0, the same as the PyPI release, so poetry show cannot tell them apart. Tagging the commit in the fork would protect the fork's CI. For the avengineers PR, this has to wait for an spl-core release.
Line 9 in 6b38f40
2. [Medium] All build directories share one generated link
Configure Disco/test, then Spa/test, then run cmake --build build/Disco/test/Debug without reconfiguring (CMake Tools "Build", or plain ninja). spl-core's build-directory check stops the build, and in the test kit that includes the default target, because reports is part of ALL. Two variants' test builds also cannot run at the same time in one checkout. build.sh / build.ps1 always reconfigure, so the normal workflow is fine. #2 was silently wrong in the same case, so this is louder rather than worse. Still, it deserves a line in the PR description, not just in AGENTS.md / VARIANTS.md.
Line 112 in 6b38f40
3. [Low-medium] The parts.cmake parser accepts conditions it says it rejects (carried over from #2)
The guard is a substring test (TEST_KIT_GUARD.lower() not in lowered), so these all pass:
if(NOT BUILD_KIT STREQUAL test)is read as the test branch, so the component lands in the wrong kit.if(BUILD_KIT STREQUAL test OR FOO)andSTREQUAL testingare read as the plain guard.- The lowercasing also accepts
STREQUAL Test, although CMake compares case-sensitively.
A quoted spl_add_component("components/x") keeps its quotes, so the component's membership rule never matches and its documents silently disappear. A call split over several lines fails with a bare ValueError from rindex. None of today's parts.cmake files do this, but the docstring promises to raise on anything outside the grammar. An exact match on the normalised condition, plus stripping quotes, would make that true.
Line 147 in 6b38f40
Line 174 in 6b38f40
4. [Low-medium] The Windows junction path has never run for real
The unit tests replace _create_junction with a fake. GitHub's Windows runners run as admin and always get a symlink, and the documentation job is Linux-only. So _winapi.CreateJunction, removing a junction, the \\?\ handling in _generated_points_at, and CMake's file(REAL_PATH) through a junction have not been exercised. The code looks right to me; one manual run on a Windows machine without Developer Mode would settle it.
Lines 277 to 283 in 6b38f40
5. [Low] Only works on Python 3.12
Path.is_junction() is new in 3.12, and _remove_link calls it on every configure, on every OS. That is fine here, but upstream avengineers is still on 3.11, where upstreaming this as-is gives an AttributeError at configure time, on Linux too.
6. [Low] Nothing checks that the spl-core settings come before parts.cmake
The four set()s are at lines 102–116 and parts.cmake is included at line 137, which is correct. The raw-tags test only checks the set() text and runs clanguru directly, not through spl-core. So it would still pass if the line moved below the include, where it no longer has any effect. A small assertion that all four settings come before the include would guard this.
Lines 102 to 137 in 6b38f40
7. [Low] Please drop uv.lock in this PR
I agree with @LuSilber. Upstream deleted it and the fork added it back in c8a0f44. Nothing reads it: CI and the scripts use Poetry, and uv is only used to install CPython. This PR still rewrites about 260 lines of it, so deleting it here has no functional effect.
8. [Nit] Stale wording
.vscode/tasks.json:35says the report fences "glob into build/**"; they now name/generated/...pages..gitignore:4-6saysgeneratedis "a copy" on Windows without Developer Mode; it is now a junction, or a plain directory with a marker file.doc/cross-platform-alignment-proposal.md:299still saysspl-core==8.6.0.- VARIANTS.md §18 says "a link (a junction on Windows)"; with Developer Mode it is a symlink.
- VARIANTS.md §4:
--currentwithout--build-dirleavesgeneratedpointing at the last configured build. That is harmless, but a sentence would avoid confusion.
On the light controller comment: that diagram change isn't new scope from this PR. It is the upstream fix b8beee9 brought in by the merge, and the merge only moved it into the {if} var.features.BLINKING fence, so I would keep it.
Nice work overall. VARIANTS.md in particular is a great addition.
This branch has not been deployed
5 participants
Stacked on #2. This branch pins spl-core to a commit of the useblocks fork that adds the settings #2 was missing, and deletes the workarounds #2 needed without them. It also brings in the two upstream commits the fork lacked, so the branch is 0 commits behind avengineers/SPLed
develop.spl-core changes: useblocks/spl-core#4, five commits on top of spl-core 8.9.0.
What changes
SPL_SOURCE_DOCS_JINJA_RAW_TAGS OFF{% raw %}markers. conf.py'ssource-readstrip is gone, and conf.py registers no handler at all.SPL_VARIANT_DATA_FILE_DOCS/_REPORTS-D needs_variant_data_file=. The fixed-namebuild/variant-data-*.jsoncopies and conf.py's variant selection are gone. Tests and the VS Code task select a cell the same way, with the keyubc check -coverrides.SPL_SPHINX_BINARY_DIR=generatedgenerated/components/<c>/reports/..., so the report sections name them directly and no document globs/build/**. Coverage links follow the pages. conf.py prunesbuildfrom the walk and forwards only spl-core'sgenerated/patterns, and thegenerated/**rule keeps them out of docs builds.KConfig.declared_boolean_symbols()tools/variant_data.pyis gone.On Windows without Developer Mode,
generatednow becomes a junction rather than a marker directory, because the report pages are read through it.The upstream merge
The first commit merges avengineers/SPLed
develop: spl-core 8.9 with a refreshed lock, and the light controller state machine fix. The commit message lists how each conflict was resolved.One conflict only showed at run time. The refreshed lock brings sphinx-needs 8.5.0, which resolves the variant data right after loading
needs_from_toml. #2's conf.py selected the file later, atconfig-initedpriority 20. With 8.5.0 every build therefore either failed on a missingbuild/autoconf.jsonor quietly read that pointer instead of its cell, which leaves a reports build without reports. The merge fixes that at priority 10. The last commit replaces the mechanism with the command-line override, which sphinx-needs keeps ahead of the TOML.Verification
test_documentation.py,test_ubproject_config.pyandtest_variant_data.py: 94 passed, 1 skipped (the ubc requirement check, which only runs in CI).pytest -m "docs and gate_develop_pr"withCI_REQUIRE_UBC=1and ubc 0.35.0: 31 passed, on this branch and on the merge and accessor commits below it.reportsanddocstargets. Every report page sits undergenerated/, both kinds of coverage link resolve, the verification sections render in the reports build and not in the docs build, andSplBuildfinds all 16 report artifacts. A docs build whosegeneratedlink was re-pointed at another directory stops with spl-core's message.generated. https://github.com/useblocks/SPLed/actions/runs/35985268428test_a_refused_symlink_falls_back_to_a_junction.Unchanged here
generated/reports/*_index.mdare not in any toctree. That gives five warnings on Disco, as before underbuild/.