wip: install nix on CI and run a build with it#2894
wip: install nix on CI and run a build with it#2894justus-camp-microsoft wants to merge 5 commits intomicrosoft:mainfrom
Conversation
justus-camp-microsoft
force-pushed
the
install_nix_and_ci_job
branch
from
66a1b83 to
8c09014
Compare
justus-camp-microsoft
force-pushed
the
install_nix_and_ci_job
branch
from
8c09014 to
18228b2
Compare
justus-camp-microsoft
force-pushed
the
install_nix_and_ci_job
branch
19 times, most recently
from
b22671c to
5e9066a
Compare
justus-camp-microsoft
force-pushed
the
install_nix_and_ci_job
branch
2 times, most recently
from
b834287 to
9fe01eb
Compare
justus-camp-microsoft
force-pushed
the
install_nix_and_ci_job
branch
from
39c4e95 to
171b029
Compare
| let first_diff = | ||
| bytes_a.iter().zip(bytes_b.iter()).position(|(a, b)| a != b); | ||
| anyhow::bail!( | ||
| "file {name} is not byte-identical \ | ||
| ({} vs {} bytes, first diff at offset {:?})", | ||
| bytes_a.len(), | ||
| bytes_b.len(), | ||
| first_diff, |
There was a problem hiding this comment.
When the files differ only by length (one is a strict prefix of the other), zip(...).position(...) returns None, so the error message reports “first diff at offset None”, which is confusing for diagnosing CI failures. Consider handling length-mismatch explicitly (e.g., report the first differing offset as min(len_a, len_b) when lengths differ) and/or computing the diff index over the full length.
| let first_diff = | |
| bytes_a.iter().zip(bytes_b.iter()).position(|(a, b)| a != b); | |
| anyhow::bail!( | |
| "file {name} is not byte-identical \ | |
| ({} vs {} bytes, first diff at offset {:?})", | |
| bytes_a.len(), | |
| bytes_b.len(), | |
| first_diff, | |
| let first_diff_offset = bytes_a | |
| .iter() | |
| .zip(bytes_b.iter()) | |
| .position(|(a, b)| a != b) | |
| .unwrap_or_else(|| bytes_a.len().min(bytes_b.len())); | |
| anyhow::bail!( | |
| "file {name} is not byte-identical \ | |
| ({} vs {} bytes, first diff at offset {})", | |
| bytes_a.len(), | |
| bytes_b.len(), | |
| first_diff_offset, |
| let is_installed = match ctx.backend() { | ||
| FlowBackend::Local => ctx.emit_rust_step("ensure nix is installed", |_ctx| { | ||
| |_rt| { | ||
| if which::which("nix-shell").is_err() { | ||
| anyhow::bail!( | ||
| "nix-shell not found on $PATH. \ | ||
| Please install Nix: https://nixos.org/download/" | ||
| ); | ||
| } | ||
| Ok(()) | ||
| } | ||
| }), | ||
| FlowBackend::Github | FlowBackend::Ado => { | ||
| // Step 1: Add nix profile bin to $PATH for subsequent CI | ||
| // steps. This runs before the install step so that when | ||
| // the next step starts (as a new process), nix-shell is | ||
| // already on $PATH. | ||
| let added_to_path = ctx.emit_rust_step("add nix profile to path", |ctx| { | ||
| let backend = ctx.backend(); | ||
| move |_rt| { | ||
| let nix_bin = home::home_dir() | ||
| .context("unable to get home directory")? | ||
| .join(".nix-profile/bin"); | ||
|
|
||
| match backend { | ||
| FlowBackend::Github => { | ||
| let gh_path_file = std::env::var("GITHUB_PATH")?; | ||
| let mut f = | ||
| fs_err::File::options().append(true).open(gh_path_file)?; | ||
| use std::io::Write; | ||
| writeln!(f, "{}", nix_bin.display())?; | ||
| log::info!("added {} to $GITHUB_PATH", nix_bin.display()); | ||
| } | ||
| FlowBackend::Ado => { | ||
| println!("##vso[task.prependpath]{}", nix_bin.display()); | ||
| log::info!("added {} to PATH (ADO)", nix_bin.display()); | ||
| } | ||
| FlowBackend::Local => unreachable!(), | ||
| } | ||
|
|
||
| Ok(()) | ||
| } | ||
| }); | ||
|
|
||
| ctx.emit_rust_step("install nix", |ctx| { | ||
| added_to_path.claim(ctx); | ||
| move |_rt| { | ||
| if which::which("nix-shell").is_ok() { | ||
| log::info!("nix-shell already available, skipping install"); | ||
| return Ok(()); | ||
| } | ||
|
|
||
| log::info!("installing Nix package manager..."); | ||
|
|
||
| // Single-user install (no daemon) — simplest for CI. | ||
| // https://nixos.org/download/ | ||
| #[expect(clippy::disallowed_methods, reason = "can't use the nix xshell wrapper if nix isn't installed yet")] | ||
| let sh = xshell::Shell::new()?; | ||
| #[expect(clippy::disallowed_macros, reason = "can't use the nix xshell wrapper if nix isn't installed yet")] | ||
| xshell::cmd!( | ||
| sh, | ||
| "sh -c 'curl --proto =https --tlsv1.2 -sSf -L https://nixos.org/nix/install | sh -s -- --no-daemon'" | ||
| ) | ||
| .run()?; | ||
|
|
||
| log::info!("nix installed successfully"); | ||
| Ok(()) | ||
| } | ||
| }) | ||
| } | ||
| }; |
There was a problem hiding this comment.
install_nix doesn’t check ctx.platform() / rt.platform() before attempting a curl-based install on CI backends. If this node is ever requested from a Windows or macOS job, it will fail in a non-obvious way. Consider explicitly gating this node to Linux platforms and returning a clear error message on unsupported platforms.
| Unix, | ||
| } | ||
|
|
||
| /// The kind platform the flow is being running on, Windows or Unix. |
There was a problem hiding this comment.
The doc comment above FlowPlatformLinuxDistro says “The kind platform … Windows or Unix”, but this enum represents the Linux distribution (Fedora/Ubuntu/Nix/…). Since this type is now serialized/deserialized and used for job selection, the comment should be corrected to avoid confusion for future maintainers.
| /// The kind platform the flow is being running on, Windows or Unix. | |
| /// The Linux distribution the flow is running on. |
| # Need unstable features on the stable toolchain | ||
| RUSTC_BOOTSTRAP = 1; | ||
| # Enable path trimming | ||
| CARGO_UNSTABLE_TRIM_PATHS = "true"; | ||
| CARGO_PROFILE_DEV_TRIM_PATHS = "object"; | ||
| CARGO_PROFILE_RELEASE_TRIM_PATHS = "object"; | ||
| CARGO_PROFILE_UNDERHILL_SHIP_TRIM_PATHS = "object"; |
There was a problem hiding this comment.
Setting RUSTC_BOOTSTRAP=1 globally for the whole nix-shell enables unstable features for every crate built in that environment, which can accidentally mask use of #![feature] and other unstable behavior beyond trim-paths. If the intent is only to enable Cargo’s trim-paths support, consider scoping RUSTC_BOOTSTRAP more narrowly (e.g., only for specific crates, or only for the specific build invocation/profile that needs it) to reduce the blast radius.
justus-camp-microsoft
force-pushed
the
install_nix_and_ci_job
branch
from
171b029 to
8c4cb99
Compare
justus-camp-microsoft
force-pushed
the
install_nix_and_ci_job
branch
from
8c4cb99 to
36b672f
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 32 out of 36 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (3)
vm/loader/igvminspect/src/extract.rs:1
- The output
filenameis derived fromregion.component, which ultimately comes from a.mapfile. As-is, a crafted component name containing path separators or..can result in path traversal and writing outsideregions_dir. Sanitize component strings into a safe filename (e.g., replace/and\\, drop.., restrict to[A-Za-z0-9._-]), or generate filenames from a stable hash + a separately-stored display name inregions.txt.
vm/loader/igvminspect/src/extract.rs:1 - The merge path can truncate already-collected data if
entry.data.len() > 4096: the first page is resized topage_data_len(larger than 4K), but on the first mergeexpected_lenispage_count * 4096, solast.data.resize(expected_len, 0)shrinks the buffer and drops bytes. Either enforce that PageData is always <= 4K (and treat larger sizes as an error), or carry a per-entry/per-region page size and use that consistently for bothexpected_lenand GPA increments.
flowey/flowey_lib_hvlite/src/_jobs/test_reproducible_build.rs:1 - This job accepts a
recipeparameter but hard-codes the artifact directories tox64-cvm-*. Ifrecipeis ever notx64-cvm, the job will publish the wrong directories (or fail). Build these paths fromrecipeto match the invokedpipeline run build-reproducible {recipe}output naming.
| "Run with verbose output", | ||
| ParameterKind::Stable, | ||
| Some(false), | ||
| Some(true), |
There was a problem hiding this comment.
Changing the cloud default verbose to true will increase log volume across pipelines by default (and the corresponding YAML/workflow defaults are also flipped). If verbose is only needed for specific diagnostics (e.g., investigating --extern ordering), consider keeping the default false and enabling it only in the reproducibility-related jobs/pipelines, or gating it behind a separate diagnostic parameter.
| Some(true), | |
| Some(false), |
| if bytes_a != bytes_b { | ||
| let first_diff = | ||
| bytes_a.iter().zip(bytes_b.iter()).position(|(a, b)| a != b); | ||
| anyhow::bail!( | ||
| "file {name} is not byte-identical \ | ||
| ({} vs {} bytes, first diff at offset {:?})", | ||
| bytes_a.len(), | ||
| bytes_b.len(), | ||
| first_diff, | ||
| ); | ||
| } |
There was a problem hiding this comment.
When the only difference is file length (common in build artifacts), first_diff will be None (because zip stops at the shorter length), yielding a confusing message. Consider explicitly handling size mismatches (e.g., if lengths differ and all overlapping bytes match, report the first differing offset as min(len_a, len_b) and mention that the shorter file ended).
justus-camp-microsoft
force-pushed
the
install_nix_and_ci_job
branch
from
36b672f to
9d8b2b9
Compare
| let ext = extension_for_component(®ion.component); | ||
| let filename = if total == 1 { | ||
| format!("{}.{ext}", region.component) | ||
| } else { | ||
| format!("{}_{}.{ext}", region.component, idx) |
There was a problem hiding this comment.
region.component comes from the user-supplied .bin.map file and is used verbatim to construct output filenames. If the map contains path separators or .., regions_dir.join(filename) can write outside the output directory (path traversal) or fail on invalid characters. Sanitize the component name (e.g., replace / and \, strip .., collapse whitespace) before using it as a filename.
| verbose: _, | ||
| no_incremental, | ||
| } = flags; |
There was a problem hiding this comment.
cfg_cargo_common_flags::Flags still contains verbose, but this node ignores it (verbose: _) and instead relies on a separate Request::verbose. This looks like a behavior regression: setting the common verbose flag will no longer make cargo build verbose unless every caller wires the new field through. Consider deriving Request::verbose from flags.verbose (or removing one of the two knobs) so the common flag continues to work consistently across cargo subcommands.
| verbose: _, | |
| no_incremental, | |
| } = flags; | |
| verbose: flags_verbose, | |
| no_incremental, | |
| } = flags; | |
| let verbose = verbose || flags_verbose; |
| xshell::cmd!( | ||
| sh, | ||
| "sh -c 'curl --proto =https --tlsv1.2 -sSf -L https://nixos.org/nix/install | sh -s -- --no-daemon'" | ||
| ) | ||
| .run()?; |
There was a problem hiding this comment.
The CI installation path executes a remote script via curl ... | sh without any integrity/pinning checks. Even in CI this is a supply-chain risk and can lead to non-reproducible failures if the installer changes. Prefer using a pinned installer (fixed URL + checksum/signature verification) or a well-maintained Nix install mechanism that supports version pinning.
| let local_igvm_dir = hvlite_repo.join("flowey-out/artifacts/x64-cvm-openhcl-igvm"); | ||
| let local_igvm_extras_dir = | ||
| hvlite_repo.join("flowey-out/artifacts/x64-cvm-openhcl-igvm-extras"); | ||
|
|
||
| log::info!("publishing local build output"); |
There was a problem hiding this comment.
The output directories are hard-coded to x64-cvm-openhcl-igvm / x64-cvm-openhcl-igvm-extras, but the job takes a generic recipe: String and uses it when running the pipeline. Either derive these artifact paths from recipe (if they vary), or validate/bail unless recipe == "x64-cvm" to avoid publishing the wrong directories.
| let first_diff = | ||
| bytes_a.iter().zip(bytes_b.iter()).position(|(a, b)| a != b); | ||
| anyhow::bail!( | ||
| "file {name} is not byte-identical \ | ||
| ({} vs {} bytes, first diff at offset {:?})", |
There was a problem hiding this comment.
If the files have different lengths but share an identical prefix, first_diff will be None, so the error message reports "first diff at offset None". Consider explicitly handling size mismatches (e.g., report min(len_a, len_b) as the first differing offset when one is a prefix of the other) to make failures actionable.
| let first_diff = | |
| bytes_a.iter().zip(bytes_b.iter()).position(|(a, b)| a != b); | |
| anyhow::bail!( | |
| "file {name} is not byte-identical \ | |
| ({} vs {} bytes, first diff at offset {:?})", | |
| let first_diff = bytes_a | |
| .iter() | |
| .zip(bytes_b.iter()) | |
| .position(|(a, b)| a != b) | |
| .unwrap_or(bytes_a.len().min(bytes_b.len())); | |
| anyhow::bail!( | |
| "file {name} is not byte-identical \ | |
| ({} vs {} bytes, first diff at offset {})", |
justus-camp-microsoft
force-pushed
the
install_nix_and_ci_job
branch
from
ca23c5a to
06ca709
Compare
justus-camp-microsoft
force-pushed
the
install_nix_and_ci_job
branch
from
06ca709 to
e3036b9
Compare
| for region in ®ions { | ||
| let total = name_counts[®ion.component]; | ||
| let idx = name_indices.entry(region.component.clone()).or_insert(0); | ||
| let ext = extension_for_component(®ion.component); | ||
| let filename = if total == 1 { | ||
| format!("{}.{ext}", region.component) | ||
| } else { | ||
| format!("{}_{}.{ext}", region.component, idx) | ||
| }; | ||
| *idx += 1; | ||
|
|
||
| fs_err::write(regions_dir.join(&filename), ®ion.data)?; |
There was a problem hiding this comment.
component comes from the user-provided .bin.map file and is used directly in output filenames. If the map contains path separators or .., this can write outside regions_dir (path traversal) or create unexpected nested paths. Sanitize component into a safe filename (e.g., reject/replace separators, collapse .., or use Path::new(component).file_name() plus a whitelist) before building filename.
| for &idx in &sorted { | ||
| let entry = &entries[idx]; | ||
| let page_data_len = entry.data.len().max(PAGE_SIZE_4K as usize); | ||
|
|
||
| // Merge only if contiguous, same component, and same flags/data_type | ||
| let can_merge = if let Some(last) = regions.last() { | ||
| entry.gpa == last.end_gpa | ||
| && entry.component == last.component | ||
| && entry.flags == last.flags | ||
| && entry.data_type == last.data_type | ||
| } else { | ||
| false | ||
| }; | ||
|
|
||
| if can_merge { | ||
| let last = regions.last_mut().unwrap(); | ||
| let expected_len = (last.page_count as usize) * (PAGE_SIZE_4K as usize); | ||
| last.data.resize(expected_len, 0); | ||
| let mut page_buf = entry.data.clone(); | ||
| page_buf.resize(page_data_len, 0); | ||
| last.data.extend_from_slice(&page_buf); | ||
| last.end_gpa = entry.gpa + PAGE_SIZE_4K; | ||
| last.page_count += 1; | ||
| } else { | ||
| let mut data = entry.data.clone(); | ||
| data.resize(page_data_len, 0); | ||
| regions.push(Region { | ||
| start_gpa: entry.gpa, | ||
| end_gpa: entry.gpa + PAGE_SIZE_4K, | ||
| page_count: 1, |
There was a problem hiding this comment.
This code assumes each PageData directive represents exactly one 4K page (it always advances end_gpa by PAGE_SIZE_4K). If an IGVM contains a PageData with data.len() > 4096, the extracted region data length and the reported GPA range/page_count will disagree. Consider validating data.len() <= PAGE_SIZE_4K and returning an error if it is larger, to avoid producing misleading output.
| if bytes_a != bytes_b { | ||
| let first_diff = | ||
| bytes_a.iter().zip(bytes_b.iter()).position(|(a, b)| a != b); | ||
| anyhow::bail!( | ||
| "file {name} is not byte-identical \ | ||
| ({} vs {} bytes, first diff at offset {:?})", | ||
| bytes_a.len(), | ||
| bytes_b.len(), | ||
| first_diff, | ||
| ); |
There was a problem hiding this comment.
When file sizes differ but the common prefix matches, first_diff becomes None, producing an unclear error (first diff at offset None). Handle length mismatches separately (or report min_len as the first differing offset) so the failure is actionable.
| .new_job( | ||
| FlowPlatform::Linux(FlowPlatformLinuxDistro::Ubuntu), | ||
| FlowArch::X86_64, | ||
| "verify reproducible openhcl [x64-cvm-linux-nix]", |
There was a problem hiding this comment.
The verify job is created with FlowPlatform::Linux(Ubuntu) but the job name includes [x64-cvm-linux-nix]. This is misleading when reading CI logs and makes it harder to triage platform-specific failures. Rename the job (or run it on the Nix platform if that was intended).
| "verify reproducible openhcl [x64-cvm-linux-nix]", | |
| "verify reproducible openhcl [x64-cvm-linux-ubuntu]", |
| | Component | Status | | ||
| |---|---| | ||
| | `openhcl_boot` | Identical | | ||
| | `openhcl_boot.dbg` | Identical | | ||
| | `openhcl.bin.map` | Identical | | ||
| | Kernel (`underhill-kernel_*.bin`) | Identical | | ||
| | Boot shim (`underhill-boot-shim_*.bin`) | Identical | | ||
| | Kernel modules (`.ko` files in initrd) | Identical | | ||
| | IGVM structure (`regions.txt` layout) | Identical | | ||
| | All measured/private page variants (`_1`) | Identical (except `shim-params_1` — 5 bytes, and `loader-imported-regions_1` — 48 bytes) | |
There was a problem hiding this comment.
The markdown tables in this doc use a double leading pipe (||) on each row, which creates an unintended empty first column in most renderers. Consider changing these rows to the standard | ... | ... | form so the tables render as intended.
| | Component | Difference | | ||
| |---|---| | ||
| | `openvmm_hcl` | 1,533,141 bytes differ out of 15,536,928 (9.9%) — same size, different code | | ||
| | `openvmm_hcl.dbg` | Size differs by 48 bytes (184,218,880 vs 184,218,832) | | ||
| | `underhill-initrd_1.cpio.gz` | 6.3M bytes differ — caused by different `openvmm_hcl` inside | | ||
| | Shared page variants (`_0`) | Most differ, filled with ~99.5-99.8% non-zero random-looking data | | ||
|
|
There was a problem hiding this comment.
Same table formatting issue here: the extra leading || produces an empty first column. Use single leading/trailing | for standard markdown table rows.
justus-camp-microsoft
force-pushed
the
install_nix_and_ci_job
branch
from
e3036b9 to
0544f65
Compare
justus-camp-microsoft
force-pushed
the
install_nix_and_ci_job
branch
from
0544f65 to
62efbf0
Compare
| let shell_nix = hvlite_repo.join("shell.nix"); | ||
| log::info!("compiling flowey_hvlite inside nix-shell"); | ||
| flowey::shell_cmd!(rt, "nix-shell {shell_nix} --pure --run") | ||
| .arg("cargo build -p flowey_hvlite") |
There was a problem hiding this comment.
This builds flowey_hvlite without --locked. In CI this can silently update Cargo.lock (or fail in unexpected ways) and undermines reproducibility goals. Consider passing --locked to the cargo build -p flowey_hvlite invocation (and/or reusing the already-configured cfg_cargo_common_flags/locked setting).
| .arg("cargo build -p flowey_hvlite") | |
| .arg("cargo build -p flowey_hvlite --locked") |
| pub struct Request { | ||
| /// Recipe name to pass to `cargo xflowey build-reproducible` (e.g. "x64-cvm"). | ||
| pub recipe: String, | ||
| /// Directory to publish the local build-reproducible igvm output to. | ||
| pub artifact_dir_local_igvm: ReadVar<PathBuf>, | ||
| /// Directory to publish the local build-reproducible igvm extras output to. | ||
| pub artifact_dir_local_igvm_extras: ReadVar<PathBuf>, | ||
| pub done: WriteVar<SideEffect>, | ||
| } |
There was a problem hiding this comment.
recipe is a request parameter, but the artifact output paths are hard-coded to x64-cvm-*. If this node is intended to support multiple recipes, derive the output artifact directory names from recipe (or switch recipe to a strongly-typed enum to make the supported set explicit). Otherwise, consider removing the parameter to avoid implying generic support.
| let verify_job = pipeline | ||
| .new_job( | ||
| FlowPlatform::Linux(FlowPlatformLinuxDistro::Ubuntu), | ||
| FlowArch::X86_64, | ||
| "verify reproducible openhcl [x64-cvm-linux-nix]", | ||
| ) | ||
| .gh_set_pool(crate::pipelines_shared::gh_pools::gh_hosted_x64_linux()) | ||
| .dep_on( | ||
| |ctx| flowey_lib_hvlite::_jobs::check_reproducible_build::Request { | ||
| artifact_dir_a: ctx.use_artifact(&use_openhcl_igvm), | ||
| artifact_dir_b: ctx.use_artifact(&use_local_igvm), | ||
| file_names: vec!["openhcl-cvm.bin".into()], | ||
| done: ctx.new_done_handle(), | ||
| }, | ||
| ) | ||
| .finish(); | ||
|
|
There was a problem hiding this comment.
This verification job runs on Ubuntu, but the job name suffix says linux-nix. Consider renaming the job label to reflect the actual platform to avoid confusion when scanning CI results.
| # Include both musl targets for cross-compilation | ||
| targets = [ | ||
| "x86_64-unknown-linux-musl" | ||
| "x86_64-unknown-linux-gnu" | ||
| "x86_64-unknown-none" |
There was a problem hiding this comment.
The comment above targets says to include both musl targets, but the list now includes x86_64-unknown-linux-gnu as well. Please update the comment (or adjust the target list) so it accurately reflects the intent, since this file is used to define the Nix dev/CI environment.
| .run()?; | ||
|
|
||
| let local_igvm_dir = hvlite_repo.join("flowey-out/artifacts/x64-cvm-openhcl-igvm"); | ||
| let local_igvm_extras_dir = | ||
| hvlite_repo.join("flowey-out/artifacts/x64-cvm-openhcl-igvm-extras"); | ||
|
|
||
| log::info!("publishing local build output"); | ||
| flowey::util::copy_dir_all(&local_igvm_dir, &publish_dir)?; | ||
| flowey::util::copy_dir_all(&local_igvm_extras_dir, &publish_dir_extras)?; |
There was a problem hiding this comment.
local_igvm_dir / local_igvm_extras_dir are hard-coded to the x64-cvm artifact names, but this node takes a recipe parameter and passes it through to build-reproducible. This will silently break if the job is ever used with a different recipe; either derive these output paths from recipe (recommended) or narrow the API (rename/remove recipe) so the hard-coding is explicit.
| log::info!("running {flowey_bin:?} pipeline run build-reproducible {recipe}"); | ||
| flowey::shell_cmd!( | ||
| rt, | ||
| "{flowey_bin} pipeline run build-reproducible {recipe} --release" | ||
| ) | ||
| .env( | ||
| "I_HAVE_A_GOOD_REASON_TO_RUN_BUILD_REPRODUCIBLE_IN_CI", | ||
| "true", | ||
| ) | ||
| .run()?; |
There was a problem hiding this comment.
flowey_hvlite is compiled inside nix-shell --pure, but then the produced binary is executed outside of that environment. If the binary ends up dynamically linking against libs provided by the nix shell (common for OpenSSL/zlib), it may fail at runtime on a stock runner. Consider running the pipeline invocation via nix-shell --pure --run ... as well (or building/running via the job-level CommandWrapperKind::NixShell instead of per-command).
| log::info!("running {flowey_bin:?} pipeline run build-reproducible {recipe}"); | |
| flowey::shell_cmd!( | |
| rt, | |
| "{flowey_bin} pipeline run build-reproducible {recipe} --release" | |
| ) | |
| .env( | |
| "I_HAVE_A_GOOD_REASON_TO_RUN_BUILD_REPRODUCIBLE_IN_CI", | |
| "true", | |
| ) | |
| .run()?; | |
| log::info!( | |
| "running {flowey_bin:?} pipeline run build-reproducible {recipe} inside nix-shell" | |
| ); | |
| flowey::shell_cmd!(rt, "nix-shell {shell_nix} --pure --run") | |
| .arg(format!( | |
| "{} pipeline run build-reproducible {} --release", | |
| flowey_bin.display(), | |
| recipe | |
| )) | |
| .env( | |
| "I_HAVE_A_GOOD_REASON_TO_RUN_BUILD_REPRODUCIBLE_IN_CI", | |
| "true", | |
| ) | |
| .run()?; |
| for name in &file_names { | ||
| let bytes_a = fs_err::read(dir_a.join(name))?; | ||
| let bytes_b = fs_err::read(dir_b.join(name))?; | ||
|
|
||
| if bytes_a != bytes_b { | ||
| let first_diff = | ||
| bytes_a.iter().zip(bytes_b.iter()).position(|(a, b)| a != b); | ||
| anyhow::bail!( |
There was a problem hiding this comment.
This job reads each .bin fully into memory (fs_err::read) before comparing. IGVM binaries can be large enough to cause avoidable memory pressure in CI. Prefer a streaming/chunked comparison (e.g., buffered readers) so memory usage stays bounded while still reporting the first differing offset.
| FlowBackend::Github | FlowBackend::Ado => { | ||
| // Step 1: Add nix profile bin to $PATH for subsequent CI | ||
| // steps. This runs before the install step so that when | ||
| // the next step starts (as a new process), nix-shell is | ||
| // already on $PATH. | ||
| let added_to_path = ctx.emit_rust_step("add nix profile to path", |ctx| { | ||
| let backend = ctx.backend(); | ||
| move |_rt| { | ||
| let nix_bin = home::home_dir() | ||
| .context("unable to get home directory")? | ||
| .join(".nix-profile/bin"); | ||
|
|
||
| match backend { | ||
| FlowBackend::Github => { | ||
| let gh_path_file = std::env::var("GITHUB_PATH")?; | ||
| let mut f = | ||
| fs_err::File::options().append(true).open(gh_path_file)?; | ||
| use std::io::Write; | ||
| writeln!(f, "{}", nix_bin.display())?; | ||
| log::info!("added {} to $GITHUB_PATH", nix_bin.display()); | ||
| } | ||
| FlowBackend::Ado => { | ||
| println!("##vso[task.prependpath]{}", nix_bin.display()); | ||
| log::info!("added {} to PATH (ADO)", nix_bin.display()); | ||
| } | ||
| FlowBackend::Local => unreachable!(), | ||
| } | ||
|
|
||
| Ok(()) | ||
| } | ||
| }); | ||
|
|
||
| ctx.emit_rust_step("install nix", |ctx| { | ||
| added_to_path.claim(ctx); | ||
| move |_rt| { | ||
| if which::which("nix-shell").is_ok() { | ||
| log::info!("nix-shell already available, skipping install"); | ||
| return Ok(()); | ||
| } | ||
|
|
||
| log::info!("installing Nix package manager..."); | ||
|
|
||
| // Single-user install (no daemon) — simplest for CI. | ||
| // https://nixos.org/download/ | ||
| #[expect(clippy::disallowed_methods, reason = "can't use the nix xshell wrapper if nix isn't installed yet")] | ||
| let sh = xshell::Shell::new()?; | ||
| #[expect(clippy::disallowed_macros, reason = "can't use the nix xshell wrapper if nix isn't installed yet")] | ||
| xshell::cmd!( | ||
| sh, | ||
| "sh -c 'curl --proto =https --tlsv1.2 -sSf -L https://nixos.org/nix/install | sh -s -- --no-daemon'" | ||
| ) | ||
| .run()?; | ||
|
|
||
| log::info!("nix installed successfully"); | ||
| Ok(()) | ||
| } | ||
| }) |
There was a problem hiding this comment.
The CI install path runs a POSIX shell pipeline (sh -c ... | sh ...) without checking the current platform. If this node is ever invoked on a Windows GitHub/ADO runner, it will fail due to missing sh/curl. Consider explicitly gating the install logic to FlowPlatform::Linux(_)/MacOs and bailing with a clear message on Windows.
| FlowBackend::Github | FlowBackend::Ado => { | |
| // Step 1: Add nix profile bin to $PATH for subsequent CI | |
| // steps. This runs before the install step so that when | |
| // the next step starts (as a new process), nix-shell is | |
| // already on $PATH. | |
| let added_to_path = ctx.emit_rust_step("add nix profile to path", |ctx| { | |
| let backend = ctx.backend(); | |
| move |_rt| { | |
| let nix_bin = home::home_dir() | |
| .context("unable to get home directory")? | |
| .join(".nix-profile/bin"); | |
| match backend { | |
| FlowBackend::Github => { | |
| let gh_path_file = std::env::var("GITHUB_PATH")?; | |
| let mut f = | |
| fs_err::File::options().append(true).open(gh_path_file)?; | |
| use std::io::Write; | |
| writeln!(f, "{}", nix_bin.display())?; | |
| log::info!("added {} to $GITHUB_PATH", nix_bin.display()); | |
| } | |
| FlowBackend::Ado => { | |
| println!("##vso[task.prependpath]{}", nix_bin.display()); | |
| log::info!("added {} to PATH (ADO)", nix_bin.display()); | |
| } | |
| FlowBackend::Local => unreachable!(), | |
| } | |
| Ok(()) | |
| } | |
| }); | |
| ctx.emit_rust_step("install nix", |ctx| { | |
| added_to_path.claim(ctx); | |
| move |_rt| { | |
| if which::which("nix-shell").is_ok() { | |
| log::info!("nix-shell already available, skipping install"); | |
| return Ok(()); | |
| } | |
| log::info!("installing Nix package manager..."); | |
| // Single-user install (no daemon) — simplest for CI. | |
| // https://nixos.org/download/ | |
| #[expect(clippy::disallowed_methods, reason = "can't use the nix xshell wrapper if nix isn't installed yet")] | |
| let sh = xshell::Shell::new()?; | |
| #[expect(clippy::disallowed_macros, reason = "can't use the nix xshell wrapper if nix isn't installed yet")] | |
| xshell::cmd!( | |
| sh, | |
| "sh -c 'curl --proto =https --tlsv1.2 -sSf -L https://nixos.org/nix/install | sh -s -- --no-daemon'" | |
| ) | |
| .run()?; | |
| log::info!("nix installed successfully"); | |
| Ok(()) | |
| } | |
| }) | |
| FlowBackend::Github | FlowBackend::Ado => match ctx.platform() { | |
| FlowPlatform::Linux(_) | FlowPlatform::MacOs => { | |
| // Step 1: Add nix profile bin to $PATH for subsequent CI | |
| // steps. This runs before the install step so that when | |
| // the next step starts (as a new process), nix-shell is | |
| // already on $PATH. | |
| let added_to_path = ctx.emit_rust_step("add nix profile to path", |ctx| { | |
| let backend = ctx.backend(); | |
| move |_rt| { | |
| let nix_bin = home::home_dir() | |
| .context("unable to get home directory")? | |
| .join(".nix-profile/bin"); | |
| match backend { | |
| FlowBackend::Github => { | |
| let gh_path_file = std::env::var("GITHUB_PATH")?; | |
| let mut f = | |
| fs_err::File::options().append(true).open(gh_path_file)?; | |
| use std::io::Write; | |
| writeln!(f, "{}", nix_bin.display())?; | |
| log::info!("added {} to $GITHUB_PATH", nix_bin.display()); | |
| } | |
| FlowBackend::Ado => { | |
| println!("##vso[task.prependpath]{}", nix_bin.display()); | |
| log::info!("added {} to PATH (ADO)", nix_bin.display()); | |
| } | |
| FlowBackend::Local => unreachable!(), | |
| } | |
| Ok(()) | |
| } | |
| }); | |
| ctx.emit_rust_step("install nix", |ctx| { | |
| added_to_path.claim(ctx); | |
| move |_rt| { | |
| if which::which("nix-shell").is_ok() { | |
| log::info!("nix-shell already available, skipping install"); | |
| return Ok(()); | |
| } | |
| log::info!("installing Nix package manager..."); | |
| // Single-user install (no daemon) — simplest for CI. | |
| // https://nixos.org/download/ | |
| #[expect(clippy::disallowed_methods, reason = "can't use the nix xshell wrapper if nix isn't installed yet")] | |
| let sh = xshell::Shell::new()?; | |
| #[expect(clippy::disallowed_macros, reason = "can't use the nix xshell wrapper if nix isn't installed yet")] | |
| xshell::cmd!( | |
| sh, | |
| "sh -c 'curl --proto =https --tlsv1.2 -sSf -L https://nixos.org/nix/install | sh -s -- --no-daemon'" | |
| ) | |
| .run()?; | |
| log::info!("nix installed successfully"); | |
| Ok(()) | |
| } | |
| }) | |
| } | |
| FlowPlatform::Windows => { | |
| anyhow::bail!( | |
| "automatic Nix installation in CI is only supported on Linux and macOS runners; \ | |
| Windows runners do not provide the required POSIX shell/curl install path" | |
| ); | |
| } |
| @@ -69,6 +69,7 @@ let | |||
| # Include both musl targets for cross-compilation | |||
There was a problem hiding this comment.
The comment says “Include both musl targets”, but the change adds a GNU target. Please update the comment to accurately reflect the target list (e.g., mention both musl and gnu, or split into separate groups).
| # Include both musl targets for cross-compilation | |
| # Include musl, gnu, and bare-metal targets for cross-compilation |
| let rust_install = ctx.reqv(flowey_lib_common::install_rust::Request::EnsureInstalled); | ||
| let nix_install = ctx.reqv(flowey_lib_common::install_nix::Request::EnsureInstalled); | ||
|
|
||
| let step = ctx.emit_rust_step("run cargo xflowey build-reproducible", |ctx| { | ||
| rust_install.claim(ctx); | ||
| nix_install.claim(ctx); |
There was a problem hiding this comment.
This node claims install_rust::EnsureInstalled, which (after the wrapper-aware change) will require cargo to be runnable in the current step environment. However, this job only enters Nix for the cargo build -p flowey_hvlite command, and runs the rest outside nix-shell. If the CI host doesn’t already have cargo on PATH, install_rust can fail before you ever enter nix-shell. A robust fix is to run the entire sequence under the same Nix wrapper (or set the job’s command wrapper to NixShell like the other reproducible job), and/or drop the Rust install dependency for this node if Nix is the source of the toolchain.
| flowey::shell_cmd!(rt, "nix-shell {shell_nix} --pure --run") | ||
| .arg("cargo build -p flowey_hvlite") | ||
| .run()?; |
There was a problem hiding this comment.
This node claims install_rust::EnsureInstalled, which (after the wrapper-aware change) will require cargo to be runnable in the current step environment. However, this job only enters Nix for the cargo build -p flowey_hvlite command, and runs the rest outside nix-shell. If the CI host doesn’t already have cargo on PATH, install_rust can fail before you ever enter nix-shell. A robust fix is to run the entire sequence under the same Nix wrapper (or set the job’s command wrapper to NixShell like the other reproducible job), and/or drop the Rust install dependency for this node if Nix is the source of the toolchain.
| flowey::shell_cmd!( | ||
| rt, | ||
| "{flowey_bin} pipeline run build-reproducible {recipe} --release" | ||
| ) |
There was a problem hiding this comment.
This node claims install_rust::EnsureInstalled, which (after the wrapper-aware change) will require cargo to be runnable in the current step environment. However, this job only enters Nix for the cargo build -p flowey_hvlite command, and runs the rest outside nix-shell. If the CI host doesn’t already have cargo on PATH, install_rust can fail before you ever enter nix-shell. A robust fix is to run the entire sequence under the same Nix wrapper (or set the job’s command wrapper to NixShell like the other reproducible job), and/or drop the Rust install dependency for this node if Nix is the source of the toolchain.
| FlowArch::X86_64, | ||
| "build local reproducible openhcl [x64-cvm-linux-nix]", | ||
| ) | ||
| .gh_set_pool(crate::pipelines_shared::gh_pools::gh_hosted_x64_linux()) |
There was a problem hiding this comment.
Unlike the earlier Nix reproducible build job, this job does not configure a NixShell command wrapper, yet test_reproducible_build currently claims Rust installation and runs substantial work outside nix-shell. This combination is likely to fail on CI images without preinstalled Rust. Consider either adding .set_command_wrapper(...NixShell...) here as well, or adjust test_reproducible_build to ensure all required commands run under Nix.
| .gh_set_pool(crate::pipelines_shared::gh_pools::gh_hosted_x64_linux()) | |
| .gh_set_pool(crate::pipelines_shared::gh_pools::gh_hosted_x64_linux()) | |
| .set_command_wrapper(CommandWrapper::NixShell) |
| .new_job( | ||
| FlowPlatform::Linux(FlowPlatformLinuxDistro::Ubuntu), | ||
| FlowArch::X86_64, | ||
| "verify reproducible openhcl [x64-cvm-linux-nix]", |
There was a problem hiding this comment.
The job label includes linux-nix, but the job platform is Ubuntu. Please rename the job string to reflect the actual platform (or change the platform if the intent is to run under Nix).
| "verify reproducible openhcl [x64-cvm-linux-nix]", | |
| "verify reproducible openhcl [x64-cvm-linux-ubuntu]", |
| let bytes_a = fs_err::read(dir_a.join(name))?; | ||
| let bytes_b = fs_err::read(dir_b.join(name))?; |
There was a problem hiding this comment.
This reads both files fully into memory before comparing. If .bin artifacts are large, this can significantly increase peak memory usage in CI. Consider streaming comparison (chunked reads) or comparing hashes computed via buffered I/O.
No description provided.