refactor: route Bazel docs inputs through the CLI - #836
Merged
AlexanderLanin merged 8 commits intoSep 17, 2026
Merged
AlexanderLanin merged 8 commits into
AlexanderLanin merged 8 commits into
Conversation
…in the Needs build action The sandboxed Needs build action received these three Bazel-generated files as pre-formatted `--define=...` strings baked into `SPHINX_EXTRA_OPTS`, while the interactive `bazel run` targets already passed the metamodel and mounts manifest as plain environment variables consumed by cli.py. Declare them as typed label attributes on `sphinx_docs` instead, so the action can declare them as sandbox inputs and expose their execroot paths through the same env vars (`SCORE_SOURCELINKS`, `MOUNTS_MANIFEST`, `SCORE_METAMODEL_YAML`) already used elsewhere, removing the duplicated per-caller `$(location ...)` string formatting.
…nly in cli.py Extensions read these Bazel-provided paths through two parallel channels: a direct os.environ lookup and the app.config value cli.py populates via --define. The env-var path bypassed cli.py's runfiles resolution entirely, so it only worked by coincidence for values that happened to already be cwd-relative. Make cli.py the single place that resolves SCORE_SOURCELINKS (like it already did for SCORE_METAMODEL_YAML, now shared through _resolve_runfiles_relative_path) into a --define, and drop the now-redundant direct env reads in score_source_code_linker, score_mounts, and score_cross_module_compatibility. mounts_manifest's env fallback was already dead code, since cli.py has unconditionally defined it for every invocation; sourcelinks_json's was not, so docs.bzl's interactive SCORE_SOURCELINKS now passes an rlocationpath like its metamodel/mounts siblings, resolved correctly by cli.py instead of read as a bare path by the extension.
cli.py is only ever invoked via `bazel run` or as the sandboxed Needs action's executable (see src/docs_cli/README.md); ExecutionEnvironment.DIRECT exists solely so DocsCliConfig/sphinx_arguments stay unit-testable without a real runfiles tree. Assert this invariant right after building the config so it's readable in the file instead of only discoverable by tracing callers.
app was optional and score_sourcelinks_json was read via getattr with a fallback, but the only production caller (setup_combined_linker) always passes a real app, and add_config_value guarantees the config value exists before any event handler can call this function. Make app mandatory and read the config value directly; update the one test that relied on the app=None branch to pass an explicit fake config instead.
xml_parser.py read env.optional_path("KNOWN_GOOD_JSON") directly, bypassing
the app.config.KNOWN_GOOD_JSON value score_source_code_linker already
registers and cli.py already emits via --define. That define carried an
unresolved value, since cli.py never ran it through the runfiles resolution
used for SCORE_METAMODEL_YAML/SCORE_SOURCELINKS; it only worked because
docs.bzl passed $(location ...), which happens to be cwd-relative under
bazel run.
Resolve KNOWN_GOOD_JSON through the shared _resolve_runfiles_relative_path
helper like its siblings, switch docs.bzl to $(rlocationpath ...), and
thread the resolved path through xml_parser.py's call chain
(build_test_needs_from_files -> read_test_xml_file ->
get_metadata_from_test_path) instead of reading the env var deep inside a
plain parsing function that has no Sphinx app access.
AlexanderLanin
requested review from
MaximilianSoerenPollak,
a-zw,
dcalavrezo-qorix and
nradakovic
as code owners
September 16, 2026 21:19
Contributor
|
Documentation preview for this pull request is available at: |
a-zw
approved these changes
Sep 17, 2026
a-zw
left a comment
a-zw
left a comment
Contributor
There was a problem hiding this comment.
I like this. It should ease the reasoning about the different invocations.
Comment on lines
+209
to
+210
| # The sandboxed Needs action and ``bazel run`` both set this env var; | ||
| # only the extension reads ``app.config.score_sourcelinks_json``. |
Contributor
There was a problem hiding this comment.
It is not clear to me what "sandboxed Needs action" and "the extension" is.
Member
Author
There was a problem hiding this comment.
will improve in next PR
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
cli.pyThe larger
external_needs_sourcelabel-resolution migration remains outside this change.Review note
The commits are intentionally ordered as incremental refactor steps. Please review them commit by commit, in order, rather than only as one combined diff.