[bug#34040] Suggest input changes when updating packages

[bug#34040] Suggest input changes when updating packages

[Ricardo Wurmus]

Hi Guix,

Here are two patches: the first changes (guix upstream) and (guix
scripts refresh) to support reporting of input changes when updating a
package.  The second lets the CRAN and Bioconductor importers use these
features.

--
Ricardo

[bug#34040] [PATCH 1/2] refresh: Suggest input changes when updating.

[Ricardo Wurmus]

* guix/upstream.scm (<upstream-source>)[input-changes]: New field.
(<upstream-input-change>): New record.
(upstream-input-change?, upstream-input-change-name,
upstream-input-change-type, upstream-input-change-action, changed-inputs): New
procedures.
(package-update): Pass along input changes.
* guix/script/refresh.scm (update-package): Process input changes.
---
 guix/scripts/refresh.scm | 23 +++++++++-
 guix/upstream.scm        | 90 ++++++++++++++++++++++++++++++++++++----
 2 files changed, 104 insertions(+), 9 deletions(-)

diff --git a/guix/scripts/refresh.scm b/guix/scripts/refresh.scm
index 003c915da..15cf385fb 100644
--- a/guix/scripts/refresh.scm
+++ b/guix/scripts/refresh.scm
@@ -6,6 +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>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -224,7 +225,7 @@ KEY-DOWNLOAD specifies a download policy for missing OpenPGP keys; allowed
 values: 'interactive' (default), 'always', and 'never'.  When WARN? is true,
 warn about packages that have no matching updater."
   (if (lookup-updater package updaters)
-      (let-values (((version tarball)
+      (let-values (((version tarball changes)
                     (package-update store package updaters
                                     #:key-download key-download))
                    ((loc)
@@ -238,6 +239,26 @@ warn about packages that have no matching updater."
                         (location->string loc)
                         (package-name package)
                         (package-version package) version)
+                (for-each
+                 (lambda (change)
+                   (format (current-error-port)
+                           (match (list (upstream-input-change-action change)
+                                        (upstream-input-change-type change))
+                             (('add 'regular)
+                              (G_ "~a: consider adding this input: ~a~%"))
+                             (('add 'native)
+                              (G_ "~a: consider adding this native input: ~a~%"))
+                             (('add 'propagated)
+                              (G_ "~a: consider adding this propagated input: ~a~%"))
+                             (('remove 'regular)
+                              (G_ "~a: consider removing this input: ~a~%"))
+                             (('remove 'native)
+                              (G_ "~a: consider removing this native input: ~a~%"))
+                             (('remove 'propagated)
+                              (G_ "~a: consider removing this propagated input: ~a~%")))
+                           (package-name package)
+                           (upstream-input-change-name change)))
+                 (changes))
                 (let ((hash (call-with-input-file tarball
                               port-sha256)))
                   (update-package-source package version hash)))
diff --git a/guix/upstream.scm b/guix/upstream.scm
index 9e1056f7a..880cb9094 100644
--- a/guix/upstream.scm
+++ b/guix/upstream.scm
@@ -1,6 +1,7 @@
 ;;; GNU Guix --- Functional package management for GNU
 ;;; Copyright © 2010, 2011, 2012, 2013, 2014, 2015, 2016, 2017, 2018 Ludovic Courtès <ludo@gnu.org>
 ;;; Copyright © 2015 Alex Kost <alezost@gmail.com>
+;;; Copyright © 2019 Ricardo Wurmus <rekado@elephly.net>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -45,6 +46,7 @@
             upstream-source-urls
             upstream-source-signature-urls
             upstream-source-archive-types
+            upstream-source-input-changes
 
             url-prefix-predicate
             coalesce-sources
@@ -56,6 +58,12 @@
             upstream-updater-predicate
             upstream-updater-latest
 
+            upstream-input-change?
+            upstream-input-change-name
+            upstream-input-change-type
+            upstream-input-change-action
+            changed-inputs
+
             %updaters
             lookup-updater
 
@@ -82,7 +90,73 @@
   (version        upstream-source-version)        ;string
   (urls           upstream-source-urls)           ;list of strings
   (signature-urls upstream-source-signature-urls  ;#f | list of strings
-                  (default #f)))
+                  (default #f))
+  (input-changes  upstream-source-input-changes
+                  (default '()) (thunked)))
+
+;; Representation of an upstream input change.
+(define-record-type* <upstream-input-change>
+  upstream-input-change make-upstream-input-change
+  upstream-input-change?
+  (name    upstream-input-change-name)    ;string
+  (type    upstream-input-change-type)    ;symbol: regular | native | propagated
+  (action  upstream-input-change-action)) ;symbol: add | remove
+
+(define (changed-inputs package package-sexp)
+  "Return a list of input changes for PACKAGE based on the newly imported
+S-expression PACKAGE-SEXP."
+  (match package-sexp
+    ((and expr ('package fields ...))
+     (let* ((input->name (match-lambda ((name pkg . out) name)))
+            (new-regular
+             (match expr
+               ((path *** ('inputs
+                           ('quasiquote ((label ('unquote sym)) ...)))) label)
+               (_ '())))
+            (new-native
+             (match expr
+               ((path *** ('native-inputs
+                           ('quasiquote ((label ('unquote sym)) ...)))) label)
+               (_ '())))
+            (new-propagated
+             (match expr
+               ((path *** ('propagated-inputs
+                           ('quasiquote ((label ('unquote sym)) ...)))) label)
+               (_ '())))
+            (current-regular
+             (map input->name (package-inputs package)))
+            (current-native
+             (map input->name (package-native-inputs package)))
+            (current-propagated
+             (map input->name (package-propagated-inputs package))))
+       (append-map
+        (match-lambda
+          ((action type names)
+           (map (lambda (name)
+                  (upstream-input-change
+                   (name name)
+                   (type type)
+                   (action action)))
+                names)))
+        `((add regular
+           ,(lset-difference equal?
+                             new-regular current-regular))
+          (remove regular
+           ,(lset-difference equal?
+                             current-regular new-regular))
+          (add native
+           ,(lset-difference equal?
+                             new-native current-native))
+          (remove native
+           ,(lset-difference equal?
+                             current-native new-native))
+          (add propagated
+           ,(lset-difference equal?
+                             new-propagated current-propagated))
+          (remove propagated
+           ,(lset-difference equal?
+                             current-propagated new-propagated))))))
+    (_ '())))
 
 (define (url-prefix-predicate prefix)
   "Return a predicate that returns true when passed a package where one of its
@@ -268,12 +342,12 @@ values: the item from LST1 and the item from LST2 that match PRED."
 
 (define* (package-update store package updaters
                          #:key (key-download 'interactive))
-  "Return the new version and the file name of the new version tarball for
-PACKAGE, or #f and #f when PACKAGE is up-to-date.  KEY-DOWNLOAD specifies a
-download policy for missing OpenPGP keys; allowed values: 'always', 'never',
-and 'interactive' (default)."
+  "Return the new version, the file name of the new version tarball and input
+changes for PACKAGE, or #f and #f when PACKAGE is up-to-date.  KEY-DOWNLOAD
+specifies a download policy for missing OpenPGP keys; allowed values:
+'always', 'never', and 'interactive' (default)."
   (match (package-latest-release* package updaters)
-    (($ <upstream-source> _ version urls signature-urls)
+    (($ <upstream-source> _ version urls signature-urls changes)
      (let*-values (((name)
                     (package-name package))
                    ((archive-type)
@@ -299,9 +373,9 @@ and 'interactive' (default)."
                            (or signature-urls (circular-list #f)))))
        (let ((tarball (download-tarball store url signature-url
                                         #:key-download key-download)))
-         (values version tarball))))
+         (values version tarball changes))))
     (#f
-     (values #f #f))))
+     (values #f #f #f))))
 
 (define (update-package-source package version hash)
   "Modify the source file that defines PACKAGE to refer to VERSION,
-- 
2.20.1

[bug#34040] [PATCH 2/2] import: cran: Suggest input changes.

[Ricardo Wurmus]

* guix/import/cran.scm (latest-cran-release, latest-bioconductor-release):
Return input-changes.
---
 guix/import/cran.scm | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/guix/import/cran.scm b/guix/import/cran.scm
index 15163bd16..b287be694 100644
--- a/guix/import/cran.scm
+++ b/guix/import/cran.scm
@@ -1,5 +1,5 @@
 ;;; GNU Guix --- Functional package management for GNU
-;;; Copyright © 2015, 2016, 2017, 2018 Ricardo Wurmus <rekado@elephly.net>
+;;; Copyright © 2015, 2016, 2017, 2018, 2019 Ricardo Wurmus <rekado@elephly.net>
 ;;; Copyright © 2015, 2016, 2017 Ludovic Courtès <ludo@gnu.org>
 ;;; Copyright © 2017 Mathieu Othacehe <m.othacehe@gmail.com>
 ;;;
@@ -390,11 +390,11 @@ s-expression corresponding to that package, or #f on failure."
              (_ #f)))
           (_ #f)))))
 
-(define (latest-cran-release package)
-  "Return an <upstream-source> for the latest release of PACKAGE."
+(define (latest-cran-release pkg)
+  "Return an <upstream-source> for the latest release of the package PKG."
 
   (define upstream-name
-    (package->upstream-name package))
+    (package->upstream-name pkg))
 
   (define meta
     (fetch-description 'cran upstream-name))
@@ -403,15 +403,18 @@ s-expression corresponding to that package, or #f on failure."
        (let ((version (assoc-ref meta "Version")))
          ;; CRAN does not provide signatures.
          (upstream-source
-          (package (package-name package))
+          (package (package-name pkg))
           (version version)
-          (urls (cran-uri upstream-name version))))))
+          (urls (cran-uri upstream-name version))
+          (input-changes
+           (changed-inputs pkg
+                           (description->package 'cran meta)))))))
 
-(define (latest-bioconductor-release package)
-  "Return an <upstream-source> for the latest release of PACKAGE."
+(define (latest-bioconductor-release pkg)
+  "Return an <upstream-source> for the latest release of the package PKG."
 
   (define upstream-name
-    (package->upstream-name package))
+    (package->upstream-name pkg))
 
   (define version
     (latest-bioconductor-package-version upstream-name))
@@ -419,9 +422,13 @@ s-expression corresponding to that package, or #f on failure."
   (and version
        ;; Bioconductor does not provide signatures.
        (upstream-source
-        (package (package-name package))
+        (package (package-name pkg))
         (version version)
-        (urls (bioconductor-uri upstream-name version)))))
+        (urls (bioconductor-uri upstream-name version))
+        (input-changes
+         (changed-inputs
+          pkg
+          (cran->guix-package upstream-name 'bioconductor))))))
 
 (define (cran-package? package)
   "Return true if PACKAGE is an R package from CRAN."
-- 
2.20.1

[bug#34040] [PATCH 1/2] refresh: Suggest input changes when updating.

[Ludovic Courtès]

Hello,

Ricardo Wurmus <rekado@elephly.net> skribis:

> * guix/upstream.scm (<upstream-source>)[input-changes]: New field.
> (<upstream-input-change>): New record.
> (upstream-input-change?, upstream-input-change-name,
> upstream-input-change-type, upstream-input-change-action, changed-inputs): New
> procedures.
> (package-update): Pass along input changes.
> * guix/script/refresh.scm (update-package): Process input changes.

Really cool!

> +;; Representation of an upstream input change.
> +(define-record-type* <upstream-input-change>
> +  upstream-input-change make-upstream-input-change
> +  upstream-input-change?
> +  (name    upstream-input-change-name)    ;string
> +  (type    upstream-input-change-type)    ;symbol: regular | native | propagated
> +  (action  upstream-input-change-action)) ;symbol: add | remove

Perhaps in some cases in action could be, say, (upgrade "1.2").  Though
that’s of course something we can add later.

>  (define* (package-update store package updaters
>                           #:key (key-download 'interactive))
> -  "Return the new version and the file name of the new version tarball for
> -PACKAGE, or #f and #f when PACKAGE is up-to-date.  KEY-DOWNLOAD specifies a
> -download policy for missing OpenPGP keys; allowed values: 'always', 'never',
> -and 'interactive' (default)."
> +  "Return the new version, the file name of the new version tarball and input
                                                                      ^
Missing comma.  :-)

> +changes for PACKAGE, or #f and #f when PACKAGE is up-to-date.  KEY-DOWNLOAD
                                   ^
Instead of “, or …”:

  ; return #f (three values) when PACKAGE is up-to-date.

Otherwise LGTM, thanks!

Ludo’.

[bug#34040] [PATCH 2/2] import: cran: Suggest input changes.

[Ludovic Courtès]

Ricardo Wurmus <rekado@elephly.net> skribis:

> * guix/import/cran.scm (latest-cran-release, latest-bioconductor-release):
> Return input-changes.

LGTM!

> -(define (latest-cran-release package)
> -  "Return an <upstream-source> for the latest release of PACKAGE."
> +(define (latest-cran-release pkg)

Unless there’s a name clash I think it’s OK to keep ‘package’.

Thanks!

Ludo’.

[bug#34040] [PATCH 2/2] import: cran: Suggest input changes.

[Ricardo Wurmus]

Hi,

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

> Ricardo Wurmus <rekado@elephly.net> skribis:
>
>> * guix/import/cran.scm (latest-cran-release, latest-bioconductor-release):
>> Return input-changes.
>
> LGTM!

Thank you for taking the time to review this!

>> -(define (latest-cran-release package)
>> -  "Return an <upstream-source> for the latest release of PACKAGE."
>> +(define (latest-cran-release pkg)
>
> Unless there’s a name clash I think it’s OK to keep ‘package’.

There is a name clash, which I found very surprising.  Take this procedure:

--8<---------------cut here---------------start------------->8---
(define (latest-cran-release package)
  "Return an <upstream-source> for the latest release of the package PKG."

  (define upstream-name
    (package->upstream-name package))

  (define meta
    (fetch-description 'cran upstream-name))

  (and meta
       (let ((version (assoc-ref meta "Version")))
         ;; CRAN does not provide signatures.
         (upstream-source
          (package (package-name package))
          (version version)
          (urls (cran-uri upstream-name version))
          (input-changes
           (changed-inputs
            package ; <– this is the value of the “package” field,
                    ;    not the value of the procedure argument.
            (description->package 'cran meta)))))))
--8<---------------cut here---------------end--------------->8---

That’s why I renamed the argument to “pkg”.

-- 
Ricardo

[bug#34040] [PATCH 1/2] refresh: Suggest input changes when updating.

[Ludovic Courtès]

Hello,

Other comments/questions came to mind…  :-)

Ricardo Wurmus <rekado@elephly.net> skribis:

>    (version        upstream-source-version)        ;string
>    (urls           upstream-source-urls)           ;list of strings
>    (signature-urls upstream-source-signature-urls  ;#f | list of strings
> -                  (default #f)))
> +                  (default #f))
> +  (input-changes  upstream-source-input-changes
> +                  (default '()) (thunked)))

Any particular reason for making ‘input-changes’ thunked?

This causes a failure in tests/upstream.scm (because two evaluator
procedures are unlikely to be eq?).  I would fix it by removing the
‘thunked’ property but I’m not sure if it’d make sense.

Another thing: “upstream source” designates something
absolute/stateless, but “input changes” designates something
relative/stateful.  So on second thought, I wonder whether
<upstream-source> is the right place for it.

I was thinking that updaters could maybe return two values
(<upstream-source> + list of changed inputs), which would be equivalent
but somewhat clearer.  The downside is that we’d have to change all
updaters to return multiple values.

Alternately, we could change ‘input-changes’ to ‘inputs’, which would be
absolute, not relative, and thus ‘package-update’ would take care of
calling ‘changed-inputs’ etc.

WDYT?

Apologies for not asking these questions earlier!

Thanks,
Ludo’.

[bug#34040] [PATCH 1/2] refresh: Suggest input changes when updating.

[Ricardo Wurmus]

Hi,

sorry for not repyling earlier.  I did not see this message.

> Ricardo Wurmus <rekado@elephly.net> skribis:
>
>>    (version        upstream-source-version)        ;string
>>    (urls           upstream-source-urls)           ;list of strings
>>    (signature-urls upstream-source-signature-urls  ;#f | list of strings
>> -                  (default #f)))
>> +                  (default #f))
>> +  (input-changes  upstream-source-input-changes
>> +                  (default '()) (thunked)))
>
> Any particular reason for making ‘input-changes’ thunked?
>
> This causes a failure in tests/upstream.scm (because two evaluator
> procedures are unlikely to be eq?).  I would fix it by removing the
> ‘thunked’ property but I’m not sure if it’d make sense.

Oh, I’m sorry for breaking this.

My thinking was that this field should be lazily evaluated, because it
is somewhat expensive.  “input-changes” gets its value from calling
“changed-inputs” on a newly imported package.  I wanted to avoid the
import when a user is not interested in the “input-changes” field.

> Another thing: “upstream source” designates something
> absolute/stateless, but “input changes” designates something
> relative/stateful.  So on second thought, I wonder whether
> <upstream-source> is the right place for it.
>
> I was thinking that updaters could maybe return two values
> (<upstream-source> + list of changed inputs), which would be equivalent
> but somewhat clearer.  The downside is that we’d have to change all
> updaters to return multiple values.

This sounds like a good idea and I’m fine with changing all of the
importers.

> Alternately, we could change ‘input-changes’ to ‘inputs’, which would be
> absolute, not relative, and thus ‘package-update’ would take care of
> calling ‘changed-inputs’ etc.

That would also work, but I think I prefer an updater to report changes
rather than a new list of inputs.

--
Ricardo

[bug#34040] [PATCH 1/2] refresh: Suggest input changes when updating.

[Ludovic Courtès]

Hello,

Ricardo Wurmus <rekado@elephly.net> skribis:

>> Ricardo Wurmus <rekado@elephly.net> skribis:
>>
>>>    (version        upstream-source-version)        ;string
>>>    (urls           upstream-source-urls)           ;list of strings
>>>    (signature-urls upstream-source-signature-urls  ;#f | list of strings
>>> -                  (default #f)))
>>> +                  (default #f))
>>> +  (input-changes  upstream-source-input-changes
>>> +                  (default '()) (thunked)))
>>
>> Any particular reason for making ‘input-changes’ thunked?
>>
>> This causes a failure in tests/upstream.scm (because two evaluator
>> procedures are unlikely to be eq?).  I would fix it by removing the
>> ‘thunked’ property but I’m not sure if it’d make sense.
>
> Oh, I’m sorry for breaking this.
>
> My thinking was that this field should be lazily evaluated, because it
> is somewhat expensive.  “input-changes” gets its value from calling
> “changed-inputs” on a newly imported package.  I wanted to avoid the
> import when a user is not interested in the “input-changes” field.

I see, that makes sense.

>> I was thinking that updaters could maybe return two values
>> (<upstream-source> + list of changed inputs), which would be equivalent
>> but somewhat clearer.  The downside is that we’d have to change all
>> updaters to return multiple values.
>
> This sounds like a good idea and I’m fine with changing all of the
> importers.

Cool!

>> Alternately, we could change ‘input-changes’ to ‘inputs’, which would be
>> absolute, not relative, and thus ‘package-update’ would take care of
>> calling ‘changed-inputs’ etc.
>
> That would also work, but I think I prefer an updater to report changes
> rather than a new list of inputs.

OTOH if we return a complete list of inputs, then we don’t have to worry
about the cost of ‘changed-inputs’ since that would only be called on
demand; also there’d be a single call site for ‘changed-inputs’.

Well, you tell us!  :-)

Thanks,
Ludo’.

[bug#34040] [PATCH 1/2] refresh: Suggest input changes when updating.

[Ricardo Wurmus]

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

>>> Alternately, we could change ‘input-changes’ to ‘inputs’, which would be
>>> absolute, not relative, and thus ‘package-update’ would take care of
>>> calling ‘changed-inputs’ etc.
>>
>> That would also work, but I think I prefer an updater to report changes
>> rather than a new list of inputs.
>
> OTOH if we return a complete list of inputs, then we don’t have to worry
> about the cost of ‘changed-inputs’ since that would only be called on
> demand; also there’d be a single call site for ‘changed-inputs’.

The potentially expensive part is to get the inputs, though.  Getting a
list of new inputs means that the importer needs to be run.  For CRAN
packages this requires downloading the tarball and unpacking parts of
it.

--
Ricardo

[bug#34040] [PATCH 1/2] refresh: Suggest input changes when updating.

[Ludovic Courtès]

Halo!

Ricardo Wurmus <rekado@elephly.net> skribis:

> Ludovic Courtès <ludo@gnu.org> writes:
>
>>>> Alternately, we could change ‘input-changes’ to ‘inputs’, which would be
>>>> absolute, not relative, and thus ‘package-update’ would take care of
>>>> calling ‘changed-inputs’ etc.
>>>
>>> That would also work, but I think I prefer an updater to report changes
>>> rather than a new list of inputs.
>>
>> OTOH if we return a complete list of inputs, then we don’t have to worry
>> about the cost of ‘changed-inputs’ since that would only be called on
>> demand; also there’d be a single call site for ‘changed-inputs’.
>
> The potentially expensive part is to get the inputs, though.  Getting a
> list of new inputs means that the importer needs to be run.  For CRAN
> packages this requires downloading the tarball and unpacking parts of
> it.

Oh right.  Then perhaps <upstream-updater> could have a separate
‘inputs’ field, which would be either #f or that procedure to fetch the
list of inputs for the given package.  Does that make sense?

(I’m thinking out loud…)

Ludo’.