replaceVars: allow exemptions

[wolfgangwalther]

replaceVars has a checkPhase to confirm that no left-over @...@ patterns remain. Since it's not possible to use "placeholder" with replaceVars, the substitution of some patterns must be delayed to a later step.

By passing "false" for those keys explicitly, replaceVars can make an exemption just for this case, but keep checking all other references.

This replaces the approach taken in #339303. @philiptaron

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
  • sandbox = relaxed
  • sandbox = true
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 25.05 Release Notes (or backporting 24.11 and 25.05 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
• Fits CONTRIBUTING.md.

---

Add a 👍 reaction to pull requests you find important.

[philiptaron]

I like it.

[emilazy]

I like the idea, but I don’t love false here, semantically. (What would true be?)

I would prefer null or perhaps an explicit placeholder value, if those are options.

(Or, semi‐serious straw proposal: foo = "@foo@"; would “naturally” have the semantics we want… (and would simplify the implementation, since we could continue doing the replacement and just treat @foo@s present in the replacements as things to ignore during scanning?))

[wolfgangwalther]

I like the idea, but I don’t love false here, semantically. (What would true be?)

I would prefer null or perhaps an explicit placeholder value, if those are options.

I started to write this comment to argue for the semantics of false.. only to convince myself that null has even better semantics. "false" means "don't replace this / disable replacing" for me, which is more of an "imperative" style. But null doesn't only mean "absent", but also "unknown" - and I like that semantic very much. "I don't know that value, yet, I will handle it later.". In that sense, it's a more declarative approach.

(Or, semi‐serious straw proposal: foo = "@foo@"; would “naturally” have the semantics we want… (and would simplify the implementation, since we could continue doing the replacement and just treat @foo@s present in the replacements as things to ignore during scanning?))

I like the idea, but that would open a can of worms: If foo = "@foo@" was a thing, you could reasonably expect foo = "@bar@" and foo = "@foo@/something/else to work, too. But I don't see how to put those into grep's regex, so that the check phase will actually work.

[philiptaron]

I'm fine with either false or null as a sentinel value, and if @emilazy prefers null, that tips the scales.

The argument in favor of false is that it's much less commonly used in package argument lists, so inherit foo is less likely to pass it through.

Consider:

{ lib, stdenv, dep, optionalDep ? null }:

...
   (replaceVars ./patch { inherit dep optionalDep; })

It's kind of a weak argument, and if we adopt a sentinel-based approach at all, null is a fine thing to settle on.

I originally used builtins.toString on the values passed in, and was guided out of that decision -- and very glad of it now, since true, false, null, and many other values aren't stringified but rather raise evaluation errors.

As @wolfgangwalther points out, "@foo@" isn't a good sentinel because it looks like any other string value, and hence would take substantially more logic to avoid. While it could be done, it would up the "magic factor" of the replaceVars expressions, and so I think we should not do it.

The really rogue sentinel value option is replaceVars itself.

replaceVars ./something { foo = replaceVars; }

There's some language that advocates against well-known sentinels in favor of these bespoke, qunine-ish beasts, but it's slipping my mind at the moment. I prefer null on familiarity grounds.

[emilazy]

FWIW, I believe the logic i had in mind could be implemented by dropping the conditional on subst-var-by here, and something like: instead of matching on @name@ explicitly in lookahead, simply include all replacement values in left-overs. That could handle @wolfgangwalther’s examples and it might actually be simpler, implementation‐wise. (I think the regex there doesn’t quite work out, but you get the concept.)

However I agree it’s sort of weird. I like null, in this context (not in general!). I definitely don’t like anything that uses equality on functions.

My one remaining counterproposal is just replaceVarsWith { ignore = ["out"]; } { … }, which avoids all in‐band signalling. I would be fine with both null and that.

[philiptaron]

My one remaining counterproposal is just replaceVarsWith { ignore = ["out"]; } { … }, which avoids all in‐band signalling. I would be fine with both null and that.

Me too. I was on deck to build it before Revenge of the Work Schedule dismayed my efforts. The day may yet come, it's just not now.

[msanft]

Thanks!

[wolfgangwalther]

FWIW, I believe the logic i had in mind could be implemented by dropping the conditional on subst-var-by here, and something like: instead of matching on @name@ explicitly in lookahead, simply include all replacement values in left-overs. That could handle @wolfgangwalther’s examples and it might actually be simpler, implementation‐wise. (I think the regex there doesn’t quite work out, but you get the concept.)

I thought about that, but couldn't think of a regex to actually make this work. In any case ...

However I agree it’s sort of weird. I like null, in this context (not in general!).

... I went with the null.

My one remaining counterproposal is just replaceVarsWith { ignore = ["out"]; } { … }, which avoids all in‐band signalling.

I'd leave that for a future improvement once it becomes necessary.

[wolfgangwalther]

Re-targeted to staging, because of quite a few rebuilds for darwin.

Should be good to go from my side.

[emilazy]

I'd leave that for a future improvement once it becomes necessary.

FWIW, I don‘t think that will be possible: avoiding the pitfalls of null would involve rejecting it, which would be a breaking change, which is not really something we can do for infrastructure like this. So we do have to decide on whether want a sentinel value or not now. I am fine with either choice; I lean slightly towards out‐of‐band signalling as being safer albeit clunkier, but I will defer to your judgement.

[wolfgangwalther]

I'd leave that for a future improvement once it becomes necessary.

FWIW, I don‘t think that will be possible: avoiding the pitfalls of null would involve rejecting it, which would be a breaking change, which is not really something we can do for infrastructure like this. So we do have to decide on whether want a sentinel value or not now. I am fine with either choice; I lean slightly towards out‐of‐band signalling as being safer albeit clunkier, but I will defer to your judgement.

Hm, I don't think replaceVars would need to be touched necessarily when introducing replaceVarsWith. So you could still introduce that, pass the ignore list out-of-band for that - but keep the null handling for the regular replaceVars. And you could also treat nulls differently for replaceVarsWith, then.

I see the theoretical risk for passing optional dependencies accidentally as null with unwanted results, but (1) the build will almost certainly fail and (2) using null for optional dependencies is an anti-pattern anyway. I'd much rather see us making progress on NixOS/rfcs#169 or alternatives, than supporting that anti-pattern explicitly.

[emilazy]

I suppose that is true if you accept replaceVars and replaceVarsWith { } being different. This would result in people nitpicking replaceVars into being essentially deprecated, or introducing replaceVars2 = replaceVarsWith { }, though. Anyway, I don’t know if null will be a meaningful problem here in practice, so I’m happy to proceed with this version.

[philiptaron]

Could you update the commit message to reflect the switch from false to null? I'll merge after that.

[wolfgangwalther]

Could you update the commit message to reflect the switch from false to null? I'll merge after that.

Done, no changes except for the commit message.