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

* 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(-)

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

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

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

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

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

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