bug#64358: “guix refresh” chokes on cran.scm

bug#64358: “guix refresh” chokes on cran.scm

[Ricardo Wurmus]

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:

--8<---------------cut here---------------start------------->8---
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>
--8<---------------cut here---------------end--------------->8---

Prior to that we see warnings like this:

--8<---------------cut here---------------start------------->8---
/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
--8<---------------cut here---------------end--------------->8---

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

bug#64358: “guix refresh” chokes on cran.scm

[Ricardo Wurmus]

> 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

bug#64358: “guix refresh” chokes on cran.scm

[Ludovic Courtès]

Hi,

Ricardo Wurmus <rekado@elephly.net> skribis:

>> 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’.

bug#64358: “guix refresh” chokes on cran.scm

[Ricardo Wurmus]

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

> 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

bug#64358: [PATCH] refresh: Sort update specs by package location.

[Ricardo Wurmus]

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.
---
 guix/scripts/refresh.scm | 33 ++++++++++++++++++++++-----------
 1 file changed, 22 insertions(+), 11 deletions(-)

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

bug#64358: [PATCH] refresh: Sort update specs by package location.

[Ludovic Courtès]

Hi,

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/ ?

Otherwise LGTM, thanks!

Ludo’.

bug#64358: [PATCH] refresh: Sort update specs by package location.

[Ricardo Wurmus]

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

> 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

bug#64358: [PATCH] refresh: Sort update specs by package location.

[Ludovic Courtès]

Hallo,

Ricardo Wurmus <rekado@elephly.net> skribis:

> 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.

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

Awesome, thanks!

Ludo’.

bug#64358: “guix refresh” chokes on cran.scm

[Ricardo Wurmus]

Fixed in commit b43841c124d15eaecc41b3928f08a26dbd5c653a
-- 
Ricardo