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.<name>] and [override.group.<name>] 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.
