Skip to content

Commit fa242cd

Browse files
hoe-jocastler
authored andcommitted
[rules score] flatten HTML merge over transitive module closure
- SphinxModuleInfo gains own_html_dir and transitive_modules which fixes a diamond-dependency bug: a module required via two or more dependency paths (e.g. A -> B, C; B -> D; C -> D) - sphinx_html_merge.py: dropped that nested-skip heuristic - Added module_d_lib test fixture: a diamond dep of both module_b_lib and module_c_lib, asserts its HTML lands exactly once, flat, in the real merged build output.
1 parent c5ff8ce commit fa242cd

8 files changed

Lines changed: 223 additions & 34 deletions

File tree

‎bazel/rules/rules_score/private/sphinx_module.bzl‎

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -366,13 +366,16 @@ def _score_html_impl(ctx):
366366
]
367367
merge_inputs = [sphinx_html_output]
368368

369-
# Add each dependency
370-
for dep in ctx.attr.deps:
371-
if SphinxModuleInfo in dep:
372-
dep_html_dir = dep[SphinxModuleInfo].html_dir
373-
dep_name = dep.label.name
374-
merge_inputs.append(dep_html_dir)
375-
merge_args.extend(["--dep", dep_name + ":" + dep_html_dir.path])
369+
# Every module transitively required by this one, each contributing its
370+
# own_html_dir (never another module's already-merged html_dir), so each
371+
# lands exactly once, at depth 1, regardless of how many paths reach it
372+
# in a diamond dependency graph (see SphinxModuleInfo.transitive_modules).
373+
transitive_modules_from_deps = depset(
374+
transitive = [dep[SphinxModuleInfo].transitive_modules for dep in ctx.attr.deps if SphinxModuleInfo in dep],
375+
).to_list()
376+
for module in transitive_modules_from_deps:
377+
merge_inputs.append(module.own_html_dir)
378+
merge_args.extend(["--dep", module.name + ":" + module.own_html_dir.path])
376379

377380
# Auto-detect static files from srcs: any file whose short_path contains
378381
# '/_static/' is a static asset that Sphinx may not copy correctly in the
@@ -400,6 +403,13 @@ def _score_html_impl(ctx):
400403
DefaultInfo(files = depset([html_output])),
401404
SphinxModuleInfo(
402405
html_dir = html_output,
406+
own_html_dir = sphinx_html_output,
407+
# Reuses the already-flattened transitive_modules_from_deps list
408+
# collected above for the merge step, plus this module itself.
409+
transitive_modules = depset(
410+
[struct(name = ctx.label.name, own_html_dir = sphinx_html_output)] +
411+
transitive_modules_from_deps,
412+
),
403413
),
404414
OutputGroupInfo(
405415
sphinx_sources = depset([config_file] + sphinx_source_files),

‎bazel/rules/rules_score/providers.bzl‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -258,7 +258,21 @@ SphinxIndexFileInfo = provider(
258258
SphinxModuleInfo = provider(
259259
doc = "Provider for Sphinx HTML module documentation",
260260
fields = {
261-
"html_dir": "Directory containing HTML files",
261+
"html_dir": "Directory containing this module's HTML, merged with its " +
262+
"transitive dependencies (what sphinx_html_merge.py produces).",
263+
"own_html_dir": "Directory containing only this module's own HTML, before " +
264+
"any dependency is merged in. What the flat merge (see " +
265+
"transitive_modules) copies from — each transitive module " +
266+
"contributes its own_html_dir, never another module's " +
267+
"already-merged html_dir, so a diamond dependency doesn't " +
268+
"get copied once per path to it.",
269+
"transitive_modules": "Depset of struct(name, own_html_dir), one entry per " +
270+
"module transitively required by this one, including " +
271+
"this module itself. Self-inclusive union: each dep's " +
272+
"own transitive_modules already contains itself, so " +
273+
"unioning deps' depsets yields the full flat closure " +
274+
"without this module needing to add each dep " +
275+
"individually on top.",
262276
},
263277
)
264278

‎bazel/rules/rules_score/src/sphinx_html_merge.py‎

Lines changed: 16 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -15,9 +15,14 @@
1515
"""Merge multiple Sphinx HTML output directories.
1616
1717
This script merges Sphinx HTML documentation from multiple modules into a single
18-
output directory. It copies the main module's HTML as-is, and then copies each
19-
dependency module's HTML into a subdirectory, excluding nested module directories
20-
to avoid duplication.
18+
output directory. It copies the main module's HTML as-is, then copies every
19+
transitively required module's *own* (unmerged) HTML into a subdirectory --
20+
flat, one level deep, one copy per module regardless of how many dependency
21+
paths reach it. sphinx_module.bzl is responsible for passing --dep as the
22+
transitive closure (SphinxModuleInfo.transitive_modules), with each --dep
23+
pointing at that module's own_html_dir rather than its (possibly
24+
further-merged) html_dir -- this script has no way to tell the two apart and
25+
would duplicate a diamond dependency if handed the latter.
2126
2227
Usage:
2328
sphinx_html_merge.py --output OUTPUT_DIR --main MAIN_HTML_DIR [--dep NAME:PATH ...]
@@ -94,10 +99,12 @@ def copy_html_files(src_dir, dst_dir, is_dependency=False, sibling_modules=None)
9499
dropped (the merged site uses one shared _static/ at the
95100
root) and their internal links rewritten for the new
96101
nesting depth.
97-
sibling_modules: Set of sibling module directory names to skip (so nested
98-
copies of other modules already merged elsewhere aren't
99-
duplicated) and to rewrite intra-site links for. Only
100-
meaningful when is_dependency is True.
102+
sibling_modules: Set of other module directory names to rewrite intra-site
103+
links for (e.g. href="other_module/..." needs a "../" prefix
104+
added for the new nesting depth). Only meaningful when
105+
is_dependency is True. src_dir is always a module's own,
106+
unmerged HTML now (never another module's already-merged
107+
tree), so there is nothing nested under it to skip.
101108
"""
102109
src_path = Path(src_dir)
103110
dst_path = Path(dst_dir)
@@ -175,9 +182,6 @@ def copy_tree(src, dst, rel_path):
175182
# Never publish Sphinx's own build cache.
176183
if item.name in BUILD_ARTIFACT_DIRS:
177184
continue
178-
# Skip nested copies of sibling modules to avoid duplication.
179-
if item.name in sibling_modules:
180-
continue
181185
# Dependencies use the merged site's shared _static/ instead
182186
# of their own.
183187
if is_dependency and item.name in (
@@ -217,15 +221,14 @@ def merge_html_dirs(output_dir, main_html_dir, dependencies, extra_static=None):
217221
shutil.copy2(src_file, dst)
218222
logging.info("Copied extra static %s → _static/%s", src_file, dest_subpath)
219223

220-
# Collect all dependency names for link fixing and exclusion
224+
# Collect all dependency names for intra-site link rewriting.
221225
dep_names = [name for name, _ in dependencies]
222226

223227
# Then copy each dependency into a subdirectory with link fixing
224228
for dep_name, dep_html_dir in dependencies:
225229
dep_output = output_path / dep_name
226230
logging.info("Copying dependency %s from %s to %s", dep_name, dep_html_dir, dep_output)
227-
# Exclude other module directories to avoid nested modules
228-
# Remove current module from the list to get actual siblings to exclude
231+
# Other modules in this merge, to rewrite intra-site links for.
229232
sibling_modules = set(n for n in dep_names if n != dep_name)
230233
copy_html_files(
231234
dep_html_dir,

‎bazel/rules/rules_score/test/BUILD‎

Lines changed: 37 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,7 @@ load(
7777
"score_toolchain_override_test",
7878
"sphinx_module_info_fields_test",
7979
"sphinx_module_providers_test_suite",
80+
"transitive_modules_dedup_test",
8081
"two_phase_html_second_test",
8182
"two_phase_needs_first_test",
8283
)
@@ -148,17 +149,32 @@ toolchain(
148149
# Test 1: Multi-Module Aggregation
149150
# Dependency graph: module_a_lib -> module_b_lib -> module_c_lib
150151
# module_a_lib -> module_c_lib (also direct)
152+
# module_d_lib is a diamond dependency reachable only transitively, via
153+
# module_b_lib AND module_c_lib (never directly from module_a_lib) -- a
154+
# regression fixture for the HTML-merge flattening: module_d_lib's HTML must
155+
# be merged into module_a_lib's published site exactly once, not once per
156+
# path that reaches it.
157+
sphinx_module(
158+
name = "module_d_lib",
159+
srcs = glob(["fixtures/module_d/*.rst"]),
160+
index = "fixtures/module_d/index.rst",
161+
)
162+
151163
sphinx_module(
152164
name = "module_c_lib",
153165
srcs = glob(["fixtures/module_c/*.rst"]),
154166
index = "fixtures/module_c/index.rst",
167+
deps = [":module_d_lib"],
155168
)
156169

157170
sphinx_module(
158171
name = "module_b_lib",
159172
srcs = glob(["fixtures/module_b/*.rst"]),
160173
index = "fixtures/module_b/index.rst",
161-
deps = [":module_c_lib"],
174+
deps = [
175+
":module_c_lib",
176+
":module_d_lib",
177+
],
162178
)
163179

164180
sphinx_module(
@@ -878,6 +894,17 @@ sh_test(
878894
tags = ["manual"],
879895
)
880896

897+
# End-to-end regression test: module_d_lib (a diamond dep of both
898+
# module_b_lib and module_c_lib, never directly of module_a_lib) must be
899+
# merged into module_a_lib's published site exactly once, flat, not once
900+
# per dependency path that reaches it.
901+
sh_test(
902+
name = "module_d_dedup_test",
903+
srcs = ["check_module_d_dedup.sh"],
904+
data = [":module_a_lib"],
905+
tags = ["manual"],
906+
)
907+
881908
# ============================================================================
882909
# SEooC-Specific Tests
883910
# ============================================================================
@@ -1092,6 +1119,14 @@ sphinx_module_info_fields_test(
10921119
target_under_test = ":module_a_lib",
10931120
)
10941121

1122+
# Regression test for the diamond-dependency HTML-merge flattening:
1123+
# module_d_lib is a dep of both module_b_lib and module_c_lib (never
1124+
# directly of module_a_lib), so it's reachable via two paths.
1125+
transitive_modules_dedup_test(
1126+
name = "transitive_modules_dedup_test",
1127+
target_under_test = ":module_a_lib",
1128+
)
1129+
10951130
score_needs_info_fields_test(
10961131
name = "score_needs_info_fields_test",
10971132
target_under_test = ":module_a_lib_needs",
@@ -1395,6 +1430,7 @@ test_suite(
13951430
":test_sphinx_html_merge",
13961431
":test_sphinx_module_ext",
13971432
":test_trlc_rst_image_rendering",
1433+
":transitive_modules_dedup_test",
13981434
":unit_component_tests",
13991435
"//fixtures/image_srcs:requirements_image_tests",
14001436
],
Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
#!/bin/bash
2+
# *******************************************************************************
3+
# Copyright (c) 2026 Contributors to the Eclipse Foundation
4+
#
5+
# See the NOTICE file(s) distributed with this work for additional
6+
# information regarding copyright ownership.
7+
#
8+
# This program and the accompanying materials are made available under the
9+
# terms of the Apache License Version 2.0 which is available at
10+
# https://www.apache.org/licenses/LICENSE-2.0
11+
#
12+
# SPDX-License-Identifier: Apache-2.0
13+
# *******************************************************************************
14+
set -euo pipefail
15+
16+
# End-to-end regression test for the diamond-dependency HTML-merge
17+
# flattening: module_d_lib is a dep of both module_b_lib and module_c_lib
18+
# (never directly of module_a_lib), so before the fix its HTML was copied
19+
# once per path that reached it -- nested under module_b_lib/module_d_lib/
20+
# AND module_c_lib/module_d_lib/, never at the top level. After the fix it
21+
# must land exactly once, flat, at the top level of the merged site.
22+
# TODO: pass the HTML dir via args instead of using a hardcoded relative path,
23+
# e.g. args = ["$(rootpath :module_a_lib)"] in the sh_test.
24+
html_dir="./module_a_lib/html"
25+
26+
if [[ ! -d "$html_dir" ]]; then
27+
echo "Error: Directory not found: $html_dir" >&2
28+
exit 1
29+
fi
30+
31+
if [[ ! -f "$html_dir/module_d_lib/index.html" ]]; then
32+
echo "Error: Expected flat, top-level $html_dir/module_d_lib/index.html not found" >&2
33+
exit 1
34+
fi
35+
36+
if [[ -d "$html_dir/module_b_lib/module_d_lib" ]]; then
37+
echo "Error: module_d_lib was duplicated, nested under module_b_lib/" >&2
38+
exit 1
39+
fi
40+
41+
if [[ -d "$html_dir/module_c_lib/module_d_lib" ]]; then
42+
echo "Error: module_d_lib was duplicated, nested under module_c_lib/" >&2
43+
exit 1
44+
fi
45+
46+
echo "✓ module_d_lib's HTML appears exactly once, flat, in the merged output"
Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
..
2+
# *******************************************************************************
3+
# Copyright (c) 2026 Contributors to the Eclipse Foundation
4+
#
5+
# See the NOTICE file(s) distributed with this work for additional
6+
# information regarding copyright ownership.
7+
#
8+
# This program and the accompanying materials are made available under the
9+
# terms of the Apache License Version 2.0 which is available at
10+
# https://www.apache.org/licenses/LICENSE-2.0
11+
#
12+
# SPDX-License-Identifier: Apache-2.0
13+
# *******************************************************************************
14+
Module D Documentation
15+
======================
16+
17+
This is the documentation for Module D.
18+
19+
.. document:: Documentation for Module D
20+
:id: doc__module_fixtures_module_d
21+
:status: valid
22+
:safety: ASIL_B
23+
:security: NO
24+
:realizes:
25+
26+
27+
Overview
28+
--------
29+
30+
Module D is a base module with no dependencies, shared as a diamond
31+
dependency by Module B and Module C (both depend on it, neither directly
32+
via Module A) — used to regression-test that its HTML is merged exactly
33+
once into Module A's published site.
34+
Local need link: :need:`doc__module_fixtures_module_d`
35+
36+
Features
37+
--------
38+
39+
.. needlist::
40+
:tags: module_d
41+
42+
Content
43+
-------
44+
45+
Module D provides foundational functionality shared by Module B and Module C.

‎bazel/rules/rules_score/test/score_module_providers_test.bzl‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,32 @@ def _sphinx_module_info_fields_test_impl(ctx):
4949

5050
sphinx_module_info_fields_test = analysistest.make(_sphinx_module_info_fields_test_impl)
5151

52+
def _transitive_modules_dedup_test_impl(ctx):
53+
"""Regression test: a module reachable via multiple dependency paths (a
54+
diamond — module_d_lib is a dep of both module_b_lib and module_c_lib,
55+
never directly of module_a_lib) must appear exactly once in
56+
transitive_modules, not once per path that reaches it.
57+
"""
58+
env = analysistest.begin(ctx)
59+
target_under_test = analysistest.target_under_test(env)
60+
61+
score_info = target_under_test[SphinxModuleInfo]
62+
transitive_modules = score_info.transitive_modules.to_list()
63+
64+
matches = [m for m in transitive_modules if m.name == "module_d_lib"]
65+
asserts.equals(
66+
env,
67+
1,
68+
len(matches),
69+
"module_d_lib is reachable via two paths (module_b_lib and " +
70+
"module_c_lib) but must appear exactly once in transitive_modules, " +
71+
"got: %s" % matches,
72+
)
73+
74+
return analysistest.end(env)
75+
76+
transitive_modules_dedup_test = analysistest.make(_transitive_modules_dedup_test_impl)
77+
5278
# ============================================================================
5379
# SphinxNeedsInfo Provider Tests
5480
# ============================================================================
@@ -353,6 +379,7 @@ def sphinx_module_providers_test_suite(name):
353379
tests = [
354380
# Provider field tests
355381
":sphinx_module_info_fields_test",
382+
":transitive_modules_dedup_test",
356383
":score_needs_info_fields_test",
357384
":score_needs_transitive_collection_test",
358385

‎bazel/rules/rules_score/test/test_sphinx_html_merge.py‎

Lines changed: 20 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -157,21 +157,28 @@ def test_extra_static_copied_after_main(self) -> None:
157157

158158
self.assertEqual((self.output / "_static" / "logo.svg").read_text(), "custom-logo")
159159

160-
def test_nested_sibling_module_copy_is_skipped(self) -> None:
161-
"""Regression test: if a dependency's own HTML tree happens to contain
162-
a subdirectory named after a sibling module (e.g. because that
163-
dependency was itself built with sphinx_html_merge and already
164-
embeds the sibling), that nested copy must be skipped rather than
165-
duplicated into the merged output - the sibling is placed once, at
166-
the site root, by its own top-level merge entry.
160+
def test_deps_are_always_a_flat_transitive_set_no_nested_skip_needed(self) -> None:
161+
"""sphinx_module.bzl always passes --dep as the transitive closure
162+
(SphinxModuleInfo.transitive_modules), each pointing at that module's
163+
own_html_dir — a module's own, unmerged Sphinx output, never another
164+
module's already-merged tree. There is therefore nothing nested to
165+
skip: this merge script has no special-casing for a dep's
166+
subdirectory happening to share a name with another dep, and copies
167+
it verbatim. Guards against reintroducing the old nested-sibling-skip
168+
heuristic, which existed only because deps used to be each other's
169+
*merged* trees (recursively containing further-nested deps) before
170+
the merge was flattened.
167171
"""
168172
main = self.root / "main"
169173
_write(main / "index.html", "<html></html>")
170174

171175
dep_a = self.root / "dep_a"
172176
_write(dep_a / "index.html", "<html></html>")
173-
# dep_a already contains its own nested (stale) copy of dep_b.
174-
_write(dep_a / "dep_b" / "index.html", "<html>stale nested copy</html>")
177+
# Coincidentally named the same as another --dep entry below. Under
178+
# the old nested-merge design this could only happen via actual
179+
# nesting and had to be skipped; under the flat design it's just a
180+
# same-named subdirectory in dep_a's own tree, copied like any other.
181+
_write(dep_a / "dep_b" / "page.html", "<html>dep_a's own dep_b/ subdirectory</html>")
175182

176183
dep_b = self.root / "dep_b"
177184
_write(dep_b / "index.html", "<html>canonical dep_b</html>")
@@ -182,9 +189,10 @@ def test_nested_sibling_module_copy_is_skipped(self) -> None:
182189
[("dep_a", dep_a), ("dep_b", dep_b)],
183190
)
184191

185-
# The nested copy under dep_a/dep_b/ must not have been copied.
186-
self.assertFalse((self.output / "dep_a" / "dep_b").exists())
187-
# The canonical dep_b, copied from its own top-level entry, is intact.
192+
self.assertEqual(
193+
(self.output / "dep_a" / "dep_b" / "page.html").read_text(),
194+
"<html>dep_a's own dep_b/ subdirectory</html>",
195+
)
188196
self.assertEqual(
189197
(self.output / "dep_b" / "index.html").read_text(),
190198
"<html>canonical dep_b</html>",

0 commit comments

Comments
 (0)