Repository navigation
Reproducible Builds + GitHub Merge Queue Optimizations - #429
Open
wchilders-nvidia wants to merge 2 commits into
Open
wchilders-nvidia wants to merge 2 commits into
wchilders-nvidia wants to merge 2 commits into
Conversation
…#426] This commit also: - updates the copyright notice in the version command to credit contributors - and removes the Edison Design Group-specific DEMO_VERSION_ID configuration option.
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Build identifier and reported version src/CMakeLists.txt, src/version.h, src/cmd_line.c, src/pch.c, doc/source/build.rst, src/pending_changes/*, src/host_envir.h |
CMake accepts EDG_BUILD_IDENTIFIER. Version and configuration output and PCH identification use build_stamp; the build documentation and release entry describe the identifier and related output changes. |
Deterministic archives and generated names lib_src/CMakeLists.txt, lib_src/cmake/lib_build_support.cmake, src/il_to_str.c |
Linux archive commands use deterministic options. Runtime-library builds retain generated C files in the object directory. Backend temporary names use per-file sequential IDs. |
Updated compiler diagnostic expectations tests/tests/{cwg,edg,imported}/... |
Expected-output fixtures use concrete generated temporary identifiers and updated diagnostic locations across C and C++ tests. |
CI workflow updates
| Layer / File(s) | Summary |
|---|---|
Workflow triggers and commit validation .github/workflows/commit-messages.yml, .github/workflows/dev-tools-tests.yml, .github/workflows/strict-build-check.yml, .github/workflows/typecheck-python.yml, .github/workflows/build-and-test-ci.yml |
Commit-message validation uses pull-request or merge-queue ranges. The affected workflows remove or broaden push triggers and filters. |
CI inputs and runner environment .github/workflows/build-and-test-ci.yml, .github/workflows/strict-build-check.yml, dev_tools/docker/ci-runner/setup-scripts/install-software.sh |
The prepare job hashes test inputs. CI jobs use updated runner images, debug builds set EDG_BUILD_IDENTIFIER=ci, and the runner image installs zstd. |
Test-result and coverage caching .github/workflows/build-and-test-ci.yml |
The GCC and i686 test jobs key cache records using configuration, test-input, and build-product hashes. Cache hits restore coverage and skip testing. Successful uncached runs save coverage and run metadata. |
Priority:
Estimated code review effort: 4 (Complex) | ~45 minutes
Change: Feature
Sequence Diagram(s)
sequenceDiagram
participant PrepareJob
participant TestJob
participant ActionsCache
participant Edgy
participant CoverageReport
PrepareJob->>TestJob: provide test_inputs hash
TestJob->>ActionsCache: look up test record
ActionsCache-->>TestJob: restore coverage or report cache miss
alt cache miss
TestJob->>Edgy: run tests
Edgy-->>TestJob: test result
TestJob->>CoverageReport: generate coverage JSON
TestJob->>ActionsCache: save coverage and run metadata
else cache hit
ActionsCache-->>TestJob: restore coverage JSON
end
Possibly related issues
- Premerge check of commit messages #164 — The PR changes commit-message validation to run for pull requests and merge-queue entries.
- Reproducible Builds #426 — The PR adds a build identifier and deterministic generated-name and archive settings.
Possibly related PRs
- edgcpp/compiler#394 — It adds CI workflows that this PR updates with merge-queue handling and revised push triggers.
Suggested labels: infrastructure
Suggested reviewers: nv-cmeerw, darkarc
Merge Risk: 
17c95
-
No concrete merge-blocking regression is established by the supplied evidence; the change is mergeable after normal checks.
Security architecture risk: 
17c95
-
The inspected changes do not establish an introduced security exploit or privilege expansion. Cache keys bind results to build products and test inputs, and normal cache creation requires successful tests. Residual uncertainty concerns cache-service isolation and enforcement of the merge-queue-only workflow.
Retained concerns
No architecture-level concerns identified.Security review details
-
Security Blast Radius
- inferred — The new security-relevant shared state is repository CI pass records and coverage. Their integrity affects whether PR and merge-group test legs execute. The inspected workflow retains contents-read permission; it does not add a repository-write permission.
Trust Boundaries and Controls
- observed — The reader treats an exact cache hit as sufficient to skip tests. It logs cached metadata without validating its schema and tolerates missing coverage copies. The normal writer supplies both metadata and coverage, so this asymmetry alone does not establish poisoning or an attacker-controlled bypass. Cross-ref authority isolation is delegated to the cache service.
Hardening Proposals
- proposed — Validate restored record structure and configuration-specific coverage before skipping tests, falling back to execution if validation fails. If input identity is stored in the record, validate the digest rather than requiring the current commit SHA, preserving intentional reuse across equivalent commits.
-
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely identifies the two main change areas: reproducible builds and GitHub merge queue CI optimizations. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 3 functions across 4 files. (51 skipped: 50 unsupported, 1 too large.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
| build date and time in the --version and --dump_configuration output and | ||
| in the precompiled header identification string, so rebuilding the same | ||
| sources produces identical executables. The runtime library is likewise | ||
| reproducible: its archives are deterministic, and its generated C files no |
Contributor
There was a problem hiding this comment.
Should lib_src/pending_changes get a similar entry?
Member
Author
There was a problem hiding this comment.
Hm... if we want to note the changes to the build files maybe?
| @@ -8580,48 +8605,40 @@ STATIC_THREAD a_ptr_unique_id_map | |||
| *ptr_to_unique_id; | |||
Contributor
There was a problem hiding this comment.
This is pre-existing, but:
ptr_to_unique_idsounds like this is "a pointer to a unique id";unique_id_mapwould be clearer, IMO.- If we're going to use this in earnest (as opposed to just in DEBUG builds), maybe it should be part of
an_il_to_str_output_control_block?
Member
Author
There was a problem hiding this comment.
RE 1:
It still is a (map of) pointers to unique IDs ... so I think the existing name is fine (I prefer it personally to just unique_id_map). A counter-proposal would be perhaps calling it temporary_to_id?
That also removes the slight UUID implication/suggestion.
RE 2:
It's a good question ... that would surface it more visibly, but I'm not sure that buys us anything. I think I'd prefer to keep this an implementation detail unless anyone asks for that to be controllable by an_il_to_str_output_control_block.
WDYT?
Collaborator
|
AcknowlEDG review results updated for
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Collaborator
|
AcknowlEDG review results updated for
CI failure. |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR:
--versionoutput for the open source project,In terms of CI optimizations:
Summary by CodeRabbit
Build Improvements
Version Information
Documentation