Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 1 commit into from Feb 19, 2019
Merged

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

merged 1 commit into from Feb 19, 2019

Conversation

Ma27
Copy link
Member

@Ma27 Ma27 commented Feb 7, 2019 •
edited

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.

@Ma27 Ma27 force-pushed the proper-error-with-writeText-in-buildenv branch from 45d5183 to 1161e44 Compare February 7, 2019 02:25
@Ma27 Ma27 changed the base branch from master to staging February 7, 2019 02:42
@Ma27 Ma27 changed the base branch from staging to master February 7, 2019 02:42
@Ma27
buildEnv: break with a proper error if one path is actually a file
40f8546 
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:

``` nix
{ pkgs ? import <nixpkgs> { } }:
let
  file = pkgs.writeText "test" ''
    content
  '';
in
  pkgs.buildEnv {
    name = "test-env";
    paths = [ /* ... */ file ];
  }
```
@Ma27 Ma27 force-pushed the proper-error-with-writeText-in-buildenv branch from 1161e44 to 40f8546 Compare February 7, 2019 02:44
@Ma27 Ma27 changed the base branch from master to staging February 7, 2019 02:45
FRidh
FRidh reviewed Feb 7, 2019
View reviewed changes
pkgs/build-support/buildenv/builder.pl
@@ -84,6 +84,10 @@ 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!";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...but should it not be possible?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

Ma27 commented Feb 18, 2019

are there any further concerns? :)

@FRidh FRidh merged commit 1cab56e into NixOS:staging Feb 19, 2019
@vcunat
Copy link
Member

vcunat commented Feb 19, 2019

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

@vcunat
Copy link
Member

vcunat commented Feb 19, 2019

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 Ma27 deleted the proper-error-with-writeText-in-buildenv branch February 19, 2019 08:57
@Ma27
Copy link
Member Author

Ma27 commented Feb 19, 2019

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

@Ma27
Copy link
Member Author

Ma27 commented Feb 19, 2019

@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
Copy link
Member

vcunat commented Feb 19, 2019

Today seems certainly fast enough for staging.

@Ma27
Copy link
Member Author

Ma27 commented Feb 19, 2019

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 :)

Ma27 added a commit to Ma27/nixpkgs that referenced this pull request Feb 20, 2019
@Ma27
buildEnv: improve file check to avoid false-positives
5491592 
The original change in NixOS#55372 was supposed to fix the case where a store
path which is a file should be placed into `buildEnv` which broke with a
fairly misleading Perl error.

Unfortunately this introduced a regression, `findFiles` can have targets
that are files if the file isn't a store path. Rather than adding more
obscure checks with probably further regressions, I figured that it's
better to replicate the behavior of `lib.isStorePath` and explicitly
check if the store path is a file and break in this case only.

This should also fix recent staging issues.
@Ma27 Ma27 mentioned this pull request Feb 20, 2019
Merged
10 tasks
FRidh pushed a commit that referenced this pull request Feb 21, 2019
@Ma27 @FRidh
buildEnv: improve file check to avoid false-positives
9fcd3bf 
The original change in #55372 was supposed to fix the case where a store
path which is a file should be placed into `buildEnv` which broke with a
fairly misleading Perl error.

Unfortunately this introduced a regression, `findFiles` can have targets
that are files if the file isn't a store path. Rather than adding more
obscure checks with probably further regressions, I figured that it's
better to replicate the behavior of `lib.isStorePath` and explicitly
check if the store path is a file and break in this case only.

This should also fix recent staging issues.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FRidh FRidh FRidh left review comments

@matthewbauer matthewbauer matthewbauer approved these changes

Assignees
No one assigned
Labels
10.rebuild-darwin: 501+ 10.rebuild-darwin: 1001-2500 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@Ma27 @vcunat @matthewbauer @FRidh @GrahamcOfBorg