[PATCH] gnu: Mark /gnu/store as needed for boot.

[PATCH] gnu: Mark /gnu/store as needed for boot.

[John Darrington]

* gnu/system/file-systems.scm (all-subpaths): New procedure.
(file-system-needed-for-boot?): Use it to check for ancestors
of %store-directory.
---
 gnu/system/file-systems.scm | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/gnu/system/file-systems.scm b/gnu/system/file-systems.scm
index 4cc1221..d42f271 100644
--- a/gnu/system/file-systems.scm
+++ b/gnu/system/file-systems.scm
@@ -20,6 +20,7 @@
   #:use-module (ice-9 match)
   #:use-module (guix records)
   #:use-module (guix store)
+  #:use-module (guix build utils)
   #:use-module ((gnu build file-systems)
                 #:select (string->uuid uuid->string))
   #:re-export (string->uuid
@@ -95,11 +96,29 @@
   (dependencies     file-system-dependencies      ; list of <file-system>
                     (default '())))               ; or <mapped-device>
 
+
+(define (all-subpaths path)
+  "Given a directory PATH return a list of all paths which
+are ancestors of this path, including PATH itself"
+ (let loop ((path (string-split path #\/))
+	    (ac '()))
+   (if (null? path)
+       ac
+       (loop (cdr path)
+	     (cons
+	      (string-append
+	       (match ac
+		 (()  "/")
+		 ((x _ . _) (string-append x "/"))
+		 ((x . _) x))
+	       (car path))
+	      ac)))))
+
 (define-inlinable (file-system-needed-for-boot? fs)
-  "Return true if FS has the 'needed-for-boot?' flag set, or if it's the root
-file system."
+  "Return true if FS has the 'needed-for-boot?' flag set, or if it holds
+the store directory."
   (or (%file-system-needed-for-boot? fs)
-      (string=? "/" (file-system-mount-point fs))))
+      (member (file-system-mount-point fs) (all-subpaths (%store-directory)))))
 
 (define (file-system->spec fs)
   "Return a list corresponding to file-system FS that can be passed to the
-- 
2.1.4

Re: [PATCH] gnu: Mark /gnu/store as needed for boot.

[Chris Marusich]

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

John Darrington <jmd@gnu.org> writes:

> * gnu/system/file-systems.scm (all-subpaths): New procedure.
> (file-system-needed-for-boot?): Use it to check for ancestors
> of %store-directory.
> ---
>  gnu/system/file-systems.scm | 25 ++++++++++++++++++++++---
>  1 file changed, 22 insertions(+), 3 deletions(-)
>
> diff --git a/gnu/system/file-systems.scm b/gnu/system/file-systems.scm
> index 4cc1221..d42f271 100644
> --- a/gnu/system/file-systems.scm
> +++ b/gnu/system/file-systems.scm
> @@ -20,6 +20,7 @@
>    #:use-module (ice-9 match)
>    #:use-module (guix records)
>    #:use-module (guix store)
> +  #:use-module (guix build utils)
>    #:use-module ((gnu build file-systems)
>                  #:select (string->uuid uuid->string))
>    #:re-export (string->uuid
> @@ -95,11 +96,29 @@
>    (dependencies     file-system-dependencies      ; list of <file-system>
>                      (default '())))               ; or <mapped-device>
>  
> +
> +(define (all-subpaths path)
> +  "Given a directory PATH return a list of all paths which
> +are ancestors of this path, including PATH itself"
> + (let loop ((path (string-split path #\/))
> +	    (ac '()))
> +   (if (null? path)
> +       ac
> +       (loop (cdr path)
> +	     (cons
> +	      (string-append
> +	       (match ac
> +		 (()  "/")
> +		 ((x _ . _) (string-append x "/"))
> +		 ((x . _) x))
> +	       (car path))
> +	      ac)))))
> +

Is this procedure supposed to work with relative paths?  I get strange
results for "foo/bar":

--8<---------------cut here---------------start------------->8---
scheme@(guile-user)> (all-subpaths "foo/bar")
$4 = ("/foobar" "/foo")
scheme@(guile-user)>
--8<---------------cut here---------------end--------------->8---


>  (define-inlinable (file-system-needed-for-boot? fs)
> -  "Return true if FS has the 'needed-for-boot?' flag set, or if it's the root
> -file system."
> +  "Return true if FS has the 'needed-for-boot?' flag set, or if it holds
> +the store directory."
>    (or (%file-system-needed-for-boot? fs)
> -      (string=? "/" (file-system-mount-point fs))))
> +      (member (file-system-mount-point fs) (all-subpaths (%store-directory)))))

It seems to me like we could just use 'string-prefix?' here instead.  Am
I missing something?  Is it really necessary to define an 'all-subpaths'
procedure and use 'member' if 'string-prefix?' is good enough?

The title of this patch is "Mark /gnu/store as needed for boot."  I can
see how this change makes that happen.  But what is the purpose?  I
imagine that in some cases, if you don't do this, the initrd might not
mount the file system containing the store, which of course would not be
good.  Do you have a specific problem case in mind?

-- 
Chris

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

Re: [PATCH] gnu: Mark /gnu/store as needed for boot.

[John Darrington]

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

On Fri, Jan 13, 2017 at 12:59:20AM -0800, Chris Marusich wrote:

     Is this procedure supposed to work with relative paths?  I get strange
     results for "foo/bar":

No.  It was not indended to be passed a relative path.  I cannot see what meaning
that would have - it raises the question "relative to what?".  However if somehow
we can define what it means to pass the procedure a relative path, then I suppose
it could be modified to work.
     
     It seems to me like we could just use 'string-prefix?' here instead.  Am
     I missing something?  Is it really necessary to define an 'all-subpaths'
     procedure and use 'member' if 'string-prefix?' is good enough?

Well, (string-prefix? "/dev/net" "/dev/networking") would return #t, but
the former is not an ancestor of the other.
     
     The title of this patch is "Mark /gnu/store as needed for boot."  I can
     see how this change makes that happen.  But what is the purpose?  I
     imagine that in some cases, if you don't do this, the initrd might not
     mount the file system containing the store, which of course would not be
     good.  Do you have a specific problem case in mind?

Yes.

I tried installing GuixSD with /gnu/store on a different partition to /
The installation seemed to go ok, but it failed to boot, presumably because 
lots of essential things (such as the kernel!) are in /gnu/store

J'


-- 
Avoid eavesdropping.  Send strong encrypted email.
PGP Public key ID: 1024D/2DE827B3 
fingerprint = 8797 A26D 0854 2EAB 0285  A290 8A67 719C 2DE8 27B3
See http://sks-keyservers.net or any PGP keyserver for public key.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] gnu: Mark /gnu/store as needed for boot.

[Ludovic Courtès]

Hi!

John Darrington <jmd@gnu.org> skribis:

> * gnu/system/file-systems.scm (all-subpaths): New procedure.
> (file-system-needed-for-boot?): Use it to check for ancestors
> of %store-directory.

I guess the idea is to have ‘needed-for-boot?’ automatically set for
users who store /gnu or /gnu/store on a separate partition, right?

In ‘%separate-store-os’ in (gnu tests install), it is set explicitly:

    (file-systems (cons* (file-system
                           (device "root-fs")
                           (title 'label)
                           (mount-point "/")
                           (type "ext4"))
                         (file-system
                           (device "store-fs")
                           (title 'label)
                           (mount-point "/gnu")
                           (type "ext4")
                           (needed-for-boot? #t)) ;definitely!
                         %base-file-systems))

Making it automatic can help though.

>  (define-inlinable (file-system-needed-for-boot? fs)
> -  "Return true if FS has the 'needed-for-boot?' flag set, or if it's the root
> -file system."
> +  "Return true if FS has the 'needed-for-boot?' flag set, or if it holds
> +the store directory."
>    (or (%file-system-needed-for-boot? fs)
> -      (string=? "/" (file-system-mount-point fs))))
> +      (member (file-system-mount-point fs) (all-subpaths (%store-directory)))))

The problem is that we need to exclude bind mounts, as done in
‘store-file-system’ in (gnu system).

What about:

  (define (file-system-needed-for-boot? fs)
    (or (%file-system-needed-for-boot? fs)
        (and (string-prefix? (file-system-mount-point fs) (%store-directory))
             (not (memq 'bind-mount (file-system-flags fs))))))

?

Thanks,
Ludo’.

Re: [PATCH] gnu: Mark /gnu/store as needed for boot.

[John Darrington]

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

On Sat, Jan 14, 2017 at 10:30:43PM +0100, Ludovic Court??s wrote:
     
     > * gnu/system/file-systems.scm (all-subpaths): New procedure.
     > (file-system-needed-for-boot?): Use it to check for ancestors
     > of %store-directory.
     
     I guess the idea is to have ???needed-for-boot???? automatically set for
     users who store /gnu or /gnu/store on a separate partition, right?

Correct.
     
     The problem is that we need to exclude bind mounts, as done in
     ???store-file-system??? in (gnu system).

Thanks for pointing that out.
     
     What about:
     
       (define (file-system-needed-for-boot? fs)
         (or (%file-system-needed-for-boot? fs)
             (and (string-prefix? (file-system-mount-point fs) (%store-directory))
                  (not (memq 'bind-mount (file-system-flags fs))))))
     

Perhaps I am misunderstanding something, but

     (string-prefix? (file-system-mount-point fs) (%store-directory))
     
will erroneously return #t when (file-system-mount-point fs) evaluates
to "/gn"  and (%store-directory) to "/gnu/store".  Will it not???

That is why I wrote a procedure to fix that problem.

J'

-- 
Avoid eavesdropping.  Send strong encrypted email.
PGP Public key ID: 1024D/2DE827B3 
fingerprint = 8797 A26D 0854 2EAB 0285  A290 8A67 719C 2DE8 27B3
See http://sks-keyservers.net or any PGP keyserver for public key.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] gnu: Mark /gnu/store as needed for boot.

[David Craven]

> will erroneously return #t when (file-system-mount-point fs) evaluates
> to "/gn"  and (%store-directory) to "/gnu/store".  Will it not???

The trick is to revert the arguments:

(string-prefix? (%store-directory) (file-system-mount-point fs))

Re: [PATCH] gnu: Mark /gnu/store as needed for boot.

[John Darrington]

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

On Sun, Jan 15, 2017 at 10:53:02AM +0100, David Craven wrote:
     > will erroneously return #t when (file-system-mount-point fs) evaluates
     > to "/gn"  and (%store-directory) to "/gnu/store".  Will it not???
     
     The trick is to revert the arguments:
     
     (string-prefix? (%store-directory) (file-system-mount-point fs))

No that will also be wrong.

(string-prefix? "/gnu/store" "/gnu") => #f

I think the procedure  I wrote does it correctly.

J'

-- 
Avoid eavesdropping.  Send strong encrypted email.
PGP Public key ID: 1024D/2DE827B3 
fingerprint = 8797 A26D 0854 2EAB 0285  A290 8A67 719C 2DE8 27B3
See http://sks-keyservers.net or any PGP keyserver for public key.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] gnu: Mark /gnu/store as needed for boot.

[Ludovic Courtès]

John Darrington <john@darrington.wattle.id.au> skribis:

> On Sat, Jan 14, 2017 at 10:30:43PM +0100, Ludovic Court??s wrote:
>      
>      > * gnu/system/file-systems.scm (all-subpaths): New procedure.
>      > (file-system-needed-for-boot?): Use it to check for ancestors
>      > of %store-directory.
>      
>      I guess the idea is to have ???needed-for-boot???? automatically set for
>      users who store /gnu or /gnu/store on a separate partition, right?
>
> Correct.
>      
>      The problem is that we need to exclude bind mounts, as done in
>      ???store-file-system??? in (gnu system).
>
> Thanks for pointing that out.
>      
>      What about:
>      
>        (define (file-system-needed-for-boot? fs)
>          (or (%file-system-needed-for-boot? fs)
>              (and (string-prefix? (file-system-mount-point fs) (%store-directory))
>                   (not (memq 'bind-mount (file-system-flags fs))))))
>      
>
> Perhaps I am misunderstanding something, but
>
>      (string-prefix? (file-system-mount-point fs) (%store-directory))
>      
> will erroneously return #t when (file-system-mount-point fs) evaluates
> to "/gn"  and (%store-directory) to "/gnu/store".  Will it not???
>
> That is why I wrote a procedure to fix that problem.

Good point.

Then maybe this:

  (define (file-system-needed-for-boot? fs)
    (or (%file-system-needed-for-boot? fs)
        (and (file-prefix? (file-system-needed-for-boot? fs)
                           (%store-directory))
             (not (memq 'bind-mount (file-system-flags fs))))))

with:

--8<---------------cut here---------------start------------->8---
scheme@(guile-user)>   (define (file-prefix? file1 file2)
			 (define not-slash
			   (char-set-complement (char-set #\/)))
			 (and (string-prefix? "/" file1)
			      (let loop ((file1 (string-tokenize file1 not-slash))
					 (file2 (string-tokenize file2 not-slash)))
				(match file1
				  (()
				   #t)
				  ((head1 tail1 ...)
				   (match file2
				     ((head2 tail2 ...)
				      (and (string=? head1 head2)
					   (loop tail1 tail2)))
				     (()
				      #f)))))))
scheme@(guile-user)> (file-prefix? "/gn" "/gnu/store")
$13 = #f
scheme@(guile-user)> (file-prefix? "/gnu/store/foo" "/gnu/store")
$14 = #f
scheme@(guile-user)> (file-prefix? "/gnu/store" "/gnu/store")
$15 = #t
scheme@(guile-user)> (file-prefix? "/gnu" "/gnu/store")
$16 = #t
scheme@(guile-user)> (file-prefix? "/" "/gnu/store")
$17 = #t
--8<---------------cut here---------------end--------------->8---

This seems more natural to me than computing the set of prefixes like
‘all-subpaths’ does.

WDYT?

If that’s fine with you I can commit this.

Thanks!

Ludo’.

Re: [PATCH] gnu: Mark /gnu/store as needed for boot.

[John Darrington]

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

On Sun, Jan 15, 2017 at 11:24:30PM +0100, Ludovic Court??s wrote:
     
     Then maybe this:
     
       (define (file-system-needed-for-boot? fs)
         (or (%file-system-needed-for-boot? fs)
             (and (file-prefix? (file-system-needed-for-boot? fs)
                                (%store-directory))
                  (not (memq 'bind-mount (file-system-flags fs))))))
     
     with:
     
     --8<---------------cut here---------------start------------->8---
     scheme@(guile-user)>   (define (file-prefix? file1 file2)
     			 (define not-slash
     			   (char-set-complement (char-set #\/)))
     			 (and (string-prefix? "/" file1)
     			      (let loop ((file1 (string-tokenize file1 not-slash))
     					 (file2 (string-tokenize file2 not-slash)))
     				(match file1
     				  (()
     				   #t)
     				  ((head1 tail1 ...)
     				   (match file2
     				     ((head2 tail2 ...)
     				      (and (string=? head1 head2)
     					   (loop tail1 tail2)))
     				     (()
     				      #f)))))))
     scheme@(guile-user)> (file-prefix? "/gn" "/gnu/store")
     $13 = #f
     scheme@(guile-user)> (file-prefix? "/gnu/store/foo" "/gnu/store")
     $14 = #f
     scheme@(guile-user)> (file-prefix? "/gnu/store" "/gnu/store")
     $15 = #t
     scheme@(guile-user)> (file-prefix? "/gnu" "/gnu/store")
     $16 = #t
     scheme@(guile-user)> (file-prefix? "/" "/gnu/store")
     $17 = #t
     --8<---------------cut here---------------end--------------->8---
     
     This seems more natural to me than computing the set of prefixes like
     ???all-subpaths??? does.
     
     WDYT?
     
     If that???s fine with you I can commit this.
     

It looks fine to me.  I haven't tested it ....


J'

-- 
Avoid eavesdropping.  Send strong encrypted email.
PGP Public key ID: 1024D/2DE827B3 
fingerprint = 8797 A26D 0854 2EAB 0285  A290 8A67 719C 2DE8 27B3
See http://sks-keyservers.net or any PGP keyserver for public key.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] gnu: Mark /gnu/store as needed for boot.

[Ludovic Courtès]

John Darrington <john@darrington.wattle.id.au> skribis:

> On Sun, Jan 15, 2017 at 11:24:30PM +0100, Ludovic Court??s wrote:
>      
>      Then maybe this:
>      
>        (define (file-system-needed-for-boot? fs)
>          (or (%file-system-needed-for-boot? fs)
>              (and (file-prefix? (file-system-needed-for-boot? fs)
>                                 (%store-directory))
>                   (not (memq 'bind-mount (file-system-flags fs))))))
>      
>      with:
>      
>      --8<---------------cut here---------------start------------->8---
>      scheme@(guile-user)>   (define (file-prefix? file1 file2)
>      			 (define not-slash
>      			   (char-set-complement (char-set #\/)))
>      			 (and (string-prefix? "/" file1)
>      			      (let loop ((file1 (string-tokenize file1 not-slash))
>      					 (file2 (string-tokenize file2 not-slash)))
>      				(match file1
>      				  (()
>      				   #t)
>      				  ((head1 tail1 ...)
>      				   (match file2
>      				     ((head2 tail2 ...)
>      				      (and (string=? head1 head2)
>      					   (loop tail1 tail2)))
>      				     (()
>      				      #f)))))))
>      scheme@(guile-user)> (file-prefix? "/gn" "/gnu/store")
>      $13 = #f
>      scheme@(guile-user)> (file-prefix? "/gnu/store/foo" "/gnu/store")
>      $14 = #f
>      scheme@(guile-user)> (file-prefix? "/gnu/store" "/gnu/store")
>      $15 = #t
>      scheme@(guile-user)> (file-prefix? "/gnu" "/gnu/store")
>      $16 = #t
>      scheme@(guile-user)> (file-prefix? "/" "/gnu/store")
>      $17 = #t
>      --8<---------------cut here---------------end--------------->8---
>      
>      This seems more natural to me than computing the set of prefixes like
>      ???all-subpaths??? does.
>      
>      WDYT?
>      
>      If that???s fine with you I can commit this.
>      
>
> It looks fine to me.  I haven't tested it ....

Pushed with tests as 384344198dcaa97847e66d3dd82f279ede08d690.

Thanks!

Ludo’.

ABI break!

[Ludovic Courtès]

ludo@gnu.org (Ludovic Courtès) skribis:

> Pushed with tests as 384344198dcaa97847e66d3dd82f279ede08d690.

And if you get a weird error when running ‘make’ about
‘file-system-needed-for-boot?’, just run:

  rm gnu/system.go && make

This is because of an ABI break: ‘file-system-needed-for-boot?’ was a
macro and now is a procedure.

Ludo’.

Re: ABI break!

[John Darrington]

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

On Tue, Jan 17, 2017 at 11:52:35PM +0100, Ludovic Court??s wrote:
     ludo@gnu.org (Ludovic Court??s) skribis:
     
     > Pushed with tests as 384344198dcaa97847e66d3dd82f279ede08d690.
     
     And if you get a weird error when running ???make??? about
     ???file-system-needed-for-boot????, just run:
     
       rm gnu/system.go && make
     
     This is because of an ABI break: ???file-system-needed-for-boot???? was a
     macro and now is a procedure.
     
Surely this means that the Makefile is wrong?

It should recognise that gnu/system.scm has changed and rebuild gnu/system.go accordingly.

J'



-- 
Avoid eavesdropping.  Send strong encrypted email.
PGP Public key ID: 1024D/2DE827B3 
fingerprint = 8797 A26D 0854 2EAB 0285  A290 8A67 719C 2DE8 27B3
See http://sks-keyservers.net or any PGP keyserver for public key.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: ABI break!

[David Craven]

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

> Surely this means that the Makefile is wrong?

I think that in projects that use Makefiles it's the expectation that
incremental compilation only works in "most" cases.

I really like ninja - I think it is un-opinionated and has solid
fundamentals. I think that a cool project would be to write a ninja build
file generator in guile.

[-- Attachment #2: Type: text/html, Size: 427 bytes --]

Re: ABI break!

[Ludovic Courtès]

John Darrington <john@darrington.wattle.id.au> skribis:

> On Tue, Jan 17, 2017 at 11:52:35PM +0100, Ludovic Court??s wrote:
>      ludo@gnu.org (Ludovic Court??s) skribis:
>      
>      > Pushed with tests as 384344198dcaa97847e66d3dd82f279ede08d690.
>      
>      And if you get a weird error when running ???make??? about
>      ???file-system-needed-for-boot????, just run:
>      
>        rm gnu/system.go && make
>      
>      This is because of an ABI break: ???file-system-needed-for-boot???? was a
>      macro and now is a procedure.
>      
> Surely this means that the Makefile is wrong?
>
> It should recognise that gnu/system.scm has changed and rebuild gnu/system.go accordingly.

gnu/system.scm has not changed; gnu/system/file-system.scm did.

Guile modules are composed at run time for the most part, so when you
change one, you don’t have to rebuild those that use it.

However, you do have to recompile them if they rely on it for macros and
those macros have changed or are no longer macros.

This is what happened here.

Ludo’.