“guix refresh” chokes on cran.scm

  • Done
  • quality assurance status badge
Details
2 participants
  • Ludovic Courtès
  • Ricardo Wurmus
Owner
unassigned
Submitted by
Ricardo Wurmus
Severity
normal
R
R
Ricardo Wurmus wrote on 29 Jun 2023 23:26
“guix refresh” chokes on cran.scm
(address . bug-guix@gnu.org)(name . Ludovic Courtès)(address . ludo@gnu.org)
87bkgyklbv.fsf@elephly.net
The new refresh behavior to update inputs automatically seems to
sometimes cause the updater to lose track of the position in the file,
leading to errors like this:

Toggle snippet (40 lines)
Backtrace:
In ice-9/boot-9.scm:
1752:10 18 (with-exception-handler _ _ #:unwind? _ #:unwind-for-type _)
In unknown file:
17 (apply-smob/0 #<thunk 7f4fe56ba2a0>)
In ice-9/boot-9.scm:
724:2 16 (call-with-prompt _ _ #<procedure default-prompt-handler (k proc)>)
In ice-9/eval.scm:
619:8 15 (_ #(#(#<directory (guile-user) 7f4fe56bfc80>)))
In guix/ui.scm:
2309:7 14 (run-guix . _)
2272:10 13 (run-guix-command _ . _)
In ice-9/boot-9.scm:
1752:10 12 (with-exception-handler _ _ #:unwind? _ #:unwind-for-type _)
1752:10 11 (with-exception-handler _ _ #:unwind? _ #:unwind-for-type _)
In guix/store.scm:
659:37 10 (thunk)
2168:25 9 (run-with-store #<store-connection 256.99 7f4fc7ad02d0> _ #:guile-for-build _ #:system _ #:target _)
In guix/scripts/refresh.scm:
592:16 8 (_ _)
In srfi/srfi-1.scm:
634:9 7 (for-each #<procedure 7f4fc6860080 at guix/scripts/refresh.scm:593:17 (update)> _)
In guix/scripts/refresh.scm:
363:21 6 (update-package _ #<package r-dygraphs@1.1.1.6 /home/rekado/dev/gx/branches/master/gnu/packages/cran.scm:30341 7f4fc8e040b0> _ …)
In ice-9/boot-9.scm:
1747:15 5 (with-exception-handler #<procedure 7f4fc0fe54e0 at ice-9/boot-9.scm:1831:7 (exn)> _ #:unwind? _ #:unwind-for-type _)
In ice-9/ports.scm:
433:17 4 (call-with-input-file _ _ #:binary _ #:encoding _ #:guess-encoding _)
In guix/packages.scm:
762:17 3 (_ #<input: /home/rekado/dev/gx/branches/master/gnu/packages/cran.scm 16>)
In guix/utils.scm:
423:23 2 (go-to-location #<input: /home/rekado/dev/gx/branches/master/gnu/packages/cran.scm 16> 30341 2)
In ice-9/boot-9.scm:
1685:16 1 (raise-exception _ #:continuable? _)
1685:16 0 (raise-exception _ #:continuable? _)

ice-9/boot-9.scm:1685:16: In procedure raise-exception:
unexpected end of line #<input: /home/rekado/dev/gx/branches/master/gnu/packages/cran.scm 16>

Prior to that we see warnings like this:

Toggle snippet (4 lines)
/home/rekado/dev/gx/branches/master/gnu/packages/cran.scm:36350:2: r-readtext: updating from version 0.82 to version 0.90...
/home/rekado/dev/gx/branches/master/gnu/packages/cran.scm:36350:2: warning: r-readtext: no `version' field in source; skipping

It’s as if the position in the file has been lost and it tries to update
the definition of r-readtext that is no longer where the current port
position is.

I use “./pre-inst-env guix refresh -t cran -u”, which updates dozens of
packages. Perhaps the changes result in untracked position shifts in
the file?

--
Ricardo
R
R
Ricardo Wurmus wrote on 3 Jul 2023 21:29
(address . 64358@debbugs.gnu.org)(address . ludo@gnu.org)
87r0pou6h3.fsf@elephly.net
Toggle quote (9 lines)
> Prior to that we see warnings like this:
>
> /home/rekado/dev/gx/branches/master/gnu/packages/cran.scm:36350:2: r-readtext: updating from version 0.82 to version 0.90...
> /home/rekado/dev/gx/branches/master/gnu/packages/cran.scm:36350:2: warning: r-readtext: no `version' field in source; skipping
>
> It’s as if the position in the file has been lost and it tries to update
> the definition of r-readtext that is no longer where the current port
> position is.

It seems that this is indeed the problem. The value for a <package>’s
“location” field is known at compile/eval time and this value will not
be correct after the first substantial edit has taken place.

I see these options:

1 - pass a value to “update-package” that corresponds to line changes so
far and let it return an updated value, making “update-package” aware
of file changes. This would be rather ugly.

2 - compute the location of the target package anew if the file it is
located in has since been edited.

3 - never rely on the line number of the package location value; just open
the specified file and always search it for a package definition.
Optionally take the line number into account as a starting point for a
search in both directions.

4 - integrate changes with git, so that all edits are commits that are
applied to the very same base commit.

The third option seems the most reasonable and lightweight to me.

--
Ricardo
L
L
Ludovic Courtès wrote on 7 Jul 2023 15:42
(name . Ricardo Wurmus)(address . rekado@elephly.net)(address . 64358@debbugs.gnu.org)
87v8evkfbw.fsf@gnu.org
Hi,

Ricardo Wurmus <rekado@elephly.net> skribis:

Toggle quote (13 lines)
>> Prior to that we see warnings like this:
>>
>> /home/rekado/dev/gx/branches/master/gnu/packages/cran.scm:36350:2: r-readtext: updating from version 0.82 to version 0.90...
>> /home/rekado/dev/gx/branches/master/gnu/packages/cran.scm:36350:2: warning: r-readtext: no `version' field in source; skipping
>>
>> It’s as if the position in the file has been lost and it tries to update
>> the definition of r-readtext that is no longer where the current port
>> position is.
>
> It seems that this is indeed the problem. The value for a <package>’s
> “location” field is known at compile/eval time and this value will not
> be correct after the first substantial edit has taken place.

The way ‘guix style -S inputs’ handles it is by starting editing
packages from the bottom of the file and upwards (see the bottom of
(guix scripts style)). That way, source location is valid as it edits
things.

Perhaps we can do that here?

Ludo’.
R
R
Ricardo Wurmus wrote on 7 Jul 2023 22:13
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 64358@debbugs.gnu.org)
87y1jrscm2.fsf@elephly.net
Ludovic Courtès <ludo@gnu.org> writes:

Toggle quote (7 lines)
> The way ‘guix style -S inputs’ handles it is by starting editing
> packages from the bottom of the file and upwards (see the bottom of
> (guix scripts style)). That way, source location is valid as it edits
> things.
>
> Perhaps we can do that here?

Oh, that’s a very good idea. Worth a try!

--
Ricardo
R
R
Ricardo Wurmus wrote on 10 Jul 2023 20:41
[PATCH] refresh: Sort update specs by package location.
(address . 64358@debbugs.gnu.org)(name . Ricardo Wurmus)(address . rekado@elephly.net)
74997f913e60491d74afcea38a13e5edb0c71b09.1689014499.git.rekado@elephly.net

* guix/scripts/refresh.scm (guix-refresh): Sort update specs by location from
bottom to top before updating packages.
---
guix/scripts/refresh.scm | 33 ++++++++++++++++++++++-----------
1 file changed, 22 insertions(+), 11 deletions(-)

Toggle diff (55 lines)
diff --git a/guix/scripts/refresh.scm b/guix/scripts/refresh.scm
index 9676271542..7a0c312f9d 100644
--- a/guix/scripts/refresh.scm
+++ b/guix/scripts/refresh.scm
@@ -6,7 +6,7 @@
;;; Copyright © 2016 Ben Woodcroft <donttrustben@gmail.com>
;;; Copyright © 2017 Mathieu Othacehe <m.othacehe@gmail.com>
;;; Copyright © 2018 Efraim Flashner <efraim@flashner.co.il>
-;;; Copyright © 2019 Ricardo Wurmus <rekado@elephly.net>
+;;; Copyright © 2019, 2023 Ricardo Wurmus <rekado@elephly.net>
;;; Copyright © 2020 Simon Tournier <zimon.toutoune@gmail.com>
;;; Copyright © 2021 Sarah Morgensen <iskarian@mgsn.dev>
;;; Copyright © 2022 Hartmut Goebel <h.goebel@crazy-compilers.com>
@@ -589,16 +589,27 @@ (define-command (guix-refresh . args)
(or (assoc-ref opts 'keyring)
(string-append (config-directory)
"/upstream/trustedkeys.kbx"))))
- (for-each
- (lambda (update)
- (update-package store
- (update-spec-package update)
- (update-spec-version update)
- updaters
- #:key-server (%openpgp-key-server)
- #:key-download key-download
- #:warn? warn?))
- update-specs)
+ (let* ((get-line
+ (compose location-line
+ package-location
+ update-spec-package))
+ ;; Sort the specs so that we update packages from the
+ ;; bottom of the file to the top. This way we can be
+ ;; sure that the package locations are always correct
+ ;; and never shifted due to previous edits.
+ (sorted-update-specs
+ (sort update-specs
+ (lambda (a b) (> (get-line a) (get-line b))))))
+ (for-each
+ (lambda (update)
+ (update-package store
+ (update-spec-package update)
+ (update-spec-version update)
+ updaters
+ #:key-server (%openpgp-key-server)
+ #:key-download key-download
+ #:warn? warn?))
+ sorted-update-specs))
(return #t)))
(else
(for-each (cut check-for-package-update <> updaters

base-commit: d0296970fb8ed97ac17bd4c580351af961a8c0fb
--
2.40.1
L
L
Ludovic Courtès wrote on 10 Jul 2023 23:49
(name . Ricardo Wurmus)(address . rekado@elephly.net)
87v8er5tdo.fsf@gnu.org
Hi,

Ricardo Wurmus <rekado@elephly.net> skribis:

Toggle quote (5 lines)
>
> * guix/scripts/refresh.scm (guix-refresh): Sort update specs by location from
> bottom to top before updating packages.

[…]

Toggle quote (5 lines)
> + (let* ((get-line
> + (compose location-line
> + package-location
> + update-spec-package))

Maybe s/get-line/spec-line/ ?

Otherwise LGTM, thanks!

Ludo’.
R
R
Ricardo Wurmus wrote on 11 Jul 2023 00:17
(name . Ludovic Courtès)(address . ludo@gnu.org)
87ilarjtnt.fsf@elephly.net
Ludovic Courtès <ludo@gnu.org> writes:

Toggle quote (16 lines)
> Ricardo Wurmus <rekado@elephly.net> skribis:
>
>> Fixes <https://issues.guix.gnu.org/64358>.
>>
>> * guix/scripts/refresh.scm (guix-refresh): Sort update specs by location from
>> bottom to top before updating packages.
>
> […]
>
>> + (let* ((get-line
>> + (compose location-line
>> + package-location
>> + update-spec-package))
>
> Maybe s/get-line/spec-line/ ?

Okay, will change it.

I found that it’s probably not a good idea to sort by location-line
without also taking into account the file name. My latest version uses
“location->string” and “string>” instead of “location-line” and “>”,
ensuring that changes to the same file remain grouped.

I’ll push this together with the bulk update of CRAN packages that I’m
working on.

Thanks for the review!

--
Ricardo
R
R
Ricardo Wurmus wrote on 12 Jul 2023 14:35
“guix refresh” chokes on cran.scm
(address . 64358-done@debbugs.gnu.org)
87jzv5i9yl.fsf@elephly.net
Fixed in commit b43841c124d15eaecc41b3928f08a26dbd5c653a
--
Ricardo
Closed
L
L
Ludovic Courtès wrote on 14 Jul 2023 15:19
Re: bug#64358: [PATCH] refresh: Sort update specs by package location.
(name . Ricardo Wurmus)(address . rekado@elephly.net)
87zg3ywrz3.fsf@gnu.org
Hallo,

Ricardo Wurmus <rekado@elephly.net> skribis:

Toggle quote (5 lines)
> I found that it’s probably not a good idea to sort by location-line
> without also taking into account the file name. My latest version uses
> “location->string” and “string>” instead of “location-line” and “>”,
> ensuring that changes to the same file remain grouped.

Oops, good point.

Toggle quote (3 lines)
> I’ll push this together with the bulk update of CRAN packages that I’m
> working on.

Awesome, thanks!

Ludo’.
?
Your comment

This issue is archived.

To comment on this conversation send an email to 64358@debbugs.gnu.org

To respond to this issue using the mumi CLI, first switch to it
mumi current 64358
Then, you may apply the latest patchset in this issue (with sign off)
mumi am -- -s
Or, compose a reply to this issue
mumi compose
Or, send patches to this issue
mumi send-email *.patch