All updaters are broken

All updaters are broken

[Ricardo Wurmus]

Hi Guix,

Running “guix refresh -t cran -u” I get this error:

--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 7f0061a232e0>)
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) 7f0061a28c80>)))
In guix/ui.scm:
   2276:7 14 (run-guix . _)
  2239: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:
   661:37 10 (thunk)
In guix/monads.scm:
    576:2  9 (run-with-store #<store-connection 256.99 7f0056ccd550> #<procedure 7f005cae7a60 at ice-9/eval.scm:333:13 (a)> #:guile-for-build …)
In ice-9/eval.scm:
   191:27  8 (_ #(#(#<directory (guix scripts refresh) 7f005edfb1e0> #<procedure 7f005cae7b20 at ice-9/eval.scm:333:13 (a)>) (#<package…> …) …))
In ice-9/boot-9.scm:
    152:2  7 (with-fluid* _ _ _)
    152:2  6 (with-fluid* _ _ _)
    152:2  5 (with-fluid* _ _ _)
In ice-9/eval.scm:
    619:8  4 (_ #(#(#<directory (guix scripts refresh) 7f005edfb1e0> #<store-connection 256.99 7f0056ccd550> (#<<upstream-updater> name:…>) …)))
In srfi/srfi-1.scm:
    634:9  3 (for-each #<procedure 7f0056a6dc00 at ice-9/eval.scm:333:13 (a)> (#<package node-openzwave-shared@1.7.2 gnu/packages/zwave.sc…> …))
In ice-9/eval.scm:
   173:47  2 (_ #(#(#<directory (guix scripts refresh) 7f005edfb1e0> #<store-connection 256.99 7f0056ccd550> (#<<upstream-updater> nam…>) …) …))
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:
In procedure update-spec-package: Wrong type argument: #<package node-openzwave-shared@1.7.2 gnu/packages/zwave.scm:94 7f0051378a50>
--8<---------------cut here---------------end--------------->8---

There’s nothing wrong with the package “node-openzwave-shared”.  The
problem is that for some reason the updater now gets a <package> value
instead of an <update-spec>.

Commit 8aeccc6240ec45f0bc7bed655e0c8149ae4253eb seems like the problem
here.  Hartmut, can you please fix this?  Otherwise I’d like to revert
this and related commits ASAP.

I haven’t seen these patches on the mailing list or the issue tracker.
Have they been reviewed?  These changes also break existing tests, which
should have been run before merging.

Let us please do better and avoid breaking the master branch by
discussing changes and running tests.

Thanks!

-- 
Ricardo

Re: All updaters are broken

[Ricardo Wurmus]

Ricardo Wurmus <rekado@elephly.net> writes:

> I haven’t seen these patches on the mailing list or the issue tracker.
> Have they been reviewed?  These changes also break existing tests, which
> should have been run before merging.
>
> Let us please do better and avoid breaking the master branch by
> discussing changes and running tests.

My bad: this *was* in fact discussed here:

    https://issues.guix.gnu.org/57460

So… if the changes are here to stay we’ll need to come up with fixes to
the tests and the refresh mechanism.

-- 
Ricardo

Re: All updaters are broken

[Hartmut Goebel]

Hi Ricardo,

my fault, I missed running the tests again after the latest changes.

I'll work on fixing this tomorrow (Monday).

-- 
Regards
Hartmut Goebel

| Hartmut Goebel          | h.goebel@crazy-compilers.com               |
| www.crazy-compilers.com | compilers which you thought are impossible |

Re: All updaters are broken

[Hartmut Goebel]

Hi Ricardo,

I managed working on this this evening already.

Am 31.12.22 um 15:27 schrieb Ricardo Wurmus:
> Commit 8aeccc6240ec45f0bc7bed655e0c8149ae4253eb seems like the problem
> here.  Hartmut, can you please fix this?  Otherwise I’d like to revert
> this and related commits ASAP.

I fixed he tests and pushed as d7a9d72bb02a2a3b1a99183655bf878547116032.

Regarding the command "guix refresh": According to my tests only 
invocations not providing a package name failed (see below). Anyhow I 
did not manage fixing this:

options->update-specs need to return update-specs in all cases, and 
currently returns packages if no packages have been named on the command 
line.

guix/scripts/refresh.scm (options->update-specs), lines 252ff:

   (if (assoc-ref opts 'recursive?)
       (mlet %store-monad ((edges (node-edges %bag-node-type
                                              (all-packages))))
         (return (node-transitive-edges packages edges)))
       (with-monad %store-monad
         (return packages))))

Any hints?

These invocations fail:

/pre-inst-env guix refresh -t crane -u
/pre-inst-env guix refresh -t hexpm -u
/pre-inst-env guix refresh -t hexpm

All these invocations pass:

./pre-inst-env guix refresh --list-updaters
./pre-inst-env guix refresh -u python-flask
./pre-inst-env guix refresh -u python-flask=2.2.1
./pre-inst-env guix refresh --list-transitive python-flask
./pre-inst-env guix refresh --list-dependent python-flask
./pre-inst-env guix refresh -l python-flask

Untested:

--recursive — did nothing?
--select

-- 
Regards
Hartmut Goebel

| Hartmut Goebel          | h.goebel@crazy-compilers.com               |
| www.crazy-compilers.com | compilers which you thought are impossible |
H

Re: All updaters are broken

[Ricardo Wurmus]

Hartmut Goebel <h.goebel@crazy-compilers.com> writes:

> I managed working on this this evening already.

Thank you for taking the time!

> Am 31.12.22 um 15:27 schrieb Ricardo Wurmus:
>> Commit 8aeccc6240ec45f0bc7bed655e0c8149ae4253eb seems like the problem
>> here.  Hartmut, can you please fix this?  Otherwise I’d like to revert
>> this and related commits ASAP.
>
> I fixed he tests and pushed as d7a9d72bb02a2a3b1a99183655bf878547116032.
>
> Regarding the command "guix refresh": According to my tests only
> invocations not providing a package name failed (see below). Anyhow I
> did not manage fixing this:
>
> options->update-specs need to return update-specs in all cases, and
> currently returns packages if no packages have been named on the
> command line.

It also returns packages when a manifest is provided, or when an
expression is evaluated.  All the internal procedures also assume that
we’re dealing with packages here.  The only special case is in
“args-packages” where “update-specification->update-spec” is used.

We’ll either have to wrap all package values in (update-spec pkg #false)
only to unpack that moments later to get at the wrapped package value or
dispatch on the return type of options->update-specs (possibly after
renaming it to make clear that this returns <package> or <update-spec>
values), treating <update-spec> values differently from <package>
values.

It’s a bit messy because options->update-specs is poorly typed now.  We
could also have it return a compound value (or a union type) with a list
of <update-spec> values and a list of <package> values, and process the
components separately.

-- 
Ricardo

Re: All updaters are broken

[Ricardo Wurmus]

[-- Attachment #1: Type: text/plain, Size: 539 bytes --]


Ricardo Wurmus <rekado@elephly.net> writes:

> It’s a bit messy because options->update-specs is poorly typed now.  We
> could also have it return a compound value (or a union type) with a list
> of <update-spec> values and a list of <package> values, and process the
> components separately.

Attached is a crude implementation of that.  I just consed the lists
together instead of returning multiple values, because the compound
value is to be used inside the store monad where we can’t easily access
multiple values.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: refresh.diff --]
[-- Type: text/x-patch, Size: 7356 bytes --]

diff --git a/guix/scripts/refresh.scm b/guix/scripts/refresh.scm
index e0b94ce48d..b2e9e81299 100644
--- a/guix/scripts/refresh.scm
+++ b/guix/scripts/refresh.scm
@@ -183,9 +183,9 @@ (define (show-help)
   (newline)
   (show-bug-report-information))
 
-(define (options->update-specs opts)
-  "Return the list of packages requested by OPTS, honoring options like
-'--recursive'."
+(define (options->packages+update-specs opts)
+  "Return the list of packages and update specs requested by OPTS, honoring
+options like '--recursive'."
   (define core-package?
     (let* ((input->package (match-lambda
                              ((name (? package? package) _ ...) package)
@@ -220,15 +220,15 @@ (define (keep-newest package lst)
         (_
          (cons package lst)))))
 
-  (define args-packages
+  (define args-packages+update-specs
     ;; Packages explicitly passed as command-line arguments.
     (match (filter-map (match-lambda
                          (('argument . spec)
                           ;; Take either the specified version or the
                           ;; latest one.
-                          (update-specification->update-spec spec))
+                          (cons '() (update-specification->update-spec spec)))
                          (('expression . exp)
-                          (read/eval-package-expression exp))
+                          (cons (read/eval-package-expression exp) '()))
                          (_ #f))
                        opts)
       (()                                         ;default to all packages
@@ -236,25 +236,29 @@ (define args-packages
                         ('core core-package?)
                         ('non-core (negate core-package?))
                         (_ (const #t)))))
-         (fold-packages (lambda (package result)
-                          (if (select? package)
-                              (keep-newest package result)
-                              result))
-                        '())))
+         (cons (fold-packages (lambda (package result)
+                                (if (select? package)
+                                    (keep-newest package result)
+                                    result))
+                              '())
+               '())))
       (some                                       ;user-specified packages
        some)))
 
-  (define packages
+  (define packages+update-specs
     (match (assoc-ref opts 'manifest)
-      (#f args-packages)
-      ((? string? file) (packages-from-manifest file))))
+      (#f args-packages+update-specs)
+      ((? string? file) (cons (packages-from-manifest file) '()))))
 
   (if (assoc-ref opts 'recursive?)
-      (mlet %store-monad ((edges (node-edges %bag-node-type
-                                             (all-packages))))
-        (return (node-transitive-edges packages edges)))
+      (match packages+update-specs
+        ((packages . update-specs)
+         (mlet %store-monad ((edges (node-edges %bag-node-type
+                                                (all-packages))))
+           (return (values (node-transitive-edges packages edges)
+                           update-specs)))))
       (with-monad %store-monad
-        (return packages))))
+        (return packages+update-specs))))
 
 \f
 ;;;
@@ -561,35 +565,47 @@ (define (options->updaters opts)
     (with-error-handling
       (with-store store
         (run-with-store store
-          (mlet %store-monad ((update-specs (options->update-specs opts)))
-            (cond
-             (list-dependent?
-              (list-dependents (map update-spec-package update-specs)))
-             (list-transitive?
-              (list-transitive (map update-spec-package update-specs)))
-             (update?
-              (parameterize ((%openpgp-key-server
-                              (or (assoc-ref opts 'key-server)
-                                  (%openpgp-key-server)))
-                             (%gpg-command
-                              (or (assoc-ref opts 'gpg-command)
-                                  (%gpg-command)))
-                             (current-keyring
-                              (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-download key-download
-                                   #:warn? warn?))
-                 update-specs)
-                (return #t)))
-             (else
-              (for-each (cut check-for-package-update <> updaters
-                             #:warn? warn?)
-                        (map update-spec-package update-specs))
-              (return #t)))))))))
+          (mlet %store-monad ((packages+update-specs (options->packages+update-specs opts)))
+            (match packages+update-specs
+              ((pkgs . update-specs)
+               (pk 'pkgs (length pkgs) 'specs (length update-specs))
+               (cond
+                (list-dependent?
+                 (list-dependents (append pkgs (map update-spec-package update-specs))))
+                (list-transitive?
+                 (list-transitive (append pkgs (map update-spec-package update-specs))))
+                (update?
+                 (parameterize ((%openpgp-key-server
+                                 (or (assoc-ref opts 'key-server)
+                                     (%openpgp-key-server)))
+                                (%gpg-command
+                                 (or (assoc-ref opts 'gpg-command)
+                                     (%gpg-command)))
+                                (current-keyring
+                                 (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-download key-download
+                                      #:warn? warn?))
+                    update-specs)
+                   (for-each
+                    (lambda (pkg)
+                      (update-package store
+                                      pkg
+                                      #false
+                                      updaters
+                                      #:key-download key-download
+                                      #:warn? warn?))
+                    pkgs)
+                   (return #t)))
+                (else
+                 (for-each (cut check-for-package-update <> updaters
+                                #:warn? warn?)
+                           (map update-spec-package update-specs))
+                 (return #t)))))))))))

[-- Attachment #3: Type: text/plain, Size: 13 bytes --]


-- 
Ricardo

Re: All updaters are broken

[Hartmut Goebel]

[-- Attachment #1: Type: text/plain, Size: 1524 bytes --]

Hello Ricardo,

Am 02.01.23 um 14:16 schrieb Ricardo Wurmus:
> Attached is a crude implementation of that.  I just consed the lists
> together instead of returning multiple values, because the compound
> value is to be used inside the store monad where we can’t easily access
> multiple values.

Thanks for providing the patch. For me this looks huge and hard to 
maintain. I'd rather make "options->update-specs" return update-specs in 
any cases. This adds a small overhead only in the case of --recursive.

Enclosed please find my proposal. WDY?

Tested cases

./pre-inst-env guix refresh --list-updaters
./pre-inst-env guix refresh -u python-flask
./pre-inst-env guix refresh -u python-flask=2.2.1
./pre-inst-env guix refresh python-flask
./pre-inst-env guix refresh python-flask=2.2.1
./pre-inst-env guix refresh --list-transitive python-flask
./pre-inst-env guix refresh --list-dependent python-flask
./pre-inst-env guix refresh -l python-flask

./pre-inst-env guix refresh -t hexpm -u
./pre-inst-env guix refresh -t hexpm
./pre-inst-env guix refresh -t hexpm erlang-relx -u
./pre-inst-env guix refresh -t hexpm erlang-relx

./pre-inst-env guix refresh -e '(@ (gnu packages erlang) erlang-relx)'
./pre-inst-env guix refresh -m test-manifest.scm

./pre-inst-env guix refresh --recursive python-flask
./pre-inst-env guix refresh --select=core

-- 
Regards
Hartmut Goebel

| Hartmut Goebel          | h.goebel@crazy-compilers.com               |
| www.crazy-compilers.com | compilers which you thought are impossible |

[-- Attachment #2: refresh.diff --]
[-- Type: text/x-patch, Size: 3946 bytes --]

diff --git a/guix/scripts/refresh.scm b/guix/scripts/refresh.scm
index e0b94ce48d..f9c4a4c87c 100644
--- a/guix/scripts/refresh.scm
+++ b/guix/scripts/refresh.scm
@@ -184,8 +184,8 @@ specified with `--select'.\n"))
   (show-bug-report-information))
 
 (define (options->update-specs opts)
-  "Return the list of packages requested by OPTS, honoring options like
-'--recursive'."
+  "Return the list of <update-spec> records requested by OPTS, honoring
+options like '--recursive'."
   (define core-package?
     (let* ((input->package (match-lambda
                              ((name (? package? package) _ ...) package)
@@ -220,15 +220,15 @@ update would trigger a complete rebuild."
         (_
          (cons package lst)))))
 
-  (define args-packages
-    ;; Packages explicitly passed as command-line arguments.
+  (define args-packages->update-specs
+    ;; update-specs for packages explicitly passed as command-line arguments.
     (match (filter-map (match-lambda
                          (('argument . spec)
                           ;; Take either the specified version or the
                           ;; latest one.
                           (update-specification->update-spec spec))
                          (('expression . exp)
-                          (read/eval-package-expression exp))
+                          (package->update-spec (read/eval-package-expression exp)))
                          (_ #f))
                        opts)
       (()                                         ;default to all packages
@@ -236,25 +236,29 @@ update would trigger a complete rebuild."
                         ('core core-package?)
                         ('non-core (negate core-package?))
                         (_ (const #t)))))
-         (fold-packages (lambda (package result)
-                          (if (select? package)
-                              (keep-newest package result)
-                              result))
-                        '())))
+         (map package->update-spec
+              (fold-packages (lambda (package result)
+                               (if (select? package)
+                                   (keep-newest package result)
+                                   result))
+                             '()))))
       (some                                       ;user-specified packages
        some)))
 
-  (define packages
+  (define update-specs
     (match (assoc-ref opts 'manifest)
-      (#f args-packages)
-      ((? string? file) (packages-from-manifest file))))
+      (#f args-packages->update-specs)
+      ((? string? file) (map package->update-spec
+                             (packages-from-manifest file)))))
 
   (if (assoc-ref opts 'recursive?)
       (mlet %store-monad ((edges (node-edges %bag-node-type
                                              (all-packages))))
-        (return (node-transitive-edges packages edges)))
+        (return (map package->update-spec
+                     (node-transitive-edges (map update-spec-package update-specs)
+                                            edges))))
       (with-monad %store-monad
-        (return packages))))
+        (return update-specs))))
 
 \f
 ;;;
@@ -268,13 +272,17 @@ update would trigger a complete rebuild."
   (version update-spec-version))
 
 (define (update-specification->update-spec spec)
-  "Given SPEC, a package name like \"guile@2.0=2.0.8\", return a <update>
+  "Given SPEC, a package name like \"guile@2.0=2.0.8\", return a <update-spec>
 record with two fields: the package to upgrade, and the target version."
   (match (string-rindex spec #\=)
     (#f  (update-spec (specification->package spec) #f))
     (idx (update-spec (specification->package (substring spec 0 idx))
                       (substring spec (1+ idx))))))
 
+(define (package->update-spec package)
+  "Given PACKAGE return an <update-spec> record."
+  (update-spec package #f))
+
 \f
 ;;;
 ;;; Updates.

Re: All updaters are broken

[Ricardo Wurmus]

Hartmut Goebel <h.goebel@crazy-compilers.com> writes:

> Hello Ricardo,
>
> Am 02.01.23 um 14:16 schrieb Ricardo Wurmus:
>> Attached is a crude implementation of that.  I just consed the lists
>> together instead of returning multiple values, because the compound
>> value is to be used inside the store monad where we can’t easily access
>> multiple values.
>
> Thanks for providing the patch. For me this looks huge and hard to
> maintain.

“Hard to maintain”?  How so?

It’s no larger than your patch.  The big diff is due to indentation
changes inside the mlet (as I introduce a “match” to split the compound
value).  The actual difference is pretty small.

> I'd rather make "options->update-specs" return update-specs
> in any cases. This adds a small overhead only in the case of
> --recursive.

I think it’s rather inelegant to wrap packages in a structure that we
throw away moments later because we only do the wrapping so that we can
access the wrapped package again — and we *know* the contents already,
so this whole wrapping and unwrapping is just obscuring the purpose.

The overhead is also substantial, because it happens in the worst case:
when no single package is provided, e.g. with “guix refresh -t cran -u”.
This means that *a lot* of packages may get wrapped up and unwrapped
again.  Certainly not ideal.

-- 
Ricardo

Re: All updaters are broken

[Hartmut Goebel]

[-- Attachment #1: Type: text/plain, Size: 550 bytes --]

Am 02.01.23 um 20:17 schrieb Ricardo Wurmus:
>> Thanks for providing the patch. For me this looks huge and hard to
>> maintain.
> “Hard to maintain”?  How so?

For me this double structure is hard to understand and thus to maintain. 
YMMV.

Anyhow, if you want me to implement a solution bases on your code, I'll 
do so. You are far more experienced in Scheme than I am.

-- 
Regards
Hartmut Goebel

| Hartmut Goebel          |h.goebel@crazy-compilers.com                |
|www.crazy-compilers.com  | compilers which you thought are impossible |

[-- Attachment #2: Type: text/html, Size: 1231 bytes --]

Re: All updaters are broken

[Ricardo Wurmus]

[-- Attachment #1: Type: text/plain, Size: 336 bytes --]

Hi Hartmut,

> Am 02.01.23 um 20:17 schrieb Ricardo Wurmus:
>
>  Thanks for providing the patch. For me this looks huge and hard to
> maintain.
>
> “Hard to maintain”?  How so?
>
> For me this double structure is hard to understand and thus to maintain. YMMV.

Okay.  Here’s something simpler using “partition”:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: refresh-w.diff --]
[-- Type: text/x-patch, Size: 3941 bytes --]

commit 96fb123832b262a3453fe1b7646758da235a343e
Author: Ricardo Wurmus <rekado@elephly.net>
Date:   Tue Jan 3 10:14:52 2023 +0100

    WIP

diff --git a/guix/scripts/refresh.scm b/guix/scripts/refresh.scm
index e0b94ce48d..bbda2df35a 100644
--- a/guix/scripts/refresh.scm
+++ b/guix/scripts/refresh.scm
@@ -183,9 +183,9 @@ (define (show-help)
   (newline)
   (show-bug-report-information))
 
-(define (options->update-specs opts)
-  "Return the list of packages requested by OPTS, honoring options like
-'--recursive'."
+(define (options->packages+update-specs opts)
+  "Return the list of packages and update-specs requested by OPTS, honoring
+options like '--recursive'."
   (define core-package?
     (let* ((input->package (match-lambda
                              ((name (? package? package) _ ...) package)
@@ -220,7 +220,7 @@ (define (keep-newest package lst)
         (_
          (cons package lst)))))
 
-  (define args-packages
+  (define args-packages+update-specs
     ;; Packages explicitly passed as command-line arguments.
     (match (filter-map (match-lambda
                          (('argument . spec)
@@ -244,17 +244,18 @@ (define args-packages
       (some                                       ;user-specified packages
        some)))
 
-  (define packages
+  (define packages+update-specs
     (match (assoc-ref opts 'manifest)
-      (#f args-packages)
+      (#f args-packages+update-specs)
       ((? string? file) (packages-from-manifest file))))
 
   (if (assoc-ref opts 'recursive?)
+      (let ((packages update-specs (partition package? packages+update-specs)))
         (mlet %store-monad ((edges (node-edges %bag-node-type
                                                (all-packages))))
-        (return (node-transitive-edges packages edges)))
+          (return (append (node-transitive-edges packages edges) update-specs))))
       (with-monad %store-monad
-        (return packages))))
+        (return packages+update-specs))))
 
 \f
 ;;;
@@ -561,12 +562,13 @@ (define (options->updaters opts)
     (with-error-handling
       (with-store store
         (run-with-store store
-          (mlet %store-monad ((update-specs (options->update-specs opts)))
+          (mlet %store-monad ((packages+update-specs (options->packages+update-specs opts)))
+            (let ((packages update-specs (partition package? packages+update-specs)))
               (cond
                (list-dependent?
-              (list-dependents (map update-spec-package update-specs)))
+                (list-dependents (append packages (map update-spec-package update-specs))))
                (list-transitive?
-              (list-transitive (map update-spec-package update-specs)))
+                (list-transitive (append packages (map update-spec-package update-specs))))
                (update?
                 (parameterize ((%openpgp-key-server
                                 (or (assoc-ref opts 'key-server)
@@ -587,9 +589,18 @@ (define (options->updaters opts)
                                      #:key-download key-download
                                      #:warn? warn?))
                    update-specs)
+                  (for-each
+                   (lambda (package)
+                     (update-package store
+                                     package
+                                     #false
+                                     updaters
+                                     #:key-download key-download
+                                     #:warn? warn?))
+                   packages)
                   (return #t)))
                (else
                 (for-each (cut check-for-package-update <> updaters
                                #:warn? warn?)
-                        (map update-spec-package update-specs))
-              (return #t)))))))))
+                          (append packages (map update-spec-package update-specs)))
+                (return #t))))))))))

[-- Attachment #3: Type: text/plain, Size: 88 bytes --]


(This patch ignores white-space.) 

Here’s the patch with white-space changes:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: refresh.diff --]
[-- Type: text/x-patch, Size: 6087 bytes --]

commit 96fb123832b262a3453fe1b7646758da235a343e
Author: Ricardo Wurmus <rekado@elephly.net>
Date:   Tue Jan 3 10:14:52 2023 +0100

    WIP

diff --git a/guix/scripts/refresh.scm b/guix/scripts/refresh.scm
index e0b94ce48d..bbda2df35a 100644
--- a/guix/scripts/refresh.scm
+++ b/guix/scripts/refresh.scm
@@ -183,9 +183,9 @@ (define (show-help)
   (newline)
   (show-bug-report-information))
 
-(define (options->update-specs opts)
-  "Return the list of packages requested by OPTS, honoring options like
-'--recursive'."
+(define (options->packages+update-specs opts)
+  "Return the list of packages and update-specs requested by OPTS, honoring
+options like '--recursive'."
   (define core-package?
     (let* ((input->package (match-lambda
                              ((name (? package? package) _ ...) package)
@@ -220,7 +220,7 @@ (define (keep-newest package lst)
         (_
          (cons package lst)))))
 
-  (define args-packages
+  (define args-packages+update-specs
     ;; Packages explicitly passed as command-line arguments.
     (match (filter-map (match-lambda
                          (('argument . spec)
@@ -244,17 +244,18 @@ (define args-packages
       (some                                       ;user-specified packages
        some)))
 
-  (define packages
+  (define packages+update-specs
     (match (assoc-ref opts 'manifest)
-      (#f args-packages)
+      (#f args-packages+update-specs)
       ((? string? file) (packages-from-manifest file))))
 
   (if (assoc-ref opts 'recursive?)
-      (mlet %store-monad ((edges (node-edges %bag-node-type
-                                             (all-packages))))
-        (return (node-transitive-edges packages edges)))
+      (let ((packages update-specs (partition package? packages+update-specs)))
+        (mlet %store-monad ((edges (node-edges %bag-node-type
+                                               (all-packages))))
+          (return (append (node-transitive-edges packages edges) update-specs))))
       (with-monad %store-monad
-        (return packages))))
+        (return packages+update-specs))))
 
 \f
 ;;;
@@ -561,35 +562,45 @@ (define (options->updaters opts)
     (with-error-handling
       (with-store store
         (run-with-store store
-          (mlet %store-monad ((update-specs (options->update-specs opts)))
-            (cond
-             (list-dependent?
-              (list-dependents (map update-spec-package update-specs)))
-             (list-transitive?
-              (list-transitive (map update-spec-package update-specs)))
-             (update?
-              (parameterize ((%openpgp-key-server
-                              (or (assoc-ref opts 'key-server)
-                                  (%openpgp-key-server)))
-                             (%gpg-command
-                              (or (assoc-ref opts 'gpg-command)
-                                  (%gpg-command)))
-                             (current-keyring
-                              (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-download key-download
-                                   #:warn? warn?))
-                 update-specs)
-                (return #t)))
-             (else
-              (for-each (cut check-for-package-update <> updaters
-                             #:warn? warn?)
-                        (map update-spec-package update-specs))
-              (return #t)))))))))
+          (mlet %store-monad ((packages+update-specs (options->packages+update-specs opts)))
+            (let ((packages update-specs (partition package? packages+update-specs)))
+              (cond
+               (list-dependent?
+                (list-dependents (append packages (map update-spec-package update-specs))))
+               (list-transitive?
+                (list-transitive (append packages (map update-spec-package update-specs))))
+               (update?
+                (parameterize ((%openpgp-key-server
+                                (or (assoc-ref opts 'key-server)
+                                    (%openpgp-key-server)))
+                               (%gpg-command
+                                (or (assoc-ref opts 'gpg-command)
+                                    (%gpg-command)))
+                               (current-keyring
+                                (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-download key-download
+                                     #:warn? warn?))
+                   update-specs)
+                  (for-each
+                   (lambda (package)
+                     (update-package store
+                                     package
+                                     #false
+                                     updaters
+                                     #:key-download key-download
+                                     #:warn? warn?))
+                   packages)
+                  (return #t)))
+               (else
+                (for-each (cut check-for-package-update <> updaters
+                               #:warn? warn?)
+                          (append packages (map update-spec-package update-specs)))
+                (return #t))))))))))

[-- Attachment #5: Type: text/plain, Size: 379 bytes --]


I can’t say whether this is better than your proposal as I’m biased, so
maybe let’s get someone else’s opinion on this before merging either of
them.  I have a slight preference for this approach over wrapping and
unwrapping.  Ideally we would avoid mixing up packages and update specs
in the first place, but that’s not easily accomplished now.

-- 
Ricardo

Re: All updaters are broken

[Ludovic Courtès]

[-- Attachment #1: Type: text/plain, Size: 2201 bytes --]

Hello!

Ricardo Wurmus <rekado@elephly.net> skribis:

> Okay.  Here’s something simpler using “partition”:
>
> commit 96fb123832b262a3453fe1b7646758da235a343e
> Author: Ricardo Wurmus <rekado@elephly.net>
> Date:   Tue Jan 3 10:14:52 2023 +0100
>
>     WIP
>
> diff --git a/guix/scripts/refresh.scm b/guix/scripts/refresh.scm
> index e0b94ce48d..bbda2df35a 100644
> --- a/guix/scripts/refresh.scm
> +++ b/guix/scripts/refresh.scm
> @@ -183,9 +183,9 @@ (define (show-help)
>    (newline)
>    (show-bug-report-information))
>  
> -(define (options->update-specs opts)
> -  "Return the list of packages requested by OPTS, honoring options like
> -'--recursive'."
> +(define (options->packages+update-specs opts)
> +  "Return the list of packages and update-specs requested by OPTS, honoring
> +options like '--recursive'."
>    (define core-package?
>      (let* ((input->package (match-lambda
>                               ((name (? package? package) _ ...) package)
> @@ -220,7 +220,7 @@ (define (keep-newest package lst)
>          (_
>           (cons package lst)))))
>  
> -  (define args-packages
> +  (define args-packages+update-specs
>      ;; Packages explicitly passed as command-line arguments.
>      (match (filter-map (match-lambda
>                           (('argument . spec)
> @@ -244,17 +244,18 @@ (define args-packages
>        (some                                       ;user-specified packages
>         some)))
>  
> -  (define packages
> +  (define packages+update-specs
>      (match (assoc-ref opts 'manifest)
> -      (#f args-packages)
> +      (#f args-packages+update-specs)
>        ((? string? file) (packages-from-manifest file))))

I tried something different and perhaps simpler: making sure
‘options->update-specs’ always returns a list of <update-spec>, as the
name implies, and does the right thing with manifests, -r, and -e.
(Part of the patch moves the <update-spec> definition before its first
use.)

WDYT?

This is on top of <https://issues.guix.gnu.org/60368>, which also
clarified a couple of things.

After that I’d like to thing about tests for the CLI.

Thanks,
Ludo’.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-patch, Size: 4772 bytes --]

diff --git a/guix/scripts/refresh.scm b/guix/scripts/refresh.scm
index 65c3ce9c16..9438df870d 100644
--- a/guix/scripts/refresh.scm
+++ b/guix/scripts/refresh.scm
@@ -183,9 +183,31 @@ (define (show-help)
   (newline)
   (show-bug-report-information))
 
+\f
+;;;
+;;; Utilities.
+;;;
+
+(define-record-type <update-spec>
+  (%update-spec package version)
+  update?
+  (package update-spec-package)
+  (version update-spec-version))
+
+(define* (update-spec package #:optional version)
+  (%update-spec package version))
+
+(define (update-specification->update-spec spec)
+  "Given SPEC, a package name like \"guile@2.0=2.0.8\", return a <update>
+record with two fields: the package to upgrade, and the target version."
+  (match (string-rindex spec #\=)
+    (#f  (update-spec (specification->package spec) #f))
+    (idx (update-spec (specification->package (substring spec 0 idx))
+                      (substring spec (1+ idx))))))
+
 (define (options->update-specs opts)
-  "Return the list of packages requested by OPTS, honoring options like
-'--recursive'."
+  "Return the list of <update-spec> records requested by OPTS, honoring
+options like '--recursive'."
   (define core-package?
     (let* ((input->package (match-lambda
                              ((name (? package? package) _ ...) package)
@@ -220,60 +242,43 @@ (define (keep-newest package lst)
         (_
          (cons package lst)))))
 
-  (define args-packages
-    ;; Packages explicitly passed as command-line arguments.
-    (match (filter-map (match-lambda
+  (define update-specs
+    ;; Update specs explicitly passed as command-line arguments.
+    (match (append-map (match-lambda
                          (('argument . spec)
                           ;; Take either the specified version or the
                           ;; latest one.
-                          (update-specification->update-spec spec))
+                          (list (update-specification->update-spec spec)))
                          (('expression . exp)
-                          (read/eval-package-expression exp))
-                         (_ #f))
+                          (list (update-spec (read/eval-package-expression exp))))
+                         (('manifest . manifest)
+                          (map update-spec (packages-from-manifest manifest)))
+                         (_
+                          '()))
                        opts)
       (()                                         ;default to all packages
        (let ((select? (match (assoc-ref opts 'select)
                         ('core core-package?)
                         ('non-core (negate core-package?))
                         (_ (const #t)))))
-         (fold-packages (lambda (package result)
-                          (if (select? package)
-                              (keep-newest package result)
-                              result))
-                        '())))
+         (map update-spec
+              (fold-packages (lambda (package result)
+                               (if (select? package)
+                                   (keep-newest package result)
+                                   result))
+                             '()))))
       (some                                       ;user-specified packages
        some)))
 
-  (define packages
-    (match (assoc-ref opts 'manifest)
-      (#f args-packages)
-      ((? string? file) (packages-from-manifest file))))
-
   (if (assoc-ref opts 'recursive?)
-      (mlet %store-monad ((edges (node-edges %bag-node-type
-                                             (all-packages))))
-        (return (node-transitive-edges packages edges)))
+      (mlet* %store-monad ((edges (node-edges %bag-node-type (all-packages)))
+                           (packages -> (node-transitive-edges
+                                         (map update-spec-package update-specs)
+                                         edges)))
+        ;; FIXME: We're losing the 'version' field of each update spec.
+        (return (map update-spec packages)))
       (with-monad %store-monad
-        (return packages))))
-
-\f
-;;;
-;;; Utilities.
-;;;
-
-(define-record-type <update-spec>
-  (update-spec package version)
-  update?
-  (package update-spec-package)
-  (version update-spec-version))
-
-(define (update-specification->update-spec spec)
-  "Given SPEC, a package name like \"guile@2.0=2.0.8\", return a <update>
-record with two fields: the package to upgrade, and the target version."
-  (match (string-rindex spec #\=)
-    (#f  (update-spec (specification->package spec) #f))
-    (idx (update-spec (specification->package (substring spec 0 idx))
-                      (substring spec (1+ idx))))))
+        (return update-specs))))
 
 \f
 ;;;

Re: All updaters are broken

[Hartmut Goebel]

Am 03.01.23 um 10:49 schrieb Ludovic Courtès:
> I tried something different and perhaps simpler: making sure
> ‘options->update-specs’ always returns a list of <update-spec>, as the
> name implies, and does the right thing with manifests, -r, and -e.
> (Part of the patch moves the <update-spec> definition before its first
> use.)
>
> WDYT?

I'm biased, as this look much like my last proposal :-) plus some good 
simplifications which I would never been able of. Esp. getting rid of 
this (package) function is good, as this function made the control-flow 
obscure,

FMPOV please go ahead and merge.

-- 
Regards
Hartmut Goebel

| Hartmut Goebel          | h.goebel@crazy-compilers.com               |
| www.crazy-compilers.com | compilers which you thought are impossible |

Re: All updaters are broken

[Ludovic Courtès]

Hi!

Hartmut Goebel <h.goebel@crazy-compilers.com> skribis:

> Am 03.01.23 um 10:49 schrieb Ludovic Courtès:
>> I tried something different and perhaps simpler: making sure
>> ‘options->update-specs’ always returns a list of <update-spec>, as the
>> name implies, and does the right thing with manifests, -r, and -e.
>> (Part of the patch moves the <update-spec> definition before its first
>> use.)
>>
>> WDYT?
>
> I'm biased, as this look much like my last proposal :-) plus some good
> simplifications which I would never been able of. Esp. getting rid of
> this (package) function is good, as this function made the
> control-flow obscure,
>
> FMPOV please go ahead and merge.

Ricardo and I discussed it earlier today on IRC; I went ahead and pushed
it as 473692b812b4ab4267d9bddad0fb27787d2112ff and then submitted tests
for the CLI that would have caught these issues:

  https://issues.guix.gnu.org/60520

Ludo’.