[PATCH 0/3] Make time-machine commit check cheaper; make test effective

  • Done
  • quality assurance status badge
Details
2 participants
  • Ludovic Courtès
  • Maxim Cournoyer
Owner
unassigned
Submitted by
Ludovic Courtès
Severity
normal
L
L
Ludovic Courtès wrote on 28 Oct 2023 16:02
(address . guix-patches@gnu.org)
cover.1698501649.git.ludo@gnu.org
Hello Guix!

These changes were prompted by https://issues.guix.gnu.org/65788.

Feedback welcome!

Ludo'.

Ludovic Courtès (3):
tests: Make ‘guix time-machine’ test effective.
time-machine: Make target commit check cheaper.
time-machine: Warn when no command is given.

guix/inferior.scm | 58 +++++++++++-----------
guix/scripts/time-machine.scm | 91 +++++++++++++++++------------------
tests/guix-time-machine.sh | 24 +++++++--
3 files changed, 95 insertions(+), 78 deletions(-)


base-commit: ff1146fb4f7254a8f644f89d7af6b4b566528603
--
2.41.0
L
L
Ludovic Courtès wrote on 28 Oct 2023 16:08
[PATCH 1/3] tests: Make ‘guix time-machin e’ test effective.
(address . 66793@debbugs.gnu.org)(name . Ludovic Courtès)(address . ludo@gnu.org)
d55f09b93e2f727d841cd4fba075a1049738712f.1698501649.git.ludo@gnu.org
The test as added in 79ec651a286c71a3d4c72be33a1f80e76a560031 had no
effect: first because ‘guix time-machine --commit=X’, not followed by a
command, does nothing, and second because the “! COMMAND” shell stanza
does not have the desired effect (see https://issues.guix.gnu.org/62406).

This change rewrites the test to make it effective.

* tests/guix-time-machine.sh: Rewrite.

Change-Id: Ib44a11331c8625e346139a236cffa699cdbd02f2
---
tests/guix-time-machine.sh | 24 ++++++++++++++++++++----
1 file changed, 20 insertions(+), 4 deletions(-)

Toggle diff (42 lines)
diff --git a/tests/guix-time-machine.sh b/tests/guix-time-machine.sh
index 8b62ef75ea..a78c1533fb 100644
--- a/tests/guix-time-machine.sh
+++ b/tests/guix-time-machine.sh
@@ -1,5 +1,6 @@
# GNU Guix --- Functional package management for GNU
# Copyright © 2023 Maxim Cournoyer <maxim.cournoyer@gmail.com>
+# Copyright © 2023 Ludovic Courtès <ludo@gnu.org>
#
# This file is part of GNU Guix.
#
@@ -20,9 +21,24 @@
# Test the 'guix time-machine' command-line utility.
#
-guix time-machine --version
+if [ -d "$abs_top_srcdir/.git" ] \
+ || guile -c '(getaddrinfo "www.gnu.org" "80" AI_NUMERICSERV)' 2> /dev/null
+then
+ guix time-machine --version
+else
+ echo "This test requires networking or a local Git checkout; skipping." >&2
+ exit 77
+fi
-# Visiting a commit older than v1.0.0 fails.
-! guix time-machine --commit=v0.15.0
+if [ -d "$abs_top_srcdir/.git" ]
+then
+ EXTRA_OPTIONS="--url=$abs_top_srcdir"
+else
+ EXTRA_OPTIONS=""
+fi
-exit 0
+# Visiting a commit older than v1.0.0 must fail (this test is expensive
+# because it clones the whole repository).
+guix time-machine -q --commit=v0.15.0 $EXTRA_OPTIONS -- describe && false
+
+true
--
2.41.0
L
L
Ludovic Courtès wrote on 28 Oct 2023 16:08
[PATCH 2/3] time-machine: Make target commit check cheaper.
(address . 66793@debbugs.gnu.org)
648020c38a11915d2d147c4b05da682b011b593e.1698501649.git.ludo@gnu.org
Commit 79ec651a286c71a3d4c72be33a1f80e76a560031 introduced a check to
error out when attempting to use ‘time-machine’ to travel to a commit
before ‘v1.0.0’.

This commit fixes a performance issue with the strategy used in
79ec651a286c71a3d4c72be33a1f80e76a560031 (the repository was opened,
updated, and traversed a second time by ‘validate-guix-channel’) as well
as a user interface issue (“Updating channel” messages would be printed
too late).

This patch reimplements the check in terms of the existing #:validate-pull
mechanism, which is designed to avoid extra repository operations.


* guix/inferior.scm (cached-channel-instance): Change default value
of #:validate-channels. Remove call to VALIDATE-CHANNELS; pass it
as #:validate-pull to ‘latest-channel-instances’.
* guix/scripts/time-machine.scm (%reference-channels): New variable.
(validate-guix-channel): New procedure.
(guix-time-machine)[validate-guix-channel]: Remove.
Pass #:reference-channels to ‘cached-channel-instance’.

Reported-by: Simon Tournier <zimon.toutoune@gmail.com>
Change-Id: I9b0ec61fba7354fe08b04a91f4bd32b72a35460c
---
guix/inferior.scm | 58 +++++++++++++++++++----------------
guix/scripts/time-machine.scm | 58 ++++++++++++++++-------------------
2 files changed, 58 insertions(+), 58 deletions(-)

Toggle diff (173 lines)
diff --git a/guix/inferior.scm b/guix/inferior.scm
index fca6fb4b22..190ba01b3c 100644
--- a/guix/inferior.scm
+++ b/guix/inferior.scm
@@ -872,14 +872,17 @@ (define* (cached-channel-instance store
(authenticate? #t)
(cache-directory (%inferior-cache-directory))
(ttl (* 3600 24 30))
- validate-channels)
+ (reference-channels '())
+ (validate-channels (const #t)))
"Return a directory containing a guix filetree defined by CHANNELS, a list of channels.
The directory is a subdirectory of CACHE-DIRECTORY, where entries can be
reclaimed after TTL seconds. This procedure opens a new connection to the
build daemon. AUTHENTICATE? determines whether CHANNELS are authenticated.
-VALIDATE-CHANNELS, if specified, must be a one argument procedure accepting a
-list of channels that can be used to validate the channels; it should raise an
-exception in case of problems."
+
+VALIDATE-CHANNELS must be a four-argument procedure used to validate channel
+instances against REFERENCE-CHANNELS; it is passed as #:validate-pull to
+'latest-channel-instances' and should raise an exception in case a target
+channel commit is deemed \"invalid\"."
(define commits
;; Since computing the instances of CHANNELS is I/O-intensive, use a
;; cheaper way to get the commit list of CHANNELS. This limits overhead
@@ -927,30 +930,31 @@ (define* (cached-channel-instance store
(if (file-exists? cached)
cached
- (begin
- (when (procedure? validate-channels)
- (validate-channels channels))
- (run-with-store store
- (mlet* %store-monad ((instances
- -> (latest-channel-instances store channels
- #:authenticate?
- authenticate?))
- (profile
- (channel-instances->derivation instances)))
- (mbegin %store-monad
- ;; It's up to the caller to install a build handler to report
- ;; what's going to be built.
- (built-derivations (list profile))
+ (run-with-store store
+ (mlet* %store-monad ((instances
+ -> (latest-channel-instances store channels
+ #:authenticate?
+ authenticate?
+ #:current-channels
+ reference-channels
+ #:validate-pull
+ validate-channels))
+ (profile
+ (channel-instances->derivation instances)))
+ (mbegin %store-monad
+ ;; It's up to the caller to install a build handler to report
+ ;; what's going to be built.
+ (built-derivations (list profile))
- ;; Cache if and only if AUTHENTICATE? is true.
- (if authenticate?
- (mbegin %store-monad
- (symlink* (derivation->output-path profile) cached)
- (add-indirect-root* cached)
- (return cached))
- (mbegin %store-monad
- (add-temp-root* (derivation->output-path profile))
- (return (derivation->output-path profile))))))))))
+ ;; Cache if and only if AUTHENTICATE? is true.
+ (if authenticate?
+ (mbegin %store-monad
+ (symlink* (derivation->output-path profile) cached)
+ (add-indirect-root* cached)
+ (return cached))
+ (mbegin %store-monad
+ (add-temp-root* (derivation->output-path profile))
+ (return (derivation->output-path profile)))))))))
(define* (inferior-for-channels channels
#:key
diff --git a/guix/scripts/time-machine.scm b/guix/scripts/time-machine.scm
index f31fae7435..bd64364fa2 100644
--- a/guix/scripts/time-machine.scm
+++ b/guix/scripts/time-machine.scm
@@ -46,12 +46,6 @@ (define-module (guix scripts time-machine)
#:use-module (srfi srfi-71)
#:export (guix-time-machine))
-;;; The required inferiors mechanism relied on by 'guix time-machine' was
-;;; firmed up in v1.0.0; it is the oldest, safest commit that can be travelled
-;;; to.
-(define %oldest-possible-commit
- "6298c3ffd9654d3231a6f25390b056483e8f407c") ;v1.0.0
-
;;;
;;; Command-line options.
@@ -144,6 +138,31 @@ (define (parse-args args)
(("--") opts)
(("--" command ...) (alist-cons 'exec command opts))))))
+
+;;;
+;;; Avoiding traveling too far back.
+;;;
+
+;;; The required inferiors mechanism relied on by 'guix time-machine' was
+;;; firmed up in v1.0.0; it is the oldest, safest commit that can be travelled
+;;; to.
+(define %oldest-possible-commit
+ "6298c3ffd9654d3231a6f25390b056483e8f407c") ;v1.0.0
+
+(define %reference-channels
+ (list (channel (inherit %default-guix-channel)
+ (commit %oldest-possible-commit))))
+
+(define (validate-guix-channel channel start commit relation)
+ "Raise an error if CHANNEL is the 'guix' channel and the RELATION of COMMIT
+to %OLDEST-POSSIBLE-COMMIT is not that of an ancestor."
+ (unless (or (not (guix-channel? channel))
+ (memq relation '(ancestor self)))
+ (raise (formatted-message
+ (G_ "cannot travel past commit `~a' from May 1st, 2019")
+ (string-take %oldest-possible-commit 12)))))
+
+
;;;
;;; Entry point.
@@ -160,31 +179,6 @@ (define-command (guix-time-machine . args)
(ref (assoc-ref opts 'ref))
(substitutes? (assoc-ref opts 'substitutes?))
(authenticate? (assoc-ref opts 'authenticate-channels?)))
-
- (define (validate-guix-channel channels)
- "Finds the Guix channel among CHANNELS, and validates that REF as
-captured from the closure, a git reference specification such as a commit hash
-or tag associated to the channel, is valid and new enough to satisfy the 'guix
-time-machine' requirements. If the captured REF variable is #f, the reference
-validate is the one of the Guix channel found in CHANNELS. A
-`formatted-message' condition is raised otherwise."
- (let* ((guix-channel (find guix-channel? channels))
- (guix-channel-commit (channel-commit guix-channel))
- (guix-channel-branch (channel-branch guix-channel))
- (guix-channel-ref (if guix-channel-commit
- `(tag-or-commit . ,guix-channel-commit)
- `(branch . ,guix-channel-branch)))
- (reference (or ref guix-channel-ref))
- (checkout commit relation (update-cached-checkout
- (channel-url guix-channel)
- #:ref reference
- #:starting-commit
- %oldest-possible-commit)))
- (unless (memq relation '(ancestor self))
- (raise (formatted-message
- (G_ "cannot travel past commit `~a' from May 1st, 2019")
- (string-take %oldest-possible-commit 12))))))
-
(when command-line
(let* ((directory
(with-store store
@@ -197,6 +191,8 @@ (define-command (guix-time-machine . args)
(set-build-options-from-command-line store opts)
(cached-channel-instance store channels
#:authenticate? authenticate?
+ #:reference-channels
+ %reference-channels
#:validate-channels
validate-guix-channel)))))
(executable (string-append directory "/bin/guix")))
--
2.41.0
L
L
Ludovic Courtès wrote on 28 Oct 2023 16:08
[PATCH 3/3] time-machine: Warn when no command is given.
(address . 66793@debbugs.gnu.org)(name . Ludovic Courtès)(address . ludo@gnu.org)
b9d9549ef6b794b0284717dbe819b35c272b5892.1698501649.git.ludo@gnu.org
* guix/scripts/time-machine.scm (guix-time-machine): Emit a warning when
COMMAND-LINE is false.

Change-Id: I26e6b608915ecaf6d9372f9b03dc5ebd1b4c68f9
---
guix/scripts/time-machine.scm | 37 ++++++++++++++++++-----------------
1 file changed, 19 insertions(+), 18 deletions(-)

Toggle diff (47 lines)
diff --git a/guix/scripts/time-machine.scm b/guix/scripts/time-machine.scm
index bd64364fa2..2c30fe7cfd 100644
--- a/guix/scripts/time-machine.scm
+++ b/guix/scripts/time-machine.scm
@@ -179,21 +179,22 @@ (define-command (guix-time-machine . args)
(ref (assoc-ref opts 'ref))
(substitutes? (assoc-ref opts 'substitutes?))
(authenticate? (assoc-ref opts 'authenticate-channels?)))
- (when command-line
- (let* ((directory
- (with-store store
- (with-status-verbosity (assoc-ref opts 'verbosity)
- (with-build-handler (build-notifier #:use-substitutes?
- substitutes?
- #:verbosity
- (assoc-ref opts 'verbosity)
- #:dry-run? #f)
- (set-build-options-from-command-line store opts)
- (cached-channel-instance store channels
- #:authenticate? authenticate?
- #:reference-channels
- %reference-channels
- #:validate-channels
- validate-guix-channel)))))
- (executable (string-append directory "/bin/guix")))
- (apply execl (cons* executable executable command-line))))))))
+ (if command-line
+ (let* ((directory
+ (with-store store
+ (with-status-verbosity (assoc-ref opts 'verbosity)
+ (with-build-handler (build-notifier #:use-substitutes?
+ substitutes?
+ #:verbosity
+ (assoc-ref opts 'verbosity)
+ #:dry-run? #f)
+ (set-build-options-from-command-line store opts)
+ (cached-channel-instance store channels
+ #:authenticate? authenticate?
+ #:reference-channels
+ %reference-channels
+ #:validate-channels
+ validate-guix-channel)))))
+ (executable (string-append directory "/bin/guix")))
+ (apply execl (cons* executable executable command-line)))
+ (warning (G_ "no command specified; nothing to do~%")))))))
--
2.41.0
M
M
Maxim Cournoyer wrote on 31 Oct 2023 16:06
Re: [bug#66793] [PATCH 1/3] tests: Make ‘guix time-machine’ test effective.
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 66793@debbugs.gnu.org)
877cn2sufi.fsf@gmail.com
Hi,

Ludovic Courtès <ludo@gnu.org> writes:

Toggle quote (5 lines)
> The test as added in 79ec651a286c71a3d4c72be33a1f80e76a560031 had no
> effect: first because ‘guix time-machine --commit=X’, not followed by a
> command, does nothing, and second because the “! COMMAND” shell stanza
> does not have the desired effect (see <https://issues.guix.gnu.org/62406>).

Interesting. I had tested it, but I guess not with that script :-).

[...]

Toggle quote (16 lines)
> -guix time-machine --version
> +if [ -d "$abs_top_srcdir/.git" ] \
> + || guile -c '(getaddrinfo "www.gnu.org" "80" AI_NUMERICSERV)' 2> /dev/null
> +then
> + guix time-machine --version
> +else
> + echo "This test requires networking or a local Git checkout; skipping." >&2
> + exit 77
> +fi
>
> -# Visiting a commit older than v1.0.0 fails.
> -! guix time-machine --commit=v0.15.0
> +if [ -d "$abs_top_srcdir/.git" ]
> +then
> + EXTRA_OPTIONS="--url=$abs_top_srcdir"

Should the --url valE here be prefixed with "file://", just to make it
extra clear we are cloning from a local file?

Toggle quote (11 lines)
> +else
> + EXTRA_OPTIONS=""
> +fi
>
> -exit 0
> +# Visiting a commit older than v1.0.0 must fail (this test is expensive
> +# because it clones the whole repository).
> +guix time-machine -q --commit=v0.15.0 $EXTRA_OPTIONS -- describe && false
> +
> +true

Otherwise LGTM.

--
Thanks,
Maxim
M
M
Maxim Cournoyer wrote on 31 Oct 2023 16:15
Re: [bug#66793] [PATCH 2/3] time-machine: Make target commit check cheaper.
(name . Ludovic Courtès)(address . ludo@gnu.org)
8734xqsu16.fsf@gmail.com
Hi,

Ludovic Courtès <ludo@gnu.org> writes:

Toggle quote (22 lines)
> Commit 79ec651a286c71a3d4c72be33a1f80e76a560031 introduced a check to
> error out when attempting to use ‘time-machine’ to travel to a commit
> before ‘v1.0.0’.
>
> This commit fixes a performance issue with the strategy used in
> 79ec651a286c71a3d4c72be33a1f80e76a560031 (the repository was opened,
> updated, and traversed a second time by ‘validate-guix-channel’) as well
> as a user interface issue (“Updating channel” messages would be printed
> too late).
>
> This patch reimplements the check in terms of the existing #:validate-pull
> mechanism, which is designed to avoid extra repository operations.
>
> Fixes <https://issues.guix.gnu.org/65788>.
>
> * guix/inferior.scm (cached-channel-instance): Change default value
> of #:validate-channels. Remove call to VALIDATE-CHANNELS; pass it
> as #:validate-pull to ‘latest-channel-instances’.
> * guix/scripts/time-machine.scm (%reference-channels): New variable.
> (validate-guix-channel): New procedure.
> (guix-time-machine)[validate-guix-channel]: Remove.

Nitpick: I'd say the proc was moved and simplified to ease traceability
for the reader; same for %oldest-possible-commit (not mentioned in
changelog).

Otherwise LGTM!

--
Thanks,
Maxim
M
M
Maxim Cournoyer wrote on 31 Oct 2023 16:16
Re: [bug#66793] [PATCH 3/3] time-machine: Warn when no command is given.
(name . Ludovic Courtès)(address . ludo@gnu.org)
87y1firfe8.fsf@gmail.com
Hi Ludo,

Ludovic Courtès <ludo@gnu.org> writes:

Toggle quote (3 lines)
> * guix/scripts/time-machine.scm (guix-time-machine): Emit a warning when
> COMMAND-LINE is false.

[...]

Toggle quote (20 lines)
> + (if command-line
> + (let* ((directory
> + (with-store store
> + (with-status-verbosity (assoc-ref opts 'verbosity)
> + (with-build-handler (build-notifier #:use-substitutes?
> + substitutes?
> + #:verbosity
> + (assoc-ref opts 'verbosity)
> + #:dry-run? #f)
> + (set-build-options-from-command-line store opts)
> + (cached-channel-instance store channels
> + #:authenticate? authenticate?
> + #:reference-channels
> + %reference-channels
> + #:validate-channels
> + validate-guix-channel)))))
> + (executable (string-append directory "/bin/guix")))
> + (apply execl (cons* executable executable command-line)))
> + (warning (G_ "no command specified; nothing to do~%")))))))

LGTM.

--
Thanks,
Maxim
L
L
Ludovic Courtès wrote on 5 Nov 2023 21:49
Re: [bug#66793] [PATCH 1/3] tests: Make ‘guix time-machine’ test effective.
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)(address . 66793@debbugs.gnu.org)
87cywoylh9.fsf@gnu.org
Hi,

Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:

Toggle quote (18 lines)
>> +if [ -d "$abs_top_srcdir/.git" ] \
>> + || guile -c '(getaddrinfo "www.gnu.org" "80" AI_NUMERICSERV)' 2> /dev/null
>> +then
>> + guix time-machine --version
>> +else
>> + echo "This test requires networking or a local Git checkout; skipping." >&2
>> + exit 77
>> +fi
>>
>> -# Visiting a commit older than v1.0.0 fails.
>> -! guix time-machine --commit=v0.15.0
>> +if [ -d "$abs_top_srcdir/.git" ]
>> +then
>> + EXTRA_OPTIONS="--url=$abs_top_srcdir"
>
> Should the --url valE here be prefixed with "file://", just to make it
> extra clear we are cloning from a local file?

To my surprise, the test (which does little more than cloning the repo)
runs in 5s without file:// and in 30mn otherwise! So I left the file://
prefix out and added a comment.

Ludo’.
L
L
Ludovic Courtès wrote on 5 Nov 2023 23:28
Re: [bug#66793] [PATCH 2/3] time-machine: Make target commit check cheaper.
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
8734xjzvg9.fsf@gnu.org
Hi,

Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:

Toggle quote (28 lines)
> Ludovic Courtès <ludo@gnu.org> writes:
>
>> Commit 79ec651a286c71a3d4c72be33a1f80e76a560031 introduced a check to
>> error out when attempting to use ‘time-machine’ to travel to a commit
>> before ‘v1.0.0’.
>>
>> This commit fixes a performance issue with the strategy used in
>> 79ec651a286c71a3d4c72be33a1f80e76a560031 (the repository was opened,
>> updated, and traversed a second time by ‘validate-guix-channel’) as well
>> as a user interface issue (“Updating channel” messages would be printed
>> too late).
>>
>> This patch reimplements the check in terms of the existing #:validate-pull
>> mechanism, which is designed to avoid extra repository operations.
>>
>> Fixes <https://issues.guix.gnu.org/65788>.
>>
>> * guix/inferior.scm (cached-channel-instance): Change default value
>> of #:validate-channels. Remove call to VALIDATE-CHANNELS; pass it
>> as #:validate-pull to ‘latest-channel-instances’.
>> * guix/scripts/time-machine.scm (%reference-channels): New variable.
>> (validate-guix-channel): New procedure.
>> (guix-time-machine)[validate-guix-channel]: Remove.
>
> Nitpick: I'd say the proc was moved and simplified to ease traceability
> for the reader; same for %oldest-possible-commit (not mentioned in
> changelog).

Indeed; I clarified that ‘validate-guix-channel’ was moved but didn’t
write anything about ‘%oldest-possible-commit’ because it’s actually
unchanged (just moved a few lines below).

I pushed the result:

331d858e21 time-machine: Warn when no command is given.
ab13e2be69 time-machine: Make target commit check cheaper.
9f05fbb67d tests: Make ‘guix time-machine’ test effective.

Thanks for reviewing!

Ludo’.
Closed
?