`substituteAll` is a terrible function and we need to stop using it / fix it respectively

[roberth]

Issue description

substituteAll is a bash function and a Nix function in pkgs.

The bash function is bad because

• it takes variables from the environment, which is too easily affected, and often the definition of those values is through mkDerivation arguments, in which case it is not clear that any relation exists between the definition and the usage.
  • example: https://github.com/NixOS/nixpkgs/pull/237040#discussion_r1225384166
• the variables tend to be hard to grep. For one, the @ signs are omitted anywhere but in the file
  • this is a --subst-var complaint as well

The Nix function is even worse. Nix has semi-structured data, so the expectations are a bit higher. Variables passed to the pkgs.substituteAll functions all get substituted into the file, right? ... WRONG! Those variables are poured carelessly into mkDerivation, which assigns special meaning to them, and even ignores and replaces some, like system.

Steps to reproduce

• https://github.com/NixOS/nixpkgs/pull/228133#issuecomment-1586226551

Technical details

substituteAll would be useful if it wasn't such a hack. Maybe something could be achieved with structuredAttrs, so that the variables aren't taken from the environment, but from a separate, safe, json file.

There's also this. Not sure if I want to know more about the horrors inside.

• https://github.com/NixOS/nixpkgs/issues/28086

[sternenseemann]

Maybe something could be achieved with structuredAttrs, so that the variables aren't taken from the environment, but from a separate, safe, json file.

Much simpler than structured attrs is probably just prefixing the env vars or dumping the argument to substituteAll into a file via builtins.toJSON and passAsFile.

[AndersonTorres]

This should be still a Bash program?

[roberth]

This should be still a Bash program?

I think it's pure bash because that makes it easy to use as part of bootstrapping. As for the pkgs.substituteAll derivation function, I don't think it has the same requirement, but we might as well reuse what we have.

Maybe something could be achieved with structuredAttrs

prefixing the env vars or dumping the argument to substituteAll into a file via builtins.toJSON and passAsFile.

Or we could use command line arguments. I don't think the limitations of the process env+args memory space apply here because it's pure bash without any execv. Testing needed I guess.

[illode]

Also not immediately obvious is the fact that the pkgs.substituteAll nix function silently skips any variables with hyphens in them, e.g. @serial-number@.

When the bash substituteAll function is called manually, bash stops the user from creating an environment variable with a hyphen since they aren't allowed. I assume that that error gets lost in the build log when using pkgs.substituteAll.

[nixos-discourse]

This issue has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/string-interpolation-in-files-without-substituteall/38779/1

[roberth]

pkgs.substitute seems like a decent replacement for pkgs.substituteAll, especially after https://github.com/NixOS/nixpkgs/pull/287364

For generating build scripts, we could have a function that produces a substitute command invocation instead of a derivation.

At that point I think we have all the conveniences and all that remains is remove usages of the bad flags, and perhaps deprecate them.

[roberth]

Elephant in the room: This isn't Nix substitution, and that's really bad for what's supposed to be a coherent ecosystem. I think we should change Nixpkgs substitute to replace whenever we have the opportunity.

[nixos-discourse]

This issue has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/shellcheck-inline-scripts-during-test-phase/16274/7

[infinisil]

Related:

• https://github.com/NixOS/nixpkgs/pull/301350

[Guanran928]

Any updates? Hit this today while trying to replace something containing a slash. (example: @foo/bar@)

Maybe we can replace substituteAll with something like this?

substituteV2 = {src, ...} @ args: let
  args' = builtins.removeAttrs args ["src"];
in
  pkgs.substitute {
    inherit src;
    substitutions = lib.flatten (lib.mapAttrsToList (n: v: ["--subst-var-by" n v]) args');
  };

[philiptaron]

• 301350

Now that the PR above has landed, my plan is to send out a bunch of PRs for simple uses of substituteAll throughout the tree.