replaceVars.withoutCheck: init

[philiptaron]

Motivation for change

replaceVars is a wrapper around the bash function substitute in the stdenv.

It has a checkPhase which causes the derivation to fail to build if any remaining text that matches @[A-Za-z_][0-9A-Za-z_'-]@ is in the output after replacement has occurred.

As can be seen in #338962 (comment), sometimes this check phase is too strict, and there needs to be two phases of substitution.

So this PR introduces replaceVars.withoutCheck, which has the same semantics but doesn't have a check phase making the assertion about remaining unreplaced variables.

Description of changes

• Add replaceVars.withoutCheck
• Document it
• Test it
• Use it

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/)
• 24.11 Release Notes (or backporting 23.11 and 24.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.

[emilazy]

I don’t understand how this works. (x: x) // { } is an error. Is this some __functor magic? I think I would prefer something like replaceVarsWith { doCheck = false; } ./file { … }, where replaceVars = replaceVarsWith { }.

[philiptaron]

Is this some __functor magic?

Yes, kind of. An attrset with __functor is also just an attrset.

nix-repl> replaceVars = callPackage ./pkgs/build-support/replace-vars { }

nix-repl> replaceVars
{
  __functionArgs = { ... };
  __functor = «lambda __functor @ /home/philip/Code/github.com/philiptaron/nixpkgs/lib/trivial.nix:957:19»;
  override = { ... };
}

nix-repl> replaceVars // { a = 42; }
{
  __functionArgs = { ... };
  __functor = «lambda __functor @ /home/philip/Code/github.com/philiptaron/nixpkgs/lib/trivial.nix:957:19»;
  a = 42;
  override = { ... };
}

It's used a couple places in all-packages.nix:

1. https://github.com/NixOS/nixpkgs/blob/6cc3e274c947096556c57d2b05dad5acda395b74/pkgs/top-level/all-packages.nix#L4036-L4038
2. https://github.com/NixOS/nixpkgs/blob/6cc3e274c947096556c57d2b05dad5acda395b74/pkgs/top-level/all-packages.nix#L15825 (where I learned this trick)
3. https://github.com/NixOS/nixpkgs/blob/6cc3e274c947096556c57d2b05dad5acda395b74/pkgs/top-level/all-packages.nix#L33290-L33295
4. https://github.com/NixOS/nixpkgs/blob/6cc3e274c947096556c57d2b05dad5acda395b74/pkgs/top-level/all-packages.nix#L33961-L33964

[philiptaron]

I'll note that callPackage is used in cases like this where it doesn't actually resolve to a package, but rather to a factory function that will then produce packages/derivations. If there were something like a callPackageFactory function that included the ability to passthru additional contents, this would look quite a bit more sane.

The core piece of code that's creating this functor attrset is

nixpkgs/lib/customisation.nix

Line 264 in 4e2566e

then makeOverridable f allArgs

As it is, I'm pretty sure that this specific feature doesn't yet justify replaceVarsWith, but if you feel quite strongly, I could introduce it.

[emilazy]

Right, I figured callPackage might be doing something like this. I just think __functor is weird and people don’t expect to find more functions under a function.

The nice thing about replaceVarsWith is that it would compose with any future extensions, like if we want to support postPatch or something. If you’re totally sure that there’ll never be another option ever again then maybe it could just be replaceVarsWithoutCheck?

[philiptaron]

people don’t expect to find more functions under a function

One of the reasons I tilted this (odd) way was that I was able to document this variant "under" replaceVars so that it appeared in context on https://noogle.dev/f/pkgs/replaceVars. That documentation outweighs the oddity of people finding or failing to find functions under a function... I think. 😁

[infinisil]

I agree with @emilazy that __functor should be avoided when possible, it can be very confusing. I like the idea of replaceVarsWith more, especially because the same pattern is being used elsewhere too. Check out the docs for lib.fileset.gitTracked as an example how such a pair of functions can be crosslinked together in the docs, but nesting one under the other would also be fine imo.

[philiptaron]

All right, I'll do replaceVarsWith.

[msanft]

Thanks a lot for adding this, and for taking the time to properly document and test it. I'm fine with the current implementation.

[infinisil]

Thinking about alternatives to this, what if there was an alternative to replaceVars that doesn't return a derivation, but rather a command string, which when run returns the substituted path? Then one should be able to use placeholder for derivations to refer to themselves in replacements.

Edit: Or rather, it should return a list of command and arguments to run.

[wolfgangwalther]

replaceVars.withoutCheck has the problem that it will disable all of the checkPhase - losing many of the benefits. Instead, it would be much better to disable the check only for some of the arguments and thus allow checking for other unreplaced patterns.

I took this approach in #357395.

Thinking about alternatives to this, what if there was an alternative to replaceVars that doesn't return a derivation, but rather a command string, which when run returns the substituted path?

I tried, but couldn't really think of a nice user interface for this.

[philiptaron]

Closing in favor of #357395.