When to use substituteInPlace functions vs a patch?

Help
1

Sometimes I need to change a source file and I’m not sure how I should do it. What is a good guideline for when to use each of these?

  • a patch file
  • the bash stdenv substituteInPlace function
  • the nix substituteInPlace function
3 Likes
2

A patch file usually has the advantage that it fails loudly when it no longer applies. This is important
because we have automatic update mechanism like nixpkgs-update and might miss failing substituteInPlace invocations.
It should be used if you are make logical changes to the code.
substituteInPlace in works well if you change strings i.e. paths to program/libraries with files in the nix store - especially if those occurrences happens in many files.
The nix function substituteAll works well if you have a patch that contains path store references. Use this if you actually want to use a patch but still have some references to the nix store.

7 Likes
3

In some cases such as python packages which frequently need to have dependency versions changed, failing silently is sometime advantageous because the patches are only necessary to make the build succeed, but not critical for package validity. This allows for nixpkgs-update to continue with updating packages even though a “patch” couldn’t be applied. Obviously if you were doing something like --replace 'run("protoc")' 'run("${protobuf}/bin/protoc")' you would want it to fail loudly, as the patch is critical for it’s runtime behavior.

In general I agree with your other points though :+1:

4 Likes
4

nixpkgs-review-checks now also checks for stale substitutes. An example can be found here.

5

I am surprised that substitute and friends will fail to replace anything and continue as if everything’s okay, only printing a warning. I feel like in most cases, outside of exceptional cases like @jonringer mentions, a failed substitution should be inspected. It might be necessary to avoid breaking some aspect of a program. It’s also an opportunity to clean up unneeded substitutions. I see that the underlying replace program can’t indicate via exit code whether any substitutions failed though, unfortunately. I wonder if we could add that functionality, or at least document this limitation of substitute*, and maybe have better guidelines around when to use them vs. sed vs. patches. Failing patches abort the build, build scripts run with set -e… Whatever we recommend for modifying sources deserves the same treatment.

It would also be nice to catch problems earlier on, at build time. @Sandro, I don’t suppose you have any numbers on how many unused substitutions there are across all of Nixpkgs?

3 Likes
6

I don’t but it could be easily calculated by ripgrepping the logs of all builds on hydra.

4 Likes
7

Alternately, we could get the numbers from hammering: tests: add integration test to check for crashes against nixpkgs by rmcgibbo · Pull Request #97 · jtojnar/nixpkgs-hammering · GitHub

8

Right, of course all the build logs are there, good thinking.

Also I need to correct what I said, substitute does not use that replace program; the paragraph I got that from in the stable Nixpkgs manual has been removed in unstable. substituteStream in stdenv is the primitive.

9

Sorry to hijack here, but what are the advantages of substituteInPlace over sed if it does not fail loudly? I can imagine the Warning + no need to worry about the sed-character/regexp-characters to escape + easier to apply multiple modifs on a single file, am I missing something important?

1 Like
10

It’s much easier to read this:

  substituteInPlace $out/bin/foo \
    --replace /usr/local/bin ${lib.getBin foo}/bin

Than this:

  sed -i $out/bin/foo \
    -e 's@/usr/local/bin@${lib.getBin foo}/bin@g'

For the trivial subsitution cases, it’s far easier (IMHO) to both read and write.

7 Likes
11

But that being said, I would love to have a --fail-on-no-subst flag for substituteInPlace and friends.

6 Likes
12

Someone would need to add a new case entry for the argument, set a variable and fail here if set and test it.

13

Would be great to get a Hydra eval where substituteInPlace fails on no substitution to see how large the breakage (and therefore need for manual fixes) would be.

1 Like