bug#25242: Cannot build source derivations with a custom TMPDIR

bug#25242: Cannot build source derivations with a custom TMPDIR

[Leo Famulari]

[-- Attachment #1: Type: text/plain, Size: 1809 bytes --]

Since Guix 0.11.0-4.1f41, I can't build source derivations while using a
custom $TMPDIR.

I first described this issue in <http://bugs.gnu.org/25089>, but I'm
splitting it into its own report.

On my Debian system, I set 'TMPDIR=/home/leo/tmp/guix-build'.

Then, I can do this:

$ ./pre-inst-env guix build nmap
guix build: error: build failed: while setting up the build environment: changing into `/home/leo/tmp/guix-build/guix-build-nmap-7.40.tar.bz2.drv-0': No such file or directory

And some monitoring of the TMPDIR:

$ cd /home/leo/tmp/guix-build && inotifywait --monitor .
Setting up watches.
Watches established.
./ CREATE,ISDIR guix-build-nmap-7.40.tar.bz2.drv-0
./ ATTRIB,ISDIR guix-build-nmap-7.40.tar.bz2.drv-0
./ ATTRIB,ISDIR guix-build-nmap-7.40.tar.bz2.drv-0
./ OPEN,ISDIR guix-build-nmap-7.40.tar.bz2.drv-0
./ ACCESS,ISDIR guix-build-nmap-7.40.tar.bz2.drv-0
./ ACCESS,ISDIR guix-build-nmap-7.40.tar.bz2.drv-0
./ CLOSE_NOWRITE,CLOSE,ISDIR guix-build-nmap-7.40.tar.bz2.drv-0
./ DELETE,ISDIR guix-build-nmap-7.40.tar.bz2.drv-0

I ran the guix-daemon with strace, and I see these relevant lines:

15337 [pid 30675] mkdir("/home/leo/tmp/guix-build/guix-build-nmap-7.40.tar.bz2.drv-0", 0700) = 0
15338 [pid 30675] getegid()                   = 0
15339 [pid 30675] chown("/home/leo/tmp/guix-build/guix-build-nmap-7.40.tar.bz2.drv-0", -1, 0) = 0

... and just a bit later ...

15438 [pid 30693] chdir("/tmp/guix-build-nmap-7.40.tar.bz2.drv-0") = -1 ENOENT (No such file or directory)

I'm still trying to figure out where the problem is in the code. I guess
it's somewhere in these commits or code they interact with:

05ceb8dca download: Use the built-in 'download' builder when available.
f9aefa2d5 daemon: Add 'built-in-builders' RPC.
94d92c779 daemon: Add "builtin:download" derivation builder.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

bug#25242: Cannot build source derivations with a custom TMPDIR

[Leo Famulari]

[-- Attachment #1: Type: text/plain, Size: 1083 bytes --]

On Wed, Dec 21, 2016 at 03:22:40AM -0500, Leo Famulari wrote:
> I ran the guix-daemon with strace, and I see these relevant lines:
> 
> 15337 [pid 30675] mkdir("/home/leo/tmp/guix-build/guix-build-nmap-7.40.tar.bz2.drv-0", 0700) = 0
> 15338 [pid 30675] getegid()                   = 0
> 15339 [pid 30675] chown("/home/leo/tmp/guix-build/guix-build-nmap-7.40.tar.bz2.drv-0", -1, 0) = 0

I believe this corresponds to the use of createTempDir() at
nix/libstore/build.cc:1718. The path of the new directory is saved in
the tmpDir variable.

> 15438 [pid 30693] chdir("/tmp/guix-build-nmap-7.40.tar.bz2.drv-0") = -1 ENOENT (No such file or directory)

And then later, at nix/libstore/build.cc:2204, we do this:

2204         if (chdir(tmpDirInSandbox.c_str()) == -1)
2205             throw SysError(format("changing into `%1%'") % tmpDir);

It tries to change to the wrong directory (the new "out of band"
downloader doesn't use a chroot, IIUC), and then prints a misleading
error message. This explains the discrepancy between what we see in
strace and on stderr.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

bug#25242: Cannot build source derivations with a custom TMPDIR

[Ludovic Courtès]

[-- Attachment #1: Type: text/plain, Size: 1397 bytes --]

Leo Famulari <leo@famulari.name> skribis:

> On Wed, Dec 21, 2016 at 03:22:40AM -0500, Leo Famulari wrote:
>> I ran the guix-daemon with strace, and I see these relevant lines:
>> 
>> 15337 [pid 30675] mkdir("/home/leo/tmp/guix-build/guix-build-nmap-7.40.tar.bz2.drv-0", 0700) = 0
>> 15338 [pid 30675] getegid()                   = 0
>> 15339 [pid 30675] chown("/home/leo/tmp/guix-build/guix-build-nmap-7.40.tar.bz2.drv-0", -1, 0) = 0
>
> I believe this corresponds to the use of createTempDir() at
> nix/libstore/build.cc:1718. The path of the new directory is saved in
> the tmpDir variable.
>
>> 15438 [pid 30693] chdir("/tmp/guix-build-nmap-7.40.tar.bz2.drv-0") = -1 ENOENT (No such file or directory)
>
> And then later, at nix/libstore/build.cc:2204, we do this:
>
> 2204         if (chdir(tmpDirInSandbox.c_str()) == -1)
> 2205             throw SysError(format("changing into `%1%'") % tmpDir);
>
> It tries to change to the wrong directory (the new "out of band"
> downloader doesn't use a chroot, IIUC), and then prints a misleading
> error message. This explains the discrepancy between what we see in
> strace and on stderr.

Good catch!

AFAICS the flaw is that there’s one place where I wrote:

  if (useChroot && !isBuiltin(drv))

while several other places just do something like:

  if (useChroot)

Could the patch below solve the problem?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-patch, Size: 1304 bytes --]

diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc
index e823001..38048ce 100644
--- a/nix/libstore/build.cc
+++ b/nix/libstore/build.cc
@@ -1680,7 +1680,11 @@ void DerivationGoal::startBuilder()
             % drv.platform % settings.thisSystem % drvPath);
     }
 
-    useChroot = settings.useChroot;
+    /* Note: built-in builders are *not* running in a chroot environment so
+       that we can easily implement them in Guile without having it as a
+       derivation input (they are running under a separate build user,
+       though).  */
+    useChroot = settings.useChroot && !isBuiltin(drv);
 
     /* Construct the environment passed to the builder. */
     env.clear();
@@ -2048,12 +2052,7 @@ void DerivationGoal::runChild()
         commonChildInit(builderOut);
 
 #if CHROOT_ENABLED
-	/* Note: built-in builders are *not* running in a chroot environment
-	   so that we can easily implement them in Guile without having it as
-	   a derivation input (they are running under a separate build user,
-	   though).  */
-
-        if (useChroot && !isBuiltin(drv)) {
+        if (useChroot) {
             /* Initialise the loopback interface. */
             AutoCloseFD fd(socket(PF_INET, SOCK_DGRAM, IPPROTO_IP));
             if (fd == -1) throw SysError("cannot open IP socket");

[-- Attachment #3: Type: text/plain, Size: 23 bytes --]


Thanks!

Ludo’.

bug#25242: Cannot build source derivations with a custom TMPDIR

[Leo Famulari]

On Wed, Dec 21, 2016 at 10:20:20AM +0100, Ludovic Courtès wrote:
> Good catch!
> 
> AFAICS the flaw is that there’s one place where I wrote:
> 
>   if (useChroot && !isBuiltin(drv))
> 
> while several other places just do something like:
> 
>   if (useChroot)
> 
> Could the patch below solve the problem?

I'm not sure how to test patches to the daemon. Does it require creating
a new guix-devel snapshot?

> diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc
> index e823001..38048ce 100644
> --- a/nix/libstore/build.cc
> +++ b/nix/libstore/build.cc
> @@ -1680,7 +1680,11 @@ void DerivationGoal::startBuilder()
>              % drv.platform % settings.thisSystem % drvPath);
>      }
>  
> -    useChroot = settings.useChroot;
> +    /* Note: built-in builders are *not* running in a chroot environment so
> +       that we can easily implement them in Guile without having it as a
> +       derivation input (they are running under a separate build user,
> +       though).  */
> +    useChroot = settings.useChroot && !isBuiltin(drv);
>  
>      /* Construct the environment passed to the builder. */
>      env.clear();
> @@ -2048,12 +2052,7 @@ void DerivationGoal::runChild()
>          commonChildInit(builderOut);
>  
>  #if CHROOT_ENABLED
> -	/* Note: built-in builders are *not* running in a chroot environment
> -	   so that we can easily implement them in Guile without having it as
> -	   a derivation input (they are running under a separate build user,
> -	   though).  */
> -
> -        if (useChroot && !isBuiltin(drv)) {
> +        if (useChroot) {
>              /* Initialise the loopback interface. */
>              AutoCloseFD fd(socket(PF_INET, SOCK_DGRAM, IPPROTO_IP));
>              if (fd == -1) throw SysError("cannot open IP socket");

> 
> Thanks!
> 
> Ludo’.

bug#25242: Cannot build source derivations with a custom TMPDIR

[Ludovic Courtès]

Leo Famulari <leo@famulari.name> skribis:

> On Wed, Dec 21, 2016 at 10:20:20AM +0100, Ludovic Courtès wrote:
>> Good catch!
>> 
>> AFAICS the flaw is that there’s one place where I wrote:
>> 
>>   if (useChroot && !isBuiltin(drv))
>> 
>> while several other places just do something like:
>> 
>>   if (useChroot)
>> 
>> Could the patch below solve the problem?
>
> I'm not sure how to test patches to the daemon. Does it require creating
> a new guix-devel snapshot?

You can apply it to your source tree and run:

  sudo ./pre-inst-env guix-daemon --build-users-group=guixbuild

from there (or similar).

HTH!

Ludo’.

bug#25242: Cannot build source derivations with a custom TMPDIR

[Leo Famulari]

On Wed, Dec 21, 2016 at 10:20:20AM +0100, Ludovic Courtès wrote:
> AFAICS the flaw is that there’s one place where I wrote:
> 
>   if (useChroot && !isBuiltin(drv))
> 
> while several other places just do something like:
> 
>   if (useChroot)
> 
> Could the patch below solve the problem?
> 

> diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc
> index e823001..38048ce 100644
> --- a/nix/libstore/build.cc
> +++ b/nix/libstore/build.cc
> @@ -1680,7 +1680,11 @@ void DerivationGoal::startBuilder()
>              % drv.platform % settings.thisSystem % drvPath);
>      }
>  
> -    useChroot = settings.useChroot;
> +    /* Note: built-in builders are *not* running in a chroot environment so
> +       that we can easily implement them in Guile without having it as a
> +       derivation input (they are running under a separate build user,
> +       though).  */
> +    useChroot = settings.useChroot && !isBuiltin(drv);
>  
>      /* Construct the environment passed to the builder. */
>      env.clear();
> @@ -2048,12 +2052,7 @@ void DerivationGoal::runChild()
>          commonChildInit(builderOut);
>  
>  #if CHROOT_ENABLED
> -	/* Note: built-in builders are *not* running in a chroot environment
> -	   so that we can easily implement them in Guile without having it as
> -	   a derivation input (they are running under a separate build user,
> -	   though).  */
> -
> -        if (useChroot && !isBuiltin(drv)) {
> +        if (useChroot) {
>              /* Initialise the loopback interface. */
>              AutoCloseFD fd(socket(PF_INET, SOCK_DGRAM, IPPROTO_IP));
>              if (fd == -1) throw SysError("cannot open IP socket");

Yes, this does fix the problem!

I wonder if I should commit this while Ludo is away?

bug#25242: Cannot build source derivations with a custom TMPDIR

[Ludovic Courtès]

Leo Famulari <leo@famulari.name> skribis:

> On Wed, Dec 21, 2016 at 10:20:20AM +0100, Ludovic Courtès wrote:
>> AFAICS the flaw is that there’s one place where I wrote:
>> 
>>   if (useChroot && !isBuiltin(drv))
>> 
>> while several other places just do something like:
>> 
>>   if (useChroot)
>> 
>> Could the patch below solve the problem?
>> 
>
>> diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc
>> index e823001..38048ce 100644
>> --- a/nix/libstore/build.cc
>> +++ b/nix/libstore/build.cc
>> @@ -1680,7 +1680,11 @@ void DerivationGoal::startBuilder()
>>              % drv.platform % settings.thisSystem % drvPath);
>>      }
>>  
>> -    useChroot = settings.useChroot;
>> +    /* Note: built-in builders are *not* running in a chroot environment so
>> +       that we can easily implement them in Guile without having it as a
>> +       derivation input (they are running under a separate build user,
>> +       though).  */
>> +    useChroot = settings.useChroot && !isBuiltin(drv);
>>  
>>      /* Construct the environment passed to the builder. */
>>      env.clear();
>> @@ -2048,12 +2052,7 @@ void DerivationGoal::runChild()
>>          commonChildInit(builderOut);
>>  
>>  #if CHROOT_ENABLED
>> -	/* Note: built-in builders are *not* running in a chroot environment
>> -	   so that we can easily implement them in Guile without having it as
>> -	   a derivation input (they are running under a separate build user,
>> -	   though).  */
>> -
>> -        if (useChroot && !isBuiltin(drv)) {
>> +        if (useChroot) {
>>              /* Initialise the loopback interface. */
>>              AutoCloseFD fd(socket(PF_INET, SOCK_DGRAM, IPPROTO_IP));
>>              if (fd == -1) throw SysError("cannot open IP socket");
>
> Yes, this does fix the problem!

Awesome!

> I wonder if I should commit this while Ludo is away?

You could have done it.  :-)

I’ve just pushed it as 8ecc3c6c447765b1f7c15b980f985d1826f48659.

Thank you!

Ludo’.

bug#25242: Cannot build source derivations with a custom TMPDIR

[Leo Famulari]

On Thu, Dec 29, 2016 at 06:23:45PM +0100, Ludovic Courtès wrote:
> Leo Famulari <leo@famulari.name> skribis:
> > I wonder if I should commit this while Ludo is away?
> 
> You could have done it.  :-)
> 
> I’ve just pushed it as 8ecc3c6c447765b1f7c15b980f985d1826f48659.

Okay, I'll be more confident in this area in the future!