Build against Determinate Secure Packages#288
Open
edolstra wants to merge 7 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds configurable ChangesBuild Workflow Parameterization
Test Harness and Docs
Sequence DiagramsequenceDiagram
participant GitHubActions as GitHubActions
participant BuildWF as .github/workflows/build.yml
participant SecureFlake as packaging/secure-packages/flake.nix
GitHubActions->>BuildWF: workflow_call(inputs.flake, inputs.upload_artifacts)
BuildWF->>SecureFlake: nix build / nix flake check using inputs.flake#target
BuildWF->>GitHubActions: upload artifacts (if inputs.upload_artifacts)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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
![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
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)
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/build.yml (1)
135-140:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnumerate test derivations from the selected flake, not implicit
..
nix flake show --json(Line 135) currently inspects the checkout root flake, while Line 140 builds from${{ inputs.flake }}. If those differ, the key set can drift and cause incorrect/missing builds.Proposed fix
- $(nix flake show --json \ + $(nix flake show --json "${{ inputs.flake }}" \🤖 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/build.yml around lines 135 - 140, The pipeline that enumerates test derivations calls `nix flake show --json` without an explicit flake, which inspects the checkout root instead of the selected flake; change the command to target the provided flake by invoking `nix flake show --json "${{ inputs.flake }}"` (preserve the existing jq pipeline that selects `.hydraJobs.tests` and the string prefix `"${{ inputs.flake }}`#hydraJobs.tests`."`) so the key set is derived from the same flake that is later built.
🤖 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.
Inline comments:
In @.github/workflows/build.yml:
- Line 67: Replace the incorrect repository-root flake target in the Nix build
step: in the run command that builds "packages.${{ inputs.system }}.default
.#packages.${{ inputs.system }}.binaryTarball --no-link -L", change the second
target from ".#packages.${{ inputs.system }}.binaryTarball" to "${{ inputs.flake
}}`#packages`.${{ inputs.system }}.binaryTarball" so both targets consistently
reference the selected input flake.
- Around line 4-7: The workflow treats inputs.flake as an unvalidated free-form
string and mixes root `.` and `${{ inputs.flake }}` in nix commands; fix by (1)
constraining/sanitizing and always quoting `${{ inputs.flake }}` before use
(e.g., validate it matches an allowlist or a safe regex such as starting with
./, github:, or a known flake URI) and fail early if validation fails, (2)
replace any occurrences of the root `.` mixed with a flake expression so all
`nix build`/`nix flake` invocations consistently use the `${{ inputs.flake }}`
interpolation (fix the mixed `${{ inputs.flake }}#...default` vs
`.#...binaryTarball` usage), and (3) ensure `nix flake show` in the vm_tests_all
flow is invoked with `${{ inputs.flake }}` so enumeration matches the subsequent
build targets; update the run steps that call `nix build`/`nix flake`/`nix flake
show` to use the quoted, validated `${{ inputs.flake }}` variable.
---
Outside diff comments:
In @.github/workflows/build.yml:
- Around line 135-140: The pipeline that enumerates test derivations calls `nix
flake show --json` without an explicit flake, which inspects the checkout root
instead of the selected flake; change the command to target the provided flake
by invoking `nix flake show --json "${{ inputs.flake }}"` (preserve the existing
jq pipeline that selects `.hydraJobs.tests` and the string prefix `"${{
inputs.flake }}`#hydraJobs.tests`."`) so the key set is derived from the same
flake that is later built.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3c25661f-10c9-4d3b-a291-2ec3127416fb
⛔ Files ignored due to path filters (1)
packaging/secure-packages/flake.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
.github/workflows/build.yml.github/workflows/ci.ymldoc/manual/package.nixdoc/manual/source/release-notes/rl-2.34.mdpackaging/secure-packages/flake.nixsrc/libstore/include/nix/store/local-store.hhsrc/nix/unix/store-roots-daemon.mdtests/functional/json.sh
✅ Files skipped from review due to trivial changes (3)
- doc/manual/source/release-notes/rl-2.34.md
- src/nix/unix/store-roots-daemon.md
- packaging/secure-packages/flake.nix
🚧 Files skipped from review as they are similar to previous changes (3)
- src/libstore/include/nix/store/local-store.hh
- .github/workflows/ci.yml
- doc/manual/package.nix
pre-commit-run was failing with: > Running phase: unpackPhase > unpacking source archive /nix/store/57ks819h3nia029gr6r31s30jc1bn4aj-source/packaging/secure-packages/../.. > Cannot copy /nix/store/57ks819h3nia029gr6r31s30jc1bn4aj-source/packaging/secure-packages/../.. to ..: destination already exists! > Did you specify two "srcs" with the same "name"? > do not know how to unpack source archive /nix/store/57ks819h3nia029gr6r31s30jc1bn4aj-source/packaging/secure-packages/../.. because it gets confused by the `../..`. Work around that.
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