Feature/crq 016 nixify #1
Conversation
This commit introduces comprehensive documentation for the rust-bootstrap-nix repository. - Generated README.md, OVERVIEW.md, NIX_FLAKES_DOCUMENTATION.md, SCRIPTS_DOCUMENTATION.md, and CONFIGURATION.md. - Documented all Nix flakes, shell scripts, and configuration files. - Moved all *.log files into a new 'logs/' directory and added 'logs/' to .gitignore. - Updated .pre-commit-config.yaml and Makefile. - Refined existing documentation for clarity and accuracy.
…e and add local mirror setup script
This commit introduces a new Nix flake (`flakes/bootstrap-builder`) to build the Rust bootstrap compiler directly from source, eliminating the dependency on Python for this initial stage. Key changes include: - **New `bootstrap-builder` flake:** A dedicated flake (`flakes/bootstrap-builder/flake.nix`) has been created to build the `bootstrap` executable using `pkgs.rustPlatform.buildRustPackage`. This flake fetches the Rust source from `github:meta-introspector/rust` and builds the `src/bootstrap` crate. - **Updated `standalonex` flake:** The `standalonex/flake.nix` has been modified to utilize the newly built `bootstrap` executable from the `bootstrap-builder` flake. It now directly calls the `bootstrap` executable with the `dist` command to capture the full Rust build plan. - **Modified `bootstrap.py`:** The `standalonex/src/bootstrap/bootstrap.py` script has been adjusted to always output JSON for build commands and to execute all commands except `rustc` and `cargo` (unless it's the initial `cargo build` for the bootstrap compiler itself). This allows for comprehensive capture of the build process. - **Removed redundant flake:** The `flakes/xpy-json-output-flake` has been removed as its functionality is now integrated into the `standalonex` flake. - **Documentation:** A `README.md` has been added to the `standalonex` directory, explaining the JSON output generation and providing field explanations. This work is a step towards a pure Nix-based Rust bootstrap process, reducing reliance on external tools and improving reproducibility.
This commit adds documentation for the new dynamic, Nix-based configuration system. It includes:
- An update to README.md with a new section that briefly explains the new system.
- A new, more detailed documentation file, docs/Nix_Integration.md, that provides an in-depth explanation of the Nix integration, including:
- How to use generated_config.toml to configure the build.
- How the Rust bootstrap process dynamically resolves Nix paths.
- How to use the system to test different versions of Rust and its dependencies.
This documentation will help developers understand and use the new system, and it will serve as a foundation for future work on the lattice of Nix flakes and the 8-fold recursion system.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 28
♻️ Duplicate comments (5)
bootstrap-config-builder/src/utils/format_file.rs (1)
94-127: Sequential.replace()is order-dependent and can corrupt earlier substitutions.If any value contains a placeholder-like token (e.g., "{stage}") or one placeholder is a substring of another, later replaces will mutate earlier insertions. Use a single-pass renderer (regex over the original template with a map), or a templating lib (handlebars/tera) to avoid cascading replacements.
Example single-pass approach:
use regex::Regex; use std::collections::HashMap; let re = Regex::new(r"\{[a-zA-Z0-9_]+\}").unwrap(); let mut m = HashMap::<&str, String>::new(); m.insert("{system}", system.to_string()); m.insert("{flake_path_str}", flake_path_str.to_string()); // ... fill for all placeholders ... let rendered = re.replace_all(template_content, |caps: ®ex::Captures| { m.get(caps.get(0).unwrap().as_str()) .cloned() .unwrap_or_else(|| caps[0].to_string()) }); let rendered = rendered.into_owned();Optionally, error on unknown placeholders instead of leaving them.
standalonex/flake.nix (2)
6-6: Pin commit with rev, not ref.- rustSrcFlake.url = "github:meta-introspector/rust?ref=3487cd3843083db70ee30023f19344568ade9c9f"; + rustSrcFlake.url = "github:meta-introspector/rust?rev=3487cd3843083db70ee30023f19344568ade9c9f";
50-61: Toolchain override ignored; crate root ambiguous; missing src in bootstrap-main.
rustc = ...is ignored by buildRustPackage; usemakeRustPlatform.src = ./srclikely targets the workspace root; confirm crate root(s).bootstrap-mainlacks requiredsrc.Apply:
- packages.aarch64-linux = { - default = pkgs.rustPlatform.buildRustPackage { - src = pkgs.lib.cleanSource ./src; - cargoLock.lockFile = ./src/Cargo.lock; - rustc = pkgs.rust-bin.stable."1.84.1".default; - doCheck = false; - postPatch = '' - mkdir -p .cargo - cp -r ${buildHelperSrc} build_helper - cp ${configTomlPath} config.toml - ''; - }; + packages.aarch64-linux = + let + rustToolchain = pkgs.rust-bin.stable."1.84.1".default; + rustPlatform = pkgs.makeRustPlatform { cargo = rustToolchain; rustc = rustToolchain; }; + in { + default = rustPlatform.buildRustPackage { + # Point to the bootstrap crate root (verify) + src = pkgs.lib.cleanSource ./src/bootstrap; + cargoLock.lockFile = ./src/bootstrap/Cargo.lock; + doCheck = false; + postPatch = '' + mkdir -p .cargo + cp -r ${buildHelperSrc} build_helper + ''; + }; - bootstrap-main = pkgs.rustPlatform.buildRustPackage { + bootstrap-main = rustPlatform.buildRustPackage { pname = "bootstrap-main"; version = "0.1.0"; - - cargoLock.lockFile = ./src/Cargo.lock; - rustc = pkgs.rust-bin.stable."1.84.1".default; + src = pkgs.lib.cleanSource ./src/bootstrap; + cargoLock.lockFile = ./src/bootstrap/Cargo.lock; doCheck = false; cargoBuildFlags = [ "--bin" "bootstrap" ]; postPatch = '' mkdir -p .cargo cp -r ${buildHelperSrc} build_helper - cp ${configTomlPath} config.toml ''; }; - nix-bootstrap = pkgs.rustPlatform.buildRustPackage { + nix-bootstrap = rustPlatform.buildRustPackage { pname = "nix-bootstrap"; version = "0.1.0"; - src = pkgs.lib.cleanSource ./src; - cargoLock.lockFile = ./src/Cargo.lock; - rustc = pkgs.rust-bin.stable."1.84.1".default; + # Verify crate and binary name; adjust if lives under ./src/bootstrap + src = pkgs.lib.cleanSource ./src/bootstrap; + cargoLock.lockFile = ./src/bootstrap/Cargo.lock; doCheck = false; cargoBuildFlags = [ "--bin" "nix_bootstrap" ]; postPatch = '' mkdir -p .cargo cp -r ${buildHelperSrc} build_helper - cp ${configTomlPath} config.toml ''; }; };Please confirm actual crate roots and bin names. If the bootstrap crate isn’t under
./src/bootstrap, adjustsrcandcargoLock.lockFileaccordingly.Also applies to: 63-76, 78-92
flake.nix (2)
4-11: Add missing flake-utils input.
outputsparameter includesflake-utilsbut it’s not declared ininputs. Evaluation will fail.inputs = { nixpkgs.url = "github:meta-introspector/nixpkgs?ref=feature/CRQ-016-nixify"; rust-overlay.url = "github:meta-introspector/rust-overlay?ref=feature/CRQ-016-nixify"; + flake-utils.url = "github:numtide/flake-utils"; rustSrcFlake.url = "github:meta-introspector/rust?ref=feature/CRQ-016-nixify"; configuration-nix.url = "./configuration-nix"; configuration-nix.inputs.nixpkgs.follows = "nixpkgs"; standalonex.url = "./standalonex"; };
82-98: Pass config parameters via CLI; env vars are ignored by configuration-nix.The binary reads Clap args, not env vars. Provide the values as flags.
- ${configurationNix.packages.${system}.default}/bin/configuration-nix + ${configurationNix.packages.${system}.default}/bin/configuration-nix \ + --stage ${toString stageNum} \ + --target ${if system == "aarch64-linux" then "aarch64-unknown-linux-gnu" else "x86_64-unknown-linux-gnu"} \ + --nixpkgs-path ${nixpkgs.outPath} \ + --rust-overlay-path ${rust-overlay.outPath} \ + --configuration-nix-path ${configurationNix.outPath} \ + --rust-src-flake-path ${rustSrcFlake.outPath} \ + --rust-bootstrap-nix-flake-ref "github:meta-introspector/rust-bootstrap-nix?ref=feature/CRQ-016-nixify" \ + --rust-src-flake-ref "github:meta-introspector/rust?ref=feature/CRQ-016-nixify"
🧹 Nitpick comments (42)
standalonex/src/bootstrap/src/constants.rs (2)
3-21: LGTM! Consider adding doc comments.The constants are well-defined with appropriate inline comments. The
LLVM_TOOLSlist is comprehensive, andLLD_FILE_NAMEScovers all major LLD flavors.Consider adding doc comments to these public constants for better API documentation:
+/// LLVM tools commonly used in the Rust build process. +/// These include tools for coverage, inspection, transformation, and compilation. pub const LLVM_TOOLS: &[&str] = &[ "llvm-cov", // used to generate coverage report ... ]; +/// LLD linker file names for all supported flavors. +/// Includes Unix-like (ld.lld), macOS (ld64.lld), Windows (lld-link), and WebAssembly (wasm-ld). pub const LLD_FILE_NAMES: &[&str] = &["ld.lld", "ld64.lld", "lld-link", "wasm-ld"];
23-36: LGTM! Consider documentation improvements.The
EXTRA_CHECK_CFGSconstant is well-structured with consistent entries. The#[allow(clippy::type_complexity)]is appropriately justified for this hard-coded configuration list.Consider these optional improvements:
- Add a doc comment explaining the constant's purpose and tuple structure:
+/// Extra `--check-cfg` configurations for building the compiler or tools. +/// +/// Each entry is a tuple of: +/// - `Option<Mode>`: The build mode restriction (None applies to all modes) +/// - `&str`: The configuration name +/// - `Option<&[&'static str]>`: Optional configuration values #[allow(clippy::type_complexity)] // It's fine for hard-coded list and type is explained above. pub const EXTRA_CHECK_CFGS: &[(Option<Mode>, &str, Option<&[&'static str]>)] = &[
- Minor grammar fix in the comment (lines 33-35):
- // Any library specific cfgs like `target_os`, `target_arch` should be put in - // priority the `[lints.rust.unexpected_cfgs.check-cfg]` table + // Library-specific cfgs like `target_os`, `target_arch` should preferably be placed in + // the `[lints.rust.unexpected_cfgs.check-cfg]` table // in the appropriate `library/{std,alloc,core}/Cargo.toml`standalonex/src/bootstrap/src/build_impl_submodules.rs (3)
60-76: Prefer parse_gitmodules over parsinggit configoutput; handle spaces in paths and unify src usageAvoid split_once(' ').unwrap() and rely on existing helper. Also limit surprises by consistently using self.config.src.
- std::thread::scope(|s| { - // Look for `submodule.$name.path = $path` - // Sample output: `submodule.src/rust-installer.path src/tools/rust-installer` - for line in output.lines() { - let submodule = line.split_once(' ').unwrap().1; - let config = self.config.clone(); - s.spawn(move || { - Self::update_existing_submodule(&config, submodule); - }); - } - }); + std::thread::scope(|s| { + for submodule in util::parse_gitmodules(&self.config.src) { + let config = self.config.clone(); + let submodule = submodule.clone(); // &[String] -> String for 'move' + s.spawn(move || { + Self::update_existing_submodule(&config, &submodule); + }); + } + });
86-88: Use absolute path for GitInfo to avoid cwd‑dependent behaviorPassing a relative Path can mis-detect managed subrepos depending on process CWD.
- if GitInfo::new(false, Path::new(submodule)).is_managed_git_subrepository() { - config.update_submodule(submodule); + let abs = config.src.join(submodule); + if GitInfo::new(false, &abs).is_managed_git_subrepository() { + config.update_submodule(submodule); }
66-76: Optional: bound concurrency to avoid thread explosion on large reposIf a repo ever has many submodules, consider a small thread pool or rayon to cap parallelism.
Would you like a patch using rayon’s par_iter()?
docs/README_config_standalonex.md (1)
9-12: Fix Markdown list indentation (MD007).Lines 9 and 12 have incorrect list indentation (4 spaces instead of 2). Standardize the indentation across the bullet points.
- * `rustc = "/nix/store/.../bin/rustc"`: + * `rustc = "/nix/store/.../bin/rustc"`: * **Purpose:** Similar to the root `config.toml`, this specifies the absolute path to the `rustc` executable, ensuring that the `standalonex` environment uses a Nix-provided compiler. - * `cargo = "/nix/store/.../bin/cargo"`: + * `cargo = "/nix/store/.../bin/cargo"`: * **Purpose:** Specifies the absolute path to the `cargo` executable for the `standalonex` environment, guaranteeing the use of a specific, Nix-managed `cargo` instance.docs/README_nix_flake_json_processor.md (1)
1-21: Fix Markdown list indentation throughout (MD007).Multiple lines have incorrect list indentation (4 or 8 spaces instead of 2 or 4). Standardize all bullet points to the correct nesting levels.
-* `nixpkgs`: `github:meta-introspector/nixpkgs?ref=feature/CRQ-016-nixify` - * Standard `nixpkgs` from `meta-introspector`. +* `nixpkgs`: `github:meta-introspector/nixpkgs?ref=feature/CRQ-016-nixify` + * Standard `nixpkgs` from `meta-introspector`. -* **`packages.aarch64-linux.default` and `packages.x86_64-linux.default`:** - * These outputs define a Nix package for each architecture. - * The package is a `pkgs.mkShell` (which is typically used for development shells, but can also be used to create environments with specific tools). - * **Packages Included:** - * `pkgs.jq`: A lightweight and flexible command-line JSON processor. - * `pkgs.python3`: The Python 3 interpreter. +* **`packages.aarch64-linux.default` and `packages.x86_64-linux.default`:** + * These outputs define a Nix package for each architecture. + * The package is a `pkgs.mkShell` (which is typically used for development shells, but can also be used to create environments with specific tools). + * **Packages Included:** + * `pkgs.jq`: A lightweight and flexible command-line JSON processor. + * `pkgs.python3`: The Python 3 interpreter.docs/README_nix_flake_xpy_json_output_flake.md (1)
1-25: Fix Markdown list indentation throughout (MD007).Lines 10, 12, 17–23 have incorrect indentation (4 or 8 spaces instead of 2 or 4). Align all list items to the correct nesting levels: 2 spaces for primary sub-items, 4 spaces for secondary sub-items, etc.
Representative fix pattern:
-* `nixpkgs`: `github:meta-introspector/nixpkgs?ref=feature/CRQ-016-nixify` - * Standard `nixpkgs` from `meta-introspector`. -* `rustSrc`: `github:meta-introspector/rust?ref=d772ccdfd1905e93362ba045f66dad7e2ccd469b` - * This input points to a specific commit of the `rust` repository within `meta-introspector`. It's marked as `flake = false`, indicating it's treated as a plain source input rather than another Nix flake. This `rustSrc` is where the `x.py` script resides. +* `nixpkgs`: `github:meta-introspector/nixpkgs?ref=feature/CRQ-016-nixify` + * Standard `nixpkgs` from `meta-introspector`. +* `rustSrc`: `github:meta-introspector/rust?ref=d772ccdfd1905e93362ba045f66dad7e2ccd469b` + * This input points to a specific commit of the `rust` repository within `meta-introspector`. It's marked as `flake = false`, indicating it's treated as a plain source input rather than another Nix flake. This `rustSrc` is where the `x.py` script resides. -* **`packages.aarch64-linux.default`:** - * This output is a derivation named `xpy-json-output-derivation`. - * It uses `pkgs.runCommandLocal` to execute a local command. - * **`nativeBuildInputs`:** Includes `pkgs.python3` because `x.py` is a Python script. - * **`src`:** The `rustSrc` input is used as the source for this derivation. - * **Build Phase:** - * It creates an output directory `$out`. - * It then executes `python3 $src/x.py build --json-output $out`. This command is responsible for running the `x.py` build script and directing its JSON output to the `$out` directory of this derivation. +* **`packages.aarch64-linux.default`:** + * This output is a derivation named `xpy-json-output-derivation`. + * It uses `pkgs.runCommandLocal` to execute a local command. + * **`nativeBuildInputs`:** Includes `pkgs.python3` because `x.py` is a Python script. + * **`src`:** The `rustSrc` input is used as the source for this derivation. + * **Build Phase:** + * It creates an output directory `$out`. + * It then executes `python3 $src/x.py build --json-output $out`. This command is responsible for running the `x.py` build script and directing its JSON output to the `$out` directory of this derivation.docs/README_nix_flake_minimal_flake.md (1)
1-23: Fix Markdown list indentation throughout (MD007).Lines 10, 12, 14, 19–23 have incorrect indentation (4 or 8 spaces instead of 2 or 4). Standardize all list items to the correct nesting levels.
Representative fix pattern:
-* `nixpkgs`: `github:meta-introspector/nixpkgs?ref=feature/CRQ-016-nixify` - * Standard `nixpkgs` from `meta-introspector`. +* `nixpkgs`: `github:meta-introspector/nixpkgs?ref=feature/CRQ-016-nixify` + * Standard `nixpkgs` from `meta-introspector`. -* **`devShell`:** - * A development shell named `minimal-python-dev-shell`. - * **Packages Included:** `python3` and `git`. This provides a basic environment for Python development and version control. +* **`devShell`:** + * A development shell named `minimal-python-dev-shell`. + * **Packages Included:** `python3` and `git`. This provides a basic environment for Python development and version control. -* **`packages.<system>.helloPython`:** - * A Nix package named `helloPython` for the `aarch64-linux` system. - * It uses `pkgs.writeScriptBin` to create an executable script. - * The script is a simple Python program that prints "Hello from Nix Python!". +* **`packages.<system>.helloPython`:** + * A Nix package named `helloPython` for the `aarch64-linux` system. + * It uses `pkgs.writeScriptBin` to create an executable script. + * The script is a simple Python program that prints "Hello from Nix Python!".docs/README_nix_flake_standalonex.md (1)
1-42: Fix Markdown list indentation throughout (MD007).Lines 10, 12, 14, 19–26, 29–32, and 34–40 have incorrect indentation (4 or 8 spaces instead of 2 or 4). Standardize all list items to use 2-space indentation for primary sub-items and 4-space indentation for secondary sub-items.
Representative fix pattern:
-* `nixpkgs`: `github:meta-introspector/nixpkgs?ref=feature/CRQ-016-nixify` - * Standard `nixpkgs` from `meta-introspector`. -* `rustSrcFlake`: `github:meta-introspector/rust?ref=e6c1b92d0abaa3f64032d6662cbcde980c826ff2` - * The same `rust` source flake used in the root `flake.nix`, providing the `src/stage0` path. +* `nixpkgs`: `github:meta-introspector/nixpkgs?ref=feature/CRQ-016-nixify` + * Standard `nixpkgs` from `meta-introspector`. +* `rustSrcFlake`: `github:meta-introspector/rust?ref=e6c1b92d0abaa3f64032d6662cbcde980c826ff2` + * The same `rust` source flake used in the root `flake.nix`, providing the `src/stage0` path. -* **`buildPhase`:** This is the most complex part: - * It creates a writable temporary directory (`$TMPDIR/xpy_work`) and copies the flake's source into it. - * It then copies `config.old.toml` to `config.toml` and uses `sed` to inject the correct `rustc` and `cargo` paths into `config.toml`. +* **`buildPhase`:** This is the most complex part: + * It creates a writable temporary directory (`$TMPDIR/xpy_work`) and copies the flake's source into it. + * It then copies `config.old.toml` to `config.toml` and uses `sed` to inject the correct `rustc` and `cargo` paths into `config.toml`.docs/README_nix_flake_evaluate_rust.md (1)
1-27: Fix Markdown list indentation throughout (MD007).Lines 10, 12, 17–20, and 23–25 have incorrect indentation (4 or 8 spaces instead of 2 or 4). Standardize all list items to the correct nesting levels: 2 spaces for primary sub-items, 4 spaces for secondary sub-items.
Representative fix pattern:
-* `nixpkgs`: `github:meta-introspector/nixpkgs?ref=feature/CRQ-016-nixify` - * Standard `nixpkgs` from `meta-introspector`. -* `naersk`: `github:meta-introspector/naersk?ref=feature/CRQ-016-nixify` - * This input is for `rust2nix` functionality, indicating that this flake intends to use `naersk` to convert Rust projects into Nix derivations. +* `nixpkgs`: `github:meta-introspector/nixpkgs?ref=feature/CRQ-016-nixify` + * Standard `nixpkgs` from `meta-introspector`. +* `naersk`: `github:meta-introspector/naersk?ref=feature/CRQ-016-nixify` + * This input is for `rust2nix` functionality, indicating that this flake intends to use `naersk` to convert Rust projects into Nix derivations. - **Function Logic:** - * **Base Case (Recursion Limit):** If `currentDepth` reaches `maxDepth`, it returns a derivation indicating that the recursion limit was reached. - * **`cargo build` Case:** If the command is `cargo` and includes the `build` argument, it uses `naersk.lib.${pkgs.system}.buildPackage` to create a Nix derivation. It passes `cargoBuildFlags` and `env` directly to `naersk`. This is a key integration point for Rust projects. - * **Other Commands Case:** For any other command (e.g., `rustc` directly), it creates a simple `pkgs.runCommand` derivation. It executes the command with its arguments and environment variables, capturing stdout and stderr to `output.txt`. + **Function Logic:** + * **Base Case (Recursion Limit):** If `currentDepth` reaches `maxDepth`, it returns a derivation indicating that the recursion limit was reached. + * **`cargo build` Case:** If the command is `cargo` and includes the `build` argument, it uses `naersk.lib.${pkgs.system}.buildPackage` to create a Nix derivation. It passes `cargoBuildFlags` and `env` directly to `naersk`. This is a key integration point for Rust projects. + * **Other Commands Case:** For any other command (e.g., `rustc` directly), it creates a simple `pkgs.runCommand` derivation. It executes the command with its arguments and environment variables, capturing stdout and stderr to `output.txt`.bootstrap-config-builder/src/utils/format_file.rs (1)
5-37: Function has 30+ params; pass a config struct or builder.This API is brittle and hard to evolve. Replace positional params with a typed
FormatParams(or reuse your config type). Improves call‑site clarity and reduces breakage.Example:
-#[allow(clippy::too_many_arguments)] -pub fn format_file( - system: &str, - flake_path_str: &str, - // ... many more ... - change_id: &str, -) -> String { +pub struct FormatParams<'a> { + pub system: &'a str, + pub flake_path_str: &'a str, + pub nixpkgs_path: &'a str, + pub rust_overlay_path: &'a str, + pub rust_bootstrap_nix_path: &'a str, + pub configuration_nix_path: &'a str, + pub rust_src_flake_path: &'a str, + pub rust_bootstrap_nix_flake_ref: &'a str, + pub rust_src_flake_ref: &'a str, + pub stage: &'a str, + pub target: &'a str, + pub rustc_path: &'a str, + pub cargo_path: &'a str, + pub rust_channel: &'a str, + pub rust_download_rustc: bool, + pub rust_parallel_compiler: bool, + pub rust_llvm_tools: bool, + pub rust_debuginfo_level: u8, + pub patch_binaries_for_nix: bool, + pub vendor: bool, + pub build_dir: &'a str, + pub build_jobs: u32, + pub home_dir: &'a str, + pub cargo_home_dir: &'a str, + pub install_prefix: &'a str, + pub install_sysconfdir: &'a str, + pub dist_sign_folder: &'a str, + pub dist_upload_addr: &'a str, + pub llvm_download_ci_llvm: bool, + pub llvm_ninja: bool, + pub change_id: &'a str, +} + +pub fn format_file(p: &FormatParams<'_>) -> String { + let template_content = r#"... "#; + template_content + .replace("{system}", p.system) + // ... etc ... + .replace("{change_id}", p.change_id) +}bootstrap-config-builder/src/args.rs (3)
28-31:Option<PathBuf>with adefault_valueis effectively alwaysSome.This defeats “unset means inherit from config”. Make it a required value with default, or keep it truly optional without a default.
- #[arg(long, short, default_value = "config.toml")] - pub output: Option<PathBuf>, + #[arg(long, short, default_value = "config.toml")] + pub output: PathBuf,If you need tri‑state semantics, remove
default_valueand compute the default upstream.
12-18: Tighten types and add value ranges for safer CLI.
- Stage is numeric and bounded; parse it as
u8with range.- Constrain
rust_debuginfo_level(typically 0..=2).- Constrain
build_jobsto>= 1.- pub stage: Option<String>, + #[arg(long, short, value_name = "STAGE", value_parser = clap::value_parser!(u8).range(0..=2))] + pub stage: Option<u8>, @@ - #[arg(long, value_name = "LEVEL")] - pub rust_debuginfo_level: Option<u8>, + #[arg(long, value_name = "LEVEL", value_parser = clap::value_parser!(u8).range(0..=2))] + pub rust_debuginfo_level: Option<u8>, @@ - #[arg(long, value_name = "JOBS")] - pub build_jobs: Option<u32>, + #[arg(long, value_name = "JOBS", value_parser = clap::value_parser!(u32).range(1..))] + pub build_jobs: Option<u32>,Also applies to: 88-91, 104-107
76-87: Boolean tri‑state flags are hard to use asOption<bool>without guidance.As written, users must pass
--flag=true|false. Consider Boolish parser and document usage, or add explicit--no-...negations.- #[arg(long)] - pub rust_download_rustc: Option<bool>, + #[arg(long, value_name = "BOOL", value_parser = clap::builder::BoolishValueParser::new())] + pub rust_download_rustc: Option<bool>, @@ - #[arg(long)] - pub rust_parallel_compiler: Option<bool>, + #[arg(long, value_name = "BOOL", value_parser = clap::builder::BoolishValueParser::new())] + pub rust_parallel_compiler: Option<bool>, @@ - #[arg(long)] - pub llvm_ninja: Option<bool>, + #[arg(long, value_name = "BOOL", value_parser = clap::builder::BoolishValueParser::new())] + pub llvm_ninja: Option<bool>,Alternative: switch to two flags per option (
--fooand--no-foo) via manualArgdefinitions if you need friendlier UX while preserving tri‑state merges.Also applies to: 92-99, 132-139
docs/README_nix_flakes_intro.md (1)
1-1: LGTM — placeholder doc established.Consider linking to the deeper Nix Flakes docs from here when ready.
docs/README_config_docs_intro.md (1)
1-3: Good starting point.Add cross-links to Config_TOML_Reference and generation workflow when available.
docs/README_nix_flake_json_processor_flake.md (1)
1-22: Fix markdown list indentation per linter.Static analysis detects inconsistent list indentation. Apply this diff to align with standard Markdown formatting (2-space indent for top-level lists, 4-space for nested):
**Inputs:** -* `nixpkgs`: `github:meta-introspector/nixpkgs?ref=feature/CRQ-016-nixify` - * Standard `nixpkgs` from `meta-introspector`. -* `standalonex`: `github:meta-introspector/rust-bootstrap-nix?ref=feature/bootstrap-001&dir=standalonex` - * **Self-Reference:** This input directly references the `standalonex` sub-flake within the `rust-bootstrap-nix` repository, specifically pointing to the `feature/bootstrap-001` branch and the `standalonex` directory. This demonstrates how sub-flakes within the same repository can expose their outputs for consumption by other flakes. +* `nixpkgs`: `github:meta-introspector/nixpkgs?ref=feature/CRQ-016-nixify` + * Standard `nixpkgs` from `meta-introspector`. +* `standalonex`: `github:meta-introspector/rust-bootstrap-nix?ref=feature/bootstrap-001&dir=standalonex` + * **Self-Reference:** This input directly references the `standalonex` sub-flake within the `rust-bootstrap-nix` repository, specifically pointing to the `feature/bootstrap-001` branch and the `standalonex` directory. This demonstrates how sub-flakes within the same repository can expose their outputs for consumption by other flakes. **Outputs:** -* **`packages.aarch64-linux.default`:** - * This output creates a derivation named `processed-json-output`. - * It reads the `xpy_json_output.json` file from the `standalonex.packages.aarch64-linux.default` (which is the default package output of the `standalonex` flake). - * The content of `xpy_json_output.json` is parsed as JSON using `builtins.fromJSON`. - * The parsed JSON content is then written to `$out/output.txt` within the derivation. +* **`packages.aarch64-linux.default`:** + * This output creates a derivation named `processed-json-output`. + * It reads the `xpy_json_output.json` file from the `standalonex.packages.aarch64-linux.default` (which is the default package output of the `standalonex` flake). + * The content of `xpy_json_output.json` is parsed as JSON using `builtins.fromJSON`. + * The parsed JSON content is then written to `$out/output.txt` within the derivation.docs/QA_Plan.md (1)
1-40: Fix markdown list indentation per linter.Static analysis detects inconsistent list indentation throughout the QA plan. Apply this diff to align with standard Markdown formatting:
## 1. Identify Key Test Areas: -* **`bootstrap-config-builder`:** - * Configuration loading, merging, and override logic. - * Precondition checks (`preconditions.rs`). - * `nix-dir` tool functionality (output, filtering, JSON). -* **Rust Bootstrap Integration with Nix:** - * Correct reading and utilization of Nix configuration from `ParsedConfig`. - * Accurate execution of Nix commands from Rust (e.g., `nix eval`, `nix build`). - * Successful integration of resolved Nix store paths into the Rust build process. - * Verification of the 8-level bootstrap process. -* **Overall Project Build & Test Commands:** - * Functionality of the main `Makefile` targets. - * Correctness of `nix-build` and `nix-shell` commands. -* **`devShell` Environment:** - * Ensuring all necessary development tools are available and correctly configured. +* **`bootstrap-config-builder`:** + * Configuration loading, merging, and override logic. + * Precondition checks (`preconditions.rs`). + * `nix-dir` tool functionality (output, filtering, JSON). +* **Rust Bootstrap Integration with Nix:** + * Correct reading and utilization of Nix configuration from `ParsedConfig`. + * Accurate execution of Nix commands from Rust (e.g., `nix eval`, `nix build`). + * Successful integration of resolved Nix store paths into the Rust build process. + * Verification of the 8-level bootstrap process. +* **Overall Project Build & Test Commands:** + * Functionality of the main `Makefile` targets. + * Correctness of `nix-build` and `nix-shell` commands. +* **`devShell` Environment:** + * Ensuring all necessary development tools are available and correctly configured. ## 2. Determine Test Types and Tools: -* **Unit Tests:** Leverage Rust's built-in `cargo test` for individual functions and modules, especially within `bootstrap-config-builder` and the refactored `standalonex/src/bootstrap/src/lib.rs` components. -* **Integration Tests:** - * **Rust-Nix Interaction:** Write Rust tests that call the Nix interaction logic and assert on the outcomes (e.g., correct Nix command execution, valid path resolution). - * **Component Interaction:** Test the flow between `bootstrap-config-builder` and the main Rust bootstrap process. -* **System/End-to-End Tests:** - * **Shell Scripts:** Create or enhance existing shell scripts (`test.sh`, `run_bootstrap_test.sh`, etc.) to execute the full bootstrap process and verify the final output (e.g., successful compilation, correct artifacts). - * **Nix Checks:** Use `nix-build --check` and potentially `nix-diff` to ensure flake outputs are consistent and correct. -* **Static Analysis & Linting:** Ensure pre-commit hooks (`.pre-commit-config.yaml`) are comprehensive and run regularly. This includes `rustfmt`, `clippy`, and potentially `shellcheck` for shell scripts. -* **Documentation Review:** Regularly verify that `docs/` accurately reflects the current state and functionality of the project. +* **Unit Tests:** Leverage Rust's built-in `cargo test` for individual functions and modules, especially within `bootstrap-config-builder` and the refactored `standalonex/src/bootstrap/src/lib.rs` components. +* **Integration Tests:** + * **Rust-Nix Interaction:** Write Rust tests that call the Nix interaction logic and assert on the outcomes (e.g., correct Nix command execution, valid path resolution). + * **Component Interaction:** Test the flow between `bootstrap-config-builder` and the main Rust bootstrap process. +* **System/End-to-End Tests:** + * **Shell Scripts:** Create or enhance existing shell scripts (`test.sh`, `run_bootstrap_test.sh`, etc.) to execute the full bootstrap process and verify the final output (e.g., successful compilation, correct artifacts). + * **Nix Checks:** Use `nix-build --check` and potentially `nix-diff` to ensure flake outputs are consistent and correct. +* **Static Analysis & Linting:** Ensure pre-commit hooks (`.pre-commit-config.yaml`) are comprehensive and run regularly. This includes `rustfmt`, `clippy`, and potentially `shellcheck` for shell scripts. +* **Documentation Review:** Regularly verify that `docs/` accurately reflects the current state and functionality of the project. ## 3. Proposed Next Steps for Implementation: -* **Review Existing Tests:** Identify current unit, integration, and system tests. -* **Prioritize Test Cases:** Based on the `TODO.md`, focus on critical paths first (e.g., `bootstrap-config-builder` output, core Nix integration). -* **Expand Unit Test Coverage:** For new and refactored Rust code. -* **Develop Integration Tests:** Specifically for the Rust-Nix interface. -* **Enhance End-to-End Scripts:** To cover the full build process and verify outputs. +* **Review Existing Tests:** Identify current unit, integration, and system tests. +* **Prioritize Test Cases:** Based on the `TODO.md`, focus on critical paths first (e.g., `bootstrap-config-builder` output, core Nix integration). +* **Expand Unit Test Coverage:** For new and refactored Rust code. +* **Develop Integration Tests:** Specifically for the Rust-Nix interface. +* **Enhance End-to-End Scripts:** To cover the full build process and verify outputs.docs/README_nix_flake_config.md (1)
7-21: Tighten flake reference and fix list formatting.
- Prefer using
inputs.self(or afollowspin) over a GitHub self-reference forrustBootstrapNixto avoid circular/branch-drifting deps. Clarify pinning vianarHashif referencing a remote is intentional.- Specify the exact path to
xpy_json_output.jsonwithin the referenced package output (e.g.,${pkg}/xpy_json_output.json) to avoid ambiguity.- markdownlint MD007: indent unordered lists with 2 spaces (lines 9-20).
bootstrap-config-builder/src/config.rs (1)
42-76: Reduce boilerplate in merge logic.This is repetitive and error‑prone. Consider a tiny helper/macro to assign when
Some.+macro_rules! set_if_some { + ($dst:expr, $src:expr) => { if let Some(v) = $src { $dst = Some(v); } }; +} impl AppConfig { pub fn merge_with_args(&mut self, args: &crate::args::Args) { - if let Some(stage) = args.stage.clone() { self.stage = Some(stage); } + set_if_some!(self.stage, args.stage.clone()); /* repeat for other fields... */ } }docs/Nix_Integration.md (1)
51-53: Minor style nit.Three consecutive sentences start with “This …”. Reword for flow.
README.md (2)
479-483: Capitalize “GitHub”.Fix casing in “github URL”.
7-226: Fix list indentation (MD007).Multiple lists use 4 or 8 spaces. Use 2‑space indentation for nested bullets to satisfy markdownlint.
Also applies to: 229-301, 315-418
bootstrap-config-builder/src/lib.rs (4)
18-21: Move CLImainout oflib.rs.Having
main()in a library crate is non‑idiomatic and complicates reuse/testing. Expose apub fn run(args: Args)in lib and add a bin withfn main() { run(Args::parse())? }.- fn main() -> Result<()> { + pub fn run(args: Args) -> Result<()> { - env_logger::init(); - let args = Args::parse(); + env_logger::init();Create
src/bin/bootstrap-config-builder.rs:fn main() -> bootstrap_config_builder::Result<()> { use clap::Parser; let args = bootstrap_config_builder::args::Args::parse(); bootstrap_config_builder::run(args) }
67-98: Avoid silent lossy path conversion and empty defaults.Using
to_str().unwrap_or_default()can emit empty or mangled paths, producing invalid configs. Prefer explicit errors on non‑UTF8 or omit fields whenNone.Example pattern:
let nixpkgs = app_config .nixpkgs_path .as_deref() .map(|p| p.to_str().ok_or_else(|| anyhow!("nixpkgs_path not UTF-8"))) .transpose()? .unwrap_or_default();If omission is acceptable, consider skipping those lines in the template when values are
None.
101-111: Ensure output directory exists.Create parent dirs before writing to avoid IO errors for paths like
out/config/config.toml.- let output_path = app_config.output.unwrap_or_else(|| "config.toml".into()); + let output_path = app_config.output.unwrap_or_else(|| "config.toml".into()); + if let Some(parent) = output_path.parent() { + if !parent.as_os_str().is_empty() { + fs::create_dir_all(parent) + .with_context(|| format!("Failed to create parent dir: {:?}", parent))?; + } + }
88-88: Defaultingjobsto 0—verify semantics.
jobs = 0may be treated differently (or invalid) by consumers. Consider using CPU count or omitting when unset.standalonex/src/bootstrap/src/build_impl_main.rs (1)
55-55: Use idiomaticis_empty()instead oflen() > 0.- if failures.len() > 0 { + if !failures.is_empty() {TODO.md (1)
11-25: Optional: Fix nested list indentation for consistency.The static analysis tool flagged inconsistent indentation in nested list items (should be 2-space increments, currently 4). While this doesn't affect functionality, consistent formatting improves readability.
Based on learnings
docs/README_nix_flake_root.md (1)
1-44: Optional: Address nested list indentation.Similar to TODO.md, the static analysis tool flagged nested list indentation inconsistencies (MD007). Consider fixing these for better formatting consistency across documentation files.
bootstrap-config-builder/src/utils/construct_config_content.rs (2)
3-36: Consider introducing a configuration struct to reduce parameter count.While
#[allow(clippy::too_many_arguments)]suppresses the warning, 26 parameters make this function difficult to use and maintain. Consider introducing a builder pattern or configuration struct to group related parameters.For example:
pub struct ConfigContent { pub system: String, pub flake_path_str: String, // ... other fields } impl ConfigContent { pub fn builder() -> ConfigContentBuilder { ConfigContentBuilder::default() } pub fn construct(&self) -> String { format_file::format_file( &self.system, &self.flake_path_str, // ... ) } }This would improve ergonomics and make it easier to add new configuration options in the future.
37-70: Consider whether this wrapper function adds value.The function is a simple pass-through to
format_file::format_file. If it doesn't add additional logic or abstraction, consider whether callers should invokeformat_file::format_filedirectly.Makefile (3)
22-32: Unclear positional arguments in bootstrap-config-builder invocation.Lines 22 passes
0 aarch64-unknown-linux-gnuas the first two positional arguments without explanation. These appear to be a stage number and target triple, but this is not documented and makes the Makefile harder to understand.Consider either:
- Adding a comment explaining these positional arguments
- Using named flags (e.g.,
--stage 0 --target aarch64-unknown-linux-gnu) if supported- Using Make variables to make these configurable:
STAGE ?= 0 TARGET ?= aarch64-unknown-linux-gnu SYSTEM ?= aarch64-linux generate-config: build-config-builder @echo "Generating config.toml using bootstrap-config-builder..." # ... nix build evals ... @RUST_LOG=debug ./target/debug/bootstrap-config-builder $(STAGE) $(TARGET) \ --project-root $(CURDIR) \ --system $(SYSTEM) \ # ... rest of flags
17-21: Duplicated nix build evaluation blocks.The same five
nix buildcommands are duplicated in bothgenerate-config(lines 17-21) andrun-config-builder-dry-run(lines 36-40). Consider extracting these into a shared target or Make variables to reduce duplication..PHONY: compute-nix-paths compute-nix-paths: $(eval NIXPKGS_PATH := $(shell nix build .#nixpkgsOutPath --no-link --print-out-paths)) $(eval RUST_OVERLAY_PATH := $(shell nix build .#rustOverlayOutPath --no-link --print-out-paths)) $(eval RUST_BOOTSTRAP_NIX_PATH := $(shell nix build .#rustBootstrapNixOutPath --no-link --print-out-paths)) $(eval CONFIGURATION_NIX_PATH := $(shell nix build .#configurationNixOutPath --no-link --print-out-paths)) $(eval RUST_SRC_FLAKE_PATH := $(shell nix build .#rustSrcFlakeOutPath --no-link --print-out-paths)) generate-config: build-config-builder compute-nix-paths @echo "Generating config.toml using bootstrap-config-builder..." # ... rest of target using $(NIXPKGS_PATH), etc.Also applies to: 36-40
1-52: Optional: Consider adding conventional phony targets.The static analysis tool suggests adding
cleanandtestphony targets as these are conventional in Makefiles. While not required, they would improve the user experience.As per coding guidelines
standalonex/src/bootstrap/src/build_impl_new.rs (2)
217-220: Fix log argument order for symlink_dir message.Message currently prints
host => build_triplebut the call issymlink_dir(build_triple, host).- t!( - symlink_dir(&build.config, &build_triple, &host), - format!("symlink_dir({} => {})", host.display(), build_triple.display()) - ); + t!( + symlink_dir(&build.config, &build_triple, &host), + format!("symlink_dir({} => {})", build_triple.display(), host.display()) + );
95-112: Prune stale commented code or guard with cfg(test).The large commented rustc existence check adds noise. Consider deleting or gating with
#[cfg(test)]if needed.standalonex/flake.nix (1)
22-26: DevShell duplicates cargo; rust-bin toolchain already includes it.Drop
pkgs.cargoto avoid confusion.- pkgs.rust-bin.stable."1.84.1".default - pkgs.cargo + pkgs.rust-bin.stable."1.84.1".defaultflake.nix (1)
122-146: bootstrapConfigBuilder appears unwired; simplify or expose.This derivation isn’t used by
defaultand mainly arranges symlinks. Consider removing it or exposing it as a first-class output; also preferrunCommand/writeTextDirovermkDerivation src = null.If keeping, switch to runCommand and create only stage0 unless later stages are consumed:
- bootstrapConfigBuilder = pkgs_aarch64.stdenv.mkDerivation { - pname = "rust-bootstrap-config-builder"; - version = "0.1.0"; - src = null; - configStage0 = configTomlStages_aarch64.configStage0; - configStage1 = configTomlStages_aarch64.configStage1; - configStage2 = configTomlStages_aarch64.configStage2; - installPhase = '' - mkdir -p $out/standalonex/src/bootstrap/stage0 - mkdir -p $out/standalonex/src/bootstrap/stage1 - mkdir -p $out/standalonex/src/bootstrap/stage2 - ln -s $configStage0 $out/standalonex/src/bootstrap/stage0/config.toml - ln -s $configStage1 $out/standalonex/src/bootstrap/stage1/config.toml - ln -s $configStage2 $out/standalonex/src/bootstrap/stage2/config.toml - ''; - }; + bootstrapConfigBuilder = pkgs_aarch64.runCommand "rust-bootstrap-config-builder" { + configStage0 = configTomlStages_aarch64.configStage0; + } '' + mkdir -p $out/standalonex/src/bootstrap/stage0 + ln -s "$configStage0" "$out/standalonex/src/bootstrap/stage0/config.toml" + '';Repeat for x86_64.
Also applies to: 148-172
standalonex/src/bootstrap/src/build_struct.rs (1)
5-10: Remove unused imports.
ghaandGitRepoare not referenced here.-use build_helper::ci::gha; -using crate::enums::{DocTests, GitRepo}; +use crate::enums::DocTests;standalonex/src/bootstrap/src/build_impl_paths.rs (1)
20-31: Inconsistent target join style.You use
self.out.join(&*target.triple)in some places andself.out.join(target)in others. Prefer one (the latter ifTargetSelection: AsRef<Path>).Also applies to: 32-55
bootstrap-config-builder/src/lib.rs
Outdated
| let rust_src_flake_path_lossy = app_config.rust_src_flake_path | ||
| .as_ref() | ||
| .map(|p| p.to_string_lossy().to_string()) | ||
| .unwrap_or_default(); | ||
|
|
||
| debug!("rust_src_flake_path: {:?}", rust_src_flake_path_lossy); | ||
|
|
||
| preconditions::check_rust_toolchain_sysroot( | ||
| &rust_src_flake_path_lossy, | ||
| )?; | ||
| info!("Rust toolchain sysroot check passed."); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Require rust_src_flake_path explicitly.
An empty default leads to confusing precondition errors. Fail fast if missing.
- let rust_src_flake_path_lossy = app_config.rust_src_flake_path
- .as_ref()
- .map(|p| p.to_string_lossy().to_string())
- .unwrap_or_default();
+ let rust_src_flake_path_lossy = app_config
+ .rust_src_flake_path
+ .as_ref()
+ .context("`rust_src_flake_path` is required")?
+ .to_string_lossy()
+ .to_string();Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In bootstrap-config-builder/src/lib.rs around lines 52 to 63, the code currently
defaults app_config.rust_src_flake_path to an empty string which leads to
confusing precondition failures; instead, require rust_src_flake_path to be
present and fail fast: remove the unwrap_or_default usage, check if
app_config.rust_src_flake_path is Some and if not return an explicit error (with
a clear message like "rust_src_flake_path is required") from the function,
otherwise convert the Path to a lossy string and proceed to call
preconditions::check_rust_toolchain_sysroot as before.
| let template_content = r#"# Generated by bootstrap-config-builder | ||
| # | ||
| # System: {system} | ||
| # Project Root: {flake_path_str} | ||
| [nix] | ||
| nixpkgs_path = "{nixpkgs_path}" | ||
| rust_overlay_path = "{rust_overlay_path}" | ||
| rust_bootstrap_nix_path = "{rust_bootstrap_nix_path}" | ||
| configuration_nix_path = "{configuration_nix_path}" | ||
| rust_src_flake_path = "{rust_src_flake_path}" | ||
| rust_bootstrap_nix_flake_ref = "{rust_bootstrap_nix_flake_ref}" | ||
| rust_src_flake_ref = "{rust_src_flake_ref}" | ||
| [rust] | ||
| rustc = "{rustc_path}" | ||
| cargo = "{cargo_path}" | ||
| channel = "{rust_channel}" | ||
| download-rustc = {rust_download_rustc} | ||
| parallel-compiler = {rust_parallel_compiler} | ||
| llvm-tools = {rust_llvm_tools} | ||
| debuginfo-level = {rust_debuginfo_level} | ||
| [build] | ||
| stage = {stage} | ||
| target = "{target}" | ||
| patch-binaries-for-nix = {patch_binaries_for_nix} | ||
| vendor = {vendor} | ||
| build-dir = "{build_dir}" | ||
| jobs = {build_jobs} | ||
| [env] | ||
| HOME = "{home_dir}" | ||
| CARGO_HOME = "{cargo_home_dir}" | ||
| [install] | ||
| prefix = "{install_prefix}" | ||
| sysconfdir = "{install_sysconfdir}" | ||
| [dist] | ||
| sign-folder = "{dist_sign_folder}" | ||
| upload-addr = "{dist_upload_addr}" | ||
| [llvm] | ||
| download-ci-llvm = {llvm_download_ci_llvm} | ||
| ninja = {llvm_ninja} | ||
| # Example for target-specific configurations | ||
| # [target.{target_triple}] | ||
| # cc = "{target_cc}" | ||
| # android-ndk = "{target_android_ndk}" | ||
| [change-id] | ||
| id = "{change_id}" | ||
| "#; |
There was a problem hiding this comment.
Unescaped string interpolation can produce invalid TOML.
Values inserted inside quotes (paths, refs, addrs) aren’t TOML‑escaped. A quote or backslash in any value breaks the file. Either model the config as structs and toml::to_string_pretty, or minimally escape quoted fields.
Suggested minimal fix (escape helper + apply to quoted replacements):
+fn escape_toml_str(s: &str) -> String {
+ s.chars().flat_map(|c| match c {
+ '\\' => "\\\\".chars().collect::<Vec<_>>(),
+ '\"' => "\\\"".chars().collect::<Vec<_>>(),
+ '\n' => "\\n".chars().collect::<Vec<_>>(),
+ '\r' => "\\r".chars().collect::<Vec<_>>(),
+ '\t' => "\\t".chars().collect::<Vec<_>>(),
+ _ => vec![c],
+ }).collect()
+}
@@
- .replace("{nixpkgs_path}", nixpkgs_path)
+ .replace("{nixpkgs_path}", &escape_toml_str(nixpkgs_path))
- .replace("{rust_overlay_path}", rust_overlay_path)
+ .replace("{rust_overlay_path}", &escape_toml_str(rust_overlay_path))
- .replace("{rust_bootstrap_nix_path}", rust_bootstrap_nix_path)
+ .replace("{rust_bootstrap_nix_path}", &escape_toml_str(rust_bootstrap_nix_path))
- .replace("{dist_upload_addr}", dist_upload_addr)
+ .replace("{dist_upload_addr}", &escape_toml_str(dist_upload_addr))
- .replace("{change_id}", change_id)
+ .replace("{change_id}", &escape_toml_str(change_id))Repeat for all values placed inside quotes. Numeric/bool fields can remain unquoted. Longer‑term, prefer a typed struct + toml serialization to avoid manual escaping.
Also applies to: 94-127
🤖 Prompt for AI Agents
In bootstrap-config-builder/src/utils/format_file.rs around lines 38 to 92 (and
also applies to lines 94 to 127), the raw template uses unescaped string
interpolation for TOML quoted fields which allows quotes or backslashes in
inserted values to break the generated TOML; fix this by either (A) converting
the configuration to strongly-typed structs and serializing with
toml::to_string_pretty (preferred), or (B) implement and call an escape helper
that properly escapes quotes and backslashes (and other TOML string
requirements) for every value placed inside quotes in the template (apply the
helper to all quoted replacements), leaving numeric/bool fields unquoted. Ensure
the chosen approach is used consistently for all inserted fields to produce
valid TOML.
| // Create a dummy example.toml file for the test | ||
| let temp_dir = tempfile::tempdir().unwrap(); | ||
| let template_path = temp_dir.path().join("example.toml"); | ||
| std::fs::write(&template_path, template_content).unwrap(); | ||
|
|
||
| let result = format_file::format_file( | ||
| template_path.to_str().unwrap(), | ||
| "test_system", | ||
| "/test/flake_path", | ||
| "/test/nixpkgs", | ||
| "/test/rust_overlay", | ||
| "/test/bootstrap_nix_path", | ||
| "/test/config_nix_path", | ||
| "/test/rust_src_path", | ||
| "test_bootstrap_ref", | ||
| "test_src_ref", | ||
| "1", | ||
| "x86_64-unknown-linux-gnu", | ||
| "/test/rustc", | ||
| "/test/cargo", | ||
| "stable", | ||
| false, | ||
| true, | ||
| false, | ||
| 1, | ||
| true, | ||
| false, | ||
| "/test/build_dir", | ||
| 4, | ||
| "/test/home", | ||
| "/test/cargo_home", | ||
| "/test/install_prefix", | ||
| "/test/install_sysconfdir", | ||
| "/test/dist_sign_folder", | ||
| "test_upload_addr", | ||
| false, | ||
| true, | ||
| "test_change_id", | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
Broken test: wrong format_file parameter order and unused template file.
format_file does not read a template file; first arg is system. The current call passes a path as system, so assertions fail. Remove the temp file and pass args in the correct order.
- // Create a dummy example.toml file for the test
- let temp_dir = tempfile::tempdir().unwrap();
- let template_path = temp_dir.path().join("example.toml");
- std::fs::write(&template_path, template_content).unwrap();
-
- let result = format_file::format_file(
- template_path.to_str().unwrap(),
- "test_system",
+ let result = format_file::format_file(
+ "test_system",
"/test/flake_path",
"/test/nixpkgs",
"/test/rust_overlay",
"/test/bootstrap_nix_path",
"/test/config_nix_path",
"/test/rust_src_path",
"test_bootstrap_ref",
"test_src_ref",
"1",
"x86_64-unknown-linux-gnu",
"/test/rustc",
"/test/cargo",
"stable",
false,
true,
false,
1,
true,
false,
"/test/build_dir",
4,
"/test/home",
"/test/cargo_home",
"/test/install_prefix",
"/test/install_sysconfdir",
"/test/dist_sign_folder",
"test_upload_addr",
false,
true,
"test_change_id",
);🏁 Script executed:
#!/bin/bash
# Validate signature order of format_file and grep call sites
rg -n 'pub fn format_file\(' -n bootstrap-config-builder/src/utils/format_file.rs -A5
rg -n 'format_file::format_file\(' -C2 bootstrap-config-builderLength of output: 1083
Fix broken test: remove unused template file and correct format_file parameter order.
The test incorrectly passes template_path as the first argument when format_file expects system as its first parameter (followed by flake_path_str). The temp file is never used. The correct argument order is shown in bootstrap-config-builder/src/utils/construct_config_content.rs line 37-39.
Apply the suggested diff to remove the unused temp file creation (lines 159-163) and reorder arguments so "test_system" is passed first.
🤖 Prompt for AI Agents
In bootstrap-config-builder/tests/config_integration_test.rs around lines 159 to
197, remove the unused temporary template file creation (the tempfile::tempdir /
template_path / std::fs::write block around lines 159–163) and call format_file
with the correct parameter order (pass "test_system" as the first argument and
then the flake_path_str and the remaining parameters in the order expected by
bootstrap-config-builder/src/utils/construct_config_content.rs lines 37–39);
update the argument list so the temp file path is not passed and "test_system"
is the first parameter.
| echo "--- Creating config.toml using bootstrap-config-builder ---" | ||
| ./bootstrap-config-builder/target/debug/bootstrap-config-builder \ | ||
| --project-root "$(pwd)" \ | ||
| --rustc-path "$(which rustc)" \ | ||
| --cargo-path "$(which cargo)" \ | ||
| --patch-binaries-for-nix true \ | ||
| --vendor true \ | ||
| --output config.toml |
There was a problem hiding this comment.
🧩 Analysis chain
Add error handling for bootstrap-config-builder invocation.
The script lacks error handling if the bootstrap-config-builder binary fails or doesn't exist. Additionally, the hard-coded relative path assumes execution from the project root, which is fragile.
Apply this diff to add error handling and improve robustness:
+set -e # Exit on first error
+
echo "--- Creating config.toml using bootstrap-config-builder ---"
+BUILDER_PATH="./bootstrap-config-builder/target/debug/bootstrap-config-builder"
+if [ ! -f "$BUILDER_PATH" ]; then
+ echo "Error: bootstrap-config-builder not found at $BUILDER_PATH" >&2
+ exit 1
+fi
+
-./bootstrap-config-builder/target/debug/bootstrap-config-builder \
+$BUILDER_PATH \
--project-root "$(pwd)" \
--rustc-path "$(which rustc)" \
--cargo-path "$(which cargo)" \
--patch-binaries-for-nix true \
--vendor true \
--output config.toml
+
+if [ ! -f config.toml ]; then
+ echo "Error: config.toml was not generated" >&2
+ exit 1
+fiAdditionally, verify that the --patch-binaries-for-nix and --vendor flags expect true as values (not boolean flags).
---
🏁 Script executed:
```shell
#!/bin/bash
# Inspect the full debug_build.sh file to understand context and existing error handling
fd -t f "debug_build.sh" -x cat {}
Length of output: 573
🏁 Script executed:
#!/bin/bash
# Search for bootstrap-config-builder usage/invocation patterns in codebase
rg -n "bootstrap-config-builder" -A 2 -B 2 --type=shLength of output: 324
🏁 Script executed:
#!/bin/bash
# Check if there are any command invocations or error handling patterns already in shell scripts
rg -n "set -e|set -o pipefail|trap|exit 1" -C 2 --type=sh | head -40Length of output: 2120
🏁 Script executed:
#!/bin/bash
# Look for bootstrap-config-builder binary definition or help text
find . -name "bootstrap-config-builder" -o -name "*bootstrap*config*" 2>/dev/null | head -20Length of output: 60
---
🏁 Script executed:
```shell
#!/bin/bash
# Inspect run_bootstrap_config_builder.sh for proper invocation patterns
cat ./run_bootstrap_config_builder.sh
Length of output: 392
🏁 Script executed:
#!/bin/bash
# Search for bootstrap-config-builder source code or documentation in repo
find . -name "*.rs" -o -name "*.toml" | xargs grep -l "bootstrap-config-builder" 2>/dev/null | head -10Length of output: 230
🏁 Script executed:
#!/bin/bash
# Look for any comments or documentation about the flags
rg -n "patch-binaries-for-nix|--vendor" -B 3 -A 1 --type=sh --type=rsLength of output: 26
🏁 Script executed:
#!/bin/bash
# Check if bootstrap-config-builder is a script or binary, and inspect its header
file ./bootstrap-config-builder 2>/dev/null || echo "File check failed"
head -20 ./bootstrap-config-builder 2>/dev/null || echo "Not a text file"Length of output: 33
---
🏁 Script executed:
```shell
#!/bin/bash
# Find main.rs or argument parser in bootstrap-config-builder source
find ./bootstrap-config-builder -name "*.rs" -type f | head -20
Length of output: 701
🏁 Script executed:
#!/bin/bash
# Search for clap/argument parsing in bootstrap-config-builder source
rg -n "patch-binaries-for-nix|vendor|struct.*Args|clap" ./bootstrap-config-builder --type rust -A 2 -B 2Length of output: 9439
🏁 Script executed:
#!/bin/bash
# Check integration test for usage examples
cat ./bootstrap-config-builder/tests/config_integration_test.rs 2>/dev/null | head -100Length of output: 4574
Add error handling for bootstrap-config-builder invocation and ensure proper path resolution.
The script lacks error handling if bootstrap-config-builder fails, and the hard-coded relative path is fragile. Additionally, the script continues even if config.toml is not created, leading to potential silent failures.
Apply this diff to add error handling:
+set -e # Exit on first error
+
echo "--- Creating config.toml using bootstrap-config-builder ---"
+BUILDER_PATH="./bootstrap-config-builder/target/debug/bootstrap-config-builder"
+if [ ! -f "$BUILDER_PATH" ]; then
+ echo "Error: bootstrap-config-builder not found at $BUILDER_PATH" >&2
+ exit 1
+fi
+
-./bootstrap-config-builder/target/debug/bootstrap-config-builder \
+$BUILDER_PATH \
--project-root "$(pwd)" \
--rustc-path "$(which rustc)" \
--cargo-path "$(which cargo)" \
--patch-binaries-for-nix true \
--vendor true \
--output config.toml
+
+if [ ! -f config.toml ]; then
+ echo "Error: config.toml was not generated" >&2
+ exit 1
+fiThis aligns with the established error-handling pattern used in test_nix_preconditions.sh and test.sh (which use set -euo pipefail).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| echo "--- Creating config.toml using bootstrap-config-builder ---" | |
| ./bootstrap-config-builder/target/debug/bootstrap-config-builder \ | |
| --project-root "$(pwd)" \ | |
| --rustc-path "$(which rustc)" \ | |
| --cargo-path "$(which cargo)" \ | |
| --patch-binaries-for-nix true \ | |
| --vendor true \ | |
| --output config.toml | |
| set -e # Exit on first error | |
| echo "--- Creating config.toml using bootstrap-config-builder ---" | |
| BUILDER_PATH="./bootstrap-config-builder/target/debug/bootstrap-config-builder" | |
| if [ ! -f "$BUILDER_PATH" ]; then | |
| echo "Error: bootstrap-config-builder not found at $BUILDER_PATH" >&2 | |
| exit 1 | |
| fi | |
| $BUILDER_PATH \ | |
| --project-root "$(pwd)" \ | |
| --rustc-path "$(which rustc)" \ | |
| --cargo-path "$(which cargo)" \ | |
| --patch-binaries-for-nix true \ | |
| --vendor true \ | |
| --output config.toml | |
| if [ ! -f config.toml ]; then | |
| echo "Error: config.toml was not generated" >&2 | |
| exit 1 | |
| fi |
🤖 Prompt for AI Agents
In debug_build.sh around lines 10 to 17, the invocation of
./bootstrap-config-builder lacks error handling and uses a fragile relative
path; modify the script to enable strict error handling (set -euo pipefail) near
the top, resolve the bootstrap-config-builder path relative to the script
directory (e.g., compute SCRIPT_DIR and use
"$SCRIPT_DIR/bootstrap-config-builder/target/debug/bootstrap-config-builder"),
verify the binary exists and is executable before running, run the command and
check its exit status, and after running ensure config.toml was actually created
(if not, print an error to stderr and exit non-zero) so the script fails fast on
errors.
| * `prefix`: | ||
| * **Purpose:** Specifies the base directory for all installed components. In a Nix environment, this will typically be a path within the Nix store (e.g., `/nix/store/...-rust-toolchain`). All other installation paths (like `bindir`, `libdir`, etc.) will be derived from this prefix unless explicitly overridden. | ||
| * **Example:** `prefix = "/nix/store/some-hash-my-rust-package"` | ||
|
|
||
| * `bindir`: | ||
| * **Purpose:** Specifies the directory for executable binaries. | ||
| * **Behavior:** If `prefix` is set and `bindir` is *not* explicitly defined, `bindir` will automatically default to `prefix/bin`. This ensures that your executables are placed correctly within the specified installation prefix. | ||
| * **Example (explicitly set):** `bindir = "/usr/local/bin"` (overrides the default `prefix/bin`) | ||
|
|
||
| * `libdir`, `sysconfdir`, `docdir`, `mandir`, `datadir`: | ||
| * **Purpose:** These fields specify directories for libraries, configuration files, documentation, manual pages, and data files, respectively. | ||
| * **Behavior:** If `prefix` is set, these paths are typically expected to be relative to the `prefix` unless an absolute path is provided. |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Fix markdown list indentation (MD007).
Nested list items use 4-space indentation, but the markdown linter expects 2-space indentation per the MD007 rule. Adjust indentation for all nested bullet points.
-* `prefix`:
- * **Purpose:** Specifies the base directory for all installed components. In a Nix environment, this will typically be a path within the Nix store (e.g., `/nix/store/...-rust-toolchain`). All other installation paths (like `bindir`, `libdir`, etc.) will be derived from this prefix unless explicitly overridden.
- * **Example:** `prefix = "/nix/store/some-hash-my-rust-package"`
+* `prefix`:
+ * **Purpose:** Specifies the base directory for all installed components. In a Nix environment, this will typically be a path within the Nix store (e.g., `/nix/store/...-rust-toolchain`). All other installation paths (like `bindir`, `libdir`, etc.) will be derived from this prefix unless explicitly overridden.
+ * **Example:** `prefix = "/nix/store/some-hash-my-rust-package"`
-* `bindir`:
- * **Purpose:** Specifies the directory for executable binaries.
- * **Behavior:** If `prefix` is set and `bindir` is *not* explicitly defined, `bindir` will automatically default to `prefix/bin`. This ensures that your executables are placed correctly within the specified installation prefix.
- * **Example (explicitly set):** `bindir = "/usr/local/bin"` (overrides the default `prefix/bin`)
+* `bindir`:
+ * **Purpose:** Specifies the directory for executable binaries.
+ * **Behavior:** If `prefix` is set and `bindir` is *not* explicitly defined, `bindir` will automatically default to `prefix/bin`. This ensures that your executables are placed correctly within the specified installation prefix.
+ * **Example (explicitly set):** `bindir = "/usr/local/bin"` (overrides the default `prefix/bin`)
-* `libdir`, `sysconfdir`, `docdir`, `mandir`, `datadir`:
- * **Purpose:** These fields specify directories for libraries, configuration files, documentation, manual pages, and data files, respectively.
- * **Behavior:** If `prefix` is set, these paths are typically expected to be relative to the `prefix` unless an absolute path is provided.
+* `libdir`, `sysconfdir`, `docdir`, `mandir`, `datadir`:
+ * **Purpose:** These fields specify directories for libraries, configuration files, documentation, manual pages, and data files, respectively.
+ * **Behavior:** If `prefix` is set, these paths are typically expected to be relative to the `prefix` unless an absolute path is provided.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * `prefix`: | |
| * **Purpose:** Specifies the base directory for all installed components. In a Nix environment, this will typically be a path within the Nix store (e.g., `/nix/store/...-rust-toolchain`). All other installation paths (like `bindir`, `libdir`, etc.) will be derived from this prefix unless explicitly overridden. | |
| * **Example:** `prefix = "/nix/store/some-hash-my-rust-package"` | |
| * `bindir`: | |
| * **Purpose:** Specifies the directory for executable binaries. | |
| * **Behavior:** If `prefix` is set and `bindir` is *not* explicitly defined, `bindir` will automatically default to `prefix/bin`. This ensures that your executables are placed correctly within the specified installation prefix. | |
| * **Example (explicitly set):** `bindir = "/usr/local/bin"` (overrides the default `prefix/bin`) | |
| * `libdir`, `sysconfdir`, `docdir`, `mandir`, `datadir`: | |
| * **Purpose:** These fields specify directories for libraries, configuration files, documentation, manual pages, and data files, respectively. | |
| * **Behavior:** If `prefix` is set, these paths are typically expected to be relative to the `prefix` unless an absolute path is provided. | |
| * `prefix`: | |
| * **Purpose:** Specifies the base directory for all installed components. In a Nix environment, this will typically be a path within the Nix store (e.g., `/nix/store/...-rust-toolchain`). All other installation paths (like `bindir`, `libdir`, etc.) will be derived from this prefix unless explicitly overridden. | |
| * **Example:** `prefix = "/nix/store/some-hash-my-rust-package"` | |
| * `bindir`: | |
| * **Purpose:** Specifies the directory for executable binaries. | |
| * **Behavior:** If `prefix` is set and `bindir` is *not* explicitly defined, `bindir` will automatically default to `prefix/bin`. This ensures that your executables are placed correctly within the specified installation prefix. | |
| * **Example (explicitly set):** `bindir = "/usr/local/bin"` (overrides the default `prefix/bin`) | |
| * `libdir`, `sysconfdir`, `docdir`, `mandir`, `datadir`: | |
| * **Purpose:** These fields specify directories for libraries, configuration files, documentation, manual pages, and data files, respectively. | |
| * **Behavior:** If `prefix` is set, these paths are typically expected to be relative to the `prefix` unless an absolute path is provided. |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
6-6: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
7-7: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
10-10: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
11-11: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
12-12: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
15-15: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
16-16: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
🤖 Prompt for AI Agents
In docs/README_config_install_section.md around lines 5 to 16, several nested
list items are indented with 4 spaces which violates MD007; change all nested
bullet and sub-bullet indentations to use 2 spaces per nesting level (e.g.,
sub-items under a top-level bullet should be indented by 2 spaces, not 4) and
ensure consistency across the `prefix`, `bindir`, and `libdir`/`sysconfdir`
blocks so the linter no longer flags MD007.
| pub fn rust_info(&self) -> &GitInfo { | ||
| &self.config.rust_info | ||
| } | ||
|
|
||
| /// Copies a file from `src` to `dst`. | ||
| /// | ||
| /// If `src` is a symlink, `src` will be resolved to the actual path | ||
| /// and copied to `dst` instead of the symlink itself. | ||
| pub fn resolve_symlink_and_copy(&self, src: &Path, dst: &Path) { | ||
| self.copy_link_internal(src, dst, true); | ||
| } | ||
|
|
||
| /// Links a file from `src` to `dst`. | ||
| /// Attempts to use hard links if possible, falling back to copying. | ||
| /// You can neither rely on this being a copy nor it being a link, | ||
| /// so do not write to dst. | ||
| pub fn copy_link(&self, src: &Path, dst: &Path) { | ||
| self.copy_link_internal(src, dst, false); | ||
| } | ||
|
|
||
| fn copy_link_internal(&self, src: &Path, dst: &Path, dereference_symlinks: bool) { | ||
| if self.config.dry_run { | ||
| return; | ||
| } | ||
| self.verbose_than(1, || println!("Copy/Link {src:?} to {dst:?}")); | ||
| if src == dst { | ||
| return; | ||
| } | ||
| if let Err(e) = fs::remove_file(dst) { | ||
| if cfg!(windows) && e.kind() != io::ErrorKind::NotFound { | ||
| // workaround for https://github.com/rust-lang/rust/issues/127126 | ||
| // if removing the file fails, attempt to rename it instead. | ||
| let now = t!(SystemTime::now().duration_since(SystemTime::UNIX_EPOCH)); | ||
| let _ = fs::rename(dst, format!("{}-{}", dst.display(), now.as_nanos())); | ||
| } | ||
| } | ||
| let metadata = t!(src.symlink_metadata(), format!("src = {}", src.display())); | ||
| let mut src = src.to_path_buf(); | ||
| if metadata.file_type().is_symlink() { | ||
| if dereference_symlinks { | ||
| src = t!(fs::canonicalize(src)); | ||
| } else { | ||
| let link = t!(fs::read_link(src)); | ||
| t!(self.symlink_file(link, dst)); | ||
| return; | ||
| } | ||
| } | ||
| if let Ok(()) = fs::hard_link(&src, dst) { | ||
| // Attempt to "easy copy" by creating a hard link (symlinks are privileged on windows), | ||
| // but if that fails just fall back to a slow `copy` operation. | ||
| } else { | ||
| if let Err(e) = fs::copy(&src, dst) { | ||
| panic!("failed to copy `{}` to `{}`: {}", src.display(), dst.display(), e) | ||
| } | ||
| t!(fs::set_permissions(dst, metadata.permissions())); | ||
|
|
||
| // Restore file times because changing permissions on e.g. Linux using `chmod` can cause | ||
| // file access time to change. | ||
| let file_times = fs::FileTimes::new() | ||
| .set_accessed(t!(metadata.accessed())) | ||
| .set_modified(t!(metadata.modified())); | ||
| t!(set_file_times(dst, file_times)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Eliminate code duplication: rust_info and copy methods already exist.
These methods are duplicated from standalonex/src/bootstrap/src/build_impl_new.rs:
rust_info(lines 161-163) duplicates lines 681-683resolve_symlink_and_copy,copy_link, andcopy_link_internal(lines 165-224) duplicate lines 1746-1789
Additionally, line 218 has trailing whitespace.
🤖 Prompt for AI Agents
In standalonex/src/bootstrap/src/build_impl_utils.rs around lines 161-224 these
methods (rust_info, resolve_symlink_and_copy, copy_link, copy_link_internal) are
exact duplicates of the implementations in build_impl_new.rs (and line 218
contains trailing whitespace); remove the duplicated definitions from
build_impl_utils.rs and instead call or re-export the single canonical
implementations (move shared logic to a common module if needed, or have
build_impl_utils.rs import the functions from build_impl_new.rs), then delete
the trailing whitespace on line 218.
| /// Links the `src` directory recursively to `dst`. Both are assumed to exist | ||
| /// when this function is called. | ||
| /// Will attempt to use hard links if possible and fall back to copying. | ||
| pub fn cp_link_r(&self, src: &Path, dst: &Path) { | ||
| if self.config.dry_run { | ||
| return; | ||
| } | ||
| for f in self.read_dir(src) { | ||
| let path = f.path(); | ||
| let name = path.file_name().unwrap(); | ||
| let dst = dst.join(name); | ||
| if t!(f.file_type()).is_dir() { | ||
| t!(fs::create_dir_all(&dst)); | ||
| self.cp_link_r(&path, &dst); | ||
| } else { | ||
| self.copy_link(&path, &dst); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Copies the `src` directory recursively to `dst`. Both are assumed to exist | ||
| /// when this function is called. | ||
| /// Will attempt to use hard links if possible and fall back to copying. | ||
| /// Unwanted files or directories can be skipped | ||
| /// by returning `false` from the filter function. | ||
| pub fn cp_link_filtered(&self, src: &Path, dst: &Path, filter: &dyn Fn(&Path) -> bool) { | ||
| // Immediately recurse with an empty relative path | ||
| self.cp_link_filtered_recurse(src, dst, Path::new(""), filter) | ||
| } | ||
|
|
||
| // Inner function does the actual work | ||
| fn cp_link_filtered_recurse( | ||
| &self, | ||
| src: &Path, | ||
| dst: &Path, | ||
| relative: &Path, | ||
| filter: &dyn Fn(&Path) -> bool, | ||
| ) { | ||
| for f in self.read_dir(src) { | ||
| let path = f.path(); | ||
| let name = path.file_name().unwrap(); | ||
| let dst = dst.join(name); | ||
| let relative = relative.join(name); | ||
| // Only copy file or directory if the filter function returns true | ||
| if filter(&relative) { | ||
| if t!(f.file_type()).is_dir() { | ||
| let _ = fs::remove_dir_all(&dst); | ||
| self.create_dir(&dst); | ||
| self.cp_link_filtered_recurse(&path, &dst, &relative, filter); | ||
| } else { | ||
| let _ = fs::remove_file(&dst); | ||
| self.copy_link(&path, &dst); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| pub fn copy_link_to_folder(&self, src: &Path, dest_folder: &Path) { | ||
| let file_name = src.file_name().unwrap(); | ||
| let dest = dest_folder.join(file_name); | ||
| self.copy_link(src, &dest); | ||
| } | ||
|
|
||
| pub fn install(&self, src: &Path, dstdir: &Path, perms: u32) { | ||
| if self.config.dry_run { | ||
| return; | ||
| } | ||
| let dst = dstdir.join(src.file_name().unwrap()); | ||
| self.verbose_than(1, || println!("Install {src:?} to {dst:?}")); | ||
| t!(fs::create_dir_all(dstdir)); | ||
| if !src.exists() { | ||
| panic!("ERROR: File \"{}\" not found!", src.display()); | ||
| } | ||
| self.copy_link_internal(src, &dst, true); | ||
| chmod(&dst, perms); | ||
| } | ||
|
|
||
| pub fn read(&self, path: &Path) -> String { | ||
| if self.config.dry_run { | ||
| return String::new(); | ||
| } | ||
| t!(fs::read_to_string(path)) | ||
| } | ||
|
|
||
| pub fn create_dir(&self, dir: &Path) { | ||
| if self.config.dry_run { | ||
| return; | ||
| } | ||
| t!(fs::create_dir_all(dir)) | ||
| } | ||
|
|
||
| pub fn remove_dir(&self, dir: &Path) { | ||
| if self.config.dry_run { | ||
| return; | ||
| } | ||
| t!(fs::remove_dir_all(dir)) | ||
| } | ||
|
|
||
| pub fn read_dir(&self, dir: &Path) -> impl Iterator<Item = fs::DirEntry> { | ||
| let iter = match fs::read_dir(dir) { | ||
| Ok(v) => v, | ||
| Err(_) if self.config.dry_run => return vec![].into_iter(), | ||
| Err(err) => panic!("could not read dir {dir:?}: {err:?}"), | ||
| }; | ||
| iter.map(|e| t!(e)).collect::<Vec<_>>().into_iter() | ||
| } | ||
|
|
||
| pub fn symlink_file<P: AsRef<Path>, Q: AsRef<Path>>(&self, src: P, link: Q) -> io::Result<()> { | ||
| if self.config.dry_run { return Ok(()); } | ||
| if cfg!(unix) { | ||
| std::os::unix::fs::symlink(src.as_ref(), link.as_ref()) | ||
| } /* else if cfg!(windows) { | ||
| std::os::windows::fs::symlink_file(src.as_ref(), link.as_ref()) | ||
| } */ else { | ||
| Err(io::Error::new(io::ErrorKind::Other, "symlinks not supported on this platform")) | ||
| } | ||
| } |
There was a problem hiding this comment.
Eliminate code duplication: file operation methods already exist.
All methods in this section are duplicated from standalonex/src/bootstrap/src/build_impl_new.rs:
cp_link_r(226-244) → lines 1794-1809cp_link_filteredandcp_link_filtered_recurse(246-281) → lines 1816-1846copy_link_to_folder(283-287) → lines 1848-1852install(289-301) → lines 1854-1866read(303-308) → lines 1868-1873create_dir(310-315) → lines 1875-1880remove_dir(317-322) → lines 1882-1887read_dir(324-331) → lines 1889-1896symlink_file(333-342) → lines 1898-1907
This represents ~110 lines of duplicated file operation logic.
| /// Check if verbosity is greater than the `level` | ||
| pub fn is_verbose_than(&self, level: usize) -> bool { | ||
| self.verbosity > level | ||
| } | ||
|
|
||
| /// Runs a function if verbosity is greater than `level`. | ||
| pub fn verbose_than(&self, level: usize, f: impl Fn()) { | ||
| if self.is_verbose_than(level) { | ||
| f() | ||
| } | ||
| } | ||
|
|
||
| pub fn info(&self, msg: &str) { | ||
| match self.config.dry_run { | ||
| DryRun::SelfCheck => (), | ||
| DryRun::Disabled | DryRun::UserSelected => { | ||
| println!("{msg}"); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Eliminate code duplication: verbosity and info methods already exist.
These methods are duplicated from standalonex/src/bootstrap/src/build_impl_new.rs:
is_verbose_thanandverbose_than(344-354) → lines 1072-1081info(356-363) → lines 1083-1090
🤖 Prompt for AI Agents
In standalonex/src/bootstrap/src/build_impl_utils.rs around lines 344 to 363,
the methods is_verbose_than, verbose_than and info are duplicates of
implementations already present in
standalonex/src/bootstrap/src/build_impl_new.rs (lines ~1072-1090); remove the
duplicated methods from build_impl_utils.rs and instead reuse the single
implementation by either (a) moving the shared methods into a common helper
module (e.g., a new utils/mod or existing shared module) and importing them from
there, or (b) if both files share the same type, make the methods defined once
in the type impl in build_impl_new.rs and make build_impl_utils.rs call that
implementation (or re-export it) so there is one canonical definition; update
imports and visibility (pub/pub(crate)) as needed and run cargo build to ensure
no missing references.
| use std::cell::{Cell, RefCell}; | ||
| use std::collections::{HashMap, HashSet}; | ||
| use std::path::{PathBuf}; | ||
|
|
||
| use build_helper::ci::gha; | ||
| use crate::core::config::Config; | ||
| use crate::core::config::target_selection::TargetSelection; | ||
| use crate::enums::{DocTests, GitRepo}; | ||
| use crate::crate_struct::Crate; | ||
|
|
There was a problem hiding this comment.
Missing import for GitInfo breaks compilation.
GitInfo is used but not imported in this module.
use std::cell::{Cell, RefCell};
use std::collections::{HashMap, HashSet};
use std::path::{PathBuf};
use build_helper::ci::gha;
use crate::core::config::Config;
use crate::core::config::target_selection::TargetSelection;
use crate::enums::{DocTests, GitRepo};
use crate::crate_struct::Crate;
+use crate::utils::channel::GitInfo;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| use std::cell::{Cell, RefCell}; | |
| use std::collections::{HashMap, HashSet}; | |
| use std::path::{PathBuf}; | |
| use build_helper::ci::gha; | |
| use crate::core::config::Config; | |
| use crate::core::config::target_selection::TargetSelection; | |
| use crate::enums::{DocTests, GitRepo}; | |
| use crate::crate_struct::Crate; | |
| use std::cell::{Cell, RefCell}; | |
| use std::collections::{HashMap, HashSet}; | |
| use std::path::{PathBuf}; | |
| use build_helper::ci::gha; | |
| use crate::core::config::Config; | |
| use crate::core::config::target_selection::TargetSelection; | |
| use crate::enums::{DocTests, GitRepo}; | |
| use crate::crate_struct::Crate; | |
| use crate::utils::channel::GitInfo; |
🤖 Prompt for AI Agents
In standalonex/src/bootstrap/src/build_struct.rs around lines 1 to 10, GitInfo
is referenced but not imported; add a use statement at the top of the file to
import GitInfo from its defining module (for example use crate::git::GitInfo or
the correct module path in this repo) so the type is available and the file
compiles.
| [workspace] | ||
| members = [ | ||
| "bootstrap", | ||
| "stage0_parser_crate", | ||
| ] |
There was a problem hiding this comment.
🧩 Analysis chain
Verify workspace members and path consistency.
The workspace restructuring looks correct and aligns with the PR's modular build objectives. However, given the past review flagged path inconsistencies in workspace members, confirm that:
- Both workspace members (
bootstrapandstage0_parser_crate) exist with valid Cargo.toml files - No remaining relative path references (e.g.,
../build_helper) exist in member crates - All workspace member crates are accounted for
🏁 Script executed:
#!/bin/bash
# Verify workspace members exist and check for remaining path inconsistencies
BASE_DIR="standalonex/src"
# Check each workspace member
for member in "bootstrap" "stage0_parser_crate"; do
MEMBER_PATH="$BASE_DIR/$member"
if [ -d "$MEMBER_PATH" ]; then
echo "✓ Member '$member' directory exists at $MEMBER_PATH"
if [ -f "$MEMBER_PATH/Cargo.toml" ]; then
echo " ✓ Cargo.toml found"
else
echo " ✗ Cargo.toml NOT found"
fi
else
echo "✗ Member '$member' directory NOT found at $MEMBER_PATH"
fi
done
# Search for remaining ../build_helper or similar problematic relative paths in workspace members
echo ""
echo "Checking for remaining problematic relative paths in workspace members:"
rg -n '\.\./build_helper' "$BASE_DIR/bootstrap/" "$BASE_DIR/stage0_parser_crate/" 2>/dev/null || echo " (no ../build_helper references found)"
# List all Cargo.toml files in the workspace to ensure completeness
echo ""
echo "All Cargo.toml files found in workspace:"
find "$BASE_DIR" -maxdepth 2 -name "Cargo.toml" -type f | sortLength of output: 501
🏁 Script executed:
#!/bin/bash
# Investigate build_helper's status and cross-references
BASE_DIR="standalonex/src"
echo "=== Contents of build_helper/Cargo.toml ==="
cat "$BASE_DIR/build_helper/Cargo.toml" 2>/dev/null || echo "File not readable"
echo ""
echo "=== Checking for any path dependencies on build_helper ==="
# Search for path dependencies that reference build_helper
rg -n 'path.*build_helper|build_helper.*path' "$BASE_DIR/bootstrap/" "$BASE_DIR/stage0_parser_crate/" 2>/dev/null | head -20 || echo " (no path references to build_helper found)"
echo ""
echo "=== Checking if bootstrap or stage0_parser_crate depend on build_helper ==="
# Look for build_helper in dependencies section of member Cargo.toml files
for member in "bootstrap" "stage0_parser_crate"; do
echo "--- $member/Cargo.toml ---"
rg -A 10 '\[dependencies\]' "$BASE_DIR/$member/Cargo.toml" 2>/dev/null | grep -i "build_helper" || echo " (no build_helper dependency)"
doneLength of output: 886
🏁 Script executed:
#!/bin/bash
# Get full bootstrap/Cargo.toml to verify all dependencies
BASE_DIR="standalonex/src"
echo "=== Full bootstrap/Cargo.toml ==="
cat "$BASE_DIR/bootstrap/Cargo.toml"
echo ""
echo "=== Full stage0_parser_crate/Cargo.toml ==="
cat "$BASE_DIR/stage0_parser_crate/Cargo.toml"
echo ""
echo "=== Context around build_helper usage in bootstrap ==="
sed -n '625,645p' "$BASE_DIR/bootstrap/src/core/builder/mod.rs"Length of output: 3960
Add build_helper to workspace members or declare as bootstrap dependency.
The workspace member list is incomplete. The bootstrap crate directly uses build_helper::util::parse_gitmodules() (line 633 of standalonex/src/bootstrap/src/core/builder/mod.rs), but build_helper is neither a workspace member nor declared as a dependency in bootstrap/Cargo.toml. This will cause compilation failures.
Add "build_helper" to the workspace members list in standalonex/src/Cargo.toml, or add a path dependency in bootstrap/Cargo.toml:
build_helper = { path = "build_helper" }🤖 Prompt for AI Agents
In standalonex/src/Cargo.toml around lines 1-5, the workspace members list is
missing the build_helper crate which bootstrap directly uses
(bootstrap/src/core/builder/mod.rs calls build_helper::util::parse_gitmodules at
line 633), causing build failures; fix by either adding "build_helper" to the
workspace members array in this Cargo.toml or by adding a path dependency to
bootstrap/Cargo.toml pointing to the build_helper crate (e.g., add a dependency
entry for build_helper with the correct relative path).
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
standalonex/src/bootstrap/src/core/build_steps/compile.rs (1)
6-31: Unused imports will trip -D warnings.Either prune the list or silence locally to unblock.
+#![allow(unused_imports)]Place the inner attribute near the top of this file. Follow up with a cleanup PR to remove unneeded
uses.
♻️ Duplicate comments (7)
standalonex/flake.nix (4)
6-6: Userev=for commit hash pinning.Flake inputs should use
rev=rather thanref=when pinning to a specific commit hash.Apply this diff:
- rustSrcFlake.url = "github:meta-introspector/rust?ref=3487cd3843083db70ee30023f19344568ade9c9f"; + rustSrcFlake.url = "github:meta-introspector/rust?rev=3487cd3843083db70ee30023f19344568ade9c9f";
11-11: RemoveconfigTomlPathfrom outputs signature.Following the removal of the invalid
configTomlPathinput, this parameter must be removed from the outputs function signature.- outputs = { self, nixpkgs, rustSrcFlake, rustOverlay, configTomlPath }: + outputs = { self, nixpkgs, rustSrcFlake, rustOverlay }:
51-93: Critical: Apply proper Rust toolchain override for all packages.All three packages (
default,bootstrap-main,nix-bootstrap) incorrectly set arustcfield, whichbuildRustPackagesilently ignores. Additionally:
defaultpackage (lines 52-62): Missingpnameandversionfields;srcpath may not matchlockFilelocationbootstrap-mainpackage (lines 64-77): Missing requiredsrcattribute (build will fail); should point to crate rootnix-bootstrappackage (lines 79-93): Path consistency looks OK but still has ignoredrustcfield- All packages attempt
cp ${configTomlPath} config.toml, which will fail (see earlier comment)Create a shared
rustPlatformusingmakeRustPlatformand use it for all packages:pkgs = import nixpkgs { system = "aarch64-linux"; overlays = [ rustOverlay.overlays.default ]; }; buildHelperSrc = pkgs.lib.cleanSource ./src/build_helper; + + rustToolchain = pkgs.rust-bin.stable."1.84.1".default; + rustPlatform = pkgs.makeRustPlatform { + cargo = rustToolchain; + rustc = rustToolchain; + }; in { devShells.aarch64-linux.default = pkgs.mkShell { name = "standalonex-dev-shell"; packages = [ pkgs.python3 - pkgs.rust-bin.stable."1.84.1".default - pkgs.cargo + rustToolchain ]; # ... rest of devShell packages.aarch64-linux = { - default = pkgs.rustPlatform.buildRustPackage { + default = rustPlatform.buildRustPackage { + pname = "bootstrap"; + version = "0.1.0"; src = pkgs.lib.cleanSource ./src; cargoLock.lockFile = ./src/Cargo.lock; - rustc = pkgs.rust-bin.stable."1.84.1".default; doCheck = false; postPatch = '' mkdir -p .cargo cp -r ${buildHelperSrc} build_helper - cp ${configTomlPath} config.toml + # TODO: Generate config.toml properly ''; }; - bootstrap-main = pkgs.rustPlatform.buildRustPackage { + bootstrap-main = rustPlatform.buildRustPackage { pname = "bootstrap-main"; version = "0.1.0"; - + src = pkgs.lib.cleanSource ./src; # Verify this is the correct crate root cargoLock.lockFile = ./src/Cargo.lock; - rustc = pkgs.rust-bin.stable."1.84.1".default; doCheck = false; cargoBuildFlags = [ "--bin" "bootstrap" ]; postPatch = '' mkdir -p .cargo cp -r ${buildHelperSrc} build_helper - cp ${configTomlPath} config.toml + # TODO: Generate config.toml properly ''; }; - nix-bootstrap = pkgs.rustPlatform.buildRustPackage { + nix-bootstrap = rustPlatform.buildRustPackage { pname = "nix-bootstrap"; version = "0.1.0"; - src = pkgs.lib.cleanSource ./src; cargoLock.lockFile = ./src/Cargo.lock; - rustc = pkgs.rust-bin.stable."1.84.1".default; doCheck = false; cargoBuildFlags = [ "--bin" "nix_bootstrap" ]; postPatch = '' mkdir -p .cargo cp -r ${buildHelperSrc} build_helper - cp ${configTomlPath} config.toml + # TODO: Generate config.toml properly ''; }; };
8-8: Invalid flake input:path:must reference a directory, not a file.The
path:URI scheme in Nix flakes must point to a directory containing aflake.nix, not an individual file. Additionally, lines 60, 75, and 91 use${configTomlPath}expecting a file path, but flake inputs resolve to derivation outputs (directories), causing a type mismatch.Remove the
configTomlPathinput and either:
- Generate
config.tomlinline withinpostPatchusing Nix expressions, or- Create a proper flake in a subdirectory that exposes the config file via an output
inputs = { nixpkgs.url = "github:meta-introspector/nixpkgs?ref=feature/CRQ-016-nixify"; rustSrcFlake.url = "github:meta-introspector/rust?rev=3487cd3843083db70ee30023f19344568ade9c9f"; rustOverlay.url = "github:meta-introspector/rust-overlay?ref=feature/CRQ-016-nixify"; - configTomlPath.url = "path:./generated_config.toml"; };Then update the outputs signature and remove the
cp ${configTomlPath}lines (see subsequent comments).standalonex/src/bootstrap/src/bin/main.rs (2)
13-15: Type alignment: ensureparse::parse(flags)returnsConfig(or convert).Main declares/uses
Configelsewhere; ifparse::parsereturnsParsedConfig, you’ll hit type friction later (duplicate of earlier review). Convert explicitly or change the parser to returnConfig.Suggested fix (if parser returns
ParsedConfig):- let mut config = parse::parse(flags); + let parsed = parse::parse(flags); + let mut config: Config = parsed.into(); // implement From<ParsedConfig> for ConfigAlso applies to: 26-26
128-128: Fix type mismatch incheck_version:dry_run::dry_run(&config)expects&ParsedConfig, not&Config.Use the
Config’s own dry‑run access (previous behavior) or provide a compatible helper. This is a duplicate of an earlier finding.Minimal fix (if
Configexposes a method):- if io::stdout().is_terminal() && !dry_run::dry_run(&config) { + if io::stdout().is_terminal() && !config.dry_run() {Alternative: add a helper in
bootstrap_config_utils::dry_runthat accepts&Config, or implement a trait shared by both types.standalonex/src/bootstrap/src/core/build_steps/compile_modules/std_cargo.rs (1)
4-7: Imports missing (compile blocker).Same issue as previously flagged: add necessary
useitems.+use std::env; +use crate::core::builder::{Builder, Cargo}; +use crate::core::config::{RustcLto, TargetSelection}; +use super::compiler_rt_for_profiler::compiler_rt_for_profiler;Note: ensure
compiler_rt_for_profileris visible (pub(super) or pub(crate)); adjust if needed.
🧹 Nitpick comments (8)
standalonex/flake.nix (1)
25-26: Remove redundantcargopackage.The
rust-bin.stable."1.84.1".defaulttoolchain already includescargo,rustc, and other tools. Addingpkgs.cargoseparately may cause version conflicts or confusion about which toolchain is in use.packages = [ pkgs.python3 pkgs.rust-bin.stable."1.84.1".default - pkgs.cargo ];standalonex/src/bootstrap/src/bin/main.rs (2)
28-31: Prefer consistent error handling macro overexpect.Elsewhere this file uses
t!(...). For consistency and better diagnostics, use it here too.- config.resolve_nix_paths().expect("Failed to resolve Nix paths"); + t!(config.resolve_nix_paths());
74-79: Preserve panic context for failed builds.
catch_unwind(...).is_ok()loses the panic reason. Log the payload for quick triage and reflect it in the summary.- let build_result = std::panic::catch_unwind(|| { - Build::new(new_config).build(); - }); - - build_results.push((rustc_version.clone(), cargo_version.clone(), build_result.is_ok())); + let build_result = std::panic::catch_unwind(|| { + Build::new(new_config).build(); + }); + if let Err(e) = &build_result { + if let Some(s) = e.downcast_ref::<&str>() { + eprintln!("Build panicked: {s}"); + } else if let Some(s) = e.downcast_ref::<String>() { + eprintln!("Build panicked: {s}"); + } else { + eprintln!("Build panicked with non-string payload"); + } + } + build_results.push((rustc_version.clone(), cargo_version.clone(), build_result.is_ok()));Optionally, augment the summary with “OK/FAIL” and/or failure notes instead of raw booleans.
Also applies to: 82-85
standalonex/src/bootstrap/src/core/build_steps/compile_modules/stream_cargo.rs (1)
10-37: Avoid shadowing and simplify arg passing.Rename the shadowed
cargotocommand, and useargs(&tail_args).- let mut cmd = cargo.into_cmd(); - let cargo = cmd.as_command_mut(); + let mut cmd = cargo.into_cmd(); + let command = cmd.as_command_mut(); @@ - cargo.arg("--message-format").arg(message_format).stdout(Stdio::piped()); + command.arg("--message-format").arg(message_format).stdout(Stdio::piped()); @@ - for arg in tail_args { - cargo.arg(arg); - } + command.args(&tail_args); @@ - builder.verbose(|| println!("running: {cargo:?}")); + builder.verbose(|| println!("running: {command:?}")); @@ - let mut child = match cargo.spawn() { + let mut child = match command.spawn() { @@ - "command did not execute successfully: {cargo:?}\nexpected success, got: {status}" + "command did not execute successfully: {command:?}\nexpected success, got: {status}"Also applies to: 25-27, 29-29, 60-63
standalonex/src/bootstrap/src/core/build_steps/compile_modules/std_cargo.rs (1)
135-137: Remove commented-out stub.Dead/commented code at end; drop it.
-// let html_root = -// -// String::new();standalonex/src/bootstrap/src/core/build_steps/compile_modules/codegen_backend.rs (2)
22-23: De-duplicate CODEGEN_BACKEND_PREFIX constant.This constant also exists in compile.rs. Import the shared one and remove the duplicate here.
-pub(crate) const CODEGEN_BACKEND_PREFIX: &str = "rustc_codegen_"; +use crate::core::build_steps::compile::CODEGEN_BACKEND_PREFIX;
56-58: Optionally widen ShouldRun to respect configured backends.Consider deriving paths from
config.codegen_backends()to avoid hard-coding only cranelift/gcc.standalonex/src/bootstrap/src/core/build_steps/compile.rs (1)
32-32: Single source of truth for CODEGEN_BACKEND_PREFIX.If you keep the constant here, import it in codegen_backend.rs and delete the duplicate there (see related comment).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
standalonex/src/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
generated_config.toml(1 hunks)standalonex/flake.nix(3 hunks)standalonex/src/bootstrap/src/bin/main.rs(3 hunks)standalonex/src/bootstrap/src/core/build_steps/compile.rs(2 hunks)standalonex/src/bootstrap/src/core/build_steps/compile_modules/codegen_backend.rs(1 hunks)standalonex/src/bootstrap/src/core/build_steps/compile_modules/mod.rs(1 hunks)standalonex/src/bootstrap/src/core/build_steps/compile_modules/std_cargo.rs(1 hunks)standalonex/src/bootstrap/src/core/build_steps/compile_modules/stream_cargo.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- generated_config.toml
🧰 Additional context used
🧬 Code graph analysis (4)
standalonex/src/bootstrap/src/core/build_steps/compile_modules/stream_cargo.rs (1)
standalonex/src/bootstrap/src/core/builder/cargo.rs (1)
cargo(442-1211)
standalonex/src/bootstrap/src/core/build_steps/compile_modules/codegen_backend.rs (3)
standalonex/src/bootstrap/src/core/build_steps/compile_modules/rustc_cargo_env.rs (1)
rustc_cargo_env(4-81)standalonex/src/bootstrap/src/core/build_steps/compile_modules/run_cargo.rs (2)
run_cargo(4-166)t(123-126)standalonex/src/bootstrap/src/utils/helpers.rs (2)
is_dylib(57-61)write(618-620)
standalonex/src/bootstrap/src/core/build_steps/compile_modules/std_cargo.rs (2)
standalonex/src/bootstrap/src/core/builder/cargo.rs (1)
cargo(442-1211)standalonex/src/bootstrap/src/core/build_steps/compile_modules/compiler_rt_for_profiler.rs (1)
compiler_rt_for_profiler(9-25)
standalonex/src/bootstrap/src/bin/main.rs (1)
standalonex/src/bootstrap/src/core/config_utils/src/dry_run.rs (1)
dry_run(12-17)
🔇 Additional comments (3)
standalonex/flake.nix (1)
53-54: No action required—configuration is correct.All packages in a Rust workspace share a common Cargo.lock file at the workspace root. The script output confirms that
standalonex/src/is the workspace root (containing[workspace]with members"bootstrap"and"stage0_parser_crate"), and the flake.nix correctly references./src/Cargo.lock(the workspace lock). The bootstrap crate's ownCargo.lockexists separately, but lock files in workspace member subdirectories are ignored during workspace builds. The current configuration is consistent and correct.standalonex/src/bootstrap/src/bin/main.rs (1)
33-38: Confirm semantics: are these “versions” or filesystem paths?Assigning
PathBuf::from(rustc_version)/PathBuf::from(cargo_version)implies these are paths. If they are plain version strings (e.g., “1.78.0”),initial_rustc/initial_cargowill point to non-existent paths. Ensure they are resolved to actual tool paths first.standalonex/src/bootstrap/src/core/build_steps/compile_modules/mod.rs (1)
1-32: Module wiring looks good.Publicly exposing these modules matches the refactor intent.
| let mut build_lock; | ||
| let _build_lock_guard; | ||
|
|
||
| if !new_config.bypass_bootstrap_lock { | ||
| // Display PID of process holding the lock | ||
| // PID will be stored in a lock file | ||
| let lock_path = new_config.out.join("lock"); | ||
| let pid = fs::read_to_string(&lock_path); | ||
|
|
||
| build_lock = fd_lock::RwLock::new(t!(fs::OpenOptions::new() | ||
| .write(true) | ||
| .truncate(true) | ||
| .create(true) | ||
| .open(&lock_path))); | ||
| _build_lock_guard = match build_lock.try_write() { | ||
| Ok(mut lock) => { | ||
| t!(lock.write(process::id().to_string().as_ref())); | ||
| lock | ||
| } | ||
| err => { | ||
| drop(err); | ||
| if let Ok(pid) = pid { | ||
| println!("WARNING: build directory locked by process {pid}, waiting for lock"); | ||
| } else { | ||
| println!("WARNING: build directory locked, waiting for lock"); | ||
| } | ||
| let mut lock = t!(build_lock.write()); | ||
| t!(lock.write(process::id().to_string().as_ref())); | ||
| lock | ||
| } | ||
| }; |
There was a problem hiding this comment.
Won’t compile: uninitialized locals; guard lifetime pattern is incorrect.
let mut build_lock; and let _build_lock_guard; are invalid without types/initializers, and the guard may not live long enough. Use an Option<...> guard (or then(...)) so the write lock stays held through the build.
Apply this refactor:
- let mut build_lock;
- let _build_lock_guard;
-
- if !new_config.bypass_bootstrap_lock {
- // Display PID of process holding the lock
- // PID will be stored in a lock file
- let lock_path = new_config.out.join("lock");
- let pid = fs::read_to_string(&lock_path);
-
- build_lock = fd_lock::RwLock::new(t!(fs::OpenOptions::new()
- .write(true)
- .truncate(true)
- .create(true)
- .open(&lock_path)));
- _build_lock_guard = match build_lock.try_write() {
- Ok(mut lock) => {
- t!(lock.write(process::id().to_string().as_ref()));
- lock
- }
- err => {
- drop(err);
- if let Ok(pid) = pid {
- println!("WARNING: build directory locked by process {pid}, waiting for lock");
- } else {
- println!("WARNING: build directory locked, waiting for lock");
- }
- let mut lock = t!(build_lock.write());
- t!(lock.write(process::id().to_string().as_ref()));
- lock
- }
- };
- }
+ // Hold the guard for the duration of the build to keep the lock.
+ let _lock_guard = if !new_config.bypass_bootstrap_lock {
+ let lock_path = new_config.out.join("lock");
+ let pid_read = fs::read_to_string(&lock_path);
+ let lock = fd_lock::RwLock::new(t!(OpenOptions::new()
+ .write(true)
+ .truncate(true)
+ .create(true)
+ .open(&lock_path)));
+ Some(match lock.try_write() {
+ Ok(mut guard) => {
+ t!(guard.write(process::id().to_string().as_ref()));
+ guard
+ }
+ Err(e) => {
+ drop(e);
+ if let Ok(pid) = pid_read {
+ println!("WARNING: build directory locked by process {pid}, waiting for lock");
+ } else {
+ println!("WARNING: build directory locked, waiting for lock");
+ }
+ let mut guard = t!(lock.write());
+ t!(guard.write(process::id().to_string().as_ref()));
+ guard
+ }
+ })
+ } else {
+ None
+ };Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In standalonex/src/bootstrap/src/bin/main.rs around lines 41 to 71, the locals
build_lock and _build_lock_guard are declared uninitialized and the write-guard
may drop too early; change the pattern to initialize build_lock immediately
(create the RwLock when opening/creating the lock file) and store the held write
guard in an Option so it stays alive for the duration of the build. Concretely:
replace the bare declarations with a properly initialized build_lock (the
fd_lock::RwLock wrapping the opened File) and use an
Option<fd_lock::RwLockWriteGuard<std::fs::File>> for the guard, setting it to
Some(guard) when the write lock is acquired and leaving it as None when
bypass_bootstrap_lock is true; ensure types are explicit so the variable is
always initialized and lives across the required scope.
| #[derive(Debug, Clone, PartialEq, Eq, Hash)] | ||
| pub struct CodegenBackend { | ||
| pub target: TargetSelection, | ||
| pub compiler: Compiler, | ||
| pub backend: String, | ||
| } |
There was a problem hiding this comment.
Add missing imports (compile blocker).
This module references many items without use.
+use std::{fs, path::PathBuf};
+use crate::{Compiler, Mode};
+use crate::core::build_steps::tool::SourceType;
+use crate::core::builder::{self, Builder, Cargo, Kind, PathSet, RunConfig, ShouldRun, Step, TaskPath};
+use crate::core::config::TargetSelection;
+use crate::utils::helpers::{is_dylib, t};
+use super::run_cargo::run_cargo;
+use super::rustc_cargo_env::rustc_cargo_env;
+use super::codegen_backend_stamp::codegen_backend_stamp;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
standalonex/src/bootstrap/src/core/build_steps/compile_modules/codegen_backend.rs
around lines 4 to 9, the file declares CodegenBackend but references several
types and items without importing them; add the missing use statements (at
minimum import TargetSelection and Compiler from their defining modules and any
other types/functions referenced elsewhere in this module), place them at the
top of the file, prefer explicit paths (or grouped use statements) matching the
crate layout, then run cargo check and iteratively add the remaining unresolved
imports until compilation succeeds.
| fn needs_codegen_config(run: &RunConfig<'_>) -> bool { | ||
| let mut needs_codegen_cfg = false; | ||
| for path_set in &run.paths { | ||
| needs_codegen_cfg = match path_set { | ||
| PathSet::Set(set) => set.iter().any(|p| is_codegen_cfg_needed(p, run)), | ||
| PathSet::Suite(suite) => is_codegen_cfg_needed(suite, run), | ||
| } | ||
| } | ||
| needs_codegen_cfg | ||
| } |
There was a problem hiding this comment.
Bug: flag overwritten in loop; use short-circuit or OR-accumulate.
Currently only the last path_set decides. Return on first true or use ||=.
-fn needs_codegen_config(run: &RunConfig<'_>) -> bool {
- let mut needs_codegen_cfg = false;
- for path_set in &run.paths {
- needs_codegen_cfg = match path_set {
- PathSet::Set(set) => set.iter().any(|p| is_codegen_cfg_needed(p, run)),
- PathSet::Suite(suite) => is_codegen_cfg_needed(suite, run),
- }
- }
- needs_codegen_cfg
-}
+fn needs_codegen_config(run: &RunConfig<'_>) -> bool {
+ for path_set in &run.paths {
+ let needed = match path_set {
+ PathSet::Set(set) => set.iter().any(|p| is_codegen_cfg_needed(p, run)),
+ PathSet::Suite(suite) => is_codegen_cfg_needed(suite, run),
+ };
+ if needed {
+ return true;
+ }
+ }
+ false
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn needs_codegen_config(run: &RunConfig<'_>) -> bool { | |
| let mut needs_codegen_cfg = false; | |
| for path_set in &run.paths { | |
| needs_codegen_cfg = match path_set { | |
| PathSet::Set(set) => set.iter().any(|p| is_codegen_cfg_needed(p, run)), | |
| PathSet::Suite(suite) => is_codegen_cfg_needed(suite, run), | |
| } | |
| } | |
| needs_codegen_cfg | |
| } | |
| fn needs_codegen_config(run: &RunConfig<'_>) -> bool { | |
| for path_set in &run.paths { | |
| let needed = match path_set { | |
| PathSet::Set(set) => set.iter().any(|p| is_codegen_cfg_needed(p, run)), | |
| PathSet::Suite(suite) => is_codegen_cfg_needed(suite, run), | |
| }; | |
| if needed { | |
| return true; | |
| } | |
| } | |
| false | |
| } |
🤖 Prompt for AI Agents
In
standalonex/src/bootstrap/src/core/build_steps/compile_modules/codegen_backend.rs
around lines 11 to 20, the loop overwrites needs_codegen_cfg on each iteration
so only the last PathSet wins; change the logic to short-circuit return true as
soon as any path_set requires codegen (i.e., if any branch yields true return
true immediately) or accumulate with needs_codegen_cfg ||= ... (or use .any over
run.paths) and then return the flag at the end.
| if builder.no_std(target) == Some(true) { | ||
| features += " compiler-builtins-mem"; | ||
| if !target.starts_with("sbf") && !target.starts_with("bpf") { | ||
| features.push_str(compiler_builtins_c_feature); | ||
| } | ||
|
|
There was a problem hiding this comment.
Fix TargetSelection string checks.
You’re calling string methods on TargetSelection. Use target.triple.
- if builder.no_std(target) == Some(true) {
+ if builder.no_std(target) == Some(true) {
features += " compiler-builtins-mem";
- if !target.starts_with("sbf") && !target.starts_with("bpf") {
+ if !target.triple.starts_with("sbf") && !target.triple.starts_with("bpf") {
features.push_str(compiler_builtins_c_feature);
}
}
@@
- if target.contains("musl") {
+ if target.triple.contains("musl") {
if let Some(p) = builder.musl_libdir(target) {
let root = format!("native={}", p.to_str().unwrap());
cargo.rustflag("-L").rustflag(&root);
}
}
@@
- if target.contains("-wasi") {
+ if target.triple.contains("-wasi") {
if let Some(dir) = builder.wasi_libdir(target) {
let root = format!("native={}", dir.to_str().unwrap());
cargo.rustflag("-L").rustflag(&root);
}
}
@@
- if target.contains("riscv") {
+ if target.triple.contains("riscv") {
cargo.rustflag("-Cforce-unwind-tables=yes");
}Also applies to: 91-96, 98-103, 127-129
🤖 Prompt for AI Agents
In standalonex/src/bootstrap/src/core/build_steps/compile_modules/std_cargo.rs
around lines 66-71 (and similarly at 91-96, 98-103, 127-129), the code is
calling string methods directly on the TargetSelection value (e.g.,
target.starts_with(...)); update these checks to use the TargetSelection's
triple field (e.g., target.triple.starts_with(...)) so you call string methods
on the actual triple string; replace each occurrence of target.starts_with(...)
and target.contains(...) with target.triple.starts_with(...) and
target.triple.contains(...) respectively, ensuring all target string checks
throughout the listed ranges use target.triple.
| pub fn stream_cargo( | ||
| builder: &Builder<'_>, | ||
| cargo: Cargo, | ||
| tail_args: Vec<String>, | ||
| cb: &mut dyn FnMut(CargoMessage<'_>), | ||
| ) -> bool { |
There was a problem hiding this comment.
Missing imports cause compile failure (BufRead, Stdio, types).
Bring required items into scope; without BufRead the .lines() call won’t resolve.
Apply:
+use std::io::{BufRead, BufReader};
+use std::process::Stdio;
+use crate::core::builder::{Builder, Cargo};
+use super::cargo_message::CargoMessage;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
standalonex/src/bootstrap/src/core/build_steps/compile_modules/stream_cargo.rs
around lines 4 to 9, the function uses .lines() and process I/O types but
missing imports cause compile failure; add the necessary imports such as
std::io::BufRead and std::process::Stdio, and bring the module that defines the
CargoMessage/Cargo types into scope (e.g. use crate::types::{...} or the correct
path to the types) so .lines(), Stdio and the Cargo/CargoMessage types resolve.
…ooks temporarily disabled).
…s temporarily disabled).
…lake-template-generator
There was a problem hiding this comment.
Actionable comments posted: 11
♻️ Duplicate comments (12)
flakes/local-bootstrap-test/flake.nix (1)
30-47: Remove device‑specific file URLs and hardcoded system; switch to path/github + builtins.currentSystem.These absolute
git+file:///data/data/...paths are non‑portable;system = "aarch64-linux"locks the shell. Use relative path or GitHub URL andbuiltins.currentSystem.Apply:
- rustBootstrapNixJsonProcessor = { - url = "git+file:///data/data/com.termux.nix/files/home/git/meta-introspector/rust-bootstrap-nix?ref=feature/bootstrap-001&dir=flakes/json-processor"; + rustBootstrapNixJsonProcessor = { + url = "path:../../..?/dir=flakes/json-processor"; inputs.nixpkgs.follows = "nixpkgs"; inputs.rustSrc.follows = "rustSrc"; }; @@ - rustBootstrapNixEvaluateRustFlake = { - url = "git+file:///data/data/com.termux.nix/files/home/git/meta-introspector/rust-bootstrap-nix?ref=feature/bootstrap-001&dir=flakes/evaluate-rust"; + rustBootstrapNixEvaluateRustFlake = { + url = "path:../../..?/dir=flakes/evaluate-rust"; inputs.nixpkgs.follows = "nixpkgs"; inputs.naersk.follows = "naersk"; }; @@ - rustBootstrapNixStandalonex = { - url = "git+file:///data/data/com.termux.nix/files/home/git/meta-introspector/rust-bootstrap-nix?ref=feature/bootstrap-001&dir=standalonex"; + rustBootstrapNixStandalonex = { + url = "path:../../..?/dir=standalonex"; inputs.nixpkgs.follows = "nixpkgs"; inputs.rustSrcFlake.follows = "rustSrc"; inputs.rustOverlay.follows = "rustOverlay"; }; @@ - system = "aarch64-linux"; + system = builtins.currentSystem;If you prefer remote instead of local, replace
path:../../..?/dir=...with:
github:meta-introspector/rust-bootstrap-nix?ref=feature/bootstrap-001&dir=....Also applies to: 64-69
standalonex/generated_config.toml (2)
1-54: Configuration file has hard-coded system-specific paths.Similar to the other generated config file, this contains hard-coded Termux-specific paths. If this is a generated template meant to be system-specific, consider documenting this clearly or making the generation process more flexible.
25-25: Syntax issue: missing value for 'stage' key.Same as in
bootstrap-config-builder/generated_config.toml, line 25 hasstage =with no value. This should bestage = ""for valid TOML syntax and consistency.Apply this diff:
-stage = +stage = ""flake.nix (3)
86-100: Environment variables are not consumed by configuration-nix binary.Past review comments identified that environment variables
RUSTC_PATH,CARGO_PATH,HOME_PATH, andCARGO_HOME_PATHare set but the configuration-nix binary uses CLI arguments via Clap, not environment variables. This issue appears unresolved.
144-165: Unused bootstrapConfigBuilder and stage1/stage2 configs should be removed.Past review comments noted that
bootstrapConfigBuilderis never referenced in outputs and bootstrap only readsstage0/config.toml. The stage1 and stage2 derivations are unused overhead.
16-16: Missing flake-utils input causes evaluation failure.Past review flagged that line 16's outputs signature references
flake-utilsbut it's not declared in inputs. This issue appears unresolved.flakes/use-bootstrap-flake/flake.nix (3)
19-20: Incorrect path composition—sysroot is a file containing the path, not a directory.Past review comments identified that
rust_1_84_1_sysrootis a derivation output file containing the sysroot path as text, but line 20 treats it as a directory path when composingrust_1_84_1_libdir. This issue remains unresolved.
33-36: shellHook exports point to file paths instead of directory paths.Past review comments noted that the exports reference Nix store file paths rather than the actual directories the runtime expects (e.g., sysroot directory, libdir directory). This issue appears unresolved.
62-62: Stderr redirection applied to wrong command.Past review identified that the stderr redirection
2>&1is applied toteeinstead ofcargo, meaning cargo's stderr isn't captured correctly. This issue remains unresolved.bootstrap-config-builder/src/bin/bootstrap-config-generator.rs (1)
47-56: rust_src_flake_path defaults to empty string, causing confusing errors.Past review comments noted that using
unwrap_or_default()forrust_src_flake_path(line 50) leads to confusing precondition failures. The path should be required explicitly.bootstrap-config-builder/src/utils/format_file.rs (2)
94-127: Order-dependent replacements can cause correctness bugs.Sequential
.replace()calls will interact if any replacement value contains text matching another placeholder pattern. For example, ifrust_overlay_pathcontains"{stage}"and is replaced first (line 99), the subsequentreplace("{stage}", stage)(line 105) will also modify the already-substituted value.Consider using a proper templating library like
teraorhandlebars, or validate that replacement values don't contain placeholder patterns before performing substitutions.
38-127: Unescaped string interpolation can produce invalid TOML.Values inserted inside quotes (paths, refs, addrs) aren't TOML‑escaped. A quote or backslash in any value will break the generated file. Either model the config as structs and use
toml::to_string_pretty, or minimally escape quoted fields.Suggested minimal fix (escape helper + apply to quoted replacements):
fn escape_toml_str(s: &str) -> String { s.chars().flat_map(|c| match c { '\\' => vec!['\\', '\\'], '\"' => vec!['\\', '\"'], '\n' => vec!['\\', 'n'], '\r' => vec!['\\', 'r'], '\t' => vec!['\\', 't'], _ => vec![c], }).collect() }Apply escaping to all quoted string replacements (lines 98-105, 107-109, 116, 118-123):
- .replace("{nixpkgs_path}", nixpkgs_path) + .replace("{nixpkgs_path}", &escape_toml_str(nixpkgs_path)) - .replace("{rust_overlay_path}", rust_overlay_path) + .replace("{rust_overlay_path}", &escape_toml_str(rust_overlay_path))(Apply to all string fields placed in quotes in the template.)
Longer‑term, prefer typed structs +
toml::to_string_prettyto avoid manual escaping entirely.
🧹 Nitpick comments (15)
config.toml (2)
1-12: Confirm schema compatibility for new sections/keys.Ensure downstream parsers accept [rust], [env] and the CARGO_HOME key; earlier QA hinted unknown-field errors. If both [rust] and [build] are read, consider de-duplicating rustc/cargo to a single source to avoid drift.
13-20: Avoid embedding branch/step metadata directly in committed config (prefer derivation-time inputs).Hardcoding git metadata (base_branch, phase, step, arch) can cause churn. Prefer passing via flake inputs/CLI args and materializing into outputs at build time.
flakes/local-bootstrap-test/flake.nix (1)
68-77: Guard package dereferences to avoid missing-default attr errors.Accessing
...packages.${system}.defaultassumes each input exportsdefault. Consider pinning specific attrs or wrapping withor nulland filtering nulls to avoid evaluation failures.flakes/solana-rust-1.84.1/aarch64/phase0/step1/flake.nix (1)
8-15: Make system dynamic for reuse and testing.Hardcoding
"aarch64-linux"restricts builds. Usingbuiltins.currentSystemimproves portability with no downside here.- outputs = { self, nixpkgs }: + outputs = { self, nixpkgs }: let - pkgs = import nixpkgs { system = "aarch64-linux"; }; + system = builtins.currentSystem; + pkgs = import nixpkgs { inherit system; }; configTomlContent = builtins.readFile ./config.toml; in { - packages.aarch64-linux.default = pkgs.writeText "config.toml" configTomlContent; + packages.${system}.default = pkgs.writeText "config.toml" configTomlContent; };README.md (2)
94-156: Consider fixing markdown list indentation for consistency.The static analysis tool flagged numerous list indentation inconsistencies (expected 2 or 4 spaces but found different amounts). While this doesn't affect functionality, consistent indentation improves readability and maintainability.
489-489: Minor style: capitalize "GitHub" consistently.The word "github" should be capitalized as "GitHub" per the official branding.
Apply this diff:
-1. Create a `flake.nix` file in this directory that builds the `bootstrap` compiler from the rust source. -2. The `rust-src` will be an input to this flake, using a github URL with a specific git hash. +1. Create a `flake.nix` file in this directory that builds the `bootstrap` compiler from the Rust source. +2. The `rust-src` will be an input to this flake, using a GitHub URL with a specific git hash.flake-template-generator/src/flake_generator.rs (1)
1-27: Consider adding input validation and security considerations.The function directly interpolates user inputs into a Nix expression without validation. While Nix expressions are typically safe, consider:
- Validating
system_archagainst known values (e.g., "aarch64-linux", "x86_64-linux")- Validating
nixpkgs_urlformat to ensure it's a valid URL/path- Documenting expected input formats in function docs
Example validation:
pub fn generate_flake_nix_content( nixpkgs_url: &str, system_arch: &str, ) -> Result<String, String> { // Validate system_arch const VALID_SYSTEMS: &[&str] = &["aarch64-linux", "x86_64-linux", "x86_64-darwin", "aarch64-darwin"]; if !VALID_SYSTEMS.contains(&system_arch) { return Err(format!("Invalid system_arch: {}. Must be one of {:?}", system_arch, VALID_SYSTEMS)); } // Validate nixpkgs_url is not empty if nixpkgs_url.trim().is_empty() { return Err("nixpkgs_url cannot be empty".to_string()); } Ok(format!( // ... rest of template )) }bootstrap-config-builder/generated_config.toml (1)
4-4: Portability concern: hard-coded absolute paths.The file contains hard-coded absolute paths specific to a Termux/Android environment (lines 4, 12). If this is a generated template, consider:
- Making these paths relative or parametrized
- Using placeholders that get replaced during generation
- Documenting that this is system-specific
Also applies to: 12-12
flake-template-generator/Makefile (1)
15-15: Consider documenting config path requirement.The config path
../bootstrap-config-builder/generated_config.tomluses a relative path that assumes the Makefile is run from a specific directory. Consider:
- Adding a comment explaining the expected directory structure
- Using an absolute path or a configurable variable
- Adding error handling if the config file doesn't exist
Example:
CONFIG_PATH ?= ../bootstrap-config-builder/generated_config.toml generate-flake: @if [ ! -f "$(CONFIG_PATH)" ]; then \ echo "Error: Config file not found at $(CONFIG_PATH)"; \ exit 1; \ fi @echo "Generating flake directory using flake-template-generator..." # ... rest of command using $(CONFIG_PATH)Makefile (1)
1-1: Optional: Consider adding conventional phony targets.Static analysis notes that conventional
cleanandtestphony targets are missing. While not required, they improve the developer experience.flake.nix (1)
26-63: Recommended refactor: Remove commented-out sccache code.Lines 26-63 contain extensive commented-out code for the old sccache-based build. Since the PR summary states this approach was removed in favor of the new config-extractor flow, this dead code should be deleted to improve maintainability.
- # Define the sccache-enabled rustc package - # sccachedRustc = (system: pkgs: rustToolchain: - # let - # cargo_bin = "${rustToolchain}/bin/cargo"; - ... - # }) - # ); - - # Define packages.default to be the sccache-enabled rustc package - # packages.aarch64-linux.default = sccachedRustc "aarch64-linux" pkgs_aarch64 rustToolchain_aarch64; - # packages.x86_64-linux.default = sccachedRustc "x86_64-linux" pkgs_x86_64 rustToolchain_x86_64; -flake-template-generator/src/main.rs (1)
37-38: System architecture is hardcoded.Line 38 hardcodes
system_arch = "aarch64-linux". The comment on line 37 acknowledges this should be made dynamic, but it limits the utility of this generator on other architectures.Consider adding a CLI argument or detecting the current system:
// Option 1: CLI argument let system_arch = args.system.as_deref().unwrap_or("aarch64-linux"); // Option 2: Detect current system let system_arch = std::env::consts::ARCH;bootstrap-config-builder/src/bin/bootstrap-config-generator.rs (1)
61-93: Recommended refactor: Construct helper accepts 31 parameters—consider a builder or struct.The
construct_config_contentfunction is called with 31 individual parameters (lines 61-93), making the code hard to read and maintain. Consider refactoring to use a configuration struct or builder pattern.Example refactor:
// In construct_config_content.rs pub struct ConfigContentParams { pub system: String, pub flake_path: String, // ... other fields } pub fn construct_config_content(params: &ConfigContentParams) -> String { format_file::format_file(/* use params fields */) } // In this file let params = ConfigContentParams { system: app_config.system.as_deref().unwrap_or_default().to_string(), flake_path: flake_path_str.to_string(), // ... populate fields }; let config_content = construct_config_content(¶ms);flake-template-generator/src/config_parser.rs (2)
21-21: Prefer&Pathover&PathBuffor function parameters.Taking
&PathBufis less idiomatic than&Pathand less flexible for callers.Apply this diff:
-pub fn parse_config(config_path: &PathBuf) -> Result<Config, Box<dyn std::error::Error>> { +pub fn parse_config(config_path: &Path) -> Result<Config, Box<dyn std::error::Error>> {
14-19: Consider addingDefaultderive for consistency.Since
NixConfigderivesDefaultand thenixfield has#[serde(default)], addingDefaulttoConfigwould provide a consistent API.Apply this diff:
-#[derive(Debug, Deserialize)] +#[derive(Debug, Deserialize, Default)] pub struct Config {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockstandalonex/flake.lockis excluded by!**/*.lock
📒 Files selected for processing (30)
.gitignore(1 hunks)Cargo.toml(1 hunks)Makefile(1 hunks)README.md(1 hunks)bootstrap-config-builder/Cargo.toml(1 hunks)bootstrap-config-builder/flake.nix(1 hunks)bootstrap-config-builder/generated_config.toml(1 hunks)bootstrap-config-builder/src/bin/bootstrap-config-generator.rs(1 hunks)bootstrap-config-builder/src/lib.rs(1 hunks)bootstrap-config-builder/src/utils/format_file.rs(1 hunks)config.toml(1 hunks)docs/README_bootstrap_builder_flake.md(1 hunks)docs/README_flake_lattice.md(1 hunks)flake-template-generator/Cargo.toml(1 hunks)flake-template-generator/Makefile(1 hunks)flake-template-generator/flake_template.nix(1 hunks)flake-template-generator/src/args.rs(1 hunks)flake-template-generator/src/config_parser.rs(1 hunks)flake-template-generator/src/file_writer.rs(1 hunks)flake-template-generator/src/flake_generator.rs(1 hunks)flake-template-generator/src/main.rs(1 hunks)flake-template-generator/src/statix_checker.rs(1 hunks)flake.nix(1 hunks)flakes/local-bootstrap-test/flake.nix(1 hunks)flakes/solana-rust-1.84.1/aarch64/phase0/step1/config.toml(1 hunks)flakes/solana-rust-1.84.1/aarch64/phase0/step1/flake.nix(1 hunks)flakes/use-bootstrap-flake/flake.nix(1 hunks)standalonex/flake.nix(3 hunks)standalonex/generated_config.toml(1 hunks)standalonex/src/bootstrap/src/bin/git_flake_manager.rs(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- flakes/solana-rust-1.84.1/aarch64/phase0/step1/config.toml
- flake-template-generator/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (3)
- Cargo.toml
- standalonex/flake.nix
- bootstrap-config-builder/Cargo.toml
🧰 Additional context used
🧬 Code graph analysis (5)
bootstrap-config-builder/src/lib.rs (2)
bootstrap-config-builder/src/utils/validate_project_root.rs (1)
validate_project_root(7-15)bootstrap-config-builder/src/utils/construct_config_content.rs (1)
construct_config_content(4-70)
bootstrap-config-builder/src/bin/bootstrap-config-generator.rs (3)
bootstrap-config-builder/src/utils/validate_project_root.rs (1)
validate_project_root(7-15)bootstrap-config-builder/src/utils/construct_config_content.rs (1)
construct_config_content(4-70)bootstrap-config-builder/src/preconditions.rs (2)
check_nix_command_available(5-14)check_rust_toolchain_sysroot(16-27)
flake-template-generator/src/statix_checker.rs (1)
standalonex/src/bootstrap/src/utils/exec.rs (2)
stdout(287-292)stderr(305-310)
flake-template-generator/src/main.rs (4)
flake-template-generator/src/config_parser.rs (1)
parse_config(21-25)flake-template-generator/src/flake_generator.rs (1)
generate_flake_nix_content(1-27)flake-template-generator/src/file_writer.rs (1)
write_flake_and_config(4-19)flake-template-generator/src/statix_checker.rs (1)
run_statix_check(4-24)
standalonex/src/bootstrap/src/bin/git_flake_manager.rs (2)
standalonex/src/bootstrap/src/core/generate_steps/git_modules/create_branch.rs (1)
create_and_push_branch(32-70)standalonex/src/bootstrap/src/utils/helpers.rs (1)
git(553-571)
🪛 checkmake (0.2.2)
flake-template-generator/Makefile
[warning] 3-3: Missing required phony target "clean"
(minphony)
[warning] 3-3: Missing required phony target "test"
(minphony)
Makefile
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
🪛 LanguageTool
docs/README_bootstrap_builder_flake.md
[style] ~5-~5: ‘Overall Goal’ might be wordy. Consider a shorter alternative.
Context: ...ust bootstrap compiler from source. ## Overall Goal: Automate the generation, Nix build, an...
(EN_WORDINESS_PREMIUM_OVERALL_GOAL)
[style] ~16-~16: ‘Overall Goal’ might be wordy. Consider a shorter alternative.
Context: ...ave been removed. ## Detailed Plan: Overall Goal: Automate the generation, Nix build, ...
(EN_WORDINESS_PREMIUM_OVERALL_GOAL)
README.md
[uncategorized] ~489-~489: The official name of this software platform is spelled with a capital “H”.
Context: ...will be an input to this flake, using a github URL with a specific git hash. 3. The b...
(GITHUB)
docs/README_flake_lattice.md
[style] ~5-~5: ‘Overall Goal’ might be wordy. Consider a shorter alternative.
Context: ... approach to Nix flake development. ## Overall Goal To create a system where Rust programs...
(EN_WORDINESS_PREMIUM_OVERALL_GOAL)
🪛 markdownlint-cli2 (0.18.1)
README.md
94-94: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
96-96: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
98-98: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
100-100: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
102-102: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
115-115: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
117-117: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
138-138: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
139-139: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
142-142: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
143-143: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
144-144: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
147-147: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
148-148: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
155-155: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
156-156: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
205-205: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
207-207: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
209-209: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
214-214: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
215-215: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
216-216: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
217-217: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
218-218: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
219-219: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
220-220: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
221-221: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
222-222: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
223-223: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
224-224: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
225-225: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
228-228: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
229-229: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
230-230: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
231-231: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
232-232: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
233-233: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
234-234: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
237-237: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
250-250: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
252-252: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
257-257: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
258-258: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
259-259: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
260-260: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
273-273: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
275-275: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
280-280: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
281-281: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
282-282: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
283-283: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
286-286: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
287-287: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
288-288: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
301-301: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
306-306: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
307-307: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
308-308: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
309-309: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
310-310: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
323-323: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
325-325: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
330-330: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
331-331: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
332-332: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
333-333: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
346-346: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
348-348: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
353-353: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
354-354: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
355-355: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
356-356: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
357-357: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
358-358: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
359-359: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
372-372: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
377-377: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
378-378: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
381-381: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
382-382: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
383-383: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
396-396: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
398-398: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
400-400: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
405-405: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
406-406: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
407-407: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
408-408: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
409-409: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
410-410: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
411-411: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
412-412: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
415-415: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
416-416: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
417-417: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
418-418: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
419-419: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
420-420: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
421-421: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
422-422: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
423-423: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
424-424: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
425-425: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
426-426: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
🔇 Additional comments (12)
README.md (1)
1-504: Comprehensive documentation—excellent addition to the project.This README provides thorough documentation of the Nix-based Rust bootstrap system, covering architecture, configuration, flakes structure, and usage workflows. The documentation is well-organized and should help users understand the complex bootstrap process.
flake-template-generator/src/args.rs (1)
1-38: Well-structured CLI argument definitions.The Args struct is clean and well-documented with appropriate types. The use of clap's derive macros provides a good developer experience.
flake-template-generator/Makefile (1)
1-22: LGTM with optional improvements.The Makefile provides a clean interface for flake generation. The use of
evalto define variables is appropriate, and the DRY_RUN support is helpful for testing.standalonex/src/bootstrap/src/bin/git_flake_manager.rs (1)
37-39: The config path resolution is correct; the review comment is invalid.The code correctly computes
repo_rootby starting fromCARGO_MANIFEST_DIR(the binary's Cargo.toml directory) and walking up 6 parent directories to reach the repository root. The root-levelconfig.tomlfile exists and contains the required[git]section with all necessary fields (base_branch,new_flake_branch_prefix,component,arch,phase,step,output_dir_prefix). This is the intended location for git configuration used by thegit_flake_managerbinary. Thestandalonex/config.tomlis a separate configuration file for build settings and does not contain the git configuration section.Likely an incorrect or invalid review comment.
flakes/use-bootstrap-flake/flake.nix (1)
38-38: Good: LD_DEBUG now commented out.Past review requested that
LD_DEBUG=allbe made conditional or removed as it's extremely noisy. Line 38 now comments it out, which partially addresses the concern.bootstrap-config-builder/src/lib.rs (1)
1-17: LGTM: Module structure and visibility are well-organized.The public module declarations expose the necessary components (utils, preconditions, args, config) for external consumption while maintaining clear separation of concerns.
flake-template-generator/src/file_writer.rs (1)
4-19: LGTM: Clean file writing implementation with appropriate error handling.The function correctly writes both files to the output directory with proper error propagation. The success message provides useful feedback.
flake-template-generator/src/statix_checker.rs (1)
4-24: LGTM: Statix validation implemented correctly.The function properly invokes statix via nix-shell, captures output, and provides useful diagnostic information on failure. The use of
from_utf8_lossyis appropriate for handling potentially non-UTF8 output.Verify that running statix from
absolute_output_diras the current directory works correctly. If statix needs access to repository-wide configuration (e.g.,.statix.tomlin the repo root), the current directory may need adjustment.flake-template-generator/src/main.rs (1)
17-54: Overall flow is well-structured and readable.The main function follows a clear sequence: parse args → validate inputs → generate content → write outputs → validate results. Error handling with
?and context messages is appropriate throughout.bootstrap-config-builder/src/bin/bootstrap-config-generator.rs (1)
13-109: Overall logic flow is sound with good error handling.The main function follows a clear sequence: initialize logger → parse args → load/merge config → validate → construct content → output. The use of
context()for error messages and debug logging is appropriate.docs/README_flake_lattice.md (1)
1-88: LGTM! Clear and well-structured documentation.The phased approach with concrete commands and verification steps makes the flake generation workflow easy to follow.
docs/README_bootstrap_builder_flake.md (1)
1-117: LGTM! Comprehensive bootstrap plan with clear phases.The detailed workflow with verification checkpoints at each phase makes the implementation straightforward to follow and validate.
| target/ | ||
| *.d | ||
| *.so | ||
| /.pre-commit-config.local.yamlnix-build-scripts/.#Makefile |
There was a problem hiding this comment.
Fix malformed ignore entry (merge artifact).
/.pre-commit-config.local.yamlnix-build-scripts/.#Makefile is two entries stuck together and won’t match either target.
Apply:
-/.pre-commit-config.local.yamlnix-build-scripts/.#Makefile
+/.pre-commit-config.local.yaml
+nix-build-scripts/.#Makefile📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /.pre-commit-config.local.yamlnix-build-scripts/.#Makefile | |
| /.pre-commit-config.local.yaml | |
| nix-build-scripts/.#Makefile |
🤖 Prompt for AI Agents
In .gitignore around line 17, the entry
"/.pre-commit-config.local.yamlnix-build-scripts/.#Makefile" is a merged
artifact of two ignore patterns; split it into two separate lines such as
"/.pre-commit-config.local.yaml" and "nix-build-scripts/.#Makefile" (or adjust
the second path to match the repository's actual nix-build-scripts directory
name) so each pattern is on its own line and will match correctly.
| projectRoot = "/data/data/com.termux.nix/files/home/pick-up-nix2/vendor/rust/platform-tools-agave-rust-solana/vendor/rust-src/vendor/rust/rust-bootstrap-nix"; # Absolute path to the main project root | ||
| rustSrcFlakePath = "/data/data/com.termux.nix/files/home/nix/vendor/rust/platform-tools-agave-rust-solana/vendor/rust-src"; # Absolute path to rust-src |
There was a problem hiding this comment.
Critical: Hard-coded absolute paths break portability.
The projectRoot and rustSrcFlakePath variables contain hard-coded absolute paths specific to a Termux/Android environment (/data/data/com.termux.nix/...). This makes the flake non-portable and will fail on other systems.
Consider:
- Using
selfto reference the flake's own source tree - Using flake inputs to reference external dependencies
- Making paths configurable via flake outputs or environment variables
Example fix:
{
outputs = { self, nixpkgs, rustSrcFlake, rustOverlay }:
let
pkgs = import nixpkgs {
system = "aarch64-linux";
overlays = [ rustOverlay.overlays.default ];
};
rustcPath = "${pkgs.rust-bin.stable."1.89.0".default}/bin/rustc";
cargoPath = "${pkgs.cargo}/bin/cargo";
projectRoot = "${self}"; # Use flake's source directory
rustSrcFlakePath = "${rustSrcFlake}"; # Use input directly
in
{
# ... rest of outputs
};
}| parallel-compiler = false | ||
| llvm-tools = false | ||
| debuginfo-level = 0 | ||
|
|
There was a problem hiding this comment.
Syntax issue: missing value for 'stage' key.
Line 25 has stage = with no value, which may be invalid TOML syntax depending on the parser. If this is intentional (empty value), it should be stage = "" for consistency with other fields.
Apply this diff:
-stage =
+stage = ""Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In bootstrap-config-builder/generated_config.toml at line 25, the key "stage" is
assigned with no value ("stage ="), which is invalid TOML; replace it with an
explicit empty string value by changing the line to stage = "" so it matches
other fields and parses correctly.
| outputs = { self }: | ||
| let | ||
| configTomlContent = builtins.readFile ./config.toml; | ||
| in | ||
| { | ||
| packages.aarch64-linux.default = configTomlContent; # Output the string directly | ||
| }; |
There was a problem hiding this comment.
packages.default must be a derivation, not a string.
Returning configTomlContent breaks nix build .#packages.aarch64-linux.default. Use pkgs.writeText.
Apply:
- outputs = { self }:
+ inputs = { nixpkgs.url = "nixpkgs"; };
+
+ outputs = { self, nixpkgs }:
let
- configTomlContent = builtins.readFile ./config.toml;
+ configTomlContent = builtins.readFile ./config.toml;
+ pkgs = import nixpkgs { system = builtins.currentSystem; };
in
{
- packages.aarch64-linux.default = configTomlContent; # Output the string directly
+ packages.${builtins.currentSystem}.default =
+ pkgs.writeText "config.toml" configTomlContent;
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| outputs = { self }: | |
| let | |
| configTomlContent = builtins.readFile ./config.toml; | |
| in | |
| { | |
| packages.aarch64-linux.default = configTomlContent; # Output the string directly | |
| }; | |
| inputs = { nixpkgs.url = "nixpkgs"; }; | |
| outputs = { self, nixpkgs }: | |
| let | |
| configTomlContent = builtins.readFile ./config.toml; | |
| pkgs = import nixpkgs { system = builtins.currentSystem; }; | |
| in | |
| { | |
| packages.${builtins.currentSystem}.default = | |
| pkgs.writeText "config.toml" configTomlContent; | |
| }; |
🤖 Prompt for AI Agents
In flake-template-generator/flake_template.nix around lines 4 to 10,
packages.aarch64-linux.default currently returns a plain string
(configTomlContent) but must be a derivation; change the outputs function to
bring in pkgs (or nixpkgs) and use pkgs.writeText to produce a derivation, e.g.
keep or read configTomlContent with builtins.readFile, then set
packages.aarch64-linux.default = pkgs.writeText "config.toml" configTomlContent
so the flake returns a proper derivation for nix build.
| generate-seed-config: build-config-builder | ||
| @echo "Generating seed config.toml using bootstrap-config-generator..." | ||
| cargo run --bin bootstrap-config-generator -- \ | ||
| --output bootstrap-config-builder/generated_config.toml \ | ||
| --project-root $(CURDIR) \ | ||
| --rust-src-flake-path /data/data/com.termux.nix/files/home/pick-up-nix2/vendor/rust/platform-tools-agave-rust-solana/vendor/rust-src | ||
|
|
||
| generate-flake-dir: | ||
| $(MAKE) -C flake-template-generator generate-flake | ||
|
|
||
| generate-seed-config: build-config-builder | ||
| @echo "Generating seed config.toml using bootstrap-config-generator..." | ||
| cargo run --bin bootstrap-config-generator -- \ | ||
| --output bootstrap-config-builder/generated_config.toml \ | ||
| --project-root $(CURDIR) \ | ||
| --rust-src-flake-path /data/data/com.termux.nix/files/home/pick-up-nix2/vendor/rust/platform-tools-agave-rust-solana/vendor/rust-src |
There was a problem hiding this comment.
Critical: Duplicate target definition will break the build.
The generate-seed-config target is defined twice (lines 34-39 and lines 44-49). Make will either fail or silently use only the last definition, leading to unpredictable behavior.
Remove the duplicate definition:
-generate-seed-config: build-config-builder
- @echo "Generating seed config.toml using bootstrap-config-generator..."
- cargo run --bin bootstrap-config-generator -- \
- --output bootstrap-config-builder/generated_config.toml \
- --project-root $(CURDIR) \
- --rust-src-flake-path /data/data/com.termux.nix/files/home/pick-up-nix2/vendor/rust/platform-tools-agave-rust-solana/vendor/rust-src
-
generate-flake-dir:
$(MAKE) -C flake-template-generator generate-flake
-
-generate-seed-config: build-config-builder
- @echo "Generating seed config.toml using bootstrap-config-generator..."
- cargo run --bin bootstrap-config-generator -- \
- --output bootstrap-config-builder/generated_config.toml \
- --project-root $(CURDIR) \
- --rust-src-flake-path /data/data/com.termux.nix/files/home/pick-up-nix2/vendor/rust/platform-tools-agave-rust-solana/vendor/rust-src📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| generate-seed-config: build-config-builder | |
| @echo "Generating seed config.toml using bootstrap-config-generator..." | |
| cargo run --bin bootstrap-config-generator -- \ | |
| --output bootstrap-config-builder/generated_config.toml \ | |
| --project-root $(CURDIR) \ | |
| --rust-src-flake-path /data/data/com.termux.nix/files/home/pick-up-nix2/vendor/rust/platform-tools-agave-rust-solana/vendor/rust-src | |
| generate-flake-dir: | |
| $(MAKE) -C flake-template-generator generate-flake | |
| generate-seed-config: build-config-builder | |
| @echo "Generating seed config.toml using bootstrap-config-generator..." | |
| cargo run --bin bootstrap-config-generator -- \ | |
| --output bootstrap-config-builder/generated_config.toml \ | |
| --project-root $(CURDIR) \ | |
| --rust-src-flake-path /data/data/com.termux.nix/files/home/pick-up-nix2/vendor/rust/platform-tools-agave-rust-solana/vendor/rust-src | |
| generate-flake-dir: | |
| $(MAKE) -C flake-template-generator generate-flake | |
| generate-seed-config: build-config-builder | |
| @echo "Generating seed config.toml using bootstrap-config-generator..." | |
| cargo run --bin bootstrap-config-generator -- \ | |
| --output bootstrap-config-builder/generated_config.toml \ | |
| --project-root $(CURDIR) \ | |
| --rust-src-flake-path /data/data/com.termux.nix/files/home/pick-up-nix2/vendor/rust/platform-tools-agave-rust-solana/vendor/rust-src |
🤖 Prompt for AI Agents
In Makefile around lines 34 to 49, the target generate-seed-config is defined
twice (lines 34-39 and 44-49) which causes Makefile conflicts or silent
overrides; remove the duplicate block so the target is defined only once (keep
the correct invocation and flags you want to preserve), ensure any dependencies
(build-config-builder) remain and that build steps (echo and cargo run with the
desired --output, --project-root and --rust-src-flake-path) are present in the
single remaining definition, and delete the redundant copy.
| cargo run --bin bootstrap-config-generator -- \ | ||
| --output bootstrap-config-builder/generated_config.toml \ | ||
| --project-root $(CURDIR) \ | ||
| --rust-src-flake-path /data/data/com.termux.nix/files/home/pick-up-nix2/vendor/rust/platform-tools-agave-rust-solana/vendor/rust-src |
There was a problem hiding this comment.
Major: Hardcoded absolute Termux path breaks portability.
Line 39 (and the duplicate at line 49) use a hardcoded absolute path /data/data/com.termux.nix/files/home/pick-up-nix2/... that will fail on non-Termux systems and other environments.
Use a relative path or make it configurable:
- --rust-src-flake-path /data/data/com.termux.nix/files/home/pick-up-nix2/vendor/rust/platform-tools-agave-rust-solana/vendor/rust-src
+ --rust-src-flake-path $(CURDIR)/vendor/rust-srcOr use an environment variable:
+RUST_SRC_PATH ?= $(CURDIR)/vendor/rust-src
+
generate-seed-config: build-config-builder
@echo "Generating seed config.toml using bootstrap-config-generator..."
cargo run --bin bootstrap-config-generator -- \
--output bootstrap-config-builder/generated_config.toml \
--project-root $(CURDIR) \
- --rust-src-flake-path /data/data/com.termux.nix/files/home/pick-up-nix2/vendor/rust/platform-tools-agave-rust-solana/vendor/rust-src
+ --rust-src-flake-path $(RUST_SRC_PATH)🤖 Prompt for AI Agents
In Makefile around line 39 (and the duplicate at line 49), the hardcoded
absolute Termux path (/data/data/com.termux.nix/files/home/pick-up-nix2/...)
breaks portability; replace it with a configurable value by using a relative
path from the repo root or a variable (e.g., RUST_SRC_FLake_PATH or RUST_SRC)
that can be set via environment or Makefile default, update both occurrences to
reference that variable, and document the new variable in the Makefile comments
so users can override it for their environment.
| let repo_root = PathBuf::from(env!("CARGO_MANIFEST_DIR")) | ||
| .parent() // standalonex/src/bootstrap/src/bin | ||
| .unwrap() | ||
| .parent() // standalonex/src/bootstrap/src | ||
| .unwrap() | ||
| .parent() // standalonex/src/bootstrap | ||
| .unwrap() | ||
| .parent() // standalonex/src | ||
| .unwrap() | ||
| .parent() // standalonex | ||
| .unwrap() | ||
| .to_path_buf(); // rust-bootstrap-nix root |
There was a problem hiding this comment.
Critical: Chain of unwrap() calls will panic if directory structure changes.
The code uses a chain of .parent().unwrap() calls to traverse up the directory tree. If the expected directory structure changes or if any parent is None, this will panic at runtime.
Use proper error handling:
fn main() -> Result<(), Box<dyn std::error::Error>> {
let manifest_dir = PathBuf::from(env!("CARGO_MANIFEST_DIR"));
let repo_root = manifest_dir
.parent()
.and_then(|p| p.parent())
.and_then(|p| p.parent())
.and_then(|p| p.parent())
.and_then(|p| p.parent())
.ok_or("Failed to find repository root from manifest directory")?
.to_path_buf();
// ... rest of code
}Or use a more robust approach with a loop or git command to find the repo root:
fn find_repo_root(start: &Path) -> Result<PathBuf, Box<dyn std::error::Error>> {
let mut current = start.to_path_buf();
loop {
if current.join(".git").exists() {
return Ok(current);
}
match current.parent() {
Some(parent) => current = parent.to_path_buf(),
None => return Err("Could not find repository root".into()),
}
}
}🤖 Prompt for AI Agents
In standalonex/src/bootstrap/src/bin/git_flake_manager.rs around lines 24–35,
replace the chain of .parent().unwrap() calls that traverse up from
env!("CARGO_MANIFEST_DIR") with a safe lookup: either propagate an error by
returning Result and use repeated and_then(...) or ok_or(...) to fail
gracefully, or implement a loop that walks parents (checking for a .git marker
or stopping when parent is None) and returns a Result<PathBuf, _> if the repo
root cannot be found; ensure no unwrap() is used so the program reports a clear
error instead of panicking.
| &branch_name, | ||
| &base_branch_name, | ||
| &output_dir, | ||
| false, // dry_run |
There was a problem hiding this comment.
Hard-coded dry_run flag bypasses testing capability.
The dry_run parameter is hard-coded to false, which means the binary will always execute Git commands. This makes testing risky and removes the ability to preview operations.
Make it configurable:
use clap::Parser;
#[derive(Parser, Debug)]
struct Args {
/// Perform a dry run without executing Git commands
#[arg(long, default_value_t = false)]
dry_run: bool,
}
fn main() -> Result<(), Box<dyn std::error::Error>> {
let args = Args::parse();
// ... rest of code ...
create_and_push_branch(
&repo_root,
&branch_name,
&base_branch_name,
&output_dir,
args.dry_run,
)?;
Ok(())
}🤖 Prompt for AI Agents
standalonex/src/bootstrap/src/bin/git_flake_manager.rs around line 64: the
dry_run boolean is hard-coded to false so the binary always executes git
commands; make it configurable by adding a CLI args struct (using clap::Parser),
parse args in main to obtain args.dry_run, thread that value into the call to
create_and_push_branch (replace the hard-coded false), and ensure the new
--dry-run flag has a default of false and descriptive help text so callers can
opt into dry-run mode for testing.
This commit splits the large `README.md` file into multiple smaller, more manageable files in the `docs/` directory. This improves readability and navigation of the documentation. The original `README.md` has been removed and its content distributed across the new `README_*.md` files.
This commit addresses several issues encountered during the Nix flake build
process for the `generate-config` package:
1. **Corrected `bootstrap-config-generator` binary path:** Updated
`bootstrap-config-builder/Cargo.toml` to explicitly define the `path`
for the `bootstrap-config-generator` binary, resolving a build error.
2. **Resolved circular flake import:** Modified `flake.nix` to remove the
self-referential `rust-bootstrap-nix` input and adjusted the `outputs`
destructuring to avoid a circular dependency.
3. **Added `pkg-config` and `openssl` to `buildInputs`:** Included `pkgs.pkg-config`
and `pkgs.openssl` in the `buildInputs` of the `generate-config`
derivation in `flake.nix` to satisfy dependencies for Rust crates
linking against OpenSSL.
These changes ensure that the `generate-config` package can be successfully
built and executed within the Nix environment, completing "step 1: we can
build and execute rust in nix" of the overall plan.
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (1)
standalonex/flake.nix (1)
6-6: Use rev= for commit pins, not ref=.You’re pinning to a commit hash; use rev=.
Apply:
- rustSrcFlake.url = "github:meta-introspector/rust?ref=3487cd3843083db70ee30023f19344568ade9c9f"; + rustSrcFlake.url = "github:meta-introspector/rust?rev=3487cd3843083db70ee30023f19344568ade9c9f";
🧹 Nitpick comments (11)
bootstrap-config-builder/src/utils/generate_step1_flake.rs (2)
21-29: Avoid hard‑coding a single system; consider flake‑utils or systems attr.Locking to "aarch64-linux" limits portability. Suggest exposing packages for multiple systems.
I can provide a variant using flake-utils.lib.eachDefaultSystem to emit packages.${system}.default.
30-31: Prefer using the input’s outPath over passing a raw path.Instead of interpolating a host-provided path for rust-src, use Nix interpolation of the rustSrcFlake input inside the template to keep the flake self‑contained and reproducible.
Example in the template:
--rust-src-flake-path ${rustSrcFlake}(Then drop the rust_src_flake_path parameter.)
bootstrap-config-builder/Cargo.toml (1)
1-4: Add rust-version and basic package metadata.Locks compiler expectations and improves crate hygiene.
[package] name = "bootstrap-config-builder" version = "0.1.0" edition = "2024" +rust-version = "1.84" +license = "MIT OR Apache-2.0" +repository = "https://github.com/meta-introspector/rust-bootstrap-nix" +description = "Tools to generate rustc bootstrap config.toml for Nix-driven builds"Ensure the chosen license matches your project.
flake.nix (3)
22-23: Classify cargo/rustc as nativeBuildInputs and include OpenSSL/pkg-config.Aligns with Nix best practices and your crate’s git2 dependency.
- buildInputs = [ pkgs.cargo pkgs.rustc pkgs.cacert pkgs.nix pkgs.pkg-config pkgs.openssl ]; + nativeBuildInputs = [ pkgs.cargo pkgs.rustc pkgs.pkg-config ]; + buildInputs = [ pkgs.openssl pkgs.cacert pkgs.nix ];
19-31: Consider rustPlatform.buildRustPackage for caching and determinism.Using mkDerivation + cargo run misses cargo vendor/caching niceties and can recompile on every evaluation.
I can draft a buildRustPackage version that builds bootstrap-config-generator once and runs it in installPhase.
12-16: Single-arch outputs; consider flake-utils.Expose the same default package for x86_64-linux to improve dev ergonomics.
I can wire flake-utils.lib.eachDefaultSystem if desired.
bootstrap-config-builder/src/utils/git_command_helpers.rs (1)
39-55: output_result error message: include stdout for context.You already inherit stderr; including stdout on failure aids diagnosis.
- if !output.status.success() { - anyhow::bail!( - "Command did not execute successfully: {:?}\nExpected success, got: {}\nStderr: {}", - cmd, - output.status, - String::from_utf8_lossy(&output.stderr) - ); - } + if !output.status.success() { + anyhow::bail!( + "Command failed: {:?}\nStatus: {}\nStdout: {}\nStderr: {}", + cmd, + output.status, + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + }flake.old.nix (1)
134-191: Broken attribute nesting; consider removing or archiving this legacy file.
packages.x86_64-linuxis nested insidepackages.aarch64-linux, making this file invalid. Since this is “old”, either fix the braces or rename to flake.archive.nix to avoid confusion.Minimal brace fix:
- packages.aarch64-linux = configTomlStages_aarch64 // { + packages.aarch64-linux = configTomlStages_aarch64 // { # ... - default = standalonex.packages.${pkgs_aarch64.system}.default.overrideAttrs (oldAttrs: { + default = standalonex.packages.${pkgs_aarch64.system}.default.overrideAttrs (oldAttrs: { # ... - }); - - packages.x86_64-linux = configTomlStages_x86_64 // { + }); + }; + packages.x86_64-linux = configTomlStages_x86_64 // { # ... - }); + });If unused, prefer deletion to reduce maintenance overhead.
standalonex/flake.nix (3)
23-27: DevShell toolchain consistency: avoid mixing pkgs.cargo with rust-bin toolchain.Prefer using cargo from the same rustToolchain to prevent PATH precedence issues.
Apply:
- packages = [ - pkgs.python3 - pkgs.rust-bin.stable."1.84.1".default - pkgs.cargo - ]; + packages = [ + pkgs.python3 + rustToolchain + ];
12-15: Optional: avoid hard-coding system; use builtins.currentSystem or per-system outputs.Hard-coded "aarch64-linux" restricts usability.
Minimal:
- pkgs = import nixpkgs { - system = "aarch64-linux"; + pkgs = import nixpkgs { + system = builtins.currentSystem;Or adopt per-system pattern (lib.genAttrs) later.
55-56: Consider usingbuildAndTestSubdirto clarify workspace member builds.The project has a workspace at
src/containing thebootstrapcrate as a member. While the current approach works (usingcargoBuildFlags = ["--bin" "bootstrap"]to select which binary to build), it's more explicit and idiomatic to usebuildAndTestSubdir = "bootstrap"when building a specific workspace member. This makes the intent clearer and avoids building unrelated workspace members unnecessarily.Apply to lines 55–56, 64–66, and 75–77:
src = pkgs.lib.cleanSource ./src; + buildAndTestSubdir = "bootstrap"; cargoLock.lockFile = ./src/Cargo.lock;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
bootstrap-config-builder/flake.lockis excluded by!**/*.lockflake.lockis excluded by!**/*.lockflakes/bootstrap-builder/cc-flake/flake.lockis excluded by!**/*.lockflakes/bootstrap-builder/flake.lockis excluded by!**/*.lockflakes/bootstrap-compiler-flake/flake.lockis excluded by!**/*.lockflakes/bootstrap-from-json-flake/flake.lockis excluded by!**/*.lockflakes/config/flake.lockis excluded by!**/*.lockflakes/use-bootstrap-flake/flake.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
.gemini/commit-message.txt(1 hunks)bootstrap-config-builder/Cargo.toml(1 hunks)bootstrap-config-builder/src/utils/generate_step1_flake.rs(1 hunks)bootstrap-config-builder/src/utils/git.rs(1 hunks)bootstrap-config-builder/src/utils/git_command_helpers.rs(1 hunks)bootstrap-config-builder/src/utils/mod.rs(1 hunks)flake.nix(1 hunks)flake.old.nix(1 hunks)lintall.sh(1 hunks)standalonex/flake.nix(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .gemini/commit-message.txt
🧰 Additional context used
🧬 Code graph analysis (2)
bootstrap-config-builder/src/utils/git_command_helpers.rs (3)
standalonex/src/bootstrap/src/core/config_utils/src/dry_run.rs (1)
dry_run(12-17)standalonex/src/bootstrap/src/utils/helpers.rs (1)
output(297-310)standalonex/src/bootstrap/src/utils/exec.rs (3)
stdout(287-292)stderr(305-310)status(279-284)
bootstrap-config-builder/src/utils/mod.rs (8)
bootstrap-config-builder/src/utils/compose_path.rs (1)
compose_path(1-3)bootstrap-config-builder/src/utils/construct_config_content.rs (1)
construct_config_content(4-70)bootstrap-config-builder/src/utils/format_file.rs (1)
format_file(5-127)bootstrap-config-builder/src/utils/format_new.rs (1)
format_new(1-3)bootstrap-config-builder/src/utils/get_flake_input.rs (1)
get_flake_input(7-36)bootstrap-config-builder/src/utils/validate_project_root.rs (1)
validate_project_root(7-15)standalonex/src/bootstrap/src/utils/helpers.rs (1)
git(553-571)bootstrap-config-builder/src/utils/generate_step1_flake.rs (1)
generate_step1_flake(1-50)
🪛 Shellcheck (0.11.0)
lintall.sh
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
[warning] 1-1: Use 'find .. -print0 | xargs -0 ..' or 'find .. -exec .. +' to allow non-alphanumeric filenames.
(SC2038)
🔇 Additional comments (1)
bootstrap-config-builder/src/utils/mod.rs (1)
1-9: Do not apply the suggested changes toformat_file—it breaks integration tests.The module
format_fileis imported in an integration test (bootstrap-config-builder/tests/config_integration_test.rs:3), which is compiled as a separate crate. Integration tests can only accesspubitems, notpub(crate). Makingformat_filepub(crate) would break this test.The other three modules (
compose_path,format_new,git_command_helpers) are used only internally within the utils module or not at all. If visibility constraints are desired for those, they could be madepub(crate)without side effects, but there's no immediate risk from leaving them public.Likely an incorrect or invalid review comment.
| buildInputs = [ pkgs.cargo pkgs.rustc pkgs.cacert pkgs.nix ]; | ||
| buildPhase = ''' | ||
| export CARGO_HOME=$(mktemp -d) | ||
| cargo run --bin bootstrap-config-generator -- --project-root . --rust-src-flake-path {} --version 1.84.1 --target aarch64-unknown-linux-gnu --stage 0 | ||
| '''; |
There was a problem hiding this comment.
Build will likely fail: missing OpenSSL and pkg-config for git2/libgit2.
The crate depends on git2; on Nix it typically needs pkgs.openssl and pkgs.pkg-config at build time.
Apply inside the template:
- buildInputs = [ pkgs.cargo pkgs.rustc pkgs.cacert pkgs.nix ];
+ buildInputs = [ pkgs.cargo pkgs.rustc pkgs.cacert pkgs.nix pkgs.pkg-config pkgs.openssl ];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| buildInputs = [ pkgs.cargo pkgs.rustc pkgs.cacert pkgs.nix ]; | |
| buildPhase = ''' | |
| export CARGO_HOME=$(mktemp -d) | |
| cargo run --bin bootstrap-config-generator -- --project-root . --rust-src-flake-path {} --version 1.84.1 --target aarch64-unknown-linux-gnu --stage 0 | |
| '''; | |
| buildInputs = [ pkgs.cargo pkgs.rustc pkgs.cacert pkgs.nix pkgs.pkg-config pkgs.openssl ]; | |
| buildPhase = ''' | |
| export CARGO_HOME=$(mktemp -d) | |
| cargo run --bin bootstrap-config-generator -- --project-root . --rust-src-flake-path {} --version 1.84.1 --target aarch64-unknown-linux-gnu --stage 0 | |
| '''; |
🤖 Prompt for AI Agents
In bootstrap-config-builder/src/utils/generate_step1_flake.rs around lines 31 to
35, the build will fail because the git2/libgit2 crate requires OpenSSL and
pkg-config on Nix; add pkgs.openssl and pkgs.pkg-config to the buildInputs array
so the buildPhase has the required development libraries and pkg-config
available (e.g., include pkgs.openssl and pkgs.pkg-config alongside pkgs.cargo
pkgs.rustc pkgs.cacert pkgs.nix).
| cargo run --bin bootstrap-config-generator -- --project-root . --rust-src-flake-path {} --version 1.84.1 --target aarch64-unknown-linux-gnu --stage 0 | ||
| '''; |
There was a problem hiding this comment.
Quote rust_src_flake_path to avoid word splitting and injection.
Unquoted {} breaks on spaces and special chars.
Apply:
- cargo run --bin bootstrap-config-generator -- --project-root . --rust-src-flake-path {} --version 1.84.1 --target aarch64-unknown-linux-gnu --stage 0
+ cargo run --bin bootstrap-config-generator -- --project-root . --rust-src-flake-path "{}" --version 1.84.1 --target aarch64-unknown-linux-gnu --stage 0📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cargo run --bin bootstrap-config-generator -- --project-root . --rust-src-flake-path {} --version 1.84.1 --target aarch64-unknown-linux-gnu --stage 0 | |
| '''; | |
| cargo run --bin bootstrap-config-generator -- --project-root . --rust-src-flake-path "{}" --version 1.84.1 --target aarch64-unknown-linux-gnu --stage 0 | |
| '''; |
🤖 Prompt for AI Agents
In bootstrap-config-builder/src/utils/generate_step1_flake.rs around lines
34-35, the command string inserts {} unquoted which allows word-splitting and
shell injection for paths with spaces or special chars; fix by shell-escaping or
quoting the rust_src_flake_path placeholder (e.g., wrap the placeholder in
single-quotes or use a dedicated shell-escape function/crate to escape the
value) so the generated command passes a safe, single argument for
--rust-src-flake-path.
| /// Runs a git command and returns the output, handling errors. | ||
| pub fn run_git_command( | ||
| current_dir: &Path, | ||
| args: &[&str], | ||
| error_message: &str, | ||
| dry_run: bool, | ||
| ) -> Result<()> { | ||
| println!("Running git command in CWD: {:?}", current_dir); | ||
| let command_str = format!("git {}", args.join(" ")); | ||
| if dry_run { | ||
| println!("Dry run: Would execute: {}", command_str); | ||
| return Ok(()) | ||
| } |
There was a problem hiding this comment.
Fix compile error: return Ok(()) in dry‑run branch; align docstring.
Current code uses return Ok() which doesn’t type‑check.
-/// Runs a git command and returns the output, handling errors.
+/// Runs a git command in `current_dir`; returns Ok(()) on success, error with context on failure.
@@
- if dry_run {
- println!("Dry run: Would execute: {}", command_str);
- return Ok()
- }
+ if dry_run {
+ println!("Dry run: Would execute: {}", command_str);
+ return Ok(());
+ }Optional: replace println! with log::{debug,info} for consistency with env_logger.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In bootstrap-config-builder/src/utils/git_command_helpers.rs around lines 5 to
17, the dry-run branch uses return Ok() which fails to type-check and the
docstring incorrectly claims the function "returns the output"; change the
dry-run return to return Ok(()) to match the Result<()> signature, and update
the function docstring to state it returns Result<()> (no command output) or
adjust the signature if you actually intend to return command output; optionally
replace the println! calls with log::debug!/log::info! for consistency with
env_logger.
| let signature = Signature::now("bootstrap-config-generator", "[email protected]")?; | ||
|
|
||
| // Create an empty tree | ||
| let tree_id = Oid::from_str("4b825dc642cb6eb9a060e54bf8d69288fbee4904")?; | ||
| let tree = repo.find_tree(tree_id)?; | ||
|
|
||
| // Create the commit | ||
| let commit_id = repo.commit( | ||
| Some("HEAD"), | ||
| &signature, | ||
| &signature, | ||
| "Initial commit for orphan branch", | ||
| &tree, | ||
| &[&head_commit], | ||
| )?; |
There was a problem hiding this comment.
“Orphan branch” logic is incorrect and SHA‑1 empty‑tree OID is brittle.
- Orphan commit must have no parents; current code uses HEAD as parent.
- Using hardcoded OID for the empty tree fails on SHA‑256 repos and when the object isn’t present.
Apply:
- let head = repo.head()?;
- let head_commit = head.peel_to_commit()?;
-
- let signature = Signature::now("bootstrap-config-generator", "[email protected]")?;
-
- // Create an empty tree
- let tree_id = Oid::from_str("4b825dc642cb6eb9a060e54bf8d69288fbee4904")?;
- let tree = repo.find_tree(tree_id)?;
-
- // Create the commit
- let commit_id = repo.commit(
- Some("HEAD"),
- &signature,
- &signature,
- "Initial commit for orphan branch",
- &tree,
- &[&head_commit],
- )?;
-
- // Create the branch
- repo.branch(branch_name, &repo.find_commit(commit_id)?, true)?;
+ let signature = Signature::now("bootstrap-config-generator", "[email protected]")?;
+ // Create an empty tree via TreeBuilder (works for SHA-1 and SHA-256 repos)
+ let mut tb = repo.treebuilder(None)?;
+ let tree_id = tb.write()?;
+ let tree = repo.find_tree(tree_id)?;
+ // Create root commit with no parents and update branch ref directly
+ let branch_ref = format!("refs/heads/{}", branch_name);
+ repo.commit(
+ Some(&branch_ref),
+ &signature,
+ &signature,
+ "Initial commit for orphan branch",
+ &tree,
+ &[],
+ )?;
+ // Point HEAD at the new branch
+ repo.set_head(&branch_ref)?;Also avoid forcing branch creation; if you keep repo.branch(...), use force = false.
Committable suggestion skipped: line range outside the PR's diff.
| for file in files { | ||
| index.add_path(std::path::Path::new(file))?; | ||
| } | ||
|
|
There was a problem hiding this comment.
commit_files: handle absolute paths and unborn HEAD.
- index.add_path expects paths relative to workdir.
- HEAD may be unborn; committing must allow zero parents.
Apply:
- for file in files {
- index.add_path(std::path::Path::new(file))?;
- }
+ let workdir = repo.workdir().context("Repository is bare; cannot compute relative paths")?;
+ for file in files {
+ let p = std::path::Path::new(file);
+ let rel = p.strip_prefix(workdir).unwrap_or(p);
+ index.add_path(rel)?;
+ }
@@
- let parent_commit = repo.head()?.peel_to_commit()?;
-
- repo.commit(
- Some("HEAD"),
- &signature,
- &signature,
- message,
- &tree,
- &[&parent_commit],
- )?;
+ let parent_commit = match repo.head() {
+ Ok(h) => Some(h.peel_to_commit()?),
+ Err(e) if e.code() == git2::ErrorCode::UnbornBranch || e.code() == git2::ErrorCode::NotFound => None,
+ Err(e) => return Err(e.into()),
+ };
+ let parents: Vec<&Commit> = parent_commit.iter().collect();
+ repo.commit(Some("HEAD"), &signature, &signature, message, &tree, &parents)?;Also applies to: 47-57
| buildInputs = [ pkgs.cargo pkgs.rustc pkgs.cacert pkgs.nix pkgs.pkg-config pkgs.openssl ]; | ||
| buildPhase = '' | ||
| export CARGO_HOME=$(mktemp -d) | ||
| cargo run --bin bootstrap-config-generator -- --project-root . --rust-src-flake-path /nix/store/rhs81k02n3vg452abxl462g2i6xyadyf-source --version 1.84.1 --target aarch64-unknown-linux-gnu --stage 0 | ||
| ''; |
There was a problem hiding this comment.
Critical: Hard‑coded store path breaks reproducibility and portability.
Replace the literal /nix/store/... path with the rustSrcFlake input’s outPath.
Apply:
- cargo run --bin bootstrap-config-generator -- --project-root . --rust-src-flake-path /nix/store/rhs81k02n3vg452abxl462g2i6xyadyf-source --version 1.84.1 --target aarch64-unknown-linux-gnu --stage 0
+ cargo run --bin bootstrap-config-generator -- --project-root . --rust-src-flake-path ${rustSrcFlake} --version 1.84.1 --target aarch64-unknown-linux-gnu --stage 0🤖 Prompt for AI Agents
In flake.nix around lines 22 to 26, the buildPhase uses a hard‑coded
/nix/store/... path which breaks reproducibility; replace that literal with the
rustSrcFlake input’s outPath variable (e.g. use ${rustSrcFlake}.outPath or the
local variable bound to rustSrcFlake) so the cargo command references the
input’s actual output path; update the buildPhase to interpolate the
rustSrcFlake outPath into the --rust-src-flake-path argument and keep the rest
of the command unchanged.
| for x in `find -name flake.nix | xargs dirname | sort -u`; | ||
| do | ||
| echo $x; | ||
| nix build $x; | ||
|
|
||
| done |
There was a problem hiding this comment.
Add shebang, robust null‑delimited find, and quote variables.
Current form is fragile for paths with spaces and lacks interpreter declaration.
Apply:
+#!/usr/bin/env bash
+set -euo pipefail
-for x in `find -name flake.nix | xargs dirname | sort -u`;
-do
- echo $x;
- nix build $x;
-
-done
+while IFS= read -r -d '' flake; do
+ dir="$(dirname "$flake")"
+ echo "$dir"
+ nix build --no-link -L "$dir"
+done < <(find . -name flake.nix -print0 | xargs -0 -n1 printf '%s\0' | sort -z -u)This fixes SC2148 and SC2038, quotes vars, and avoids creating/overwriting a single ./result symlink. Based on static analysis hints.
🧰 Tools
🪛 Shellcheck (0.11.0)
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
[warning] 1-1: Use 'find .. -print0 | xargs -0 ..' or 'find .. -exec .. +' to allow non-alphanumeric filenames.
(SC2038)
🤖 Prompt for AI Agents
In lintall.sh around lines 1 to 6: the script lacks a shebang, is fragile with
filenames containing spaces/newlines, and unquoted variables; update by adding a
POSIX shebang (#!/usr/bin/env bash), enable safe flags (e.g. set -euo pipefail),
use find -name 'flake.nix' -print0 piped into a while IFS= read -r -d '' loop to
iterate directories, quote variable expansions (e.g. "$dir"), and call nix build
with --no-link (or a unique --out-link per directory) to avoid clobbering
./result.
| rustPlatform = pkgs.rustPlatform; | ||
|
|
There was a problem hiding this comment.
Toolchain override is ignored; use makeRustPlatform and drop rustc attrs.
buildRustPackage ignores a rustc attribute inside the derivation. Create a rustPlatform bound to pkgs.rust-bin… and use it for all packages.
Apply:
- rustPlatform = pkgs.rustPlatform;
+ rustToolchain = pkgs.rust-bin.stable."1.84.1".default;
+ rustPlatform = pkgs.makeRustPlatform {
+ cargo = rustToolchain;
+ rustc = rustToolchain;
+ };Then remove the per-derivation rustc lines (already handled by rustPlatform):
- rustc = pkgs.rust-bin.stable."1.84.1".default;Repeat for default, bootstrap-main, and nix-bootstrap.
Also applies to: 51-81
| packages.aarch64-linux = { | ||
| default = rustPlatform.buildRustPackage { | ||
| pname = "rust-bootstrap-default"; | ||
| version = "0.1.0"; | ||
| src = pkgs.lib.cleanSource ./src; | ||
| cargoLock.lockFile = ./src/Cargo.lock; | ||
| rustc = pkgs.rust-bin.stable."1.84.1".default; | ||
| doCheck = false; | ||
|
|
||
| bootstrap-main = rustPlatform.buildRustPackage { | ||
| pname = "bootstrap-main"; | ||
| version = "0.1.0"; | ||
|
|
||
| src = pkgs.lib.cleanSource ./src; | ||
| cargoLock.lockFile = ./src/Cargo.lock; | ||
| rustc = pkgs.rust-bin.stable."1.84.1".default; | ||
| doCheck = false; | ||
| cargoBuildFlags = [ "--bin" "bootstrap" ]; | ||
| }; | ||
|
|
||
| nix-bootstrap = rustPlatform.buildRustPackage { | ||
| pname = "nix-bootstrap"; | ||
| version = "0.1.0"; | ||
|
|
||
| src = pkgs.lib.cleanSource ./src; | ||
| cargoLock.lockFile = ./src/Cargo.lock; | ||
| rustc = pkgs.rust-bin.stable."1.84.1".default; | ||
| doCheck = false; | ||
| cargoBuildFlags = [ "--bin" "nix_bootstrap" ]; | ||
| }; | ||
| }; |
There was a problem hiding this comment.
Incorrect packages attrset: bootstrap-main and nix-bootstrap are nested inside default.
They won’t be exported as sibling packages; they become unused attributes of the default derivation. Restructure to top-level attributes.
Apply:
- packages.aarch64-linux = {
- default = rustPlatform.buildRustPackage {
+ packages.aarch64-linux = {
+ default = rustPlatform.buildRustPackage {
pname = "rust-bootstrap-default";
version = "0.1.0";
src = pkgs.lib.cleanSource ./src;
cargoLock.lockFile = ./src/Cargo.lock;
- rustc = pkgs.rust-bin.stable."1.84.1".default;
doCheck = false;
-
- bootstrap-main = rustPlatform.buildRustPackage {
- pname = "bootstrap-main";
- version = "0.1.0";
-
- src = pkgs.lib.cleanSource ./src;
- cargoLock.lockFile = ./src/Cargo.lock;
- rustc = pkgs.rust-bin.stable."1.84.1".default;
- doCheck = false;
- cargoBuildFlags = [ "--bin" "bootstrap" ];
- };
-
- nix-bootstrap = rustPlatform.buildRustPackage {
- pname = "nix-bootstrap";
- version = "0.1.0";
-
- src = pkgs.lib.cleanSource ./src;
- cargoLock.lockFile = ./src/Cargo.lock;
- rustc = pkgs.rust-bin.stable."1.84.1".default;
- doCheck = false;
- cargoBuildFlags = [ "--bin" "nix_bootstrap" ];
- };
- };
+ };
+
+ bootstrap-main = rustPlatform.buildRustPackage {
+ pname = "bootstrap-main";
+ version = "0.1.0";
+ src = pkgs.lib.cleanSource ./src;
+ cargoLock.lockFile = ./src/Cargo.lock;
+ doCheck = false;
+ cargoBuildFlags = [ "--bin" "bootstrap" ];
+ };
+
+ nix-bootstrap = rustPlatform.buildRustPackage {
+ pname = "nix-bootstrap";
+ version = "0.1.0";
+ src = pkgs.lib.cleanSource ./src;
+ cargoLock.lockFile = ./src/Cargo.lock;
+ doCheck = false;
+ cargoBuildFlags = [ "--bin" "nix_bootstrap" ];
+ };
};🤖 Prompt for AI Agents
In standalonex/flake.nix around lines 51 to 81, bootstrap-main and nix-bootstrap
are incorrectly nested inside the default package attrset so they are not
exported as sibling packages; move their rustPlatform.buildRustPackage blocks
out of the "default" attrset and make them top-level entries under
packages.aarch64-linux alongside default (i.e. packages.aarch64-linux = {
default = ...; bootstrap-main = ...; nix-bootstrap = ...; }), keeping each
buildRustPackage block intact, ensuring unique pnames/versions, correct
src/cargoLock/rustc/doCheck/cargoBuildFlags, and proper braces/commas so the
resulting attrset exports all three packages as siblings.
PR Type
Enhancement, Tests, Documentation
Description
Centralized CI configuration: Refactored
CiConfigstruct to manage CI-related paths (channel_file,version_file,tools_dir,llvm_project_dir,gcc_dir) instead of hardcoded referencesNix flake infrastructure: Created comprehensive Nix flake ecosystem including
bootstrap-builder,json-processor,evaluate-rust, anduse-bootstrap-flakefor hermetic Rust bootstrap buildsSimplified build system: Updated
standalonexflake to use standardrustPlatform.buildRustPackagewith cargo2nix integration, removing complex build orchestrationEnhanced error handling: Improved error messages for missing
RUSTC_SYSROOTenvironment variable and added debug logging for configuration pathsJSON output generation: Modified
bootstrap.pyto generate and persist JSON command metadata with execution control forrustcandcargocommandsRefactored symlink handling: Simplified
symlink_file()implementation with early dry-run checks and platform-specific fallbacksCode cleanup: Simplified
hex_encode()calls by removing unnecessary.as_slice()method, fixed typos, and improved code formattingComprehensive documentation: Added guides for Nix flakes, configuration, scripts, and repository architecture with troubleshooting information
Test infrastructure: Created minimal Rust test project and test scripts for JSON output generation and Nix precondition validation
Build automation: Added shell scripts for flake updates, Git mirror setup, stage0 environment configuration, and bootstrap execution
Diagram Walkthrough
File Walkthrough
4 files
config.rs
Centralize CI paths configuration into dedicated CiConfig structstandalonex/src/bootstrap/src/core/config/config.rs
CiConfigstruct to manage CI-related paths (channel_file,version_file,tools_dir,llvm_project_dir,gcc_dir)cifield to mainConfigstruct andCiTOML representationflags-based configuration
config.cifields insteadof constructing paths from
config.srcdefault_opts()to initializeCiConfigwith computed pathsfrom source root
lib.rs
Refactor symlink_file implementation with early dry-run checkstandalonex/src/bootstrap/src/lib.rs
symlink_file()to checkdry_run()at function entry andreturn early
function calls
bootstrap.py
Add JSON command introspection and execution control to bootstrapstandalonex/src/bootstrap/bootstrap.py
from __future__import to top of file (PEP 8 compliance)run()function to generate and optionally write JSON commandmetadata
rustcandcargocommands (returns 0)RUST_BOOTSTRAP_JSON_OUTPUT_DIRsupport forJSON output persistence
flake.nix
Simplify standalonex flake to use standard Rust buildstandalonex/flake.nix
rustSrcFlakereference to newer commit hash(
rust-bin.stable."1.84.1".default,cargo)config.tomlgeneration from shellHookpackages.aarch64-linux.defaultto userustPlatform.buildRustPackage2 files
config.rs
Add debug logging and improve error handling in configstandalonex/src/src/core/config/config.rs
CARGO_TARGET_DIRenvironment variable in testconfiguration
canonicalization
toml_path.canonicalize()to useunwrap_or_elsefallbackinstead of panicking
stage0_versionandsource_versioncomparisonrustc.rs
Improve RUSTC_SYSROOT error messaging and diagnosticsstandalonex/src/bootstrap/src/bin/rustc.rs
RUSTC_SYSROOTenvironment variablewith detailed error messages
RUSTC_SYSROOTrustcvariable11 files
setup.rs
Simplify editor settings template and hash encodingstandalonex/src/src/core/build_steps/setup.rs
settings_template()implementation with empty string returnvalue
hex_encode()call by removing.as_slice()method (acceptsDigestdirectly)tests.rs
Simplify hash encoding in teststandalonex/src/src/core/build_steps/setup/tests.rs
hex_encode()call by removing.as_slice()methoddownload.rs
Simplify hash encoding in download modulestandalonex/src/src/core/download.rs
hex_encode()call by removing.as_slice()methodupdate_all_flakes.sh
Add flake update automation scriptupdate_all_flakes.sh
push-clones.sh
Add local Git mirror setup scriptpush-clones.sh
setup_local_git_mirror.sh
Add Git mirror setup scriptsetup_local_git_mirror.sh
create_stage0_env.sh
Add stage0 environment setup scriptcreate_stage0_env.sh
run_rustc_shim.sh
Add rustc shim execution scriptrun_rustc_shim.sh
run_bootstrap_check.sh
Add bootstrap check execution scriptrun_bootstrap_check.sh
eval_json.sh
Add JSON evaluation scripteval_json.sh
rust_analyzer_eglot.el
Add dummy eglot configurationetc/rust_analyzer_eglot.el
1 files
lib.rs
Fix typo and simplify hash encoding callstandalonex/src/src/lib.rs
hex_encode()call by removing.as_slice()method5 files
main.rs
Add minimal test Rust projectstandalonex/test_minimal/src/main.rs
test_json_output.py
Add test script for JSON output generationstandalonex/test_json_output.py
bootstrap.run()function for JSONgeneration
test_nix_preconditions.sh
Add Nix precondition testing scripttest_nix_preconditions.sh
run_preconditions_test.sh
Add precondition test runner scriptrun_preconditions_test.sh
Cargo.toml
Minimal test Rust package configurationstandalonex/test_minimal/Cargo.toml
hello-rustversion 0.1.024 files
flake.nix
Update Rust source reference and disable sccache buildflake.nix
rustSrcFlakereference to newer commit hashsccachedRustcfunction and related packageoutputs
statixpackage to bothaarch64-linuxandx86_64-linuxdevShellsoutputsfunction signatureflake.nix
Add local bootstrap test flake with mirror referencesflakes/local-bootstrap-test/flake.nix
flake.nix
Add workspace flake with cargo2nix integrationstandalonex/src/flake.nix
Cargo.nix
Add cargo2nix generated configurationstandalonex/test_minimal/Cargo.nix
flake.nix
Add flake for using bootstrap binaryflakes/use-bootstrap-flake/flake.nix
flake.nix
Add bootstrap builder flakeflakes/bootstrap-builder/flake.nix
rustPlatform.buildRustPackageflake.nix
Add JSON processor flake with recursive evaluationflakes/json-processor/flake.nix
flake.nix
Add evaluate-rust flake for recursive build analysisflakes/evaluate-rust/flake.nix
evaluateCommandlibrary functionflake.nix
Add minimal test project flakestandalonex/test_minimal/flake.nix
flake.nix
Add bootstrap builder from JSON flakeflakes/bootstrap-from-json-flake/flake.nix
flake.nix
Add standalonex JSON processor flakeflakes/json-processor-flake/flake.nix
flake.nix
Add cc crate builder flakeflakes/bootstrap-builder/cc-flake/flake.nix
flake.nix
Add bootstrap compiler builder flakeflakes/bootstrap-compiler-flake/flake.nix
flake.nix
Add configuration processor flakeflakes/config/flake.nix
Cargo.nix
Auto-generated Nix derivations for Rust cargo dependenciesstandalonex/src/Cargo.nix
all Rust dependencies
bootstrapandbuild_helperwith theircomplete dependency trees
and Linux systems
reproducible builds
Makefile
Makefile for managing Nix flake updatesMakefile
NIX_FLAKE_ROOTvariable pointing to the Makefile directorybootstrap-builder,bootstrap-compiler-flake, and othersupdate-flakestarget that deletes existingflake.lockfilesand updates them for all specified directories
Cargo.toml
Cargo workspace configuration and path correctionstandalonex/src/Cargo.toml
build_helperdependency path from"../build_helper"to"./build_helper"package and
build_helperconfig.toml
Simplified build configuration structureconfig.toml
entries
[build]section specifying only thesrcpathstandalonexdirectory in thevendor path
config.toml
Rust toolchain version update in configurationstandalonex/config.toml
rustcpath from Rust 1.90.0 to 1.84.1 in the Nix storecargopath from Rust 1.90.0 to 1.84.1 in the Nix storereferences
.pre-commit-config.yaml
Pre-commit hooks configuration file.pre-commit-config.yaml
version
Rust version specification filesrc/version
rust_analyzer_helix.toml
Helix editor rust-analyzer configurationetc/rust_analyzer_helix.toml
channel
CI channel configuration filesrc/ci/channel
devrust_analyzer_settings.json
Rust-analyzer settings configurationetc/rust_analyzer_settings.json
9 files
NIX_FLAKES_DOCUMENTATION.md
Add comprehensive Nix flakes documentationNIX_FLAKES_DOCUMENTATION.md
flakes
CONFIGURATION.md
Add configuration documentationCONFIGURATION.md
configurations
guidance
tasks.md
Add evaluate-rust implementation planflakes/evaluate-rust/tasks.md
SCRIPTS_DOCUMENTATION.md
Documentation for build and development helper scriptsSCRIPTS_DOCUMENTATION.md
debug_build.sh,develop.sh,diagnose.sh,eval_json.sh,get_nix_paths.sh, andtest.shenvironment setup and debugging
x.py build system
qa.md
Build failure documentation and troubleshooting logsqa.md
nix buildwith Rust1.92.0-nightly
config.tomlwithunknown field
CARGO_HOMEOVERVIEW.md
Repository architecture and system design documentationOVERVIEW.md
rust-bootstrap-nixrepositorypurpose and architecture
configuration files, and the
standalonex/directoryhermetic build environments
README.md
Standalone x.py environment and JSON output guidestandalonex/README.md
x.pyenvironment and its JSON outputgeneration capabilities
execution
build invocations
nix buildREADME.md
Project introduction and quick start guideREADME.md
rust-bootstrap-nixrepository as a Nix-baseddevelopment environment for Rust projects
sccacheintegration, and
x.pybuild system supportaarch64-linuxarchitecturecommit-message.txt
Commit message for Nix bootstrap compiler implementation.gemini/commit-message.txt
compiler
bootstrap-builderflake usingpkgs.rustPlatform.buildRustPackagestandalonexflake and modifications tobootstrap.pyfor JSON outputxpy-json-output-flakeand improvements toreproducibility
2 files
Summary by CodeRabbit