Upgrading packages with substitutes only (bug #26608)

Upgrading packages with substitutes only (bug #26608)

[Timothy Sample]

Hi Guix,

I’ve been using GuixSD for a couple of months now, and it is super
cool. Thanks for all your hard work! There is one little thing,
though. :)

There’s a feature request (bug #26608) about adding an
“--only-substitutes” flag to “guix package -u”, which would only upgrade
packages that have substitutes. This sounds very useful to me, so I’ve
written code to implement it. It got a little harrier than I expected,
so I thought I would get some feedback before actually submitting a
patch.

Ludo suggested to check each package using the “substitute-paths” RPC
and filter them accordingly. I did exactly this, but there was a
problem. Packages were being built before my code even ran! It turns out
that calling “package-output” (or “package-derivation”) to get a
package’s output path may result in building the package. It’s the
grafting code that does this − it needs to get a package’s references,
and if the daemon gives it any trouble, it tells the daemon to just
build the package to figure it out. This is a real problem if you are
trying to avoid building. This all happens pretty early when running
“guix package -u”, since it compares output paths when figuring out what
needs upgrading.

Therefore, to make it work, I introduced a flag, “#:fail-on-build?”,
that I threaded through a few functions that lets me try to get a
package’s output path without resorting to building the package. The
change looks like this:

----------

diff --git a/guix/grafts.scm b/guix/grafts.scm
index d6b0e93e8..bcfc289a4 100644
--- a/guix/grafts.scm
+++ b/guix/grafts.scm
@@ -174,7 +174,7 @@ references.  Call REFERENCES to get the list of
references."
                  items))))
     (remove (cut member <> self) refs)))
 
-(define (references-oracle store drv)
+(define (references-oracle store drv fail-on-build?)
   "Return a one-argument procedure that, when passed the file name of
   DRV's
 outputs or their dependencies, returns the list of references of that
 item.
 Use either local info or substitute info; build DRV if no information
 is
@@ -186,6 +186,8 @@ available."
 
   (define (references* items)
     (guard (c ((nix-protocol-error? c)
+               (when fail-on-build?
+                 (raise c))
                ;; As a last resort, build DRV and query the references
                of the
                ;; build result.
 
@@ -298,7 +300,8 @@ derivations to the corresponding set of grafts."
                            #:key
                            (guile (%guile-for-build))
                            (outputs (derivation-output-names drv))
-                           (system (%current-system)))
+                           (system (%current-system))
+                           (fail-on-build? #f))
   "Apply GRAFTS to the OUTPUTS of DRV and all their dependencies,
   recursively.
 That is, if GRAFTS apply only indirectly to DRV, graft the dependencies
 of
 DRV, and graft DRV itself to refer to those grafted dependencies."
@@ -307,7 +310,7 @@ DRV, and graft DRV itself to refer to those grafted
dependencies."
   ;; upfront to have as much parallelism as possible when querying
   substitute
   ;; info or when building DRV.
   (define references
-    (references-oracle store drv))
+    (references-oracle store drv fail-on-build?))
 
   (match (run-with-state
              (cumulative-grafts store drv grafts references
diff --git a/guix/packages.scm b/guix/packages.scm
index f4967f98f..e792c348c 100644
--- a/guix/packages.scm
+++ b/guix/packages.scm
@@ -1123,7 +1123,9 @@ This is an internal procedure."
 
 (define* (package-derivation store package
                              #:optional (system (%current-system))
-                             #:key (graft? (%graft?)))
+                             #:key
+                             (graft? (%graft?))
+                             (fail-on-build? #f))
   "Return the <derivation> object of PACKAGE for SYSTEM."
 
   ;; Compute the derivation and cache the result.  Caching is important
@@ -1144,12 +1146,15 @@ This is an internal procedure."
                      ;; recurses anyway.
                      (graft-derivation store drv grafts
                                        #:system system
-                                       #:guile guile))))
+                                       #:guile guile
+                                       #:fail-on-build?
fail-on-build?))))
                 drv))))
 
 (define* (package-cross-derivation store package target
                                    #:optional (system
                                    (%current-system))
-                                   #:key (graft? (%graft?)))
+                                   #:key
+                                   (graft? (%graft?))
+                                   (fail-on-build? #f))
   "Cross-build PACKAGE for TARGET (a GNU triplet) from host SYSTEM (a
   Guix
 system identifying string)."
   (cached package (list system target graft?)
@@ -1164,7 +1169,8 @@ system identifying string)."
                                      #:system system
                                      #:guile
                                      (package-derivation store
                                      (default-guile)
-                                                         system
#:graft? #f))))
+                                                         system
#:graft? #f)
+                                     #:fail-on-build? fail-on-build?)))
                 drv))))
 
 (define* (package-output store package
diff --git a/guix/scripts/package.scm b/guix/scripts/package.scm
index a6bfb03ae..19b536dd5 100644

----------

If adding this extra parameter isn’t too ugly, then I will put together
two patches: one with this parameter, and another that uses it to
implement “--only-substitutes”. (And maybe a third that fixes a very
minor bug in “guix package”, but that’s another story.) I’m new to
Scheme programming, so this may be an atrocity of some sort that I just
don’t know about; go easy! :)


-- Tim

Re: Upgrading packages with substitutes only (bug #26608)

[Ludovic Courtès]

Hello Timothy,

Timothy Sample <samplet@ngyro.com> skribis:

> I’ve been using GuixSD for a couple of months now, and it is super
> cool. Thanks for all your hard work! There is one little thing,
> though. :)

If it’s just this one thing, that’s okay.  ;-)

> Ludo suggested to check each package using the “substitute-paths” RPC
> and filter them accordingly. I did exactly this, but there was a
> problem. Packages were being built before my code even ran! It turns out
> that calling “package-output” (or “package-derivation”) to get a
> package’s output path may result in building the package. It’s the
> grafting code that does this − it needs to get a package’s references,
> and if the daemon gives it any trouble, it tells the daemon to just
> build the package to figure it out. This is a real problem if you are
> trying to avoid building. This all happens pretty early when running
> “guix package -u”, since it compares output paths when figuring out what
> needs upgrading.

Indeed, I hadn’t thought of this “little detail”!

> Therefore, to make it work, I introduced a flag, “#:fail-on-build?”,
> that I threaded through a few functions that lets me try to get a
> package’s output path without resorting to building the package. The
> change looks like this:

I would probably take a different route: we could check for the
substitutes of the ungrafted packages (hydra.gnu.org does not provide
substitutes for the grafted packages anyway) and decide based on that.

How does that sound?  I haven’t checked the code, let me know if there’s
something else I’m missing.

BTW, should --only-substitutes filter out packages without a substitute,
or should it simply stop and report the list of missing substitutes
(after which the user could use --do-not-upgrade)?

> If adding this extra parameter isn’t too ugly, then I will put together
> two patches: one with this parameter, and another that uses it to
> implement “--only-substitutes”. (And maybe a third that fixes a very
> minor bug in “guix package”, but that’s another story.) I’m new to
> Scheme programming, so this may be an atrocity of some sort that I just
> don’t know about; go easy! :)

Well that’s a very nice dive into this code base in a foreign language.
:-)

Thank you!

Ludo’.

Re: Upgrading packages with substitutes only (bug #26608)

[Ricardo Wurmus]

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

> BTW, should --only-substitutes filter out packages without a substitute,
> or should it simply stop and report the list of missing substitutes
> (after which the user could use --do-not-upgrade)?

In my opinion “--only-substitutes” should stop and report a list.
If it continued without complaining there could be problems:

* partial upgrades could leave the profile in an unusable state

* an attacker could use this to trick a user into thinking that they
  have all available updates

On the other hand, it would make “--only-substitutes” less usable,
because to actually perform work one would have to deal with the failure
case.

I suppose it could download the substitutes but not build a new profile
and report an error at that point.

--
Ricardo

GPG: BCA6 89B6 3655 3801 C3C6  2150 197A 5888 235F ACAC
https://elephly.net

Re: Upgrading packages with substitutes only (bug #26608)

[Leo Famulari]

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

On Sun, Jun 18, 2017 at 11:38:45AM +0200, Ricardo Wurmus wrote:
> 
> Ludovic Courtès <ludo@gnu.org> writes:
> 
> > BTW, should --only-substitutes filter out packages without a substitute,
> > or should it simply stop and report the list of missing substitutes
> > (after which the user could use --do-not-upgrade)?

I like making it return a list on stdout so it can be composed as
suggested.

> In my opinion “--only-substitutes” should stop and report a list.
> If it continued without complaining there could be problems:
> 
> * partial upgrades could leave the profile in an unusable state
> 
> * an attacker could use this to trick a user into thinking that they
>   have all available updates
> 
> On the other hand, it would make “--only-substitutes” less usable,
> because to actually perform work one would have to deal with the failure
> case.
> 
> I suppose it could download the substitutes but not build a new profile
> and report an error at that point.

Perhaps there could be an additional flag --partial-upgrade to make it
build a new profile.

I understand why people want --only-substitutes but I'm a bit wary of it
for the reasons you gave, and I think we should solve their complaint by
improving our build infrastructure.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Upgrading packages with substitutes only (bug #26608)

[Timothy Sample]

Leo Famulari <leo@famulari.name> writes:

> On Sun, Jun 18, 2017 at 11:38:45AM +0200, Ricardo Wurmus wrote:
>> 
>> Ludovic Courtès <ludo@gnu.org> writes:
>> 
>> > BTW, should --only-substitutes filter out packages without a substitute,
>> > or should it simply stop and report the list of missing substitutes
>> > (after which the user could use --do-not-upgrade)?
>
> I like making it return a list on stdout so it can be composed as
> suggested.

While the user experience would suffer a bit, it’s no real hardship to
type

    $ guix package \
        --do-not-upgrade $(guix package --only-substitutes -u) \
        -u

This is definitely possible.

>> In my opinion “--only-substitutes” should stop and report a list.
>> If it continued without complaining there could be problems:
>> 
>> * partial upgrades could leave the profile in an unusable state

Maybe I don’t understand Guix that well yet, but I don’t think this is
possible. At least I don’t understand how it would happen. Under the
hood, the “--only-substitutes” flag would basically just be an
intelligent “--do-not-upgrade” flag. Can I ruin my profile by misusing
“--do-not-upgrade”?

>> * an attacker could use this to trick a user into thinking that they
>>   have all available updates

I can always run

    $ guix package -n -u

to learn what packages are out of date. (Except if I get frustrated by
the fact that Guix is building a package to check if is different from
an installed package and mash C-c C-c. :)) Also, my original
implementation
added a “the following packages will be excluded” line, that let the
user know which packages are being omitted from the upgrade. This will
be harder to do with Ludo’s suggestion, though.

>> On the other hand, it would make “--only-substitutes” less usable,
>> because to actually perform work one would have to deal with the failure
>> case.
>> 
>> I suppose it could download the substitutes but not build a new profile
>> and report an error at that point.
>
> Perhaps there could be an additional flag --partial-upgrade to make it
> build a new profile.
>
> I understand why people want --only-substitutes but I'm a bit wary of it
> for the reasons you gave, and I think we should solve their complaint by
> improving our build infrastructure.

I agree wholeheartedly with improving the build infrastructure! I’m just
less certain about how I could help with that…. In the short term, this
is my way of working around the infrastructure issues.


-- Tim

Re: Upgrading packages with substitutes only (bug #26608)

[Ricardo Wurmus]

Timothy Sample <samplet@ngyro.com> writes:

>>> In my opinion “--only-substitutes” should stop and report a list.
>>> If it continued without complaining there could be problems:
>>>
>>> * partial upgrades could leave the profile in an unusable state
>
> Maybe I don’t understand Guix that well yet, but I don’t think this is
> possible. At least I don’t understand how it would happen. Under the
> hood, the “--only-substitutes” flag would basically just be an
> intelligent “--do-not-upgrade” flag. Can I ruin my profile by misusing
> “--do-not-upgrade”?

There are many cases where partial profile upgrades are okay, especially
if software is independent.  Problems usually arise when you have things
like Python modules in your profile.  Upgrading some Python modules but
not others might lead to problems at runtime.

Arguably, one should be using manifests anyway when consistency is
required.

>>> * an attacker could use this to trick a user into thinking that they
>>>   have all available updates
>
> I can always run
>
>     $ guix package -n -u
>
> to learn what packages are out of date. (Except if I get frustrated by
> the fact that Guix is building a package to check if is different from
> an installed package and mash C-c C-c. :))
                                ^^^^^^^

Oh, another “M-x shell” user :)

--
Ricardo

GPG: BCA6 89B6 3655 3801 C3C6  2150 197A 5888 235F ACAC
https://elephly.net

Re: Upgrading packages with substitutes only (bug #26608)

[Carlo Zancanaro]

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


On Sun, Jun 18 2017, Timothy Sample wrote:
> While the user experience would suffer a bit, it’s no real hardship to
> type
>
>     $ guix package \
>         --do-not-upgrade $(guix package --only-substitutes -u) \
>         -u
>
> This is definitely possible.

Just checking: is the proposal that --only-substitutes /always/ prints a
list of packages without substitutes? My understanding was that it would
only print the list if there were packages without substitutes (thus
making the above potentially incorrect/wasteful if everything has a
substitute, because you shouldn't unconditionally re-run `guix
package`). I don't know if we should conflate "upgrade my profile" with
"print a list of packages without substitutes". Those aren't the same
requests.

I'm more in favour of it continuing and building the new profile, but
printing a list of packages which didn't have substitutes. If the user
wants to roll-back the upgrade then they can easily do so, but they
asked for it to be upgraded and we can still do that.

Carlo

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

Re: Upgrading packages with substitutes only (bug #26608)

[Ludovic Courtès]

Ricardo Wurmus <rekado@elephly.net> skribis:

> Ludovic Courtès <ludo@gnu.org> writes:
>
>> BTW, should --only-substitutes filter out packages without a substitute,
>> or should it simply stop and report the list of missing substitutes
>> (after which the user could use --do-not-upgrade)?
>
> In my opinion “--only-substitutes” should stop and report a list.
> If it continued without complaining there could be problems:
>
> * partial upgrades could leave the profile in an unusable state
>
> * an attacker could use this to trick a user into thinking that they
>   have all available updates

Agreed.

> On the other hand, it would make “--only-substitutes” less usable,
> because to actually perform work one would have to deal with the failure
> case.

IMO that’s OK.  “--only-substitutes” would typically be for interactive
use, when you’re in a hurry and you Understand The Risks (click on the
checkbox ;-)).  For unattended upgrades, I think one would want to
upgrade no matter what (assuming of course the build farm is not
completely broken, meaning that most substitutes are available.)

WDYT?

In the future I was also thinking that the build farm could tag Git
commits that it has fully built, and thus ‘guix pull’ could be told to
pull to the latest fully-built commit.

Ludo’.

Re: Upgrading packages with substitutes only (bug #26608)

[Timothy Sample]

ludo@gnu.org (Ludovic Courtès) writes:

> In the future I was also thinking that the build farm could tag Git
> commits that it has fully built, and thus ‘guix pull’ could be told to
> pull to the latest fully-built commit.

I like this quite a bit. In fact, this is more in the spirit of what I
want from “--only-substitutes” anyway. With that in mind, I will
probably just use the following. Maybe this is what other people want
from “--only-substitutes” as well. (Warning: major kludge.)

Here’s a script to scrape the most recent evaluated commit hash from
Hydra. I’ll call it “hydra-latest.sh”.

    #!/bin/sh

    curl -L 'https://hydra.gnu.org/jobset/gnu/master/latest-eval' \
        | grep -oe '<td>[0-9a-f]\{40\}</td>' \
        | sed 's/<\/\?td>//g';

Then, whenever I want to upgrade my system:

    $ cd guix-latest
    $ git pull && git checkout $(sh hydra-latest.sh)

I guess it doesn’t make sense if you have many different substitute
servers, though. Otherwise, adding this kind of feature to “git pull”
would be ideal.

With respect to “--only-substitutes”, I’m not sure I have need for it
anymore. I’m still happy to write it up if other people want it. Let me
know.


-- Tim

Re: Upgrading packages with substitutes only (bug #26608)

[Leo Famulari]

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

On Sun, Jun 18, 2017 at 10:44:34AM -0700, Timothy Sample wrote:
> Leo Famulari <leo@famulari.name> writes:
> > On Sun, Jun 18, 2017 at 11:38:45AM +0200, Ricardo Wurmus wrote:
> >> In my opinion “--only-substitutes” should stop and report a list.
> >> If it continued without complaining there could be problems:
> >> 
> >> * partial upgrades could leave the profile in an unusable state
> 
> Maybe I don’t understand Guix that well yet, but I don’t think this is
> possible. At least I don’t understand how it would happen. Under the
> hood, the “--only-substitutes” flag would basically just be an
> intelligent “--do-not-upgrade” flag. Can I ruin my profile by misusing
> “--do-not-upgrade”?

I'm not sure what you mean by "ruin your profile". It's unlikely you'd
break your Guix installation, if that's what you mean.

However, the Guix development model is that the master branch should
always be "deployable", and Guix developers expect the typical user to
base their installations on the master branch.

If there is some package that can't be fetched from Hydra for any reason
[0], users of `--only-substitutes` will simply never get that upgraded
package until it can be downloaded.

As time goes by, their installations will basically fork from GNU Guix,
and we won't be able to understand what Guix version they are using or
be able to support it. The non-substitutable package will drag an old
and vulnerable dependency tree (growing both up and down) along with it,
and the users will probably not notice.

By the way, the same warning applies if you are upgrading your profile
piecemeal instead of all at once with `guix package -u .`.

So, `--only-substitutes` is definitely something for people who know
what they are doing and understand the risks. For this reason, I think
that using it should require shell command composition, for example:

$ guix package -u . --do-not-upgrade $(guix package -u . --only-substitutes)

We should make it easy for users to do the safe thing (a full profile
upgrade), and not be too concerned if potentially dangerous actions like
a partial upgrade require a bit of extra typing.

If it seems confusing that `guix package -u . --only-substitutes`
doesn't actually upgrade anything, then it could be named differently,
or be part of another command such as `guix substitute`.

I still think the right way to handle this feature request is to improve
the build farm. I use NixOS sometimes and I almost *never* have to build
anything; it's possible to improve our infrastructure to the point where
nobody will think they need this feature.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]