diff mbox series

[bug#64746,2/2] scripts: time-machine: Error when attempting to visit too old commits.

Message ID a058a4de3330a6f9cf41456e0781c3865dff0553.1689823648.git.maxim.cournoyer@gmail.com
State New
Headers show
Series [bug#64746,v2,1/3] git: Clarify commit relation reference in doc. | expand

Commit Message

Maxim Cournoyer July 20, 2023, 4:34 p.m. UTC 
* doc/guix.texi (Invoking guix time-machine): Document limitation.
* guix/scripts/time-machine.scm (%oldest-possible-commit): New variable.
(guix-time-machine): Raise an error when the channel commit is too old.

Suggested-by: Simon Tournier <zimon.toutoune@gmail.com>
---
 doc/guix.texi                 |  6 ++++++
 guix/scripts/time-machine.scm | 23 ++++++++++++++++++++++-
 2 files changed, 28 insertions(+), 1 deletion(-)

Comments

Maxim Cournoyer July 22, 2023, 2 a.m. UTC | #1 
Hello,

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

> * doc/guix.texi (Invoking guix time-machine): Document limitation.
> * guix/scripts/time-machine.scm (%oldest-possible-commit): New variable.
> (guix-time-machine): Raise an error when the channel commit is too old.
>
> Suggested-by: Simon Tournier <zimon.toutoune@gmail.com>
> ---
>  doc/guix.texi                 |  6 ++++++
>  guix/scripts/time-machine.scm | 23 ++++++++++++++++++++++-
>  2 files changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/doc/guix.texi b/doc/guix.texi
> index 1d8ebcd72f..30fef813c0 100644
> --- a/doc/guix.texi
> +++ b/doc/guix.texi
> @@ -5056,6 +5056,12 @@ Invoking guix time-machine
>  large number of packages; the result is cached though and subsequent
>  commands targeting the same commit are almost instantaneous.
>  
> +Due to @command{guix time-machine} relying on the ``inferiors''
> +mechanism (@pxref{Inferiors}), the oldest commit it can travel to is
> +commit @samp{2ca299caf} (``Add (guix inferior) and (guix scripts
> +repl).''), dated July 10@sup{th}, 2018.  An error is returned when
> +attempting to navigate to older commits.
> +
>  @quotation Note
>  The history of Guix is immutable and @command{guix time-machine}
>  provides the exact same software as they are in a specific Guix
> diff --git a/guix/scripts/time-machine.scm b/guix/scripts/time-machine.scm
> index d7c71ef705..36a40a1538 100644
> --- a/guix/scripts/time-machine.scm
> +++ b/guix/scripts/time-machine.scm
> @@ -2,6 +2,7 @@
>  ;;; Copyright © 2019 Konrad Hinsen <konrad.hinsen@fastmail.net>
>  ;;; Copyright © 2019, 2020, 2021 Ludovic Courtès <ludo@gnu.org>
>  ;;; Copyright © 2021 Simon Tournier <zimon.toutoune@gmail.com>
> +;;; Copyright © 2023 Maxim Cournoyer <maxim.cournoyer@gmail.com>
>  ;;;
>  ;;; This file is part of GNU Guix.
>  ;;;
> @@ -19,13 +20,15 @@
>  ;;; along with GNU Guix.  If not, see <http://www.gnu.org/licenses/>.
>  
>  (define-module (guix scripts time-machine)
> +  #:use-module (guix channels)
> +  #:use-module (guix diagnostics)
>    #:use-module (guix ui)
>    #:use-module (guix scripts)
>    #:use-module (guix inferior)
>    #:use-module (guix store)
>    #:use-module (guix status)
>    #:use-module ((guix git)
> -                #:select (with-git-error-handling))
> +                #:select (update-cached-checkout with-git-error-handling))
>    #:use-module ((guix utils)
>                  #:select (%current-system))
>    #:use-module ((guix scripts pull)
> @@ -38,9 +41,16 @@ (define-module (guix scripts time-machine)
>    #:use-module (srfi srfi-1)
>    #:use-module (srfi srfi-11)
>    #:use-module (srfi srfi-26)
> +  #:use-module (srfi srfi-34)
>    #:use-module (srfi srfi-37)
> +  #:use-module (srfi srfi-71)
>    #:export (guix-time-machine))
>  
> +;;; The commit introducing the 'inferiors' mechanism; it is the oldest commit
> +;;; that can be travelled to.
> +(define %oldest-possible-commit
> +  "2ca299caf64489f4e1e665ec1158fb0309b0b565")

I just tried travelling to that assumed oldest commit (because it
corresponds to the introduction of the inferiors mechanism), but it
fails like:

--8<---------------cut here---------------start------------->8---
Computing Guix derivation for 'x86_64-linux'...  Backtrace:
-           5 (primitive-load "/gnu/store/b70mihsj9xx0xxp6izliqb5vm4…")
In ice-9/eval.scm:
    155:9  4 (_ _)
    159:9  3 (_ #(#(#(#(#(#(#(#(#(#(#(…) …) …) …) …) …) …) …) …) …) …))
   173:47  2 (_ #(#(#(#(#(#(#(#(#(#(#(…) …) …) …) …) …) …) …) …) …) …))
In ./guix/self.scm:
    932:4  1 (guix-derivation "/gnu/store/yfn2s94i5bvwr7j7r6xcnivwg…" …)
    903:2  0 (guile-for-build "3.0")

./guix/self.scm:903:2: In procedure guile-for-build:
Throw to key `match-error' with args `("match" "no matching pattern" "3.0")'.
Backtrace:
In ice-9/boot-9.scm:
  1752:10 19 (with-exception-handler _ _ #:unwind? _ # _)
In guix/store.scm:
   659:37 18 (thunk)
In guix/status.scm:
    839:4 17 (call-with-status-report _ _)
In guix/store.scm:
   1298:8 16 (call-with-build-handler #<procedure 7ff1daabfd20 at g…> …)
In guix/monads.scm:
    576:2 15 (run-with-store #<store-connection 256.99 7ff1dab842d0> …)
In guix/inferior.scm:
    927:8 14 (_ _)
In guix/channels.scm:
    982:2 13 (_ _)
    924:2 12 (_ _)
In guix/store.scm:
   1883:0 11 (_ _)
   1996:8 10 (_ _)
In guix/channels.scm:
   675:14  9 (_ #<store-connection 256.99 7ff1dab842d0>)
In guix/monads.scm:
    576:2  8 (run-with-store #<store-connection 256.99 7ff1dab842d0> …)
In guix/store.scm:
   1298:8  7 (call-with-build-handler _ _)
   1298:8  6 (call-with-build-handler #<procedure 7ff1db8108b8 at g…> …)
In guix/channels.scm:
   690:14  5 (_)
In guix/monads.scm:
    576:2  4 (run-with-store #<store-connection 256.99 7ff1dab842d0> …)
In ice-9/eval.scm:
   191:27  3 (_ #(#(#<directory (build-self) 7ff1da984aa0> #<pr…>) …))
In ice-9/boot-9.scm:
   2007:7  2 (error _ . _)
  1685:16  1 (raise-exception _ #:continuable? _)
  1685:16  0 (raise-exception _ #:continuable? _)

ice-9/boot-9.scm:1685:16: In procedure raise-exception:
invalid build result (#<derivation /gnu/store/rj2g4x23lqyaq16471qm94xp90slxp3h-compute-guix-derivation.drv => /gnu/store/b70mihsj9xx0xxp6izliqb5vm462yifl-compute-guix-derivation 7ff1d8b55000> "")
--8<---------------cut here---------------end--------------->8---

Is this a bug or expected?
Ludovic Courtès Aug. 8, 2023, 3:58 p.m. UTC | #2 
Hi!

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

>> +;;; The commit introducing the 'inferiors' mechanism; it is the oldest commit
>> +;;; that can be travelled to.
>> +(define %oldest-possible-commit
>> +  "2ca299caf64489f4e1e665ec1158fb0309b0b565")
>
> I just tried travelling to that assumed oldest commit (because it
> corresponds to the introduction of the inferiors mechanism), but it
> fails like:
>
> Computing Guix derivation for 'x86_64-linux'...  Backtrace:
> -           5 (primitive-load "/gnu/store/b70mihsj9xx0xxp6izliqb5vm4…")
> In ice-9/eval.scm:
>     155:9  4 (_ _)
>     159:9  3 (_ #(#(#(#(#(#(#(#(#(#(#(…) …) …) …) …) …) …) …) …) …) …))
>    173:47  2 (_ #(#(#(#(#(#(#(#(#(#(#(…) …) …) …) …) …) …) …) …) …) …))
> In ./guix/self.scm:
>     932:4  1 (guix-derivation "/gnu/store/yfn2s94i5bvwr7j7r6xcnivwg…" …)
>     903:2  0 (guile-for-build "3.0")
>
> ./guix/self.scm:903:2: In procedure guile-for-build:
> Throw to key `match-error' with args `("match" "no matching pattern" "3.0")'.

I would pick ‘v0.15.0’ (= 359fdda40f754bbf1b5dc261e7427b75463b59be) as
the oldest commit one can travel to; it’s a bit newer than the one
above, but it fails in the same way (to my surprise).  It would be
interesting to investigate.

That said, we could just as well pick ‘v1.0.0’, which is the official
warranty-void limit, and which seems to work (it needs to build things,
though…).

Ludo’.
Maxim Cournoyer Aug. 10, 2023, 2:47 p.m. UTC | #3 
Hi Ludo!

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

> Hi!
>
> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
>
>>> +;;; The commit introducing the 'inferiors' mechanism; it is the oldest commit
>>> +;;; that can be travelled to.
>>> +(define %oldest-possible-commit
>>> +  "2ca299caf64489f4e1e665ec1158fb0309b0b565")
>>
>> I just tried travelling to that assumed oldest commit (because it
>> corresponds to the introduction of the inferiors mechanism), but it
>> fails like:
>>
>> Computing Guix derivation for 'x86_64-linux'...  Backtrace:
>> -           5 (primitive-load "/gnu/store/b70mihsj9xx0xxp6izliqb5vm4…")
>> In ice-9/eval.scm:
>>     155:9  4 (_ _)
>>     159:9  3 (_ #(#(#(#(#(#(#(#(#(#(#(…) …) …) …) …) …) …) …) …) …) …))
>>    173:47  2 (_ #(#(#(#(#(#(#(#(#(#(#(…) …) …) …) …) …) …) …) …) …) …))
>> In ./guix/self.scm:
>>     932:4  1 (guix-derivation "/gnu/store/yfn2s94i5bvwr7j7r6xcnivwg…" …)
>>     903:2  0 (guile-for-build "3.0")
>>
>> ./guix/self.scm:903:2: In procedure guile-for-build:
>> Throw to key `match-error' with args `("match" "no matching pattern" "3.0")'.
>
> I would pick ‘v0.15.0’ (= 359fdda40f754bbf1b5dc261e7427b75463b59be) as
> the oldest commit one can travel to; it’s a bit newer than the one
> above, but it fails in the same way (to my surprise).  It would be
> interesting to investigate.
>
> That said, we could just as well pick ‘v1.0.0’, which is the official
> warranty-void limit, and which seems to work (it needs to build things,
> though…).

I tried building v1.0.0 but it failed the same as earlier attempts on
Python 2, which has a SSL test failing due to now-expired certificates:

--8<---------------cut here---------------start------------->8---
@ build-log 18017 117                                                                                                                                                                           
/gnu/store/gfprsx2m62cvqbh7ysc9ay9slhijvmal-module-import/guix/build/gnu-build-system.scm:369:6: In procedso                                                                                                        ure check:                                                                                                                                          
@ build-log 18017 167                                                                                                                                                                           
Throw to key `srfi-34' with args `(#<condition &invoke-error [program: "make" arguments: ("test" "-j" "24"                                                                                                          ) exit-status: 2 term-signal: #f stop-signal: #f] 9a4900>)'.                                              40                                                                                                        
\@ build-log 18017 101                                                                                                                                                                              
builder for `/gnu/store/sp947xhp8dqfzn3zd2m0aq4ia5qvklbl-python2-2.7.15.drv' failed with exit code 1
@ build-log 18017 183                                                                                     -o
@ build-failed /gnu/store/sp947xhp8dqfzn3zd2m0aq4ia5qvklbl-python2-2.7.15.drv - 1 builder for `/gnu/store/h-sp947xhp8dqfzn3zd2m0aq4ia5qvklbl-python2-2.7.15.drv' failed with exit code 1
@ build-log 18017 189
derivation '/gnu/store/sp947xhp8dqfzn3zd2m0aq4ia5qvklbl-python2-2.7.15.drv' offloaded to '10.0.0.7' failede-: build of `/gnu/store/sp947xhp8dqfzn3zd2m0aq4ia5qvklbl-python2-2.7.15.drv' failed                        ar
@ build-failed /gnu/store/sp947xhp8dqfzn3zd2m0aq4ia5qvklbl-python2-2.7.15.drv - 1 builder for `/gnu/store/cesp947xhp8dqfzn3zd2m0aq4ia5qvklbl-python2-2.7.15.drv' failed with exit code 100
Backtrace:
In ./guix/gexp.scm:
   573:13 19 (_ _)                                                                                          
In ./guix/store.scm:
   1667:8 18 (_ _)
   1667:8 17 (_ _)                                                                                        so
In ./guix/gexp.scm:
    708:2 16 (_ _)
In ./guix/monads.scm:
    482:9 15 (_ _)
In ./guix/gexp.scm:
   573:13 14 (_ _)
In ./guix/store.scm:
   1667:8 13 (_ _)
In ./guix/gexp.scm:
    708:2 12 (_ _)
In ./guix/monads.scm:                                                                                     9}
    482:9 11 (_ _)
In ./guix/gexp.scm:
   573:13 10 (_ _)
In ./guix/store.scm:                                                                                      37
   1667:8  9 (_ _)                                                                                        36
In ./guix/gexp.scm:                                                                                       1c
    708:2  8 (_ _)                                                                                        2!
In ./guix/monads.scm:                                                                                     3I
    482:9  7 (_ _)                                                                                        \1
|
In ./guix/gexp.scm:
   573:13  6 (_ _)
In ./guix/store.scm:
   1667:8  5 (_ _)
  1690:38  4 (_ #<store-connection 256.99 7f6b38e620c0>)
In ./guix/packages.scm:
   936:16  3 (cache! #<weak-table 406/883> #<package guile-gcrypt@0?> ?)
  1255:22  2 (thunk)
  1188:25  1 (bag->derivation #<store-connection 256.99 7f6b38e620c0> ?)
In srfi/srfi-1.scm:
   592:17  0 (map1 (("source" #<origin #<<git-reference> url: "?>) ?))

srfi/srfi-1.scm:592:17: In procedure map1:
Throw to key `srfi-34' with args `(#<condition &store-protocol-error [message: "build of `/gnu/store/sp947xhp8dqfzn3zd2m0aq4ia5qvklbl-python2-2.7.15.drv' failed" status: 100] 7f6b3a5b2c60>)'.
guix time-machine: erreur : You found a bug: the program '/gnu/store/d12x45lz2dv3mgp594kzjv0d121g0ncs-compute-guix-derivation'
failed to compute the derivation for Guix (version: "6298c3ffd9654d3231a6f25390b056483e8f407c"; system: "xso86_64-linux";
host version: "985638aea14720e16ed5fd94a0e1382a57dec7ac"; pull-version: 1).
Please report it by email to <bug-guix@gnu.org>.
--8<---------------cut here---------------end--------------->8---

Since we can't retroactively fix this kind of problem, it means we
should find the oldest commit which is immune to that problem?
Ludovic Courtès Aug. 10, 2023, 4:56 p.m. UTC | #4 
Hi,

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

> Since we can't retroactively fix this kind of problem, it means we
> should find the oldest commit which is immune to that problem?

Timebombs (typically expired SSL certificates as in the Python case you
mention) can be worked around.  Currently it’s inconvenient at best
because you need to manually set up a VM or physical machine with its
date set to an older date, but it’s feasible.

For other problems, there’s (guix quirks), which can, to some extent,
let us retroactively fix problems, though that should be rare.

The <https://ci.guix.gnu.org/jobset/time-travel> job set, which uses
‘etc/time-travel-manifest.scm’, is supposed to test these things, but
apparently it’s been stuck for a while.  Needs investigation!

Ludo’.
Josselin Poiret Aug. 11, 2023, 7:19 a.m. UTC | #5 
Hi everyone,

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

> For other problems, there’s (guix quirks), which can, to some extent,
> let us retroactively fix problems, though that should be rare.

Is there any established policy around what (guix quirks) is allowed to
do?  To be more specific, can/will it ever change derivation hashes?

Best,
Ludovic Courtès Aug. 12, 2023, 8:32 p.m. UTC | #6 
Hi,

Josselin Poiret <dev@jpoiret.xyz> skribis:

> Ludovic Courtès <ludo@gnu.org> writes:
>
>> For other problems, there’s (guix quirks), which can, to some extent,
>> let us retroactively fix problems, though that should be rare.
>
> Is there any established policy around what (guix quirks) is allowed to
> do?  To be more specific, can/will it ever change derivation hashes?

It should not change derivations, which means it can only touch
auxiliary files like ‘build-aux/build-self.scm’.

Ludo’.
Ludovic Courtès Aug. 15, 2023, 4:14 p.m. UTC | #7 
Hello,

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

> @@ -139,9 +149,20 @@ (define-command (guix-time-machine . args)
>      (with-git-error-handling
>       (let* ((opts         (parse-args args))
>              (channels     (channel-list opts))
> +            (guix-channel (find guix-channel? channels))
>              (command-line (assoc-ref opts 'exec))
> +            (ref          (assoc-ref opts 'ref))
> +            (checkout commit relation (update-cached-checkout
> +                                       (channel-url guix-channel)
> +                                       #:ref (or ref '())
> +                                       #:starting-commit
> +                                       %oldest-possible-commit))
>              (substitutes?  (assoc-ref opts 'substitutes?))
>              (authenticate? (assoc-ref opts 'authenticate-channels?)))

Following your question earlier today on IRC, I realized that this would
unconditionally add a Git checkout update every time ‘time-machine’ is
started.

This would noticeably degrade performance in the cache-hit case (when
running a cached channel set).  Currently, on cache hits,
‘cached-channel-instance’ directly returns ~/.cache/guix/profiles/xyz
without performing any Git or (guix store) operation.

This is important because it guarantees that one can run ‘guix
time-machine -C channels.scm -- COMMAND’ at pretty much the same speed
as ‘guix COMMAND’.

Perhaps the solution would be for ‘cached-channel-instance’ to have a
new #:validate-channel procedure (or similar) that would be passed
channel/commit/relation and would have the opportunity to validate,
before the thing is in cache.

How does that sound?

Ludo’.
Maxim Cournoyer Aug. 15, 2023, 6:57 p.m. UTC | #8 
Hello,

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

> Hi!
>
> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
>
>>> +;;; The commit introducing the 'inferiors' mechanism; it is the oldest commit
>>> +;;; that can be travelled to.
>>> +(define %oldest-possible-commit
>>> +  "2ca299caf64489f4e1e665ec1158fb0309b0b565")
>>
>> I just tried travelling to that assumed oldest commit (because it
>> corresponds to the introduction of the inferiors mechanism), but it
>> fails like:
>>
>> Computing Guix derivation for 'x86_64-linux'...  Backtrace:
>> -           5 (primitive-load "/gnu/store/b70mihsj9xx0xxp6izliqb5vm4…")
>> In ice-9/eval.scm:
>>     155:9  4 (_ _)
>>     159:9  3 (_ #(#(#(#(#(#(#(#(#(#(#(…) …) …) …) …) …) …) …) …) …) …))
>>    173:47  2 (_ #(#(#(#(#(#(#(#(#(#(#(…) …) …) …) …) …) …) …) …) …) …))
>> In ./guix/self.scm:
>>     932:4  1 (guix-derivation "/gnu/store/yfn2s94i5bvwr7j7r6xcnivwg…" …)
>>     903:2  0 (guile-for-build "3.0")
>>
>> ./guix/self.scm:903:2: In procedure guile-for-build:
>> Throw to key `match-error' with args `("match" "no matching pattern" "3.0")'.
>
> I would pick ‘v0.15.0’ (= 359fdda40f754bbf1b5dc261e7427b75463b59be) as
> the oldest commit one can travel to; it’s a bit newer than the one
> above, but it fails in the same way (to my surprise).  It would be
> interesting to investigate.
>
> That said, we could just as well pick ‘v1.0.0’, which is the official
> warranty-void limit, and which seems to work (it needs to build things,
> though…).

I've picked v1.0.0.  It already seems hard enough to get there (you'd
have to build Python 2 with the hardware clock set to a value in the
past to avoid time bombs in its test suite).

As discussed with Ludovic, it's best to avoid doing work on critical
path, that is, when there is a cache hit for the channel.  I've
implemented their idea to delay such work to within
'cached-channel-instance', where we know if there's a cache hit or not.
Here's a simple 'strace -c' benchmark:

Without this change (Guix commit
985638aea14720e16ed5fd94a0e1382a57dec7ac), on a warm cache, it results
on something like:

--8<---------------cut here---------------start------------->8---
$ strace -c guix time-machine --commit=v1.3.0 -- \
  environment --ad-hoc hello -- hello
% time     seconds  usecs/call     calls    errors syscall
[...]
100,00    0,072028           4     16963      2026 total

With this change (v2 incoming) installed:
--8<---------------cut here---------------start------------->8---
$ strace -c ./pre-inst-env guix time-machine --commit=v1.3.0 -- \
  environment --ad-hoc hello -- hello
% time     seconds  usecs/call     calls    errors syscall
[...]
100,00    0,074576           4     18005      2700 total
--8<---------------cut here---------------end--------------->8---

It seems a small cost to pay for the increased user friendliness.

What do you think?
Simon Tournier Aug. 16, 2023, 2:46 p.m. UTC | #9 
Hi,

Thanks Maxim for this improvement.


On Tue, 15 Aug 2023 at 18:14, Ludovic Courtès <ludo@gnu.org> wrote:

> Following your question earlier today on IRC, I realized that this would
> unconditionally add a Git checkout update every time ‘time-machine’ is
> started.

Please note that if git.savannah.gnu.org is not reachable, then “guix
time-machine” fails.

Let start with the regular:

--8<---------------cut here---------------start------------->8---
$ guix describe
Generation 26   Jul 12 2023 09:13:39    (current)
  guix 4a027d2
    repository URL: https://git.savannah.gnu.org/git/guix.git
    branch: master
    commit: 4a027d2b0ee68e39f21f6802a8cd1751d3065330

$ guix time-machine --commit=4a027d2 -- describe
Updating channel 'guix' from Git repository at 'https://git.savannah.gnu.org/git/guix.git'...
substitute: updating substitutes from 'https://ci.guix.gnu.org'... 100.0%
substitute: updating substitutes from 'https://bordeaux.guix.gnu.org'... 100.0%
building /gnu/store/sg8ca36rlbh4il6jy8dk2gr33lxm4z8q-compute-guix-derivation.drv...
Computing Guix derivation for 'x86_64-linux'... |
substitute: updating substitutes from 'https://ci.guix.gnu.org'... 100.0%
substitute: updating substitutes from 'https://bordeaux.guix.gnu.org'... 100.0%
The following derivations will be built:
[...]
building profile with 1 package...
  guix 4a027d2
    repository URL: https://git.savannah.gnu.org/git/guix.git
    commit: 4a027d2b0ee68e39f21f6802a8cd1751d3065330
--8<---------------cut here---------------end--------------->8---

So far, so good.  Here all is cached and so on.  Now, let make
git.savannah.gnu.org unreachable by tweaking some stuff.  Then,

--8<---------------cut here---------------start------------->8---
$ guix time-machine --commit=4a027d2 -- describe
guix time-machine: error: Git error: failed to resolve address for git.savannah.gnu.org: Name or service not known
--8<---------------cut here---------------end--------------->8---

Well, even if this patch will add another inefficiency, there is already
one. :-) I would say without having looking closely to the code that the
issue comes because something™ tries to connect before checking if the
commit is already in the local checkout.


> This would noticeably degrade performance in the cache-hit case (when
> running a cached channel set).  Currently, on cache hits,
> ‘cached-channel-instance’ directly returns ~/.cache/guix/profiles/xyz
> without performing any Git or (guix store) operation.

As shown above, I guess there is bug…


> This is important because it guarantees that one can run ‘guix
> time-machine -C channels.scm -- COMMAND’ at pretty much the same speed
> as ‘guix COMMAND’.

Yes but I guess it could also be improved as shown above. :-)

The check if the requested commit is newer than the
%oldest-possible-commit should use the Git history graph closure
similarly as the authentication mechanism, no?

And this check should come after checking the cache, no?

Cheers,
simon
Maxim Cournoyer Aug. 16, 2023, 6:41 p.m. UTC | #10 
Hi Simon,

Simon Tournier <zimon.toutoune@gmail.com> writes:

> Hi,
>
> Thanks Maxim for this improvement.
>
>
> On Tue, 15 Aug 2023 at 18:14, Ludovic Courtès <ludo@gnu.org> wrote:
>
>> Following your question earlier today on IRC, I realized that this would
>> unconditionally add a Git checkout update every time ‘time-machine’ is
>> started.
>
> Please note that if git.savannah.gnu.org is not reachable, then “guix
> time-machine” fails.
>
> Let start with the regular:
>
> $ guix describe
> Generation 26   Jul 12 2023 09:13:39    (current)
>   guix 4a027d2
>     repository URL: https://git.savannah.gnu.org/git/guix.git
>     branch: master
>     commit: 4a027d2b0ee68e39f21f6802a8cd1751d3065330
>
> $ guix time-machine --commit=4a027d2 -- describe
> Updating channel 'guix' from Git repository at 'https://git.savannah.gnu.org/git/guix.git'...
> substitute: updating substitutes from 'https://ci.guix.gnu.org'... 100.0%
> substitute: updating substitutes from 'https://bordeaux.guix.gnu.org'... 100.0%
> building /gnu/store/sg8ca36rlbh4il6jy8dk2gr33lxm4z8q-compute-guix-derivation.drv...
> Computing Guix derivation for 'x86_64-linux'... |
> substitute: updating substitutes from 'https://ci.guix.gnu.org'... 100.0%
> substitute: updating substitutes from 'https://bordeaux.guix.gnu.org'... 100.0%
> The following derivations will be built:
> [...]
> building profile with 1 package...
>   guix 4a027d2
>     repository URL: https://git.savannah.gnu.org/git/guix.git
>     commit: 4a027d2b0ee68e39f21f6802a8cd1751d3065330
>
>
> So far, so good.  Here all is cached and so on.  Now, let make
> git.savannah.gnu.org unreachable by tweaking some stuff.  Then,
>
> $ guix time-machine --commit=4a027d2 -- describe
> guix time-machine: error: Git error: failed to resolve address for git.savannah.gnu.org: Name or service not known
>

[...]

> The check if the requested commit is newer than the
> %oldest-possible-commit should use the Git history graph closure
> similarly as the authentication mechanism, no?
>
> And this check should come after checking the cache, no?

Interesting finding!  I think it'd make sense to raise this issue
separately and discuss its resolution there, too keep things focused and
discoverable :-).
Simon Tournier Aug. 17, 2023, 2:06 p.m. UTC | #11 
Hi,

As discussed in patch#64746, here the fix. :-)

-------------------- Start of forwarded message --------------------
From: Maxim Cournoyer <maxim.cournoyer@gmail.com>
Date: Wed, 16 Aug 2023 14:41:55 -0400

Simon Tournier <zimon.toutoune@gmail.com> writes:

> Please note that if git.savannah.gnu.org is not reachable, then “guix
> time-machine” fails.
>
> Let start with the regular:
>
> $ guix describe
> Generation 26   Jul 12 2023 09:13:39    (current)
>   guix 4a027d2
>     repository URL: https://git.savannah.gnu.org/git/guix.git
>     branch: master
>     commit: 4a027d2b0ee68e39f21f6802a8cd1751d3065330
>
> $ guix time-machine --commit=4a027d2 -- describe
> Updating channel 'guix' from Git repository at 'https://git.savannah.gnu.org/git/guix.git'...
> substitute: updating substitutes from 'https://ci.guix.gnu.org'... 100.0%
> substitute: updating substitutes from 'https://bordeaux.guix.gnu.org'... 100.0%
> building /gnu/store/sg8ca36rlbh4il6jy8dk2gr33lxm4z8q-compute-guix-derivation.drv...
> Computing Guix derivation for 'x86_64-linux'... |
> substitute: updating substitutes from 'https://ci.guix.gnu.org'... 100.0%
> substitute: updating substitutes from 'https://bordeaux.guix.gnu.org'... 100.0%
> The following derivations will be built:
> [...]
> building profile with 1 package...
>   guix 4a027d2
>     repository URL: https://git.savannah.gnu.org/git/guix.git
>     commit: 4a027d2b0ee68e39f21f6802a8cd1751d3065330
>
>
> So far, so good.  Here all is cached and so on.  Now, let make
> git.savannah.gnu.org unreachable by tweaking some stuff.  Then,
>
> $ guix time-machine --commit=4a027d2 -- describe
> guix time-machine: error: Git error: failed to resolve address for git.savannah.gnu.org: Name or service not known

Interesting finding!  I think it'd make sense to raise this issue
separately and discuss its resolution there, too keep things focused and
discoverable :-).
-------------------- End of forwarded message --------------------

The issue is introduced by commit
dce2cf311bc12dee4560329f53ccb53470d5793e in the procedure
reference-available?.  The variable ’ref’ is the pair

    (tag-or-commit . "123abc")

and fails with commit-id? in

  (match ref
    ((or ('commit . commit)
         ('tag-or-commit . (? commit-id? commit)))

Therefore, reference-available? returns #f and the ’when’ branch is run
in update-cached-checkout.

     ;; Only fetch remote if it has not been cloned just before.
     (when (and cache-exists?
                (not (reference-available? repository ref)))
       (remote-fetch (remote-lookup repository "origin")
                     #:fetch-options (make-default-fetch-options)))

Hence the network access required by remote-fetch.

Well, the heavy work to know if the reference is available or not in the
local checkout is done by ’resolve-reference’ in (guix git) doing all
the cases, and especially dealing with tag-or-commit:

      (match ref
        (('branch . branch)
         (let ((oid (reference-target
                     (branch-lookup repository branch BRANCH-REMOTE))))
           (object-lookup repository oid)))
        (('commit . commit)
         (let ((len (string-length commit)))
           ;; 'object-lookup-prefix' appeared in Guile-Git in Mar. 2018, so we
           ;; can't be sure it's available.  Furthermore, 'string->oid' used to
           ;; read out-of-bounds when passed a string shorter than 40 chars,
           ;; which is why we delay calls to it below.
           (if (< len 40)
               (if (module-defined? (resolve-interface '(git object))
                                    'object-lookup-prefix)
                   (object-lookup-prefix repository (string->oid commit) len)
                   (raise (condition
                           (&message
                            (message "long Git object ID is required")))))
               (object-lookup repository (string->oid commit)))))
        (('tag-or-commit . str)
         (if (or (> (string-length str) 40)
                 (not (string-every char-set:hex-digit str)))
             (resolve `(tag . ,str))              ;definitely a tag
             (catch 'git-error
               (lambda ()
                 (resolve `(tag . ,str)))
               (lambda _
                 ;; There's no such tag, so it must be a commit ID.
                 (resolve `(commit . ,str))))))
        (('tag    . tag)
         (let ((oid (reference-name->oid repository
                                         (string-append "refs/tags/" tag))))
           (object-lookup repository oid))))

Instead of duplicating, I propose to reuse it.  See the trivial first
patch.  I think it fixes the annoyance.

Aside, please note that (guix channels) provide commit-or-tag.  It
change nothing but I would find more consistent to have the same
nomenclature.

--8<---------------cut here---------------start------------->8---
(define (sexp->channel-news-entry entry)
  "Return the <channel-news-entry> record corresponding to ENTRY, an sexp."
  (define (pair language message)
    (cons (symbol->string language) message))

  (match entry
    (('entry ((and (or 'commit 'tag) type) commit-or-tag)
             ('title ((? symbol? title-tags) (? string? titles)) ...)
             ('body ((? symbol? body-tags) (? string? bodies)) ...)
             _ ...)
     (channel-news-entry (and (eq? type 'commit) commit-or-tag)
                         (and (eq? type 'tag) commit-or-tag)
--8<---------------cut here---------------end--------------->8---

WDYT about tag-or-commit everywhere?


Last, as I pointed in a naive comment [1], I do not think that
guix/scripts/pull.scm or guix/time-machine.scm need to support both the
pair (commit . x) and (tag-or-commit . x) because the value ’ref’ is set
by the option.  Hence the second patch.

1: https://yhetil.org/guix/87o7j7f2tb.fsf@gmail.com


Let me know if I am not missing something.


Cheers,
simon
Simon Tournier Sept. 6, 2023, 10:32 a.m. UTC | #12 
Hi Maxim,

Let start another branch in that thread of #65352. :-)

Let start the discussion on good basis, let start with an example:

--8<---------------cut here---------------start------------->8---
$ guix describe
Generation 26   Jul 12 2023 09:13:39    (current)
  guix 4a027d2
    repository URL: https://git.savannah.gnu.org/git/guix.git
    branch: master
    commit: 4a027d2b0ee68e39f21f6802a8cd1751d3065330

$ guix time-machine --commit=4a027d2 -- describe
Updating channel 'guix' from Git repository at 'https://git.savannah.gnu.org/git/guix.git'...
substitute: updating substitutes from 'https://ci.guix.gnu.org'... 100.0%
substitute: updating substitutes from 'https://bordeaux.guix.gnu.org'... 100.0%
building /gnu/store/sg8ca36rlbh4il6jy8dk2gr33lxm4z8q-compute-guix-derivation.drv...
Computing Guix derivation for 'x86_64-linux'... |
substitute: updating substitutes from 'https://ci.guix.gnu.org'... 100.0%
substitute: updating substitutes from 'https://bordeaux.guix.gnu.org'... 100.0%
The following derivations will be built:
[...]
building profile with 1 package...
  guix 4a027d2
    repository URL: https://git.savannah.gnu.org/git/guix.git
    commit: 4a027d2b0ee68e39f21f6802a8cd1751d3065330
--8<---------------cut here---------------end--------------->8---

So far, so good.  Here all is cached and so on.  Now, let make
git.savannah.gnu.org unreachable by tweaking some stuff.  Then,

--8<---------------cut here---------------start------------->8---
$ guix time-machine --commit=4a027d2 -- describe
guix time-machine: error: Git error: failed to resolve address for git.savannah.gnu.org: Name or service not known
--8<---------------cut here---------------end--------------->8---

Do we agree it is bug?  Do we agree that the behaviour is not POLA?

It is the same when specifying a release tag,

--8<---------------cut here---------------start------------->8---
$ guix time-machine --commit=v1.4.0 -- describe
guix time-machine: error: Git error: failed to resolve address for git.savannah.gnu.org: Name or service not known
--8<---------------cut here---------------end--------------->8---

I think Guix needs to be reliable whatever the status of Savannah when a
local Git ref is in the local cached checkout.


After this introduction, let start the core discussion.


On Tue, 05 Sep 2023 at 22:00, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote:

> I don't know if we want to consider tags are immutable or not; the
> safest is to consider them an *not* immutable, which is what we had been
> doing.  I agree it doesn't cover all the potential git refspecs; we can
> get there if we want (although I suppose it's uncommon for someone to
> try 'guix time-machine --commit=v1.3.0-47405-ge0767a24d0' or similar).

[...]

> I'm not sure if short commit IDs should be treated as immutable, since
> in theory they can collide; the safest would be to check if there are
> collisions and report an error if there is; and this requires fetching
> new objects first.

Well, the behaviour that I want is that it just works whatever the
status of Savannah when I have a local Git ref that matches what I
provide to ’guix time-machine’ (or guix pull or else).

I think you are inferring a rule from two corner-cases.  And from my
point of view, there are only hypothetical. :-)

1. About tag.  The ones from upstream are defacto immutable.  It is
uncommon that people set local tag under ~/.cache/guix/checkouts.  And,
the failure when Savannah is unreachable appears to me more annoying
than hypothetical mutable tags.  Therefore, I propose what I already
proposed. :-) It will make it works for most of the cases.

Even, what would happen if a tag is changed?  The user does not get the
same inferior for two invocations.  The question is: what triggers the
update of the cached checkout?

What is the consequence for not updating when the user-specified Git ref
is a mutable one (tag or else)?  Here, I am proposing to delay the
update until the next “guix pull” or “guix time-machine -q”, well until
the user invokes a command with a Git ref that does not belong to the
local cached checkout.

I do not see why this delay is a problem.  And it avoids an update.


2. About short commit IDs.  The same reasoning applies. :-)

About the collision, it is the same.  It only delays the collision
report.


All in all, I think that reference-available? needs to check if the Git
ref belongs to the local cached checkout and that’s all.  If it is, use
it, else update the local cached checkout.

At time t, the user-specificity Git ref can match some local Git ref but
not the upstream state.  And so?

Somehow, I am considering the local cached checkout as the primary
reference for looking up Git ref.

Do you see a potential issue that I am missing?


Cheers,
simon
Maxim Cournoyer Sept. 6, 2023, 5:41 p.m. UTC | #13 
Hi Simon, Ludovic,

Simon Tournier <zimon.toutoune@gmail.com> writes:

> Hi Maxim,
>
> Let start another branch in that thread of #65352. :-)

Alright :-).

> Let start the discussion on good basis, let start with an example:
>
> $ guix describe
> Generation 26   Jul 12 2023 09:13:39    (current)
>   guix 4a027d2
>     repository URL: https://git.savannah.gnu.org/git/guix.git
>     branch: master
>     commit: 4a027d2b0ee68e39f21f6802a8cd1751d3065330
>
> $ guix time-machine --commit=4a027d2 -- describe
> Updating channel 'guix' from Git repository at 'https://git.savannah.gnu.org/git/guix.git'...
> substitute: updating substitutes from 'https://ci.guix.gnu.org'... 100.0%
> substitute: updating substitutes from 'https://bordeaux.guix.gnu.org'... 100.0%
> building /gnu/store/sg8ca36rlbh4il6jy8dk2gr33lxm4z8q-compute-guix-derivation.drv...
> Computing Guix derivation for 'x86_64-linux'... |
> substitute: updating substitutes from 'https://ci.guix.gnu.org'... 100.0%
> substitute: updating substitutes from 'https://bordeaux.guix.gnu.org'... 100.0%
> The following derivations will be built:
> [...]
> building profile with 1 package...
>   guix 4a027d2
>     repository URL: https://git.savannah.gnu.org/git/guix.git
>     commit: 4a027d2b0ee68e39f21f6802a8cd1751d3065330
>
>
> So far, so good.  Here all is cached and so on.  Now, let make
> git.savannah.gnu.org unreachable by tweaking some stuff.  Then,
>
> $ guix time-machine --commit=4a027d2 -- describe
> guix time-machine: error: Git error: failed to resolve address for git.savannah.gnu.org: Name or service not known
>
>
> Do we agree it is bug?  Do we agree that the behaviour is not POLA?

Thanks for the example, it helps :-).  I agree it's an undesirable
behavior to reach to the network after having (supposedly) cached the
very same ref.

[...]

> On Tue, 05 Sep 2023 at 22:00, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote:
>
>> I don't know if we want to consider tags are immutable or not; the
>> safest is to consider them an *not* immutable, which is what we had been
>> doing.  I agree it doesn't cover all the potential git refspecs; we can
>> get there if we want (although I suppose it's uncommon for someone to
>> try 'guix time-machine --commit=v1.3.0-47405-ge0767a24d0' or similar).
>
> [...]
>
>> I'm not sure if short commit IDs should be treated as immutable, since
>> in theory they can collide; the safest would be to check if there are
>> collisions and report an error if there is; and this requires fetching
>> new objects first.
>
> Well, the behaviour that I want is that it just works whatever the
> status of Savannah when I have a local Git ref that matches what I
> provide to ’guix time-machine’ (or guix pull or else).
>
> I think you are inferring a rule from two corner-cases.  And from my
> point of view, there are only hypothetical. :-)

Also, from the current state of things (the code) :-).  But I agree that
there seems to be space for improvements here.

> 1. About tag.  The ones from upstream are defacto immutable.  It is
> uncommon that people set local tag under ~/.cache/guix/checkouts.  And,
> the failure when Savannah is unreachable appears to me more annoying
> than hypothetical mutable tags.  Therefore, I propose what I already
> proposed. :-) It will make it works for most of the cases.

More annoying but also, much more likely!

> Even, what would happen if a tag is changed?  The user does not get the
> same inferior for two invocations.  The question is: what triggers the
> update of the cached checkout?
>
> What is the consequence for not updating when the user-specified Git ref
> is a mutable one (tag or else)?  Here, I am proposing to delay the
> update until the next “guix pull” or “guix time-machine -q”, well until
> the user invokes a command with a Git ref that does not belong to the
> local cached checkout.
>
> I do not see why this delay is a problem.  And it avoids an update.
>
>
> 2. About short commit IDs.  The same reasoning applies. :-)
>
> About the collision, it is the same.  It only delays the collision
> report.

Sounds reasonable; it'll reduce some load from Savannah ;-).

>
> All in all, I think that reference-available? needs to check if the Git
> ref belongs to the local cached checkout and that’s all.  If it is, use
> it, else update the local cached checkout.
>
> At time t, the user-specificity Git ref can match some local Git ref but
> not the upstream state.  And so?
>
> Somehow, I am considering the local cached checkout as the primary
> reference for looking up Git ref.
>
> Do you see a potential issue that I am missing?

So all the refs such as commit(ish) or tags would be referenced locally,
and branches such as 'master' would still trigger an update.

LGTM, but I'd be curious to hear what Ludovic thinks, since their
original code treated tags as mutable (which they technically are, but I
agree to the value of treating them as immutable, and it appears low
risk to me).
Simon Tournier Sept. 6, 2023, 11:21 p.m. UTC | #14 
Hi,

On Wed, 06 Sep 2023 at 13:41, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote:

> So all the refs such as commit(ish) or tags would be referenced locally,
> and branches such as 'master' would still trigger an update.

That’s the intent of this patch:

        [bug#65352] [PATCH v2] DRAFT git: Avoid touching the network unless needed in 'reference-available?'.
        Simon Tournier <zimon.toutoune@gmail.com>
        Wed, 06 Sep 2023 16:17:08 +0200
        id:32d3fb5066e0b20e200dabef0fba897634e21dda.1694009405.git.zimon.toutoune@gmail.com
        https://issues.guix.gnu.org//65352
        https://issues.guix.gnu.org/msgid/32d3fb5066e0b20e200dabef0fba897634e21dda.1694009405.git.zimon.toutoune@gmail.com
        https://yhetil.org/guix/32d3fb5066e0b20e200dabef0fba897634e21dda.1694009405.git.zimon.toutoune@gmail.com


> LGTM, but I'd be curious to hear what Ludovic thinks, since their
> original code treated tags as mutable (which they technically are, but I
> agree to the value of treating them as immutable, and it appears low
> risk to me).

Do we have an use-case where tags are mutable?

To my knowledge, the Guix remote tags have always been immutable.  Do we
have one counter-example?

Well, here an attempt for a scenario with mutable tags – although I
think that’s a corner case considering the current state for
manipulating Guix cache checkouts.  I am using Guix 6113e05, nothing
about the patch I am proposing. :-)

$ cp -r ~/.cache/guix/checkouts/pjmkglp4t7znuugeurpurzikxq3tnlaywmisyr27shj7apsnalwq /tmp/guix,git
$ guix time-machine -q --commit=4a027d2 --url=/tmp/guix.git -- describe

So far, so good.  Let add one tag.

$ git -C /tmp/guix.git tag -a mutable -m "some tag" 4a027d2
$ git -C /tmp/guix.git tag -l mut*

And…

$ guix time-machine -q --commit=mutable --url=/tmp/guix.git -- describe
guix time-machine: error: Git error: reference 'refs/tags/mutable' not found

…bang!

Well, the basic Git tags does not seem supported by the Guile-Git
’remote-fetch’ procedure.  I have not investigated more.  Maybe I am
missing something.

My opinion is to stay focused on the current real annoyances first and
not try to fix another hypothetical use-case which seems already buggy.

Ludo, WDYT about the proposed patch?  Does it work for your use-cases?

Cheers,
simon
diff mbox series

Patch

diff --git a/doc/guix.texi b/doc/guix.texi
index 1d8ebcd72f..30fef813c0 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -5056,6 +5056,12 @@  Invoking guix time-machine
 large number of packages; the result is cached though and subsequent
 commands targeting the same commit are almost instantaneous.
 
+Due to @command{guix time-machine} relying on the ``inferiors''
+mechanism (@pxref{Inferiors}), the oldest commit it can travel to is
+commit @samp{2ca299caf} (``Add (guix inferior) and (guix scripts
+repl).''), dated July 10@sup{th}, 2018.  An error is returned when
+attempting to navigate to older commits.
+
 @quotation Note
 The history of Guix is immutable and @command{guix time-machine}
 provides the exact same software as they are in a specific Guix
diff --git a/guix/scripts/time-machine.scm b/guix/scripts/time-machine.scm
index d7c71ef705..36a40a1538 100644
--- a/guix/scripts/time-machine.scm
+++ b/guix/scripts/time-machine.scm
@@ -2,6 +2,7 @@ 
 ;;; Copyright © 2019 Konrad Hinsen <konrad.hinsen@fastmail.net>
 ;;; Copyright © 2019, 2020, 2021 Ludovic Courtès <ludo@gnu.org>
 ;;; Copyright © 2021 Simon Tournier <zimon.toutoune@gmail.com>
+;;; Copyright © 2023 Maxim Cournoyer <maxim.cournoyer@gmail.com>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -19,13 +20,15 @@ 
 ;;; along with GNU Guix.  If not, see <http://www.gnu.org/licenses/>.
 
 (define-module (guix scripts time-machine)
+  #:use-module (guix channels)
+  #:use-module (guix diagnostics)
   #:use-module (guix ui)
   #:use-module (guix scripts)
   #:use-module (guix inferior)
   #:use-module (guix store)
   #:use-module (guix status)
   #:use-module ((guix git)
-                #:select (with-git-error-handling))
+                #:select (update-cached-checkout with-git-error-handling))
   #:use-module ((guix utils)
                 #:select (%current-system))
   #:use-module ((guix scripts pull)
@@ -38,9 +41,16 @@  (define-module (guix scripts time-machine)
   #:use-module (srfi srfi-1)
   #:use-module (srfi srfi-11)
   #:use-module (srfi srfi-26)
+  #:use-module (srfi srfi-34)
   #:use-module (srfi srfi-37)
+  #:use-module (srfi srfi-71)
   #:export (guix-time-machine))
 
+;;; The commit introducing the 'inferiors' mechanism; it is the oldest commit
+;;; that can be travelled to.
+(define %oldest-possible-commit
+  "2ca299caf64489f4e1e665ec1158fb0309b0b565")
+
 
 ;;;
 ;;; Command-line options.
@@ -139,9 +149,20 @@  (define-command (guix-time-machine . args)
     (with-git-error-handling
      (let* ((opts         (parse-args args))
             (channels     (channel-list opts))
+            (guix-channel (find guix-channel? channels))
             (command-line (assoc-ref opts 'exec))
+            (ref          (assoc-ref opts 'ref))
+            (checkout commit relation (update-cached-checkout
+                                       (channel-url guix-channel)
+                                       #:ref (or ref '())
+                                       #:starting-commit
+                                       %oldest-possible-commit))
             (substitutes?  (assoc-ref opts 'substitutes?))
             (authenticate? (assoc-ref opts 'authenticate-channels?)))
+       (unless (memq relation '(ancestor self))
+         (raise (formatted-message
+                 (G_ "cannot travel past commit `~a' from July 10th, 2018")
+                 (string-take %oldest-possible-commit 12))))
        (when command-line
          (let* ((directory
                  (with-store store