Repository navigation
DR8 integration workflow - #398
PiotrKorkus wants to merge 6 commits into
Conversation
Squash of: - feat: two-stage DR-008 test workflow with per-module Stage 2 execution - fix: give central-mode test runs their own exclusion list - ci: split DR-008 test execution into its own workflow file, keep old job unchanged - fix: restore itf conftest.py - ci(stage2): isolate score_communication under a dedicated Stage-2 rc - fix(stage2): transport dependency patches into Stage 2 injection - style: apply ruff formatting - fix(stage2): add score_config_management to ref_int's GCC toolchain - chore: align DR-008 changes with upstream main Co-authored-by: Piotr Korkus <piotr.korkus.ext@qorix.ai>
- ci: add module tests workflow for PR testing - refactor: test target_sw modules via --override_module instead of MODULE.bazel injection - fix(module_tests): give module runs their own output base - fix(unified.bazelrc): clear ANDROID_HOME for module runs too - fix(module_tests): pin direct deps only and inject unified toolchains - fix(time): give time's ubuntu image pull its own name - fix(module_tests): fail runs without executed tests; patch time sanitizer flags - build: bump score_toolchains_rust to 0.10.0 - build: use score_tooling v2.2.3
score_docs_as_code@36cdc3f depends on score_devcontainer 1.11.1, which was yanked, breaking every module test. The override lives in unified_toolchains.MODULE.bazel as it is included by ref_int and by every tested module.
OliverHeilwagen
left a comment
There was a problem hiding this comment.
Really nice solution. Using --override_module and bazel fetch from ref_int context reduced the complexity drastically, also patching a include() for the unified toolchain solves the former open point.
I only see pinning of the whole module graph as required change (pinning of transitive dependencies) see suggestion line 181, all other comments are optional suggestions.
| "USE_BAZEL_VERSION": (REF_INT / ".bazelversion").read_text().strip(), | ||
| } | ||
|
|
||
| shared = direct_deps(mod_graph(workspace, env)) & resolved |
There was a problem hiding this comment.
Please change to:
| shared = direct_deps(mod_graph(workspace, env)) & resolved | |
| shared = (graph_modules(mod_graph(workspace, env)) & resolved) - {module.name} |
Only pinning direct dependencies can lead to version drifts compared to the dependency resolution done by reference_integration.
All modules in scope of reference integration version resolution should be enforced on the modules as long as they are a dependency of the given module under test.
Please also update autodoc description in line 18.
Note: direct_deps can be removed after this change.
Follow up: A override from reference integration may lead to changes is the mod graph of the module, which may require additional --overide_module=, i assume we have to run this step in a loop till we are stable.
There was a problem hiding this comment.
Fixed. Comment can be resolved.
| def override_rc(paths: dict[str, Path]) -> str: | ||
| return "".join(f"common --override_module={name}={paths[name]}\n" for name in sorted(paths)) |
There was a problem hiding this comment.
👍 usage of --override_module simplifies the approach drastically, thanks for finding this.
| A repo that fails to fetch (e.g. a dead mirror of a module no build ever touches) is just | ||
| missing from the result instead of failing the run. | ||
| """ | ||
| bazel(["fetch", "--keep_going", "--lockfile_mode=error", *(f"--repo=@@{n}+" for n in sorted(names))]) |
There was a problem hiding this comment.
👍 Using bazel fetch from reference_integration context also has the benefit that git_override / singe_version_override patches from known_good.json are applied automatically, again nice simplification of the approach!
Please add this effect to the docstring of fetch().
Should we guard agains local_path overrides?
There was a problem hiding this comment.
Fixed. Comment can be resolved.
| work = args.work_dir.resolve() | ||
| workspace, out = work / module.name, work / "reports" / module.name | ||
| shutil.rmtree(workspace, ignore_errors=True) | ||
| shutil.rmtree(out, ignore_errors=True) | ||
| out.mkdir(parents=True) | ||
| source = fetch({module.name}).get(module.name) | ||
| if source is None: | ||
| sys.exit(f"could not fetch {module.name} in ref_int") | ||
| shutil.copytree(source, workspace, symlinks=True) | ||
| shutil.copy(UNIFIED_TOOLCHAINS, workspace) | ||
| module_file = workspace / "MODULE.bazel" | ||
| module_file.chmod(0o644) # read-only when copied from Bazel's repo cache | ||
| with module_file.open("a") as f: | ||
| f.write(f'\ninclude("//:{UNIFIED_TOOLCHAINS.name}")\n') |
There was a problem hiding this comment.
nitpick (optional)
tiny refactor would further increase already good readability of the script:
_setup_working_dir(...)
_copy_fetch_module(...)
_append_unified_toolchain(...)
similar changes could be done for:
_collect_coverage(...)
_write_summary(...)
There was a problem hiding this comment.
Fixed. Comment can be resolved.
| shutil.copy(UNIFIED_TOOLCHAINS, workspace) | ||
| module_file = workspace / "MODULE.bazel" | ||
| module_file.chmod(0o644) # read-only when copied from Bazel's repo cache | ||
| with module_file.open("a") as f: |
There was a problem hiding this comment.
👍 again nice solution for ensuring identical toolchain configurations.
| env = { | ||
| **os.environ, | ||
| "PATH": f"{wrapper.parent}{os.pathsep}{os.environ['PATH']}", | ||
| "USE_BAZEL_VERSION": (REF_INT / ".bazelversion").read_text().strip(), |
There was a problem hiding this comment.
Please extend comment stating that USE_BAZEL_VERSION overrules the modules .bazelversion file.
There was a problem hiding this comment.
optional, change not required.
|
|
||
| shared = direct_deps(mod_graph(workspace, env)) & resolved | ||
| pinned = fetch(shared) | ||
| rc.write_text(override_rc(pinned)) |
There was a problem hiding this comment.
Should we not also write the extra_test_config from known_good.json here?
There was a problem hiding this comment.
Fixed. Comment can be resolved.
| startup = f"--output_base={work / 'output_base' / module.name} --bazelrc={UNIFIED_RC} --bazelrc={rc}" | ||
| wrapper.write_text(f'#!/bin/sh\nexec {shutil.which("bazel")} {startup} "$@"\n') |
There was a problem hiding this comment.
| startup = f"--output_base={work / 'output_base' / module.name} --bazelrc={UNIFIED_RC} --bazelrc={rc}" | |
| wrapper.write_text(f'#!/bin/sh\nexec {shutil.which("bazel")} {startup} "$@"\n') | |
| startup = [f"--output_base={work / 'output_base' / module.name}", f"--bazelrc={UNIFIED_RC}", f"--bazelrc={rc}"] | |
| wrapper.write_text(f'#!/bin/sh\nexec {shlex.quote(shutil.which("bazel"))} {shlex.join(startup)} "$@"\n') |
There was a problem hiding this comment.
Fixed. Comment can be resolved.
| found = {} | ||
| for name in names: | ||
| # ponytail: "<name>+" is bzlmod's canonical name; a few built-ins (platforms) have none. | ||
| for repo in (external / f"{name}+", external / name): |
There was a problem hiding this comment.
Please document that multiple_version_override is excluded for now.
I also think this is fine, we can react once it is required.
There was a problem hiding this comment.
Fixed. Comment can be resolved.
docs-as-code bump (#400) drops score_devcontainer from the graph, so the yanked-version override is obsolete and removed. Lockfile regenerated.
OliverHeilwagen
left a comment
There was a problem hiding this comment.
No findings remaining.
test_and_docs workflow issue should be resolved once PR is merged (pull_request_target).
build_and_test_qnx and _autosd issue was not investigated but appears to be unrelated to this PR.
DEMO
Demo PR exists in private fork, as to demonstrate it we need to push workflow to main branch first.
PiotrKorkus#5
Overview
What changed. ref_int no longer runs every module's unit tests inside its own workspace. The old way used quality_runners.py unit tests, the generated rust_coverage/BUILD, and resolved_dependencies.py / module_patches.py to inject modules into MODULE.bazel. Now each target_sw module runs its own tests in its own workspace, but against the dependency versions ref_int resolves.
Flow
What module_tests.py run does (scripts/module_tests.py:281)
Shared config
Running it locally