[PATCH] gc: Add option --keep-going.

[PATCH] gc: Add option --keep-going.

[Hartmut Goebel]

* guix/scripts/gc.scm (show-help, %options): Add option -k/--keep-going.
  (guix-gc): Pass value off option --keep-going on to delete-paths.
* guix/store.scm (%protocol-version): Increment to 17.
  (delete-paths) New key-word parameter `keep-going?`, pass it on to
  run-rc.
  (run-gc): New key-word parameter `keep-going?`, send value of
  keep-going? to the daemon.
* nix/libstore/store-api.hh (GCOptions): Add boolean keepGoing.
* nix/libstore/worker-protocol.hh (PROTOCOL_VERSION) Increment to 17.
* nix/nix-daemon/nix-daemon.cc (performOp)[wopCollectGarbage] Read
  keepGoing.
* nix/libstore/gc.cc (LocalStore::collectGarbage) If keepGoing is true
  print an error message instead of throwing an error.
* doc/guix.texi (Invoking guix gc): Document option --keep-going.
---
 doc/guix.texi                   |  8 +++++++-
 guix/scripts/gc.scm             |  9 ++++++++-
 guix/store.scm                  | 12 ++++++++----
 nix/libstore/gc.cc              |  7 ++++++-
 nix/libstore/store-api.hh       |  3 +++
 nix/libstore/worker-protocol.hh |  2 +-
 nix/nix-daemon/nix-daemon.cc    |  2 ++
 7 files changed, 35 insertions(+), 8 deletions(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index a3eba58..b8362d6 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -2098,7 +2098,13 @@ nothing and exit immediately.
 @itemx -d
 Attempt to delete all the store files and directories specified as
 arguments.  This fails if some of the files are not in the store, or if
-they are still live.
+they are still live (with behaviour can be changed with
+@option{--keep-going}).
+
+@item --keep-going
+@itemx -k
+Keep going when @option{--delete} is given and some of the store files
+and directories specified as arguments fail to re removed.
 
 @item --list-failures
 List store items corresponding to cached build failures.
diff --git a/guix/scripts/gc.scm b/guix/scripts/gc.scm
index bdfee43..778e9a7 100644
--- a/guix/scripts/gc.scm
+++ b/guix/scripts/gc.scm
@@ -72,6 +72,9 @@ Invoke the garbage collector.\n"))
       --clear-failures   remove PATHS from the set of cached failures"))
   (newline)
   (display (_ "
+  -k, --keep-going       keep going when some of th pathes can not be deleted"))
+  (newline)
+  (display (_ "
   -h, --help             display this help and exit"))
   (display (_ "
   -V, --version          display version information and exit"))
@@ -107,6 +110,9 @@ Invoke the garbage collector.\n"))
                 (lambda (opt name arg result)
                   (alist-cons 'action 'delete
                               (alist-delete 'action result))))
+        (option '(#\k "keep-going") #f #f
+                (lambda (opt name arg result)
+                  (alist-cons 'keep-going? #t result)))
         (option '("optimize") #f #f
                 (lambda (opt name arg result)
                   (alist-cons 'action 'optimize
@@ -228,7 +234,8 @@ Invoke the garbage collector.\n"))
              (let-values (((paths freed) (collect-garbage store)))
               (info (_ "freed ~h bytes~%") freed))))))
         ((delete)
-         (delete-paths store (map direct-store-path paths)))
+         (delete-paths store (map direct-store-path paths)
+                       #:keep-going? (assoc-ref opts 'keep-going?)))
         ((list-references)
          (list-relatives references))
         ((list-requisites)
diff --git a/guix/store.scm b/guix/store.scm
index 2023875..4276db4 100644
--- a/guix/store.scm
+++ b/guix/store.scm
@@ -137,7 +137,7 @@
             direct-store-path
             log-file))
 
-(define %protocol-version #x110)
+(define %protocol-version #x111)
 
 (define %worker-magic-1 #x6e697863)               ; "nixc"
 (define %worker-magic-2 #x6478696f)               ; "dxio"
@@ -938,7 +938,7 @@ is not an atomic operation.)  When CHECK-CONTENTS? is true, check the contents
 of store items; this can take a lot of time."
       (not (verify store check-contents? repair?)))))
 
-(define (run-gc server action to-delete min-freed)
+(define* (run-gc server action to-delete min-freed #:key (keep-going? #f))
   "Perform the garbage-collector operation ACTION, one of the
 `gc-action' values.  When ACTION is `delete-specific', the TO-DELETE is
 the list of store paths to delete.  IGNORE-LIVENESS? should always be
@@ -956,6 +956,8 @@ and the number of bytes freed."
       ;; Obsolete `use-atime' and `max-atime' parameters.
       (write-int 0 s)
       (write-int 0 s))
+    (when (>= (nix-server-minor-version server) 17)
+      (write-arg boolean keep-going? s))
 
     ;; Loop until the server is done sending error output.
     (let loop ((done? (process-stderr server)))
@@ -993,12 +995,14 @@ then collect at least MIN-FREED bytes.  Return the paths that were
 collected, and the number of bytes freed."
   (run-gc server (gc-action delete-dead) '() min-freed))
 
-(define* (delete-paths server paths #:optional (min-freed (%long-long-max)))
+(define* (delete-paths server paths #:optional (min-freed (%long-long-max))
+                       #:key (keep-going? #f))
   "Delete PATHS from the store at SERVER, if they are no longer
 referenced.  If MIN-FREED is non-zero, then stop after at least
 MIN-FREED bytes have been collected.  Return the paths that were
 collected, and the number of bytes freed."
-  (run-gc server (gc-action delete-specific) paths min-freed))
+  (run-gc server (gc-action delete-specific) paths min-freed
+          #:keep-going? keep-going?))
 
 (define (import-paths server port)
   "Import the set of store paths read from PORT into SERVER's store.  An error
diff --git a/nix/libstore/gc.cc b/nix/libstore/gc.cc
index 72eff52..6f2d8f7 100644
--- a/nix/libstore/gc.cc
+++ b/nix/libstore/gc.cc
@@ -662,8 +662,13 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results)
         foreach (PathSet::iterator, i, options.pathsToDelete) {
             assertStorePath(*i);
             tryToDelete(state, *i);
-            if (state.dead.find(*i) == state.dead.end())
+            if (state.dead.find(*i) == state.dead.end()) {
+              if (options.keepGoing) {
+                printMsg(lvlError, format("cannot delete path `%1%' since it is still alive") % *i);
+              } else {
                 throw Error(format("cannot delete path `%1%' since it is still alive") % *i);
+              }
+            }
         }
 
     } else if (options.maxFreed > 0) {
diff --git a/nix/libstore/store-api.hh b/nix/libstore/store-api.hh
index fa78d59..8b0f521 100644
--- a/nix/libstore/store-api.hh
+++ b/nix/libstore/store-api.hh
@@ -50,6 +50,9 @@ struct GCOptions
     /* Stop after at least `maxFreed' bytes have been freed. */
     unsigned long long maxFreed;
 
+    /* keep going even if some of the paths can not be collected */
+    bool keepGoing;
+
     GCOptions();
 };
 
diff --git a/nix/libstore/worker-protocol.hh b/nix/libstore/worker-protocol.hh
index 99c1ee2..8faf193 100644
--- a/nix/libstore/worker-protocol.hh
+++ b/nix/libstore/worker-protocol.hh
@@ -6,7 +6,7 @@ namespace nix {
 #define WORKER_MAGIC_1 0x6e697863
 #define WORKER_MAGIC_2 0x6478696f
 
-#define PROTOCOL_VERSION 0x110
+#define PROTOCOL_VERSION 0x111
 #define GET_PROTOCOL_MAJOR(x) ((x) & 0xff00)
 #define GET_PROTOCOL_MINOR(x) ((x) & 0x00ff)
 
diff --git a/nix/nix-daemon/nix-daemon.cc b/nix/nix-daemon/nix-daemon.cc
index a1fce25..b04cece 100644
--- a/nix/nix-daemon/nix-daemon.cc
+++ b/nix/nix-daemon/nix-daemon.cc
@@ -526,6 +526,8 @@ static void performOp(bool trusted, unsigned int clientVersion,
             readInt(from);
             readInt(from);
         }
+        if (GET_PROTOCOL_MINOR(clientVersion) >= 17)
+	  options.keepGoing = readInt(from) != 0;
 
         GCResults results;
 
-- 
2.7.4

Re: [PATCH] gc: Add option --keep-going.

[Eric Bavier]

Just a few comments/suggestions.

On Wed, 16 Nov 2016 15:37:14 +0100
Hartmut Goebel <h.goebel@crazy-compilers.com> wrote:

> * guix/scripts/gc.scm (show-help, %options): Add option -k/--keep-going.
>   (guix-gc): Pass value off option --keep-going on to delete-paths.
    ^
Changelog continuation lines should start in column 0.

> * guix/store.scm (%protocol-version): Increment to 17.
>   (delete-paths) New key-word parameter `keep-going?`, pass it on to
>   run-rc.
>   (run-gc): New key-word parameter `keep-going?`, send value of
>   keep-going? to the daemon.
> * nix/libstore/store-api.hh (GCOptions): Add boolean keepGoing.
> * nix/libstore/worker-protocol.hh (PROTOCOL_VERSION) Increment to 17.
> * nix/nix-daemon/nix-daemon.cc (performOp)[wopCollectGarbage] Read
>   keepGoing.
> * nix/libstore/gc.cc (LocalStore::collectGarbage) If keepGoing is true
>   print an error message instead of throwing an error.
> * doc/guix.texi (Invoking guix gc): Document option --keep-going.
> ---
>  doc/guix.texi                   |  8 +++++++-
>  guix/scripts/gc.scm             |  9 ++++++++-
>  guix/store.scm                  | 12 ++++++++----
>  nix/libstore/gc.cc              |  7 ++++++-
>  nix/libstore/store-api.hh       |  3 +++
>  nix/libstore/worker-protocol.hh |  2 +-
>  nix/nix-daemon/nix-daemon.cc    |  2 ++
>  7 files changed, 35 insertions(+), 8 deletions(-)
> 
> diff --git a/doc/guix.texi b/doc/guix.texi
> index a3eba58..b8362d6 100644
> --- a/doc/guix.texi
> +++ b/doc/guix.texi
> @@ -2098,7 +2098,13 @@ nothing and exit immediately.
>  @itemx -d
>  Attempt to delete all the store files and directories specified as
>  arguments.  This fails if some of the files are not in the store, or if
> -they are still live.
> +they are still live (with behaviour can be changed with
> +@option{--keep-going}).

How about: "Unless the @option{--keep-going} option is given this will
fail if some of the files are not in the store, or if they are still
live."

> +
> +@item --keep-going
> +@itemx -k
> +Keep going when @option{--delete} is given and some of the store files
                                                  ^
maybe "any of the store files or directories"?

> +and directories specified as arguments fail to re removed.
                                                  ^
"be"

>  
>  @item --list-failures
>  List store items corresponding to cached build failures.
> diff --git a/guix/scripts/gc.scm b/guix/scripts/gc.scm
> index bdfee43..778e9a7 100644
> --- a/guix/scripts/gc.scm
> +++ b/guix/scripts/gc.scm
> @@ -72,6 +72,9 @@ Invoke the garbage collector.\n"))
>        --clear-failures   remove PATHS from the set of cached failures"))
>    (newline)
>    (display (_ "
> +  -k, --keep-going       keep going when some of th pathes can not be deleted"))

"keep going if paths cannot be deleted"

> +  (newline)
> +  (display (_ "
>    -h, --help             display this help and exit"))
>    (display (_ "
>    -V, --version          display version information and exit"))
> @@ -107,6 +110,9 @@ Invoke the garbage collector.\n"))
>                  (lambda (opt name arg result)
>                    (alist-cons 'action 'delete
>                                (alist-delete 'action result))))
> +        (option '(#\k "keep-going") #f #f
> +                (lambda (opt name arg result)
> +                  (alist-cons 'keep-going? #t result)))
>          (option '("optimize") #f #f
>                  (lambda (opt name arg result)
>                    (alist-cons 'action 'optimize
> @@ -228,7 +234,8 @@ Invoke the garbage collector.\n"))
>               (let-values (((paths freed) (collect-garbage store)))
>                (info (_ "freed ~h bytes~%") freed))))))
>          ((delete)
> -         (delete-paths store (map direct-store-path paths)))
> +         (delete-paths store (map direct-store-path paths)
> +                       #:keep-going? (assoc-ref opts 'keep-going?)))
>          ((list-references)
>           (list-relatives references))
>          ((list-requisites)
> diff --git a/guix/store.scm b/guix/store.scm
> index 2023875..4276db4 100644
> --- a/guix/store.scm
> +++ b/guix/store.scm
> @@ -137,7 +137,7 @@
>              direct-store-path
>              log-file))
>  
> -(define %protocol-version #x110)
> +(define %protocol-version #x111)
>  
>  (define %worker-magic-1 #x6e697863)               ; "nixc"
>  (define %worker-magic-2 #x6478696f)               ; "dxio"
> @@ -938,7 +938,7 @@ is not an atomic operation.)  When CHECK-CONTENTS? is true, check the contents
>  of store items; this can take a lot of time."
>        (not (verify store check-contents? repair?)))))
>  
> -(define (run-gc server action to-delete min-freed)
> +(define* (run-gc server action to-delete min-freed #:key (keep-going? #f))
>    "Perform the garbage-collector operation ACTION, one of the
>  `gc-action' values.  When ACTION is `delete-specific', the TO-DELETE is
>  the list of store paths to delete.  IGNORE-LIVENESS? should always be
> @@ -956,6 +956,8 @@ and the number of bytes freed."
>        ;; Obsolete `use-atime' and `max-atime' parameters.
>        (write-int 0 s)
>        (write-int 0 s))
> +    (when (>= (nix-server-minor-version server) 17)
> +      (write-arg boolean keep-going? s))
>  
>      ;; Loop until the server is done sending error output.
>      (let loop ((done? (process-stderr server)))
> @@ -993,12 +995,14 @@ then collect at least MIN-FREED bytes.  Return the paths that were
>  collected, and the number of bytes freed."
>    (run-gc server (gc-action delete-dead) '() min-freed))
>  
> -(define* (delete-paths server paths #:optional (min-freed (%long-long-max)))
> +(define* (delete-paths server paths #:optional (min-freed (%long-long-max))
> +                       #:key (keep-going? #f))
>    "Delete PATHS from the store at SERVER, if they are no longer
>  referenced.  If MIN-FREED is non-zero, then stop after at least
>  MIN-FREED bytes have been collected.  Return the paths that were
>  collected, and the number of bytes freed."
> -  (run-gc server (gc-action delete-specific) paths min-freed))
> +  (run-gc server (gc-action delete-specific) paths min-freed
> +          #:keep-going? keep-going?))
>  
>  (define (import-paths server port)
>    "Import the set of store paths read from PORT into SERVER's store.  An error
> diff --git a/nix/libstore/gc.cc b/nix/libstore/gc.cc
> index 72eff52..6f2d8f7 100644
> --- a/nix/libstore/gc.cc
> +++ b/nix/libstore/gc.cc
> @@ -662,8 +662,13 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results)
>          foreach (PathSet::iterator, i, options.pathsToDelete) {
>              assertStorePath(*i);
>              tryToDelete(state, *i);
> -            if (state.dead.find(*i) == state.dead.end())
> +            if (state.dead.find(*i) == state.dead.end()) {
> +              if (options.keepGoing) {
> +                printMsg(lvlError, format("cannot delete path `%1%' since it is still alive") % *i);
                               ^
Is error formatting appropriate, or is there a warning level we can use?

> +              } else {
>                  throw Error(format("cannot delete path `%1%' since it is still alive") % *i);
> +              }
> +            }
>          }
>  
>      } else if (options.maxFreed > 0) {
> diff --git a/nix/libstore/store-api.hh b/nix/libstore/store-api.hh
> index fa78d59..8b0f521 100644
> --- a/nix/libstore/store-api.hh
> +++ b/nix/libstore/store-api.hh
> @@ -50,6 +50,9 @@ struct GCOptions
>      /* Stop after at least `maxFreed' bytes have been freed. */
>      unsigned long long maxFreed;
>  
> +    /* keep going even if some of the paths can not be collected */
> +    bool keepGoing;
> +
>      GCOptions();
>  };
>  
> diff --git a/nix/libstore/worker-protocol.hh b/nix/libstore/worker-protocol.hh
> index 99c1ee2..8faf193 100644
> --- a/nix/libstore/worker-protocol.hh
> +++ b/nix/libstore/worker-protocol.hh
> @@ -6,7 +6,7 @@ namespace nix {
>  #define WORKER_MAGIC_1 0x6e697863
>  #define WORKER_MAGIC_2 0x6478696f
>  
> -#define PROTOCOL_VERSION 0x110
> +#define PROTOCOL_VERSION 0x111
>  #define GET_PROTOCOL_MAJOR(x) ((x) & 0xff00)
>  #define GET_PROTOCOL_MINOR(x) ((x) & 0x00ff)
>  
> diff --git a/nix/nix-daemon/nix-daemon.cc b/nix/nix-daemon/nix-daemon.cc
> index a1fce25..b04cece 100644
> --- a/nix/nix-daemon/nix-daemon.cc
> +++ b/nix/nix-daemon/nix-daemon.cc
> @@ -526,6 +526,8 @@ static void performOp(bool trusted, unsigned int clientVersion,
>              readInt(from);
>              readInt(from);
>          }
> +        if (GET_PROTOCOL_MINOR(clientVersion) >= 17)
> +	  options.keepGoing = readInt(from) != 0;
   ^
Check tabbing/spacing here.

>  
>          GCResults results;
>  

Re: [PATCH] gc: Add option --keep-going.

[Ludovic Courtès]

Hartmut Goebel <h.goebel@crazy-compilers.com> skribis:

> * guix/scripts/gc.scm (show-help, %options): Add option -k/--keep-going.
>   (guix-gc): Pass value off option --keep-going on to delete-paths.
> * guix/store.scm (%protocol-version): Increment to 17.
>   (delete-paths) New key-word parameter `keep-going?`, pass it on to
>   run-rc.
>   (run-gc): New key-word parameter `keep-going?`, send value of
>   keep-going? to the daemon.
> * nix/libstore/store-api.hh (GCOptions): Add boolean keepGoing.
> * nix/libstore/worker-protocol.hh (PROTOCOL_VERSION) Increment to 17.
> * nix/nix-daemon/nix-daemon.cc (performOp)[wopCollectGarbage] Read
>   keepGoing.
> * nix/libstore/gc.cc (LocalStore::collectGarbage) If keepGoing is true
>   print an error message instead of throwing an error.
> * doc/guix.texi (Invoking guix gc): Document option --keep-going.

That’s a good idea!

One comment to complement Eric’s review: could you add a test, in
tests/store.scm, along the lines of the "dead path can be explicitly
collected"?  Ideally one test with keep-going, and one without.  (See
<https://www.gnu.org/software/guix/manual/html_node/Running-the-Test-Suite.html>.)

Thank you!

Ludo’.

Re: [PATCH] gc: Add option --keep-going.

[Hartmut Goebel]

Hi Eric,

thanks for the review and finding all these typos :-)

Am 18.11.2016 um 05:49 schrieb Eric Bavier:
>> > +                printMsg(lvlError, format("cannot delete path `%1%' since it is still alive") % *i);
>                                ^
> Is error formatting appropriate, or is there a warning level we can use?
>

I agree that these are warnings and no errors. There is not "lvlWarn",
thoug, just "lvlInfo". So I took this value from a few lines below which
say:

        if (state.shouldDelete)
            printMsg(lvlError, format("deleting garbage..."));
        else
            printMsg(lvlError, format("determining live/dead paths..."));

So, what do you think?

-- 
Regards
Hartmut Goebel

| Hartmut Goebel          | h.goebel@crazy-compilers.com               |
| www.crazy-compilers.com | compilers which you thought are impossible |

Re: [PATCH] gc: Add option --keep-going.

[Ludovic Courtès]

Hartmut Goebel <h.goebel@crazy-compilers.com> skribis:

> Am 18.11.2016 um 05:49 schrieb Eric Bavier:
>>> > +                printMsg(lvlError, format("cannot delete path `%1%' since it is still alive") % *i);
>>                                ^
>> Is error formatting appropriate, or is there a warning level we can use?
>>
>
> I agree that these are warnings and no errors. There is not "lvlWarn",
> thoug, just "lvlInfo". So I took this value from a few lines below which
> say:
>
>         if (state.shouldDelete)
>             printMsg(lvlError, format("deleting garbage..."));
>         else
>             printMsg(lvlError, format("determining live/dead paths..."));
>
> So, what do you think?

I think you’re right: lvlError is OK here, there’s a precedent.

Thanks,
Ludo’.