| Message ID | 1479307034-20585-1-git-send-email-h.goebel@crazy-compilers.com |
|---|---|
| State | New |
| Headers | show |
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; >
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’.
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?
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’.
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;