[PATCH] daemon: Check for HAVE_CHROOT instead of CHROOT_ENABLED.

[PATCH] daemon: Check for HAVE_CHROOT instead of CHROOT_ENABLED.

[Manolis Ragkousis]

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

Hello everyone,

There is an issue with the chrooted builds on Hurd. In
nix/libstore/build.cc CHROOT_ENABLED
is false because HAVE_UNSHARE && HAVE_SYS_MOUNT_H are not defined on Hurd.

The part that we are interested from build.cc is at line 1768. The
part of the code in that if, is used by the daemon for chrooted
builds. At this specific part of the daemon we only need
 HAVE_CHROOT to be defined, which is true on hurd, for the builds to
work. With this workaround,
things seem to work for me until now. WDYT?

I am sending this patch in order to discuss what could be done and see
if anyone has any better ideas.

Manolis

[-- Attachment #2: 0001-daemon-Check-for-HAVE_CHROOT-instead-of-CHROOT_ENABL.patch --]
[-- Type: text/x-patch, Size: 1084 bytes --]

From 9fd91c191d47d77f4abeee41c6846462badb5762 Mon Sep 17 00:00:00 2001
From: Manolis Ragkousis <manolis837@gmail.com>
Date: Mon, 10 Aug 2015 20:56:03 +0300
Subject: [PATCH] daemon: Check for HAVE_CHROOT instead of CHROOT_ENABLED.

* nix/libstore/build.cc (DerivationGoal::startBuilder): Check only if HAVE_CHROOT is
  defined in order to enable chrooted builds on Hurd.
---
 nix/libstore/build.cc | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc
index a9eedce..db85df3 100644
--- a/nix/libstore/build.cc
+++ b/nix/libstore/build.cc
@@ -1765,7 +1765,8 @@ void DerivationGoal::startBuilder()
     useChroot = settings.useChroot;
 
     if (useChroot) {
-#if CHROOT_ENABLED
+#if HAVE_CHROOT /* We only need chroot at this time and with this chroot builds are 
+                   enabled on Hurd*/
         /* Create a temporary directory in which we set up the chroot
            environment using bind-mounts.  We put it in the Nix store
            to ensure that we can create hard-links to non-directory
-- 
2.5.0

Re: [PATCH] daemon: Check for HAVE_CHROOT instead of CHROOT_ENABLED.

[Mark H Weaver]

Manolis Ragkousis <manolis837@gmail.com> writes:

> There is an issue with the chrooted builds on Hurd. In
> nix/libstore/build.cc CHROOT_ENABLED
> is false because HAVE_UNSHARE && HAVE_SYS_MOUNT_H are not defined on Hurd.
>
> The part that we are interested from build.cc is at line 1768. The
> part of the code in that if, is used by the daemon for chrooted
> builds. At this specific part of the daemon we only need
>  HAVE_CHROOT to be defined, which is true on hurd, for the builds to
> work. With this workaround,
> things seem to work for me until now. WDYT?

This won't be sufficient, because if you set up a chroot, you'll also
need to do some of the things within the "#if CHROOT_ENABLED" starting
on line 1997, such as setting up local instances of /dev and /etc.

Here's what I'd suggest: instead of using HAVE_CHROOT on line 1768,
instead split CHROOT_ENABLED into two separate flags: CHROOT_ENABLED and
maybe CONTAINER_ENABLED.  All existing occurrences of CHROOT_ENABLED
would initially be replaced with CONTAINER_ENABLED.  Then, define
CHROOT_ENABLED (now a newly defined name) to depend on a smaller set of
requirements, maybe just HAVE_CHROOT && HAVE_SYS_MOUNT_H.

We should assume that CHROOT_ENABLED is a requirement for
CONTAINER_ENABLED (which should ideally be made explicit in the code by
adding CHROOT_ENABLED to the right-hand-side expression for
CONTAINER_ENABLED), so there are only three cases to consider:

1. CONTAINER_ENABLED and CHROOT_ENABLED are both true.
2. CONTAINER_ENABLED is false and CHROOT_ENABLED is true.
3. CONTAINER_ENABLED and CHROOT_ENABLED are both false.

We should ensure that the code works properly in all three cases.

So, look at the code within "#if CONTAINER_ENABLED" and decide which
parts should still be run if CONTAINER_ENABLED is false but
CHROOT_ENABLED is true.

What do you think?

    Mark

Re: [PATCH] daemon: Check for HAVE_CHROOT instead of CHROOT_ENABLED.

[Ludovic Courtès]

Mark H Weaver <mhw@netris.org> skribis:

> Manolis Ragkousis <manolis837@gmail.com> writes:
>
>> There is an issue with the chrooted builds on Hurd. In
>> nix/libstore/build.cc CHROOT_ENABLED
>> is false because HAVE_UNSHARE && HAVE_SYS_MOUNT_H are not defined on Hurd.
>>
>> The part that we are interested from build.cc is at line 1768. The
>> part of the code in that if, is used by the daemon for chrooted
>> builds. At this specific part of the daemon we only need
>>  HAVE_CHROOT to be defined, which is true on hurd, for the builds to
>> work. With this workaround,
>> things seem to work for me until now. WDYT?
>
> This won't be sufficient, because if you set up a chroot, you'll also
> need to do some of the things within the "#if CHROOT_ENABLED" starting
> on line 1997, such as setting up local instances of /dev and /etc.
>
> Here's what I'd suggest: instead of using HAVE_CHROOT on line 1768,
> instead split CHROOT_ENABLED into two separate flags: CHROOT_ENABLED and
> maybe CONTAINER_ENABLED.  All existing occurrences of CHROOT_ENABLED
> would initially be replaced with CONTAINER_ENABLED.  Then, define
> CHROOT_ENABLED (now a newly defined name) to depend on a smaller set of
> requirements, maybe just HAVE_CHROOT && HAVE_SYS_MOUNT_H.

That sounds good.

But note that the Hurd’s libc should really provide <sys/mount.h>,
‘mount’ (including MS_BIND), and ‘umount’, as discussed earlier.  What’s
the status of this?

‘unshare’ is actually no longer used, so we can patch the daemon (in
upstream Nix) to get rid of HAVE_UNSHARE.

HTH,
Ludo’.

Re: [PATCH] daemon: Check for HAVE_CHROOT instead of CHROOT_ENABLED.

[Manolis Ragkousis]

On 18 August 2015 at 19:48, Ludovic Courtès <ludo@gnu.org> wrote:
> But note that the Hurd’s libc should really provide <sys/mount.h>,
> ‘mount’ (including MS_BIND), and ‘umount’, as discussed earlier.  What’s
> the status of this?

Half-way there. I am currently testing it in darnassus, because my
local vms are busy with the port.

> ‘unshare’ is actually no longer used, so we can patch the daemon (in
> upstream Nix) to get rid of HAVE_UNSHARE.

Will do. Is nix development happening though github?

Manolis

Re: [PATCH] daemon: Check for HAVE_CHROOT instead of CHROOT_ENABLED.

[Manolis Ragkousis]

On 18 August 2015 at 19:48, Ludovic Courtès <ludo@gnu.org> wrote:
>> ‘unshare’ is actually no longer used, so we can patch the daemon (in
>> upstream Nix) to get rid of HAVE_UNSHARE.

Done :-)

Manolis

Re: [PATCH] daemon: Check for HAVE_CHROOT instead of CHROOT_ENABLED.

[Ludovic Courtès]

Manolis Ragkousis <manolis837@gmail.com> skribis:

> On 18 August 2015 at 19:48, Ludovic Courtès <ludo@gnu.org> wrote:
>> But note that the Hurd’s libc should really provide <sys/mount.h>,
>> ‘mount’ (including MS_BIND), and ‘umount’, as discussed earlier.  What’s
>> the status of this?
>
> Half-way there. I am currently testing it in darnassus, because my
> local vms are busy with the port.

OK.

> Will do. Is nix development happening though github?

That and the nix-dev mailing list.

>>> ‘unshare’ is actually no longer used, so we can patch the daemon (in
>>> upstream Nix) to get rid of HAVE_UNSHARE.
>
> Done :-)

Excellent.  I’ll update the local copy in guix.git eventually.

Thanks,
Ludo’.