DR-008 Option 4: two-stage test-execution workflow (PR 2 of 2) - #280
Subramanian-K812 wants to merge 15 commits into
Conversation
c9462ea to
fd3df76
Compare
d293277 to
b6df34d
Compare
e51b340 to
640dfee
Compare
|
The created documentation from the pull request is available at: docu-html |
04a7a73 to
e816620
Compare
PiotrKorkus
left a comment
PiotrKorkus
left a comment
There was a problem hiding this comment.
as the workflow has been completely reworked please create a private fork and execute it there, so we can see the proof of working in CI
pull_request_target wont allow to execute it here
|
Ran the reworked workflow end-to-end on a private fork: |
b22c037 to
a992474
Compare
587dcbb to
35d83cd
Compare
There was a problem hiding this comment.
review focus ./scripts/known_good, excluding tests.
Transitive dependency are now correctly pinned 👍 .
Patches from reference integration are locally applied but patching in Stage 2 uses single_version_override and git_override without patches, which leads to missing patches for module dependencies, which were present in Stage 1.
|
@Subramanian-K812 Thanks a lot for the quick rework 👍 . Unfortunately I am not at work till 09.09., once I am back I can continue the re-review. |
MaximilianSoerenPollak
left a comment
MaximilianSoerenPollak
left a comment
There was a problem hiding this comment.
Some questions from my side.
But overall I think it does what was asked according to the DR.
| --stage1-result "${{ needs.stage1_integration.result }}" \ | ||
| --stage2-result "${{ needs.stage2_module_validation.result }}" \ | ||
| --stage2-dir "_stage2_reports/" \ | ||
| >> "$GITHUB_STEP_SUMMARY" |
There was a problem hiding this comment.
Is there an output lenght limit for the step summary?
If there is, will this hit it or is there no risk of that?
There was a problem hiding this comment.
Checked against GitHub's actual limit (1 MiB per step, upload fails with an annotation if exceeded) — a full 8-module report from the real script is ~4.3 KB, so this should not reach the cap.
| still points at the old @score_baselibs labels, so redirect them and declare the | ||
| score_communication dependency they now require. | ||
|
|
||
| Source labels only; the module's .bazelrc has the same stale labels but ref_int no longer patches it. |
There was a problem hiding this comment.
But if it's stale labels should they not be removed?
There was a problem hiding this comment.
We could, but that would require patching the module’s .bazelrc, which has already been ruled out for this PR.
| # scripts/ on sys.path, so "from known_good...." resolves under 'bazel run' as it does for | ||
| # "python3 scripts/...". Propagates to every consumer of this library. | ||
| imports = [".."], |
There was a problem hiding this comment.
Are you sure this does not break anything else?
Importing .. can have unexpected consequences in consumers of this py_library.
There was a problem hiding this comment.
Yes agreed — better to scope or drop imports than leave it shared.
|
|
||
| def repo_slug(repo_url: str) -> str: | ||
| """Derive the 'owner/name' slug actions/checkout expects from a git URL.""" | ||
| match = re.search(r"[:/]([^/:]+/[^/:]+?)(?:\.git)?/?$", repo_url or "") |
There was a problem hiding this comment.
Very sure this regex does more and less than what it is suppose to do (as all regex).
But I guess it's okay for now?
There was a problem hiding this comment.
This to be improved — switching to urllib.parse for the path url in known_good.json.
| try: | ||
| from known_good.models.known_good import load_known_good | ||
| from known_good.resolved_dependencies import repo_root, workspace_path | ||
| except ImportError: | ||
| if str(_HERE) not in sys.path: | ||
| sys.path.insert(0, str(_HERE)) | ||
| from models.known_good import load_known_good # noqa: E402 | ||
| from resolved_dependencies import repo_root, workspace_path # noqa: E402 |
There was a problem hiding this comment.
Why is this needed again?
There was a problem hiding this comment.
Agreed — same as noted below, the current try/except isn’t ideal and will be improved in a follow‑up.
| if module.repo in repo_commit_dict: | ||
| commit = repo_commit_dict[module.repo] | ||
|
|
||
| # Generate patches lines if bazel_patches exist | ||
| patches_lines = "" | ||
| if module.bazel_patches: | ||
| patches_lines = " patches = [\n" | ||
| for patch in module.bazel_patches: | ||
| patches_lines += f' "{patch}",\n' | ||
| patches_lines += " ],\n" | ||
| patch_strip_line = " patch_strip = 1,\n" if patches_lines else "" | ||
|
|
||
| if module.version: | ||
| # If version is provided, use bazel_dep with single_version_override | ||
| block = ( | ||
| f'bazel_dep(name = "{module.name}")\n' | ||
| "single_version_override(\n" | ||
| f' module_name = "{module.name}",\n' | ||
| f"{patch_strip_line}" | ||
| f"{patches_lines}" | ||
| f' version = "{module.version}",\n' | ||
| ")\n" | ||
| ) | ||
| else: | ||
| if not module.repo or not commit: | ||
| logging.warning( | ||
| "Skipping module %s with missing repo or commit: repo=%s, commit=%s", | ||
| module.name, | ||
| module.repo, | ||
| commit, | ||
| ) | ||
| continue | ||
|
|
||
| # Validate commit hash format (7-40 hex characters) | ||
| if not re.match(r"^[a-fA-F0-9]{7,40}$", commit): | ||
| logging.warning( | ||
| "Skipping module %s with invalid commit hash: %s", | ||
| module.name, | ||
| commit, | ||
| ) | ||
| continue | ||
|
|
||
| # If no version, use bazel_dep with git_override | ||
| # Only include patch_strip if there are patches to apply | ||
| block = ( | ||
| f'bazel_dep(name = "{module.name}")\n' | ||
| "git_override(\n" | ||
| f' module_name = "{module.name}",\n' | ||
| f' commit = "{commit}",\n' | ||
| f"{patch_strip_line}" | ||
| f"{patches_lines}" | ||
| f' remote = "{module.repo}",\n' | ||
| ")\n" | ||
| ) | ||
| blocks.append(block) |
There was a problem hiding this comment.
This whole logic not needed anymore, or moved somewhere else?
There was a problem hiding this comment.
Moved, not removed — now imported from resolved_dependencies.generate_override_directive so Stage 1 and Stage 2 can't emit different git_override syntax.
| _HERE = Path(__file__).resolve().parent | ||
| try: | ||
| from known_good.module_patches import patch_relpath | ||
| from known_good.resolved_dependencies import ( | ||
| INJECTED_PATCHES_PKG, | ||
| _collect_resolved_versions, | ||
| injected_override_names, | ||
| workspace_path, | ||
| ) | ||
| except ImportError: | ||
| if str(_HERE) not in sys.path: | ||
| sys.path.insert(0, str(_HERE)) | ||
| from module_patches import patch_relpath # noqa: E402 | ||
| from resolved_dependencies import ( # noqa: E402 | ||
| INJECTED_PATCHES_PKG, | ||
| _collect_resolved_versions, | ||
| injected_override_names, | ||
| workspace_path, | ||
| ) |
There was a problem hiding this comment.
I'm sure there is a better way than to this try / except.
What are you trying to achieve with this again?
There was a problem hiding this comment.
Agreed — the current try/except isn’t ideal; it’s a leftover from the same pattern used in files that are still invoked directly, which don’t have py_binary targets, so those workflows call the .py files directly instead of through bazel run.
| @@ -0,0 +1,187 @@ | |||
| #!/usr/bin/env python3 | |||
There was a problem hiding this comment.
If this file runs in CI, be careful that we already had the issue (in docs-as-code) that we ran into the upper limit of a pipe buffer in the CI output (64kb) due to folding it with long prints.
Just an FYI
There was a problem hiding this comment.
Thanks for the heads-up, this file's output isn't piped into anything in CI — it runs as a plain bazel run step straight to the job log, so the pipe-buffer deadlock docs-as-code hit shouldn't be an issue here.
PiotrKorkus
left a comment
PiotrKorkus
left a comment
There was a problem hiding this comment.
- nested bazel invokes in
ferrocene_report.shcoming from the toolchain do not carry--noworkspace_rcwhich makes rust coverage fail - huge differences in number of executed tests. e.g. communication has 6500 test cases under
//score/...but here we run only 3800. Why do we have differences when we should execute from the score of tested repo just with common config and deps? - how about appending single line with import of common bazelrc to the one in tested module and using the config from it? Would that work or?
8b5251f to
891e471
Compare
OliverHeilwagen
left a comment
OliverHeilwagen
left a comment
There was a problem hiding this comment.
Approve with suggestions.
Tests for the scripts were not in scope of this review.
From my point this is a nice initial version, which we can further be refactored to simplify the test complexity for the scripts. (ISO26262 tool qualification depending on the classification level).
Toolchain configuration overwrites for stage 2 are not be expected from my side for this PR, should however be planned in the backlog.
Current failing stage 2 steps need to be resolved, but are related to integration details (e.g. upgrade of coverage tooling score_tooling -> score_coverage) and not caused by general points in modified | added scripts.
| "verdict": "unknown", | ||
| "direction": None, | ||
| } | ||
| if not wanted: |
There was a problem hiding this comment.
Please also take over the bazel_patches for a version pin.
if module.bazel_patches:
entry["pin"]["bazel_patches"] = module.bazel_patches
| ``dev_dependency`` is not the discriminator and is never read: it does not predict whether | ||
| ref_int resolved a dependency. ``score_baselibs`` at 0.2.9 declares 13 dev-only deps that | ||
| ref_int *has* resolved and therefore pins (``score_tooling``, ``score_docs_as_code``, | ||
| ``toolchains_llvm``, ...). A dependency is absent from the resolved set for an unrelated | ||
| reason: nothing in ref_int's own graph reaches it, or ref_int pins it with an override the | ||
| manifest cannot express (``rules_boost``, an ``archive_override``). | ||
|
|
||
| Scope is the module's declared deps plus ``closure()`` of the module and of each declared | ||
| dep. The closure is what makes the rule above safe rather than merely permissive: pinning | ||
| ``score_tooling`` without ``lobster``/``trlc`` aborts with ``module lobster@0.0.0 not found | ||
| in registries``, since those are non-registry and resolvable only via a root override. | ||
| ``graph`` is therefore required -- a caller that cannot supply the closure must not pin. |
There was a problem hiding this comment.
Please keep the description free of examples
|
|
||
| # After the module's competing overrides are stripped and before ref_int's are appended, | ||
| # what is left is the module's own resolution that survives injection. | ||
| hazards = module_resolution_hazards(original) |
There was a problem hiding this comment.
Thanks for adding this check.
Out of scope for this PR.
In general i would wish for such a sanity check in stage 0, especially if we want to increase the frequency updating reference_integration. We should be informed early about *_override, patches or other 'anti patterns' within modules, which may cause issues during integration.
Hopefully this can reduce the time to analyse issues when updating known_good.json.
| graph: DependencyGraph, | ||
| *, | ||
| module_under_test: str | None = None, | ||
| patch_source: Path | None = None, |
There was a problem hiding this comment.
Stage 2 bazel_dep patching is now from my point of view complete and covers all cases.
What out fo scope for this PR from my point of view is the toolchain related patches.
In stage 2 the toolchain configuration of the module is kept (e.g. QNX sdp version), which is incorrect from the big picture.
Since these updates are less frequent i don't see it as a high priority for now but the topic should be planned in the backlog.
| default=repo_root() / "known_good.json", | ||
| help="Path to known_good.json (default: repo-root known_good.json).", | ||
| ) | ||
| parser.add_argument("--group", default="target_sw", help="Module group to list (default: target_sw).") |
There was a problem hiding this comment.
I would recommend to add choices=("target_sw", "tooling"),
| if want is None: | ||
| by_commit += 1 # git_override'd: pinned by commit, not comparable by version | ||
| continue |
There was a problem hiding this comment.
Since MODULE.bazel is source of truth for git_overrides can we add this as a data input to complete the check?
It is a little paranoid but would be nice round trip verification.
| if not STAGE2_RC.is_file(): | ||
| raise SystemExit(f"Stage-2 centralized config not found at {STAGE2_RC}") | ||
| startup = stage2_startup_flags(args.modules_to_test[0]) | ||
| dedicated_rc = stage2_module_rc(args.modules_to_test[0]) | ||
| if args.modules_to_test[0] in MODULES_WITH_OWN_RC: | ||
| print_centered( | ||
| f"QR: Layering ref_int config {STAGE2_RC} over {args.modules_to_test[0]}'s own " | ||
| ".bazelrc (registers a libclang toolchain ref_int cannot supply)" | ||
| ) | ||
| elif dedicated_rc is not None: | ||
| print_centered( | ||
| f"QR: Using ref_int config {STAGE2_RC} plus {dedicated_rc}; the module's .bazelrc is not read" | ||
| ) | ||
| else: | ||
| print_centered(f"QR: Using ref_int config {STAGE2_RC} only; the module's .bazelrc is not read") |
There was a problem hiding this comment.
Optional for this PR:
It would be nice to refactor the existing functionality into more readable steps for the main().
- select_bazelrc
- apply_patches
- inject_resolved_dependencies
- update_module_lock (currently remove)
- pin_bazel_version
- ...
e.g. wrapper class that can also store data and does not need to transport it in main to the next method.
| call = ["bazel"] + (startup or []) + ["mod", "graph", "--output=json", "--lockfile_mode=update"] | ||
| result = run_command(call, cwd=str(workspace)) |
There was a problem hiding this comment.
Please dump directly into file or is there a reason to see the full module graph in the log?
| return ("integration conflict", named) if named else ("ref_int harness defect", []) | ||
|
|
||
|
|
||
| def run_unit_test_with_coverage( |
There was a problem hiding this comment.
Out of scope for this PR.
Would be nice to refactor the individual quality checks into their own classes | files, so we can import them in quality_runners and invoke them from main().
| build:ferrocene-coverage --@rules_rust//rust/settings:extra_exec_rustc_flag=-Clink-dead-code | ||
| build:ferrocene-coverage --@rules_rust//rust/settings:extra_exec_rustc_flag=-Ccodegen-units=1 | ||
| build:ferrocene-coverage --@rules_rust//rust/settings:extra_exec_rustc_flag=-Cdebuginfo=2 | ||
| test:ferrocene-coverage --run_under=@score_tooling//coverage:llvm_profile_wrapper |
There was a problem hiding this comment.
I guess we should already change this to score_coverage.
eclipse-score/score#3258
5 participants
DR-008 Option 4: two-stage test-execution workflow (PR 2 of 2)
Closes #264. This is the second of two PRs. It is stacked on PR 1
(
Subramanian-K812_resolve_override_mechanism, "resolved-dependency resolve + overridemechanism") — please review/merge that one first. Until PR 1 merges, this PR's base branch
is the PR-1 branch, so the diff shown here is only the test-execution delta.
What this PR does
Rewires the quality workflow to the DR-008 Option 4 flow, using the resolve + override
mechanism added in PR 1:
exports the resolved dependency set as the
stage1-resolved-depsartifact(
resolved_versions.json).known_good.json(target_sw) — never hardcoded.known_goodcommit, overridesits
MODULE.bazelwith the Stage-1 resolved set (PR 1's mechanism), and runs themodule's own unit tests + coverage as the Bazel root (
//...).release-tag test-report ZIP).
Files
Workflow & runners
.github/workflows/test_and_docs.yml— two-stage restructure (+ parallel docs build)scripts/quality_runners.py— module-context mode (--module-dir/--resolved-deps); callsResolvedDependencies.overwritescripts/aggregate_quality_report.py— new, consolidated reportscripts/known_good/list_modules.py— new, dynamic Stage-2 matrix fromknown_good.jsonscripts/integration_test.py— derive build targets fromknown_good.json(dropbuild_config.json)Resolved pins + their config ripple (kept together so the tree is self-consistent)
known_good.json— pin bumps (baselibs, lifecycle) + per-modulebazel_config+ lifecyclecode_root_path/extra_test_configbazel_common/score_basic_bazel.MODULE.bazel— flatbuffers bumpbazel_common/score_modules_target_sw.MODULE.bazel— regenerated (baselibs, lifecycle)MODULE.bazel.lock— updated lockrust_coverage/BUILD— lifecycle query//src/...→//score/...(layout moved by the bump)showcases/simple_lifecycle/BUILD— lifecycle bin path//src/...→//score/launch_manager.bazelrc— coverage atomic-gcov /-no-coverage; eb-aarch64 outline-atomics link fixKnown Stage 2 integration findings (surfaced, not fixed here)
The workflow surfaces two module-owned integration failures caused by the
baselibs bump (
score_loggingCheckSizeValidstaleuint8_tassumption;score_persistencyto_stringonstd::string_view).