buildEnv: break with a proper error if one path is actually a file

[Ma27]

Motivation for this change

I noticed by creating buildEnv where I accidentally put a derivation
from pkgs.writeText into paths and got a broken build with the
following misleading error message:

Use of uninitialized value $stat1 in numeric ne (!=) at /nix/store/9g4wc31j7a2xp22xpgwr0qssfxahxdzl-builder.pl line 74.
Use of uninitialized value $stat1 in bitwise and (&) at /nix/store/9g4wc31j7a2xp22xpgwr0qssfxahxdzl-builder.pl line 75.
different permissions in `' and `/nix/store/0vy5ss91laxvwkyvrbld5hv27i88qk5w-noise': 0000 <-> 0444 at /nix/store/9g4wc31j7a2xp22xpgwr0qssfxahxdzl-builder.pl line 75.

It can be reproduced with an expression like this:

{ pkgs ? import <nixpkgs> { } }:
let
  file = pkgs.writeText "test" ''
    content
  '';
in
  pkgs.buildEnv {
    name = "test-env";
    paths = [ /* ... */ file ];
  }

Things done

• Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
• Built on platform(s)
  • NixOS
  • macOS
  • other Linux distributions
• Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
• Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
• Tested execution of all binary files (usually in ./result/bin/)
• Determined the impact on package closure size (by running nix path-info -S before and after)
• Assured whether relevant documentation is up to date
• Fits CONTRIBUTING.md.

---

[FRidh]

...but should it not be possible?

[Ma27]

I'd say it shouldn't. If I understand the code right it never worked properly and merging a file with a directory feels wrong.

if you want to have a store path which contains the file from writeText, you may want to use writeTextDir instead.

[Ma27]

are there any further concerns? :)

[vcunat]

BTW, does anyone know if the buildenv used by nix-env is every synced with this one?

[vcunat]

I see

builder for '/nix/store/q414ay3n4g7302dhi4hkpry2b7d6hmyf-libxml2+py-2.9.9.drv' failed with exit code 2; last 1 log lines:
  Path /nix/store/vdpzi7q32nkkr6bmka7cwhlr5p26pga1-libxml2-2.9.9-dev/bin/xml2-config is a file and can't be merged into an environment using pkgs.buildEnv! at /nix/store/hafzskpwv85wsz1f8c2ps6n5a87khac7-builder.pl line 88.
builder for '/nix/store/v7xm7yirw3i766d7d112shcsfxcbi5q3-python3-3.7.2-env.drv' failed with exit code 2; last 1 log lines:
  Path /nix/store/0wfymf0b22n9vjdqds4531d76zcv7cyi-python3-3.7.2/lib/libpython3.7m.so is a file and can't be merged into an environment using pkgs.buildEnv! at /nix/store/hafzskpwv85wsz1f8c2ps6n5a87khac7-builder.pl line 88.

I'm not sure if there are more; Hydra will show in time.

[Ma27]

Hmm... I'll take a look into this...

[Ma27]

@vcunat I'm currently investigating what's happening here. I'll hope that I can report back this afternoon. If this isn't fast enough, feel free to revert 😅

[vcunat]

Today seems certainly fast enough for staging.

[Ma27]

I just had a look at the buildEnv script, it seems as findFiles can be called with a file as $target and stops before reaching the collision checks (which broke with a file as argument as described in this PR).

It seems as if a file is passed to paths and $oldTarget is empty, a store path which is a file is attempted to be placed into the env. It works now with the following patch locally:

diff --git a/pkgs/build-support/buildenv/builder.pl b/pkgs/build-support/buildenv/builder.pl
index 1f77edf86cb..98442216fcd 100755
--- a/pkgs/build-support/buildenv/builder.pl
+++ b/pkgs/build-support/buildenv/builder.pl
@@ -84,10 +84,6 @@ sub checkCollision {
 sub findFiles {
     my ($relName, $target, $baseName, $ignoreCollisions, $checkCollisionContents, $priority) = @_;
 
-    if (-f $target) {
-        die "Path $target is a file and can't be merged into an environment using pkgs.buildEnv!";
-    }
-
     # Urgh, hacky...
     return if
         $relName eq "/propagated-build-inputs" ||
@@ -107,6 +103,12 @@ sub findFiles {
         return;
     }
 
+    # If target is a file and the old target is defined and empty it means that a store
+    # path represented by $target is a file and can't be merged into the environment.
+    if (-f $target && defined $oldTarget && $oldTarget eq "") {
+        die "Path $target is a file and can't be merged into an environment using pkgs.buildEnv!";
+    }
+
     # If target already exists as a symlink to a file (not a
     # directory) in a higher-priority package, skip.
     if (defined $oldTarget && $priority > $oldPriority && $oldTarget ne "" && ! -d $oldTarget) {

If this seems fine for you @vcunat, I'd file a new patch :)