Build against Determinate Secure Packages#288
Open
edolstra wants to merge 4 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds configurable ChangesBuild Workflow Parameterization
Test Harness and Docs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
edolstra
force-pushed
the
secure-packages
branch
3 times, most recently
from
565ec3c to
d26924f
Compare
edolstra
force-pushed
the
secure-packages
branch
from
d26924f to
3a04f1c
Compare
edolstra
force-pushed
the
secure-packages
branch
2 times, most recently
from
a7a1451 to
9477ee6
Compare
edolstra
changed the title
edolstra
force-pushed
the
secure-packages
branch
2 times, most recently
from
5fd0ff0 to
86c7d62
Compare
util-linux 2.42 (commit 7268e79b) added "+" to the getopt string of script(1), so option parsing stops at the first non-option argument. The previous `script -e -q /dev/null -c CMD` ordering therefore treats `-c CMD` as extra positional arguments and fails with "unexpected number of arguments". Place all options before the <file> argument, which works on both old and new util-linux. (cherry picked from commit 8b974a3)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)
99-117:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd
build_x86_64-linux_no_dspto the success job's needs list.The
build_x86_64-linux_no_dspjob is excluded from thesuccessjob'sneedsarray, even though it runs onmerge_groupevents and performs comprehensive testing (including VM and regression tests). If this job fails during a merge, the overall success status won't reflect that failure. All other build jobs are included in the needs list—this one should be too.success: runs-on: ubuntu-latest needs: - eval - build_x86_64-linux - build_x86_64-linux_no_dsp - build_aarch64-linux - build_aarch64-darwinGitHub Actions handles conditionally-skipped jobs gracefully: when
build_x86_64-linux_no_dspis skipped on non-merge_group events, it won't block the success job.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 99 - 117, Update the "success" job's needs array to include the missing job name build_x86_64-linux_no_dsp so the job depends on it like the other build jobs; specifically modify the needs list inside the success job (the block labeled success and its needs key) to add - build_x86_64-linux_no_dsp alongside eval, build_x86_64-linux, build_aarch64-linux, and build_aarch64-darwin so failures in build_x86_64-linux_no_dsp are considered by the success job.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 99-117: Update the "success" job's needs array to include the
missing job name build_x86_64-linux_no_dsp so the job depends on it like the
other build jobs; specifically modify the needs list inside the success job (the
block labeled success and its needs key) to add - build_x86_64-linux_no_dsp
alongside eval, build_x86_64-linux, build_aarch64-linux, and
build_aarch64-darwin so failures in build_x86_64-linux_no_dsp are considered by
the success job.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fc869b6f-8797-42a0-91e3-ae8e1ab00e92
⛔ Files ignored due to path filters (1)
packaging/secure-packages/flake.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
.github/workflows/build.yml.github/workflows/ci.ymlpackaging/secure-packages/flake.nixtests/functional/json.sh
Claude made this. I don't know what's going on with print.html and I don't care.
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
2 participants
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.
Motivation
The top-level flake still builds against upstream Nixpkgs. But
packaging/secure-packages/flake.nixoverrides that to use DSP.Context
Summary by CodeRabbit
Chores
Tests
Documentation