replaceVars: init

[philiptaron]

Here's an attempt at making an easy-to-use variant of pkgs.substitute that suffers from none of the pitfalls of substituteAll. It's named replaceVars since this isn't the unrelated concept of Nix substitution.

Motivating transformation

From pkgs/applications/audio/curseradio/default.nix:

  patches = [
    (substituteAll {
      src = ./mpv.patch;
      inherit mpv;
    })
  ];

Into something like this, after nixfmt has its way:

  patches = [
    (replaceVars ./mpv.patch { inherit mpv; })
  ];

According to a quick grep, there's around 340 of these throughout the codebase, making up the majority of the roughly 430 uses of substituteAll.

Simple cases:

• rg -A1 --no-heading --no-filename --no-line-number -tnix -F '(substituteAll {' | rg -F 'src = ./' | wc -l

All cases:

• rg --no-heading --no-filename --no-line-number -tnix -F 'substituteAll {' | wc -l

Goals

1. Textually shorter than substituteAll for the majority case, where the thing being replaced lives in the repository rather that acquired through a fetcher or part of a more complicated substitution pipeline.
2. Uses --replace-fail exclusively. I think the ergonomics of normal-looking names in the .nix files makes me want to continue the name to @name@ substitution throughout the codebase.
3. Has a checkPhase: when there are remaining @...@ tokens in the output, will fail. This would be wrong for the general-purpose substitute function but net positive for the patch substitution use case. This is the main reason for wanting --replace-fail exclusively.
4. Has a name that reflects its main use case. The name I initially chose, substituteAttrs, didn't hit this mark. Perhaps replaceVars might.
5. Clean separation of derivation attrs and replacement attrs. The attrsets for the derivation itself and the attrset for the substitutions cannot be confused. This is the sin that substituteAll commits.

Description of changes

• Introduce replaceVars function.
• Test it

Things done

• Added tests in pkgs/test/replace-vars/default.nix (run with nix-build -A test.replaceVars)
• Fits CONTRIBUTING.md.

[ghost]

cool. thanks!

[roberth]

Thank you for picking this up!

[roberth]

Still not a big fan of the automatic @s, also noting that the name is very generic, admittedly due to my own suggestion.
Should it be like this?

replaceInPath { variables = { inherit bash; }; } ./foo.sh

That way it can be extended with arguments like

replaceInPath { replacements = { "/store/dir" = builtins.storeDir; }; } bar

[philiptaron]

Robert, I'm pretty set on the argument order (attrset last) and the semantics (name = value, where name in the source is @name@). They correspond the actual need and usecase in nixpkgs, and they're similar to other well-known functions like callPackage. Considering future additions other than maybe directories is, in my view, out of scope. My view: YAGNI.

On the other hand, I'm very open to other names than replaceInPath, especially if they help with the ill-at-ease feeling you have about name => @name@.

What about replaceVarsInPath or replaceVarsByAttrs, following the name of --subst-var and --subst-var-by, both of which have the @name@ convention? Or replaceAllVarsInPath?

[emilazy]

I don’t know if I like the name replaceInPath but I like that it is short and relatively snappy. “Use replaceAllVarsInPath, not substituteAll” feels like an unforced error in terms of “marketing”. Ideally we would somehow give it a name that is even nicer and more appealing than substituteAll. replaceVars?

[roberth]

the argument order (attrset last)

Whoops, didn't mean to relitigate this. I just find it natural to order positional arguments from least dynamic to most dynamic, and I consider the substitutions to be the more dynamic value. EDIT: uhh wait, so actually I agree then.

My view: YAGNI.

We know we need all the functionality, because otherwise it wouldn't already exist in the bash functions.
The real question is how to turn all the functionality into nice interfaces.

I completely understand the need to scope things down, but by saying we won't need it, you're denying a clear need.
I'm probably just taking these five words too literally, but we wouldn't be doing any of this if words didn't matter.

Or maybe I missed something; is this part of a larger plan you have in mind?

Please take my comments with a grain of salt - I think you're more aware of the big picture here than I am.

[emilazy]

FWIW I think having to write { variables = …; } would also be bad for user acceptance. Not that a more general interface would be bad to have, but we should have something simple for the common case of “script or patch that needs something parameterized”, and it’s okay to optimize for the common case even if we want to expose something more advanced too.

(Also the Bash functions are more often used to deal with random third‐party files that we don’t control, which I suspect is not so much the main use case of this function.)

[roberth]

Fair enough then.
@emilazy makes a good point about "marketability", so if you could rename it to her suggestion replaceVars, that sounds good to me.

Sorry for my detour, but I think we're getting to a good place.

[philiptaron]

replaceVars?

I love replaceVars.

[infinisil]

I like it! I found some things to address, but otherwise I think we should merge this :)

[infinisil]

LGTM!

[roberth]

Without user facing docs (#301350 (comment)) this may as well not exist 😢

[philiptaron]

Without user facing docs (#301350 (comment)) this may as well not exist 😢

It needs some real life transformation experience. When I get back from camping I'm planning on doing a pass through nixpkgs on current substituteAll calls to see if it works or needs tweaks.

Then the marketing can ensue.

[roberth]

Awesome. Enjoy camping!

[philiptaron]

Docs are live on noogle at least: https://noogle.dev/f/pkgs/replaceVars

[philiptaron]

I've run into two things so far that might motivate a replaceVarsWith function:

1. isExecutable = true, so that the replacement has the executable bit set.
2. Exceptions for the check phase: if the replacements are done in two phases, that's a blocker.

I'm just skipping derivations that have these properties.

[emilazy]

1. isExecutable = true, so that the replacement has the executable bit set.

What if we made the output have the same executable bit as the input?