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 |
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?
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’.
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?
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’.
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,
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’.
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’.
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?
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
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 :-).
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
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
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).
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 --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