feat: add local Needs exports to documentation bundles - #772
Conversation
License Check Results🚀 The license check job ran with the Bazel command: bazel run --lockfile_mode=error //src:license-checkStatus: Click to expand output |
|
Documentation preview for this pull request is available at: |
MaximilianSoerenPollak
left a comment
There was a problem hiding this comment.
It seems alright to me.
I think it's hard to judge some edgecases and see if this covers all of the useages we might need.
Easiest to slowly test it in the modules that need it and see what is missing / not quiet right.
Great starting point though. 💯
There was a problem hiding this comment.
Pull request overview
Adds reusable local and upward Needs exports for source-bearing documentation bundles.
Changes:
- Adds
upward_bundleshierarchy support and generated Needs targets. - Extends external Needs resolution for bundle exports.
- Adds documentation and integration/unit coverage.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
docs.bzl |
Generates local and upward Needs exports. |
bzl/bundle_rules.bzl |
Propagates hierarchy metadata. |
default_conf.py.tpl |
Supports bundle entry documents. |
docs/reference/bazel_macros.rst |
Documents the new API and flow. |
src/extensions/score_metamodel/external_needs.py |
Resolves upward exports from runfiles. |
src/extensions/score_metamodel/tests/test_external_needs.py |
Tests bundle export resolution. |
src/tests/docs_bzl/test_upward_bundles.py |
Tests bundle export behavior. |
src/tests/docs_bzl/README.md |
Registers the new scenario. |
src/tests/docs_bzl/scenarios/upward_bundles/BUILD |
Defines the hierarchy fixture. |
src/tests/docs_bzl/scenarios/upward_bundles/platform/index.rst |
Provides ancestor Needs. |
src/tests/docs_bzl/scenarios/upward_bundles/component/index.rst |
Provides dependent Needs. |
src/tests/docs_bzl/scenarios/data_files_runfiles/BUILD |
Documents data-only behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| # Label() directly would resolve them relative to the repository containing | ||
| # this .bzl file, which is wrong when the macro is loaded by another module. | ||
| bundle_string = str(native.package_relative_label(bundle)) | ||
| return Label(bundle_string + ".__internal__.needs_local") |
There was a problem hiding this comment.
I am intentionally not applying the suggested change. upward_bundles is defined as an explicit direct-dependency list; resolving needs_upward here would import the ancestor chain implicitly and violate the no-transitive-dependencies contract. Source-less hierarchy groups are now rejected explicitly with an analysis-time error (1d33c85), and consumers must list the source-bearing ancestors they need directly. Leaving this thread open because the proposed behavior conflicts with that contract.
2aa2764 to
93ebfb9
Compare
93ebfb9 to
2dbb70b
Compare
8a8ab62 to
6205415
Compare
0fe3bed to
eee8166
Compare
3ded8d7 to
8d00c04
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Local exports break checked-in configurations and explicit-source layouts, while project URL validation regresses globally.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 5
- Review effort level: Balanced
| if config == None: | ||
| # Sphinx expects conf.py below the source root. Bundle-local Needs | ||
| # exports always use a generated config so the bundle stays | ||
| # self-contained and does not depend on a caller-provided conf.py. |
There was a problem hiding this comment.
Valid. Local exports now reuse a checked-in conf.py when present and generate a unique fallback config otherwise. The bundle deps are also passed to the private Sphinx binary so custom configuration extensions remain available.
| else: | ||
| needs_config = config | ||
|
|
||
| source_strip_prefix = _bundle_sphinx_strip_prefix(source_dir) |
There was a problem hiding this comment.
Valid. Explicit source bundles now expose a Sphinx library whose strip prefix is derived from the actual source paths, including generated outputs. This stages the bundle entry document at the local Sphinx root.
| score_sourcelinks_json = "$(location " + str(sourcelinks_json) + ")" if sourcelinks_json else None, | ||
| # ``sphinxdocs`` removes this string literally from short_path. | ||
| # Keep the separator so a source_dir/conf.py is relocated as | ||
| # conf.py rather than /conf.py. | ||
| strip_prefix = source_strip_prefix, |
There was a problem hiding this comment.
Valid. Added a successful build of the legacy component needs_local target and asserted its exported source_code_link, covering the local source-link path.
| @@ -141,12 +141,6 @@ def extend_needs_json_exporter(config: Config, params: list[str]) -> None: | |||
| # This is wrong. But good enough. | |||
| config.add(p, default="", rebuild="env", types=(), description="") | |||
There was a problem hiding this comment.
Valid. The earlier change had removed normal project_url validation. Bundle-export mode is now an explicit config value: only local exports suppress the missing-value diagnostic and serialize an empty reusable-export URL; host builds retain the validation.
| def test_build_combined_file_without_source_links_uses_empty_input( | ||
| tmp_path: Path, monkeypatch: pytest.MonkeyPatch | ||
| ): | ||
| """Standalone builds may legitimately have no source-link input.""" | ||
| monkeypatch.delenv("SCORE_SOURCELINKS", raising=False) | ||
|
|
||
| build_and_save_combined_file(tmp_path) | ||
|
|
||
| combined_file = tmp_path / "score_scl_grouped_cache.json" | ||
| assert json.loads(combined_file.read_text(encoding="utf-8")) == [] |
There was a problem hiding this comment.
Valid. Removed the duplicate no-source-links test and kept the existing test as the single authoritative coverage for that behavior.
880f54d to
30eca23
Compare
30eca23 to
19684c9
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Local exports omit bundle data, can include nested same-package sources, and mishandle explicit-source configurations.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 15/15 changed files
- Comments generated: 3
- Review effort level: Balanced
| # ``bundle_sphinx_source_files`` is important here: using the complete bundle | ||
| # would also feed nested child sources into this Sphinx invocation and | ||
| # export their Needs under the parent's local target. Ownership stays | ||
| # one-way: every source-bearing bundle exports only its own sources. | ||
| own_sources = bundle_sphinx_source_files( | ||
| name = _bundle_internal_target(name, "needs_sources"), | ||
| bundle = ":" + name, | ||
| visibility = visibility, |
There was a problem hiding this comment.
will be covered by future PRs
| needs_local = _bundle_internal_target(name, "needs_local") | ||
| _needs_sphinx_docs( | ||
| name = needs_local, | ||
| deps = [own_sources], |
There was a problem hiding this comment.
will be covered by future PRs
MaximilianSoerenPollak
left a comment
There was a problem hiding this comment.
Talked about in a call.
It's a temp solution that is okay and will be worked on further.
Why this matters
Source-bearing documentation bundles currently contribute content to composed documentation builds but do not expose a standalone Needs inventory for their own sources. That prevents each bundle from being validated or consumed independently and makes source ownership unclear at bundle boundaries.
This PR adds the local-export layer while preserving the project-wide
needs_jsontarget. Local exports intentionally remain self-contained: references to Needs owned outside the bundle are unresolved until cross-bundle propagation is added.What changed
<name>.__internal__.needs_localfor every source-bearingdocs_bundleand for the root bundle.conf.pyfor its local export, or generate aself-contained configuration when the bundle has no configuration.
sphinx_build_binarycreation in_needs_sphinx_docs.needs_jsonbehavior.project_urlvalidation for normal host builds while leaving reusable local export metadata host-independent.docs_bundles and remove the redundantstandalone_bundlefixture.