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

[Maxim Cournoyer]

Hi Ludovic,

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

> Hi again,
>
> How about this variant of the initial script?  I think it addresses the
> main issues we discussed here:
>
>   1. By default it doesn’t re-add the source in the store, so wrong
>      commit/hash issues are caught when running ‘guix build guix’.
>
>   2. It diagnoses dirty trees early on.  It does not explicitly diagnose
>      missing upstream commits though, but again they’re caught when
>      running ‘guix build guix’.
>
> WDYT?
>
> Sorry for all the back-and-forth on what looks like a tiny issue.  I do
> think we’re making progress anyway!
>
> Thanks,
> Ludo’

Reproducing as a diff over the original script for brevity:

> @@ -23,12 +24,15 @@
>  ;;;
>  ;;; Code:

> -(use-modules (guix)
> +(use-modules (git)
> +             (guix)
>               (guix git-download)
>               (guix upstream)
>               (guix utils)
>               (guix base32)
>               (guix build utils)
> +             (guix i18n)
> +             (guix diagnostics)
>               (gnu packages package-management)
>               (ice-9 match))

> @@ -101,7 +105,43 @@ COMMIT."
>        (exp
>         (error "'guix' package definition is not as expected" exp)))))

> -
> +(define (keep-source-in-store store source)
> +  "Add SOURCE to the store under the name that the 'guix' package expects."
> +
> +  ;; 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))
> +
> +    (info (G_ "source code kept in ~a (GC root: ~a)~%")
> +          source root)))
> +
> +(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.

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

Thanks,

Maxim