buildEnv: ignore empty paths derivations containing an empty file

[trueNAHO]

Description of changes

pkgs.buildEnv fails with paths derivations containing an empty file:

pkgs.buildEnv {
  name = "touch";

  paths = [
    (pkgs.stdenvNoCC.mkDerivation {
      installPhase = ''touch "$out"'';
      name = "touch";
      src = ./.;
    })
  ];
}

The lacking $out directory causes pkgs.buildEnv to fail due to:

nixpkgs/pkgs/build-support/buildenv/builder.pl

Lines 120 to 123 in b6bcda5

	# The store path must not be a file
	if (-f $target && isStorePath $target) {
	die "The store path $target is a file and can't be merged into an environment using pkgs.buildEnv!";
	}

For convenience, paths derivations containing an empty file should be ignored, avoiding workarounds like replacing touch $out with mkdir $out.

For example, git-hooks.nix' run derivation runs pre-commit hooks and fails accordingly, while including touch $out merely to satisfy Nix derivation requirements:

https://github.com/cachix/git-hooks.nix/blob/0ff4381bbb8f7a52ca4a851660fc7a437a4c6e07/modules/pre-commit.nix#L55-L82

Essentially, the following happens:

pkgs.buildEnv {
  name = "touch";

  paths = [
    (pkgs.stdenvNoCC.mkDerivation {
      installPhase = ''
        # Run validations.
        [ 0 -eq 0 ] || exit

        touch "$out"
      '';

      name = "touch";
      src = ./.;
    })
  ];
}

The following minimal working example demonstrates the issue before and after this PR:

#!/usr/bin/env bash

nix_build() {
  printf "[BEFORE]: nix_build '%s' '%s'\n" "$1" "$2"

  nix \
    build \
    --expr "
      let
        pkgs = import <nixpkgs> {};
      in
        pkgs.buildEnv {
          name = ''$2'';

          paths = [
            (pkgs.stdenvNoCC.mkDerivation {
              installPhase = ''$2 \$out'';
              name = ''$2'';
              src = ./.;
            })
          ];
        }
    " \
    --impure \
    -I "nixpkgs=$1"

  printf "[AFTER] [%d]: nix_build '%s' '%s'\n" "$?" "$1" "$2"
}

nix_build_after() {
  nix_build \
    https://github.com/trueNAHO/nixpkgs/archive/9f3e28853defa9275ce724275e9472a468e58bb1.tar.gz \
    "$@"
}

nix_build_before() {
  nix_build \
    https://github.com/NixOS/nixpkgs/archive/ed0fe13cc637546cad8c3ee903a23459b59f5080.tar.gz \
    "$@"
}

nix_build_before touch
nix_build_before mkdir

nix_build_after touch
nix_build_after mkdir

The following output should be produced:

[BEFORE]: nix_build 'https://github.com/NixOS/nixpkgs/archive/38a95579896675d1db3f6156065297d865337a85.tar.gz' 'touch'
error: builder for '/nix/store/y51b4zr2azk81sq4ccq3q07lqzi6pmqa-touch.drv' failed with exit code 255;
       last 10 log lines:
       > sourcing setup hook '/nix/store/fyaryjvghbkpfnsyw97hb3lyb37s1pd6-move-lib64.sh'
       > sourcing setup hook '/nix/store/kd4xwxjpjxi71jkm6ka0np72if9rm3y0-move-sbin.sh'
       > sourcing setup hook '/nix/store/pag6l61paj1dc9sv15l7bm5c17xn5kyk-move-systemd-user-units.sh'
       > sourcing setup hook '/nix/store/jivxp510zxakaaic7qkrb7v1dd2rdbw9-multiple-outputs.sh'
       > sourcing setup hook '/nix/store/ilaf1w22bxi6jsi45alhmvvdgy4ly3zs-patch-shebangs.sh'
       > sourcing setup hook '/nix/store/cickvswrvann041nqxb0rxilc46svw1n-prune-libtool-files.sh'
       > sourcing setup hook '/nix/store/xyff06pkhki3qy1ls77w10s0v79c9il0-reproducible-builds.sh'
       > sourcing setup hook '/nix/store/ngg1cv31c8c7bcm2n8ww4g06nq7s4zhm-set-source-date-epoch-to-latest.sh'
       > sourcing setup hook '/nix/store/gps9qrh99j7g02840wv5x78ykmz30byp-strip.sh'
       > error: The store path /nix/store/kf4bxck34i3bhcaplq1dzxbcgdcg16z9-touch is a file and can't be merged into an environment using pkgs.buildEnv! at /nix/store/77my1q03gyb36mhs0sij87yr710w9gha-builder.pl line 122.
       For full logs, run 'nix log /nix/store/y51b4zr2azk81sq4ccq3q07lqzi6pmqa-touch.drv'.
[AFTER] [1]: nix_build 'https://github.com/NixOS/nixpkgs/archive/38a95579896675d1db3f6156065297d865337a85.tar.gz' 'touch'
[BEFORE]: nix_build 'https://github.com/NixOS/nixpkgs/archive/38a95579896675d1db3f6156065297d865337a85.tar.gz' 'mkdir'
[AFTER] [0]: nix_build 'https://github.com/NixOS/nixpkgs/archive/38a95579896675d1db3f6156065297d865337a85.tar.gz' 'mkdir'
[BEFORE]: nix_build 'https://github.com/trueNAHO/nixpkgs/archive/c5bcaea1ade2dc77c8a5203cd910d46021336c3c.tar.gz' 'touch'
[AFTER] [0]: nix_build 'https://github.com/trueNAHO/nixpkgs/archive/c5bcaea1ade2dc77c8a5203cd910d46021336c3c.tar.gz' 'touch'
[BEFORE]: nix_build 'https://github.com/trueNAHO/nixpkgs/archive/c5bcaea1ade2dc77c8a5203cd910d46021336c3c.tar.gz' 'mkdir'
[AFTER] [0]: nix_build 'https://github.com/trueNAHO/nixpkgs/archive/c5bcaea1ade2dc77c8a5203cd910d46021336c3c.tar.gz' 'mkdir'

For reference, this PR follows up on #56085 and #134215.

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.

---

Add a 👍 reaction to pull requests you find important.

[trueNAHO]

Changelog

v1: 9f3e288

• Rebase on top of commit ed0fe13 (2024-09-12)

v0: c5bcaea

[philiptaron]

#56085 by @Ma27 introduced this code.

[Ma27]

I'm not sure I like this: buildEnv was built to "join" multiple directories together using symlinks, so naturally it's wrong to put files into paths.

Ignoring certain files (i.e. empty ones) for convenience seems like too much magic to me. Is it really that painful to replace touch with mkdir? The error message is relatively clear since #56085

[trueNAHO]

I'm not sure I like this: buildEnv was built to "join" multiple directories together using symlinks, so naturally it's wrong to put files into paths.

Ignoring certain files (i.e. empty ones) for convenience seems like too much magic to me.

Currently, buildEnv implicitly treats paths derivations with an empty directory as an identity element, as they do not affect the final output. However, it unintuitively does not treat paths derivations consisting with an empty file as an identity element, despite them also not affecting the final output.

This is a design inconsistency. Either all empty derivations should be rejected with an error or consistently treated as identity elements. Intuitively, all empty derivations should be treated as identity elements.

For reference, both pkgs.emptyDirectory and pkgs.emptyFile are considered to be "empty" according to their name.

Is it really that painful to replace touch with mkdir?

Considering that touch $out is significantly more common than mkdir $out for creating empty derivations in the Nix ecosystem, replacing touch $out with mkdir $out would cause unnecessary effort on a lot of existing Nix projects.

For reference, Nixpkgs extensively uses touch $out:

$ git checkout --quiet ed0fe13cc637546cad8c3ee903a23459b59f5080 &&
    rg 'touch (\$out|"\$out")($|[^/])' | wc --lines
301

[Ma27]

unintuitively

I think this is where we disagree: I think it's unintuitive to put a file into a builder that's supposed to operate on directories.

And making a distinction between whether a file is empty (you can put it in there) and non-empty (you can't) really seems like a good candidate for another Nix quirk. I mean, the difference to directories is: you can put non-empty directories in there, but you can't do the same with files.

For reference, Nixpkgs extensively uses touch $out:

Yes.
What I fail to understand is why any of these are supposed to belong into a buildEnv invocation.

[trueNAHO]

unintuitively

I think this is where we disagree:

Yes.

I think it's unintuitive to put a file into a builder that's supposed to operate on directories.

[...]

And making a distinction between whether a file is empty (you can put it in there) and non-empty (you can't) really seems like a good candidate for another Nix quirk. I mean, the difference to directories is: you can put non-empty directories in there, but you can't do the same with files.

Indeed. Maybe Nix should promote shifting the best practice from touch $out to mkdir $out to better express the intention of an "empty" derivation?

For reference, Nixpkgs extensively uses touch $out:

Yes. What I fail to understand is why any of these are supposed to belong into a buildEnv invocation.

This PR aims to "simplify" potential buildEnv usage.

Either way, you are understandably skeptical of this proposal. Unless you expect your stance to change, feel free to close this PR. Or should we Cc:/ping more people in this conversation?

---

Thanks for all the insightful feedback.

[Ma27]

Maybe Nix should promote shifting the best practice from touch $out to mkdir $out to better express the intention of an "empty" derivation?

I don't have such a strong opinion about that since those are mostly testing derivations, i.e. these run a bunch of tests and the touch $out is basically the exit 0 or whatever to tell Nix that the test is OK. No reason to put those into a buildEnv I'd argue.

OTOH I've heard the opinion that the store should allow directories only in the first place.

Anyways, if we go down that route this should be a conscious decision, so nothing that should contributors should be asked to do in random PR reviews.

Unless you expect your stance to change, feel free to close this PR.

I'm afraid I don't.
I think I explained why sufficiently above.

Or should we Cc:/ping more people in this conversation?

The thing is, buildEnv is hardly touched these days because it does what it does, so I'm not even sure who else to ping.
Feel free to ask more people who perhaps touched the code in the past, but my stance would be to close it.

Thanks for all the insightful feedback.

Sure, you're welcome :)