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

[Maxim Cournoyer]

Hello Ludovic!

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

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

Right.  I still don't like that it wouldn't work from a checkout where
someone would have modified, e.g., the .dir-locals file locally and
marked it with 'git --skip-worktree'.  Having to revert this kind of
'developer setup' is painful.  The current approach makes it unnecessary
(only committed changes are taken into account, not just git tracked
files).

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

Thanks for providing a rational for it.  I'll git-send a new patch which
adds the source to the store using the procedure you shared above, but
otherwise keeps the existing mechanism intact.

Thank you for you patience!

Maxim