DR-008 Option 4: two-stage test-execution workflow (PR 2 of 2) - #280
DR-008 Option 4: two-stage test-execution workflow (PR 2 of 2)#280Subramanian-K812 wants to merge 12 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
| @@ -0,0 +1,221 @@ | |||
| # Stage 2 — Constraint Sheet | |||
|
|
|||
| Living document. Amend it when a constraint turns out to be wrong, and record why in the | |||
There was a problem hiding this comment.
If thats AI generated and maintained, where is a skill / agent for that purpose?
There was a problem hiding this comment.
where is this file generated from? All modules should be from known_good
There was a problem hiding this comment.
not generated — known_good.json pins first-party repos by commit, these are registry deps pinned by version (confirmed via bazel mod graph --verbose: 2 of the 3 pins do move MVS's selection)
| # 0.9.1 declares the four *_rules_rust_miri toolchains score_lifecycle_health and | ||
| # score_persistency register in their own .bazelrc; 0.8.0 has none. ref_int cannot un-register | ||
| # them, so the version must match here. Locked in score_test_artifact_versions.MODULE.bazel. |
| - name: Set up Python 3 | ||
| uses: actions/setup-python@v5 | ||
| with: | ||
| python-version: '3.12' |
|
|
||
| # One label for every Python unit test, so CI runs all of them by naming a single target. | ||
| test_suite( | ||
| name = "python_tests", |
There was a problem hiding this comment.
change name, its meaningless without looking into target
There was a problem hiding this comment.
Renamed to all_python_unit_tests.
| push: | ||
| branches: | ||
| - main | ||
| - Subramanian-K812_test_execution_upgrade # TEMP: remove after CI verified |
There was a problem hiding this comment.
it doesnt work because the same workflow name is with pull_request_target on main
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Clean disk space | ||
| uses: eclipse-score/more-disk-space@v1 |
There was a problem hiding this comment.
Bumped to v1.1
| if [ ! -s _module/module_graph.json ]; then | ||
| echo "::warning::no module graph captured for ${{ matrix.module.name }}"; exit 0 | ||
| fi | ||
| PYTHONPATH=scripts python3 scripts/known_good/verify_stage2_resolution.py \ |
There was a problem hiding this comment.
if script needs a PYTHONPATH to be set its very bad design. Fix with proper modules imports
| "bazel_config": [ | ||
| "stage2-gcc", | ||
| "stage2-rust" |
There was a problem hiding this comment.
it feels like it doesnt need to be placed in this json as its common for every module
There was a problem hiding this comment.
score_time and score_communication are exceptions for the above.
| "//score/mw/com/impl/bindings/lola/tracing:tracing_runtime_test" | ||
| ], | ||
| "bazel_config": [ | ||
| "stage2-rust" |
There was a problem hiding this comment.
why com has only rust?
There was a problem hiding this comment.
score_communication passes use_base_constraints_only=True to its own gcc.toolchain(), which changes its generated toolchain target name leading to referencing a target that doesn't exist.
|
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.
this cannot be removed
| "//feature_integration_tests/configs:qemu_bridge_config.json", | ||
| "//images/qnx_x86_64:image", | ||
| ], | ||
| }) + ["conftest.py"], |
There was a problem hiding this comment.
every job has continue-on-error: true
this means that the workflow can never go red. kind of a no-op as PR check
There was a problem hiding this comment.
Removed from all four jobs — the workflow now goes red when Stage 1 or Stage 2 fails.
| # Do not flood CI with unneeded previous runs in PR | ||
| concurrency: | ||
| group: ${{ github.workflow }}-${{ github.event.pull_request.number }} | ||
| cancel-in-progress: ${{ github.ref_name != 'main' && !startsWith(github.ref_name, 'release/') }} |
There was a problem hiding this comment.
github.ref_name is the base branch / main so this evaluates to false for PRs -> old runs never get cancelled.
can you check?
There was a problem hiding this comment.
Fixed — cancel-in-progress no longer reads ref_name.
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.
| resolved: dict[str, Module] = {} | ||
| provenance: dict[str, str] = {} | ||
| declared_overrides: dict[str, dict[str, str]] = {} | ||
| declared_deps: dict[str, dict[str, object]] = {} |
There was a problem hiding this comment.
It would be nice to have more descriptive data classes or enum classes to improve the readability.
May also be taken up as a follow up cleanup.
e.g.:
provenance:
class PinProvenance(StrEnum):
ASSERTED = "asserted"
INCIDENTAL = "incidental"
provenance: dict[str, PinProvenance] = {}
declared_overrides:
@dataclass(frozen=True)
class UncarriedOverride:
module_name: str
kind: str
source_file: str
reason: str
consequence: str
def to_dict(self) -> dict[str, str]:
return {
"module": self.module_name,
"kind": self.kind,
"file": self.source_file,
"reason": self.reason,
"consequence": self.consequence,
}
declared_overrides: dict[str, UncarriedOverride] = {}
declared_deps:
@dataclass(frozen=True)
class DependencySpec:
module_name: str
version: str | None
dev_dependency: bool
source_file: str
declared_deps: dict[str, DependencySpec] = {}
There was a problem hiding this comment.
Agreed — follow-up PR, keeping this one scoped to the patch-transport fix.
|
|
||
| # The completeness guard: every override ref_int declared that did not become a manifest entry. | ||
| uncarried = [entry for name, entry in sorted(declared_overrides.items()) if name not in resolved] | ||
| report = _build_report(resolved, provenance, uncarried, declared_by, _internal_drift(resolved, declared_deps)) |
There was a problem hiding this comment.
as mentioned above report would also benefit from a dataclass approach e.g.:
@dataclass(frozen=True)
class PinReport:
pin: dict[str, str]
provenance: PinProvenance
declared_versions: list[str]
verdict: Literal["unknown", "by_commit", "agree", "differs"] = "unknown"
direction: Literal["ref_int_lower", "ref_int_higher"] | None = None
def to_dict(self) -> dict[str, object]:
return {
"pin": self.pin,
"provenance": self.provenance.value,
"declared_versions": self.declared_versions,
"verdict": self.verdict,
"direction": self.direction,
}
There was a problem hiding this comment.
Agreed — taking these as a follow-up PR, since this one already reworks the same file for patch transport.
| if entry["verdict"] == "differs": | ||
| conflicts.append(name) |
There was a problem hiding this comment.
conflicts appears to be a too strong term for the report entry.
I would expect ref_int_lower or ref_int_higher a typical case.
Can we stick to differs instead, this would also be more self-explanatory.
There was a problem hiding this comment.
Renamed to differs, matching the per-pin verdicts.
| return cls({name: Module.from_dict(name, md) for name, md in entries.items()}) | ||
|
|
||
| @staticmethod | ||
| def _parse_override_file(text: str) -> list[Module]: |
There was a problem hiding this comment.
The existing known_good.json makes use of patches (git_override or local_override).
Please also collect the patches so we are capable of injecting them in stage 2.
Related comment is in quality_runner.py
We apply the pachtes locally but the override mechanism is loosing them for dependencies since we use git_override.
There was a problem hiding this comment.
Done — _parse_override_file now collects patches/patch_strip
| """ | ||
| module_bazel = Path(module_bazel) | ||
| original = self._strip_injection(module_bazel.read_text()) | ||
|
|
There was a problem hiding this comment.
Please add a warning generation in case the module is using:
- *_override
- highlight patches?
- non bazel version resolution dependency
- http_archive
- http_file
- git_repository
- new_git_repository
- local_repository
Overrides / Patches used by the module may conflict with the version imposed by reference integration.
Ideally we should do such a scan as a initial sanity check if the versions of known_good.json are updated.
If a selected module contain such elements it may cause build / test conflicts, being aware early could reduce time for issue identification.
There was a problem hiding this comment.
Added module_resolution_hazards() — warns on a module's own *_override and on http_archive-style fetches.
| raise SystemExit(f"{args.modules_to_test[0]} is not a target_sw module in {args.known_good_path}") | ||
| print_centered(f"QR: Applying {module_under_test.name}'s own bazel_patches to {workspace}") | ||
| try: | ||
| apply_module_patches(module_under_test, workspace.resolve(), REF_INT_ROOT, log=print_centered) |
There was a problem hiding this comment.
patches are applied locally but the resolved.overwrite() patches are using single_version_override or git_override which leads to missing patches for dependencies in the stage 2 module run.
Ideally we can transport a patch of a dependency into the overwrite patching functionality.
There was a problem hiding this comment.
Fixed — Stage 1 now ships the patch bytes alongside the resolved dependency set, and overwrite() re-hosts them inside the checkout as //ref_int_patches:..., so injected dependencies build patched instead of losing their patches.
| contents: write | ||
| pull-requests: write | ||
| on: | ||
| pull_request_target: # Allow forks to access secrets |
There was a problem hiding this comment.
This should only be required once we extend to QNX build and test in this workflow.
Since we use --config=linux-x86_64 shouldn't the following be sufficient?
Then we can also check the run in this PR.
| pull_request_target: # Allow forks to access secrets | |
| pull_request: |
There was a problem hiding this comment.
Agreed, pull_request_target should not be used anywhere except where strictly needed.
There was a problem hiding this comment.
Switched — PR now exercises its own workflow.
| return tuple(int(part) for part in version.split(".")) if _RELEASE_RE.fullmatch(version) else None | ||
|
|
||
|
|
||
| def resolve_stage2_bazel_version(ref_int_version: str, module_version: str | None) -> str: |
There was a problem hiding this comment.
Why don't we demand a .bazelversion to be present under root of the module?
This way we could simplify the call and just give the root path of ref_int and module the module.
We could then also have the bazel version patching logic in this python file.
There was a problem hiding this comment.
bazel_version.py is removed; that logic now lives in quality_runners.py and always imposes ref_int's version.
| - name: Export resolved dependency manifest | ||
| if: always() | ||
| run: | | ||
| mkdir -p artifacts/stage1-resolved-deps |
There was a problem hiding this comment.
Please provide all the steps also in a README for local replication of the workflow.
There was a problem hiding this comment.
Added ci/README.md covering every step.
| actual = module_graph_versions(args.mod_graph) | ||
| resolved = json.loads(args.resolved.read_text(encoding="utf-8")) | ||
| resolved = resolved.get("modules", resolved) |
There was a problem hiding this comment.
since we base the check upon the bazel mod graph of the module and the resolved_version.json which both do not contain information about patches demanded by reference integration, the check is incomplete.
There was a problem hiding this comment.
Complete now — the manifest carries patches, and missing_patches() fails the check when an injected dep lost them.
|
@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.
| contents: write | ||
| pull-requests: write | ||
| on: | ||
| pull_request_target: # Allow forks to access secrets |
There was a problem hiding this comment.
Agreed, pull_request_target should not be used anywhere except where strictly needed.
| --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?
| 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?
| # 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.
|
|
||
| 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?
| 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?
| 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?
| _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?
| @@ -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
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?
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).