bug#43893: [PATCH v3] maint: update-guix-package: Prevent accidentally breaking guix pull.

["Ludovic Courtès" <ludo@gnu.org>]

Hi Maxim,

Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:

>> +(define (assert-clean-checkout repository)
>> +  "Error out if the working directory at REPOSITORY contains local
>> +modifications."
>> +  (define description
>> +    (let ((format-options (make-describe-format-options
>> +                           #:dirty-suffix "-dirty")))
>> +      (describe-format (describe-workdir repository) format-options)))
>> +
>> +  (when (string-suffix? "-dirty" description)
>> +    (leave (G_ "attempt to update 'guix' package from a dirty tree (~a)~%")
>> +           description))
>> +
>> +  (info (G_ "updating 'guix' package to '~a'~%") description))
>
> Unfortunately this doesn't catch the case where git has explicitly been
> told to '--skip-worktree' on a path or file (the original cause of this
> bug report).  See
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=43893#11.

Any such issue is caught when one eventually runs ‘guix build guix’
(wrong commit ID, wrong hash, etc.).

But you’re right that the above test isn’t fool-proof: it’s just a way
to catch this common mistake early and report it nicely.

>>  (define (main . args)
>>    (match args
>>      ((commit version)
>> @@ -113,32 +153,20 @@ COMMIT."
>>                (hash     (query-path-hash store source))
>>                (location (package-definition-location))
>>                (old-hash (content-hash-value
>> -                          (origin-hash (package-source guix)))))
>> +                         (origin-hash (package-source guix)))))
>> +
>> +         (unless (getenv "GUIX_ALLOW_ME_TO_USE_PRIVATE_COMMIT")
>> +           (let ((repository (repository-open ".")))
>> +             (assert-clean-checkout repository)
>> +             (repository-close! repository)))
>> +
>>           (edit-expression location
>>                            (update-definition commit hash
>>                                               #:old-hash old-hash
>>                                               #:version version))
>
>> -         ;; Re-add SOURCE to the store, but this time under the real name used
>> -         ;; in the 'origin'.  This allows us to build the package without
>> -         ;; having to make a real checkout; thus, it also works when working
>> -         ;; on a private branch.
>> -         (reload-module
>> -          (resolve-module '(gnu packages package-management)))
>> -
>> -         (let* ((source (add-to-store store
>> -                                      (origin-file-name (package-source guix))
>> -                                      #t "sha256" source))
>> -                (root   (store-path-package-name source)))
>> -
>> -           ;; Add an indirect GC root for SOURCE in the current directory.
>> -           (false-if-exception (delete-file root))
>> -           (symlink source root)
>> -           (add-indirect-root store
>> -                              (string-append (getcwd) "/" root))
>> -
>> -           (format #t "source code for commit ~a: ~a (GC root: ~a)~%"
>> -                   commit source root)))))
>> +         (when (getenv "GUIX_ALLOW_ME_TO_USE_PRIVATE_COMMIT")
>> +           (keep-source-in-store store source)))))
>
> That environment variable would now do more than it advertises.  I'd
> prefer to keep the behavior consistent in both modes, unless there's a
> very good reason not too?

Adding the source to the store, under the right name, with a GC root, is
a prerequisite for use cases like ‘make release’: there you not only
want to update the package definition to refer to your private commit
and corresponding hash, you also want to be able to build it.  If the
source isn’t already in the store, ‘guix build guix’ tries to look it up
on Savannah, which fails.

Conversely, when GUIX_ALLOW_ME_TO_USE_PRIVATE_COMMIT is unset, we don’t
add the source to the store: that way, ‘guix build guix’ is forced to
clone from Savannah, which fails if for some reason the commit or hash
is incorrect.

This catches the kinds of mistakes that we previously made, where we
sometimes unwillingly ended up updating to the wrong commit/hash.

I hope that makes sense.

Thanks for your time!

Ludo’.