PR #1515 Review: per-package build overrides ============================================= Author: shssoichiro (Josh Holmer) | +406/-6 | 4 files URL: https://github.com/Morganamilo/paru/pull/1515 Adds [override.package.] and [override.group.] sections to paru.conf for per-package MakepkgConf and env var overrides. Closes #901. Fact-checked issues ------------------- 1. EnvGuard uses std::env::set_var/remove_var which are unsound in multi-threaded programs. Build loop is sequential so builds don't race each other, but tokio runtime threads can still read environ concurrently via libc. exec.rs passes PKGDEST via Command::env(), same pattern could be extended for override env vars but requires changing new_makepkg's signature. Ref: https://doc.rust-lang.org/std/env/fn.set_var.html#safety 2. Save/restore of config.makepkg_conf in build_pkgbuild. The current code does store result then restore, so ? doesn't skip it. But the pattern is fragile: any future refactor adding a ? between mutation and restore breaks silently. RAII guard or pass-as-parameter is safer. 3. parse_overrides_map splits on ALL commas before processing pairs. Confirmed: LDFLAGS = "-Wl,--as-needed" splits into `LDFLAGS = "-Wl` and `--as-needed"`, the second part then fails on missing `=`. Quote-aware splitting needed, or document the limitation. 4. Unknown override keys use eprintln + Ok(()). This actually matches existing behavior in [options] (config.rs:1148) and repo sections (config.rs:981). Consistent with the codebase. NOT a bug. 5. get_base_override merges with last-writer-wins across base.packages(). Iteration is over a Vec so it IS deterministic, but the order depends on srcinfo parse order which is opaque to the user. If two split packages (e.g. mesa/mesa-debug) have conflicting overrides, the winner depends on internal ordering. Still worth flagging. 6. No validation that MakepkgConf path exists at parse time. Errors surface later from makepkg. Quick Path::exists() check would help. 7. No tests for parse_overrides_map, finalize_override, or get_base_override. Project has mock framework already. Verdict ------- Addresses a real need (#901) and the config syntax design is reasonable. The set_var soundness issue and comma parsing bug should be resolved before merging. The rest are quality improvements worth requesting. Review comments (kai tone, ready to paste) ========================================== --- COMMENT 1: install.rs, EnvGuard --- `EnvGuard` uses `std::env::set_var`/`remove_var` to mutate the process environment. these are unsound in multi-threaded programs (https://doc.rust-lang.org/std/env/fn.set_var.html#safety). the build loop itself is sequential so two builds won't race each other, but tokio's runtime threads can still read `environ` concurrently via libc calls during the window where the env is modified. `new_makepkg` in `exec.rs` already passes `PKGDEST` via `Command::env()` on the subprocess. would it work to extend that pattern to pass override env vars through `Command::envs()` instead of mutating the process environment? that would require adding a parameter to `new_makepkg`/`makepkg_dest` for the extra env, but it removes `EnvGuard` entirely and avoids the soundness issue. --- COMMENT 2: install.rs, build_pkgbuild save/restore --- the save/restore of `config.makepkg_conf` works today because you store the result before restoring. but the pattern is fragile: if someone adds a `?` between the mutation and the restore during a future refactor, it silently leaks the overridden config into subsequent builds. passing the override makepkg conf as a parameter to `build_pkgbuild_inner` instead of mutating `config` would side-step this entirely and make the data flow clearer. --- COMMENT 3: config.rs, parse_overrides_map --- `parse_overrides_map` does `inner.split(',')` before processing key=value pairs, so values containing commas break even when quoted. i checked out the branch and verified this: ``` // input: { LDFLAGS = "-Wl,--as-needed" } // split produces: [`LDFLAGS = "-Wl`, `--as-needed"`] // second element has no `=`, parse errors out parse_overrides_map(r#"{ LDFLAGS = "-Wl,--as-needed" }"#) // => Err("invalid override entry, expected KEY = value") parse_overrides_map(r#"{ LDFLAGS = "-Wl,--as-needed", CFLAGS = "-O2" }"#) // => same error ``` `-Wl,` flags in LDFLAGS are very common in the wild. either the parser needs to skip commas inside `"..."` pairs, or the manpage should explicitly document that values cannot contain commas. --- COMMENT 4: install.rs, get_base_override --- `get_base_override` iterates `base.packages()` and merges overrides with last-writer-wins. the iteration order comes from a Vec so it's deterministic, but the order depends on srcinfo parse order which the user can't predict. if two packages in a split PKGBUILD (e.g. mesa/mesa-debug) have conflicting overrides, the result is stable but opaque. would it make sense to either match only on `base.package_base()`, or warn when conflicting overrides exist across packages in the same base? --- COMMENT 5: config.rs, MakepkgConf path validation --- when `MakepkgConf` is set in an override section, the path isn't checked at parse time. a typo surfaces later as a cryptic makepkg error during the build. a `Path::exists()` check in `parse_override_directive` or `finalize_override` would catch this early with a clear message. --- COMMENT 6: general, test coverage --- `parse_overrides_map` has a custom format parser with quoting, comma splitting, and error handling. `finalize_override` has a state machine. `get_base_override` has merge semantics. none of these have tests. the project already uses `#[cfg(test)]` and has a mock framework, so the infrastructure is there. even a handful of unit tests for `parse_overrides_map` covering happy paths, malformed input, and the comma-in-value edge case would make this safer to land and easier to refactor later.