unofficial mirror of bug-guix@gnu.org 
 help / color / mirror / Atom feed
* bug#43893: make update-guix-package produced an incorrect hash
@ 2020-10-09 21:58 Maxim Cournoyer
  2020-10-10  0:04 ` Danny Milosavljevic
                   ` (2 more replies)
  0 siblings, 3 replies; 31+ messages in thread
From: Maxim Cournoyer @ 2020-10-09 21:58 UTC (permalink / raw)
  To: 43893

Hello,

Today, I used 'make update-guix-package' and pushed the result (commit
f08587682a).  Users later reported being unable to pull because of a
hash mismatch on the newly updated Guix package :-/.

I haven't investigated why yet, and simply reverted the faulty update
for now in commit a279f7c61c.

Thank you,

Maxim




^ permalink raw reply	[flat|nested] 31+ messages in thread

* bug#43893: make update-guix-package produced an incorrect hash
  2020-10-09 21:58 bug#43893: make update-guix-package produced an incorrect hash Maxim Cournoyer
@ 2020-10-10  0:04 ` Danny Milosavljevic
  2020-10-10  5:08   ` Maxim Cournoyer
  2020-10-10  5:08 ` bug#43893: [PATCH] maint: update-guix-package: Ensure sources are clean Maxim Cournoyer
  2020-10-11 19:57 ` bug#43893: [PATCH v2] maint: update-guix-package: Prevent accidentally breaking guix pull Maxim Cournoyer
  2 siblings, 1 reply; 31+ messages in thread
From: Danny Milosavljevic @ 2020-10-10  0:04 UTC (permalink / raw)
  To: Maxim Cournoyer; +Cc: 43893

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

I'm guessing it has something to do with update-guix-package using git-predicate
to add only git-known (but not necessarily committed) files to the store and then
calculating the checksum of that--but the git-fetch for the guix package not
necessarily doing the same.

Then update-guix-package.scm does one worse and actively prevents guix from
doing the checkout from git when building that "guix" package.  That means the
person invoking update-guix-package.scm can't notice even when the sha256 hash
is definitely wrong--because guix will have the source for package "guix" in
the store already (a faked entry added by update-guix-package.scm) and thus
won't fetch it again.

Also, doesn't this entire approach have a problem?

If you make a commit into the git repo of guix in order to update the
package "guix" to commit A, at that point you can't know what commit hash
commit A will have (since you haven't committed it yet) and yet you have
to know the commit hash of commit A in order to write it into the package
definition of package "guix".

That cannot work.

The only way it works, more or less by accident is that,

(1) At first, update-guix-package.scm does NOT update the "guix" package
inside, and calculates the hash of the working copy (hash A).
(2) Then, it updates the "guix" package inside to refer to hash A and to a
USER-SPECIFIED COMMIT HASH (the latter is determined by the user via
git rev-parse HEAD).
(3) Then, it commits that changed working copy as commit B.  Commit B is
essentially not referred-to by anyone--it's just to make it to the
git repository so guix pull can pick it up.

That works only as long as there will be no reference to a nested-nested "guix"
package, by the eventual user.

@Maxim: I think this entire thing has to assume that

  git rev-parse HEAD

(which it did at the very beginning of make update-guix-package) actually
refers to a commit that is available on the guix git repository on savannah.

That means as soon as you change anything (no matter what) (and not actually
commit that) before invoking

  make update-guix-package

the commit it refers to in the "guix" package will be one which cannot be
resolved by users.

Worse, if you change anything but not commit it (even locally), then that
surely counts as "part of the checkout" for make update-guix-package, so
the hash will be calculated including those change--but the changes are
not committed, so no one can build the resulting guix package (because
of a hash mismatch).  That can happen automatically very easily if "make"
updates po files.

An easy fix, also done by a lot of other such release tools, is to make

  make update-guix-package

first check whether there are any uncommitted changes.  If so, make it fail.

There's

  guix build guix --with-git-url=guix=.

but even that won't work with (locally) uncommitted changes.

Note: uncommitted and unpushed are different.

It's totally fine to have UNPUSHED changes and then use

  ./pre-inst-env guix build guix --with-git-url=guix=`pwd`

in order to build it anyway.

But it's not fine to do that with UNCOMMITTED changes--because the sha256
hash will include those, but the commit id won't.

Long story short, we should make "make update-guix-package" check for
uncommitted changes in the working copy, and fail if any such exist[1].
There are no downsides that I can see.  Even building from local working
copy still works then.

Also, let's please document update-guix-package.

[1] git diff-index --quiet HEAD || echo fail

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 31+ messages in thread

* bug#43893: make update-guix-package produced an incorrect hash
  2020-10-10  0:04 ` Danny Milosavljevic
@ 2020-10-10  5:08   ` Maxim Cournoyer
  0 siblings, 0 replies; 31+ messages in thread
From: Maxim Cournoyer @ 2020-10-10  5:08 UTC (permalink / raw)
  To: Danny Milosavljevic; +Cc: 43893

Hello Danny,

Thanks for the quick investigation.

Danny Milosavljevic <dannym@scratchpost.org> writes:

> I'm guessing it has something to do with update-guix-package using git-predicate
> to add only git-known (but not necessarily committed) files to the store and then
> calculating the checksum of that--but the git-fetch for the guix package not
> necessarily doing the same.

That's a good observation; it is indeed dangerous.  In my case, my tree
looks clean (no modified git-tracked files), but I had in fact
modifications made to .dir-locals that I've been testing and these were
hidden from the view by using:

$ git update-index --skip-worktree .dir-locals

But since the .dir-locals file is known to git, it was probably picked
up with my invisible changes, causing the hash mismatch.

> Then update-guix-package.scm does one worse and actively prevents guix from
> doing the checkout from git when building that "guix" package.  That means the
> person invoking update-guix-package.scm can't notice even when the sha256 hash
> is definitely wrong--because guix will have the source for package "guix" in
> the store already (a faked entry added by update-guix-package.scm) and thus
> won't fetch it again.
>
> Also, doesn't this entire approach have a problem?
>
> If you make a commit into the git repo of guix in order to update the
> package "guix" to commit A, at that point you can't know what commit hash
> commit A will have (since you haven't committed it yet) and yet you have
> to know the commit hash of commit A in order to write it into the package
> definition of package "guix".
>
> That cannot work.
> The only way it works, more or less by accident is that,
>
> (1) At first, update-guix-package.scm does NOT update the "guix" package
> inside, and calculates the hash of the working copy (hash A).
> (2) Then, it updates the "guix" package inside to refer to hash A and to a
> USER-SPECIFIED COMMIT HASH (the latter is determined by the user via
> git rev-parse HEAD).
> (3) Then, it commits that changed working copy as commit B.  Commit B is
> essentially not referred-to by anyone--it's just to make it to the
> git repository so guix pull can pick it up.

Yes, that's my understanding of how it works too.  This means you have
to be extra careful doing this while no-one else is commiting changes,
else you have to start over because rebasing is not an option (it'd
change the hashes, breaking the computed Guix hash).  That's how I broke
'guix pull' the first time I used 'make update-guix-package' :-).  But I
think it's inevitable, so perhaps the best we can do is documement it
well and print a warning when running the target.

> That works only as long as there will be no reference to a nested-nested "guix"
> package, by the eventual user.

What do you mean by nested-nested Guix? Are there valid uses of such a
thing?

> @Maxim: I think this entire thing has to assume that
>
>   git rev-parse HEAD
>
> (which it did at the very beginning of make update-guix-package) actually
> refers to a commit that is available on the guix git repository on savannah.
>
> That means as soon as you change anything (no matter what) (and not actually
> commit that) before invoking
>
>   make update-guix-package
>
> the commit it refers to in the "guix" package will be one which cannot be
> resolved by users.

Indeed.

[...]

> Long story short, we should make "make update-guix-package" check for
> uncommitted changes in the working copy, and fail if any such exist[1].
> There are no downsides that I can see.  Even building from local working
> copy still works then.

Yes, that's a good step.  Actually I just had an idea to use a clean
worktree to do the computation, because that's even safer as it prevents
subtle things like "git update-index --skip-worktree some/path" from
interacting with the computed hash too.

> Also, let's please document update-guix-package.

I'll send a first commit.  I haven't found a way to build it locally
with the command in the message; it seems to create a cycle.  Let me
know what you think.

Maxim




^ permalink raw reply	[flat|nested] 31+ messages in thread

* bug#43893: [PATCH] maint: update-guix-package: Ensure sources are clean.
  2020-10-09 21:58 bug#43893: make update-guix-package produced an incorrect hash Maxim Cournoyer
  2020-10-10  0:04 ` Danny Milosavljevic
@ 2020-10-10  5:08 ` Maxim Cournoyer
  2020-10-10 11:59   ` Danny Milosavljevic
  2020-10-10 20:08   ` Ludovic Courtès
  2020-10-11 19:57 ` bug#43893: [PATCH v2] maint: update-guix-package: Prevent accidentally breaking guix pull Maxim Cournoyer
  2 siblings, 2 replies; 31+ messages in thread
From: Maxim Cournoyer @ 2020-10-10  5:08 UTC (permalink / raw)
  To: 43893; +Cc: Maxim Cournoyer

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=yes, Size: 4544 bytes --]

By using a fresh copy of the last commit, we ensure the computed hash is
stable in the face of local edits.  This change also computes the hash
externally from the store, which allows to verify that the hashes are valid
using, e.g.:

 #FIXME: This doesn't work (recursion?)
./pre-inst-env guix build guix --with-git-url=guix=file://$PWD

* build-aux/update-guix-package.scm (git-add-worktree): New procedure.
(main): Use it to checkout a clean copy of the used commit, and compute the
hash from it.  Print a user warning after completion.
---
 build-aux/update-guix-package.scm | 59 +++++++++++++------------------
 1 file changed, 25 insertions(+), 34 deletions(-)

diff --git a/build-aux/update-guix-package.scm b/build-aux/update-guix-package.scm
index f695e91cfd..b609e57b8f 100644
--- a/build-aux/update-guix-package.scm
+++ b/build-aux/update-guix-package.scm
@@ -1,5 +1,6 @@
 ;;; GNU Guix --- Functional package management for GNU
 ;;; Copyright © 2017, 2018 Ludovic Courtès <ludo@gnu.org>
+;;; Copyright © 2020 Maxim Cournoyer <maxim.cournoyer@gmail.com>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -29,6 +30,7 @@
              (guix utils)
              (guix base32)
              (guix build utils)
+             (guix scripts hash)
              (gnu packages package-management)
              (ice-9 match))
 
@@ -101,44 +103,33 @@ COMMIT."
       (exp
        (error "'guix' package definition is not as expected" exp)))))
 
+(define (git-add-worktree directory commit-ish)
+  "Create a new git worktree at DIRECTORY, detached on commit COMMIT-ISH."
+  (invoke "git" "worktree" "add" "--detach" directory commit-ish))
+
 \f
 (define (main . args)
   (match args
     ((commit version)
-     (with-store store
-       (let* ((source   (add-to-store store
-                                      "guix-checkout" ;dummy name
-                                      #t "sha256" %top-srcdir
-                                      #:select? version-controlled?))
-              (hash     (query-path-hash store source))
-              (location (package-definition-location))
-              (old-hash (content-hash-value
-                          (origin-hash (package-source guix)))))
-         (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)))))
+     (with-directory-excursion %top-srcdir
+       (call-with-temporary-directory
+        (lambda (tmp-directory)
+          (let* ((dummy (git-add-worktree tmp-directory commit))
+                 (hash (nix-base32-string->bytevector
+                        (string-trim-both
+                         (with-output-to-string
+		           (lambda ()
+		             (guix-hash "-rx" tmp-directory))))))
+                 (location (package-definition-location))
+                 (old-hash (content-hash-value
+                            (origin-hash (package-source guix)))))
+            (edit-expression location
+                             (update-definition commit hash
+                                                #:old-hash old-hash
+                                                #:version version))
+            (format #t "Updated Guix to commit ~s.  You must ensure this
+commit hash exists in the public repository, else 'guix pull' will break.
+Beware of 'git rebase'~%" commit))))))
     ((commit)
      ;; Automatically deduce the version and revision numbers.
      (main commit #f))))
-- 
2.28.0





^ permalink raw reply	[flat|nested] 31+ messages in thread

* bug#43893: [PATCH] maint: update-guix-package: Ensure sources are clean.
  2020-10-10  5:08 ` bug#43893: [PATCH] maint: update-guix-package: Ensure sources are clean Maxim Cournoyer
@ 2020-10-10 11:59   ` Danny Milosavljevic
  2020-10-11  2:35     ` Maxim Cournoyer
  2020-10-10 20:08   ` Ludovic Courtès
  1 sibling, 1 reply; 31+ messages in thread
From: Danny Milosavljevic @ 2020-10-10 11:59 UTC (permalink / raw)
  To: Maxim Cournoyer; +Cc: 43893

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

Hi Maxim,

hmm, git worktree can fail if the commit already is checked out somewhere (for
example if you invoke make update-guix-package twice in a row), or if the user
used git worktree on that repo for that commit for other purposes.  That would
mean that

  make update-guix-package

would fail in weird undocumented ways again.  Please please let's document
stuff at least.

Also, why not just fail when there's uncommitted stuff?

This patch looks like it goes to quite some length to enable you to build a
guix package of committed stuff only (which is NOT what your working copy is
actually like).  Is there a use case for that?  Sounds weird to me.

Even if there's a use case for that, please add a warning if there are
uncommitted changes that are now not included in the "guix" package.

Other than that, okay.

>#FIXME: This doesn't work (recursion?)
>./pre-inst-env guix build guix --with-git-url=guix=file://$PWD

Why doesn't it work?  That sounds like a big limitation--that basically means
you can't test with local-only commits, you'd always have to push.  We
should find out why this doesn't work and fix it.

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 31+ messages in thread

* bug#43893: [PATCH] maint: update-guix-package: Ensure sources are clean.
  2020-10-10  5:08 ` bug#43893: [PATCH] maint: update-guix-package: Ensure sources are clean Maxim Cournoyer
  2020-10-10 11:59   ` Danny Milosavljevic
@ 2020-10-10 20:08   ` Ludovic Courtès
  2020-10-10 21:14     ` Danny Milosavljevic
  2020-10-11 19:43     ` Maxim Cournoyer
  1 sibling, 2 replies; 31+ messages in thread
From: Ludovic Courtès @ 2020-10-10 20:08 UTC (permalink / raw)
  To: Maxim Cournoyer; +Cc: 43893

Hi,

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

> By using a fresh copy of the last commit, we ensure the computed hash is
> stable in the face of local edits.  This change also computes the hash
> externally from the store, which allows to verify that the hashes are valid
> using, e.g.:
>
>  #FIXME: This doesn't work (recursion?)
> ./pre-inst-env guix build guix --with-git-url=guix=file://$PWD

Works for me, please open a separate bug report.  :-)

> * build-aux/update-guix-package.scm (git-add-worktree): New procedure.
> (main): Use it to checkout a clean copy of the used commit, and compute the
> hash from it.  Print a user warning after completion.

I’m not quite enthusiastic about the tool creating a worktree behind my
back.

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

So this preserves this possibility, right?

> +            (format #t "Updated Guix to commit ~s.  You must ensure this
> +commit hash exists in the public repository, else 'guix pull' will break.
> +Beware of 'git rebase'~%" commit))))))

I think this is the most important bit.  :-)

I could also suggest running ‘guix build guix --check’.

Honestly, I would simply add this last message; better yet, we could use
Guile-Git to (1) check whether we’re on a dirty tree and stop right away
if we are, and (2) check whether the commit exists in the official Git
repo and error out if it doesn’t, unless
GUIX_ALLOW_ME_TO_USE_PRIVATE_COMMIT is set.

#2 could also be implemented by building the derivation of
(package-source guix) in ‘check’ mode (perhaps easier).

WDYT?

Thanks for looking into it, experience has shown that this really needs
to be addressed!

Ludo’.




^ permalink raw reply	[flat|nested] 31+ messages in thread

* bug#43893: [PATCH] maint: update-guix-package: Ensure sources are clean.
  2020-10-10 20:08   ` Ludovic Courtès
@ 2020-10-10 21:14     ` Danny Milosavljevic
  2020-10-12  4:40       ` Maxim Cournoyer
  2020-10-12  9:40       ` Ludovic Courtès
  2020-10-11 19:43     ` Maxim Cournoyer
  1 sibling, 2 replies; 31+ messages in thread
From: Danny Milosavljevic @ 2020-10-10 21:14 UTC (permalink / raw)
  To: Ludovic Courtès; +Cc: Maxim Cournoyer, 43893

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

Hi Ludo,

On Sat, 10 Oct 2020 22:08:19 +0200
Ludovic Courtès <ludo@gnu.org> wrote:

> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
> 
> >  #FIXME: This doesn't work (recursion?)
> > ./pre-inst-env guix build guix --with-git-url=guix=file://$PWD  
> 
> Works for me, please open a separate bug report.  :-)

Doesn't work for me on x86_64, using a checkout of guix at commit 93d3cfec32bbbe1dfbe0be686b371973545b35b8.

$ guix environment --pure guix --ad-hoc git guile-readline guile-json nano guile-zlib guile-lzlib bash -- ./pre-inst-env guix build guix --with-git-url=guix=file://$PWD

I get:

[...]
updating checkout of 'file:///home/dannym/src/guix-master/guix'...
retrieved commit 93d3cfec32bbbe1dfbe0be686b371973545b35b8
[...]
test-name: channel-instances->manifest
location: /tmp/guix-build-guix-1.1.0-30.5918cb5.drv-0/source/tests/channels.scm:233
source:
+ (test-assert
+   "channel-instances->manifest"
+   (let* ((spec (lambda deps
+                  `(channel
+                     (version 0)
+                     (dependencies
+                       ,@(map (lambda (dep)
+                                `(channel
+                                   (name ,dep)
+                                   (url "http://example.org")))
+                              deps)))))
+          (guix (make-instance #:name 'guix))
+          (instance0 (make-instance #:name 'a))
+          (instance1
+            (make-instance #:name 'b #:spec (spec 'a)))
+          (instance2
+            (make-instance #:name 'c #:spec (spec 'b)))
+          (instance3
+            (make-instance #:name 'd #:spec (spec 'c 'a))))
+     (%graft? #f)
+     (let ((source (channel-instance-checkout guix)))
+       (mkdir (string-append source "/build-aux"))
+       (call-with-output-file
+         (string-append
+           source
+           "/build-aux/build-self.scm")
+         (lambda (port)
+           (write '(begin
+                     (use-modules (guix) (gnu packages bootstrap))
+                     (lambda _ (package->derivation %bootstrap-guile)))
+                  port))))
+     (with-store
+       store
+       (let ()
+         (define manifest
+           (run-with-store
+             store
+             (channel-instances->manifest
+               (list guix
+                     instance0
+                     instance1
+                     instance2
+                     instance3))))
+         (define entries (manifest-entries manifest))
+         (define (depends? drv in out)
+           (let ((set (list->set
+                        (requisites
+                          store
+                          (list (derivation-file-name drv)))))
+                 (in (map derivation-file-name in))
+                 (out (map derivation-file-name out)))
+             (and (every (cut set-contains? set <>) in)
+                  (not (any (cut set-contains? set <>) out)))))
+         (define (lookup name)
+           (run-with-store
+             store
+             (lower-object
+               (manifest-entry-item
+                 (manifest-lookup
+                   manifest
+                   (manifest-pattern (name name)))))))
+         (let ((drv-guix (lookup "guix"))
+               (drv0 (lookup "a"))
+               (drv1 (lookup "b"))
+               (drv2 (lookup "c"))
+               (drv3 (lookup "d")))
+           (and (depends?
+                  drv-guix
+                  '()
+                  (list drv0 drv1 drv2 drv3))
+                (depends? drv0 (list) (list drv1 drv2 drv3))
+                (depends? drv1 (list drv0) (list drv2 drv3))
+                (depends? drv2 (list drv1) (list drv3))
+                (depends? drv3 (list drv2 drv0) (list))))))))
actual-value: #f
actual-error:
+ (wrong-type-arg
+   "struct-vtable"
+   "Wrong type argument in position 1 (expecting struct): ~S"
+   (#f)
+   (#f))
result: FAIL
[...]
command "make" "check" failed with status 2
builder for `/gnu/store/csagsyh01rq7ilqkcpaa2d7vp1bn41w3-guix-1.1.0-30.5918cb5.drv' failed with exit code 1
build of /gnu/store/csagsyh01rq7ilqkcpaa2d7vp1bn41w3-guix-1.1.0-30.5918cb5.drv failed
View build log at '/var/log/guix/drvs/cs/agsyh01rq7ilqkcpaa2d7vp1bn41w3-guix-1.1.0-30.5918cb5.drv.bz2'.
guix build: error: build of `/gnu/store/csagsyh01rq7ilqkcpaa2d7vp1bn41w3-guix-1.1.0-30.5918cb5.drv' failed

I didn't even RUN update-guix-package or apply this patch--and the above still fails.

This is reproducible every time--both on my laptop and on a x86_64 build host.

So I'm confused how there's a substitute for the package "guix" available.  How was it built? O_o.

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 31+ messages in thread

* bug#43893: [PATCH] maint: update-guix-package: Ensure sources are clean.
  2020-10-10 11:59   ` Danny Milosavljevic
@ 2020-10-11  2:35     ` Maxim Cournoyer
  0 siblings, 0 replies; 31+ messages in thread
From: Maxim Cournoyer @ 2020-10-11  2:35 UTC (permalink / raw)
  To: Danny Milosavljevic, ludo; +Cc: 43893

Hi Danny,

Danny Milosavljevic <dannym@scratchpost.org> writes:

> Hi Maxim,
>
> hmm, git worktree can fail if the commit already is checked out somewhere (for
> example if you invoke make update-guix-package twice in a row), or if the user
> used git worktree on that repo for that commit for other purposes.  That would
> mean that
>
>   make update-guix-package

The "--detached" option is the one allowing to have multiple checkouts of
the same commit.

> would fail in weird undocumented ways again.  Please please let's document
> stuff at least.
>
> Also, why not just fail when there's uncommitted stuff?

Well, if the tool says clearly it's going to update the guix package to
commit X, which is the latest commit in your tree, and doesn't have
technical reasons preventing it from doing so successfully, why enforce
a clean tree?

Using a worktree to setup a pristine checkout also covers for uses such
as 'git update-index --skip-worktree some-file-or-path' or 'git
update-index --assume-unchanged some-file-or-path', which would go
undetected by checking for modifications with 'git diff-index --quiet
HEAD'.  We'd also have to check for the output of

> This patch looks like it goes to quite some length to enable you to build a
> guix package of committed stuff only (which is NOT what your working copy is
> actually like).  Is there a use case for that?  Sounds weird to me.

That's exactly what the tool should do, as there's no point upgrading
the guix package definition to a state that doesn't exist as a commit.

> Even if there's a use case for that, please add a warning if there are
> uncommitted changes that are now not included in the "guix" package.

They were never included in the upgraded guix package (the one you'd
'git push' anyway), they were just put in your store for computing the
hash, which is problematic as we saw.

> Other than that, okay.
>
>>#FIXME: This doesn't work (recursion?)
>>./pre-inst-env guix build guix --with-git-url=guix=file://$PWD
>
> Why doesn't it work?  That sounds like a big limitation--that basically means
> you can't test with local-only commits, you'd always have to push.  We
> should find out why this doesn't work and fix it.

I've tested it again, and it worked... I think what got is surprising is
that the only output I had for multiple minutes was:

--8<---------------cut here---------------start------------->8---
updating checkout of 'file:///home/maxim/src/guix'...
--8<---------------cut here---------------end--------------->8---

And the CPU peaked and the RAM stayed around 1 GiB while it was doing
its magic.

Maxim




^ permalink raw reply	[flat|nested] 31+ messages in thread

* bug#43893: [PATCH] maint: update-guix-package: Ensure sources are clean.
  2020-10-10 20:08   ` Ludovic Courtès
  2020-10-10 21:14     ` Danny Milosavljevic
@ 2020-10-11 19:43     ` Maxim Cournoyer
  2020-10-12  9:43       ` Ludovic Courtès
  1 sibling, 1 reply; 31+ messages in thread
From: Maxim Cournoyer @ 2020-10-11 19:43 UTC (permalink / raw)
  To: Ludovic Courtès; +Cc: 43893

Hello!

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

[...]

>> * build-aux/update-guix-package.scm (git-add-worktree): New procedure.
>> (main): Use it to checkout a clean copy of the used commit, and compute the
>> hash from it.  Print a user warning after completion.
>
> I’m not quite enthusiastic about the tool creating a worktree behind my
> back.

May I ask why?  It's not something you'd (need to) be aware of (it
doesn't leave traces in my v2 as 'git worktree prune' clears up the
temporary worktree entries), and it's quite cheap to create.

>> -         ;; 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.
>
> So this preserves this possibility, right?

Yes.

[...]

> I could also suggest running ‘guix build guix --check’.
>
> Honestly, I would simply add this last message; better yet, we could use
> Guile-Git to (1) check whether we’re on a dirty tree and stop right away
> if we are

Using the worktree approach, checking for local changes is made
unnecessary; as long as the tool clearly says which commit will be used
for the updated Guix package, I don't see why we should force the
developer to stash their changes (in the same way git checkout doesn't
forces you to do so in the absence of conflicts).

Also, this bug reported was triggered by something like:

--8<---------------cut here---------------start------------->8---
echo ";;some comment" >> .dir-locals \
     && git update-index --skip-worktree .dir-locals \
     && make update-guix-package)
--8<---------------cut here---------------end--------------->8---

leading 'update-guix-package' to produce a wrong hash.  Git has been
told to ignore changes on .dir-locals so checking for changes would not
have been sufficient (you'd also need to 'git ls-files -v' and interpret
the result).

> and (2) check whether the commit exists in the official Git
> repo and error out if it doesn’t, unless
> GUIX_ALLOW_ME_TO_USE_PRIVATE_COMMIT is set.

That's a good idea; implemented in the v2 patch I'm about to send.

Thanks,

Maxim




^ permalink raw reply	[flat|nested] 31+ messages in thread

* bug#43893: [PATCH v2] maint: update-guix-package: Prevent accidentally breaking guix pull.
  2020-10-09 21:58 bug#43893: make update-guix-package produced an incorrect hash Maxim Cournoyer
  2020-10-10  0:04 ` Danny Milosavljevic
  2020-10-10  5:08 ` bug#43893: [PATCH] maint: update-guix-package: Ensure sources are clean Maxim Cournoyer
@ 2020-10-11 19:57 ` Maxim Cournoyer
  2020-10-13 16:00   ` Marius Bakke
  2 siblings, 1 reply; 31+ messages in thread
From: Maxim Cournoyer @ 2020-10-11 19:57 UTC (permalink / raw)
  To: 43893; +Cc: Maxim Cournoyer

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=yes, Size: 9095 bytes --]

Fixes <https://issues.guix.gnu.org/43893>.

This changes the 'update-guix-package' tool so that it:

1. Always uses a clean checkout to compute the hash of the updated 'guix'
package.
2. Ensures the commit used in the updated 'guix' package definition has already
been pushed upstream.

* build-aux/update-guix-package.scm (%savannah-guix-git-repo-push-url): New
variable.
(with-input-pipe-to-string): New syntax.
(find-origin-remote, git-add-worktree): New procedures.
(commit-already-pushed?): New predicate.
(main): Check the commit used has already been pushed upstream and compute the
hash from a clean checkout.
* doc/contributing.texi (Updating the Guix Package): Document it.
---
 build-aux/update-guix-package.scm | 103 ++++++++++++++++++++----------
 doc/contributing.texi             |  43 +++++++++++++
 2 files changed, 111 insertions(+), 35 deletions(-)

diff --git a/build-aux/update-guix-package.scm b/build-aux/update-guix-package.scm
index f695e91cfd..397b404922 100644
--- a/build-aux/update-guix-package.scm
+++ b/build-aux/update-guix-package.scm
@@ -1,5 +1,6 @@
 ;;; GNU Guix --- Functional package management for GNU
 ;;; Copyright © 2017, 2018 Ludovic Courtès <ludo@gnu.org>
+;;; Copyright © 2020 Maxim Cournoyer <maxim.cournoyer@gmail.com>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -24,13 +25,20 @@
 ;;; Code:
 
 (use-modules (guix)
+             (guix ui)
              (guix git-download)
              (guix upstream)
              (guix utils)
              (guix base32)
              (guix build utils)
+             (guix scripts hash)
              (gnu packages package-management)
-             (ice-9 match))
+             (ice-9 match)
+             (ice-9 popen)
+             (ice-9 textual-ports)
+             (srfi srfi-1)
+             (srfi srfi-2)
+             (srfi srfi-26))
 
 (define %top-srcdir
   (string-append (current-source-directory) "/.."))
@@ -101,44 +109,69 @@ COMMIT."
       (exp
        (error "'guix' package definition is not as expected" exp)))))
 
-\f
-(define (main . args)
-  (match args
-    ((commit version)
-     (with-store store
-       (let* ((source   (add-to-store store
-                                      "guix-checkout" ;dummy name
-                                      #t "sha256" %top-srcdir
-                                      #:select? version-controlled?))
-              (hash     (query-path-hash store source))
-              (location (package-definition-location))
-              (old-hash (content-hash-value
-                          (origin-hash (package-source guix)))))
-         (edit-expression location
-                          (update-definition commit hash
-                                             #:old-hash old-hash
-                                             #:version version))
+(define (git-add-worktree directory commit-ish)
+  "Create a new git worktree at DIRECTORY, detached on commit COMMIT-ISH."
+  (invoke "git" "worktree" "add" "--detach" directory commit-ish))
+
+(define %savannah-guix-git-repo-push-url
+  "git.savannah.gnu.org/srv/git/guix.git")
 
-         ;; 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)))
+(define-syntax-rule (with-input-pipe-to-string prog arg ...)
+  (let* ((input-pipe (open-pipe* OPEN_READ prog arg ...))
+	 (output (get-string-all input-pipe))
+	 (exit-val (status:exit-val (close-pipe input-pipe))))
+    (unless (zero? exit-val)
+      (error (format #f "Command ~s exited with non-zero exit status: ~s"
+                     (string-join (list prog arg ...)) exit-val)))
+    (string-trim-both output)))
 
-         (let* ((source (add-to-store store
-                                      (origin-file-name (package-source guix))
-                                      #t "sha256" source))
-                (root   (store-path-package-name source)))
+(define (find-origin-remote)
+  "Find the name of the git remote with the Savannah Guix git repo URL."
+  (and-let* ((remotes (string-split (with-input-pipe-to-string
+                                     "git" "remote" "-v")
+                                    #\newline))
+             (origin-entry (find (cut string-contains <>
+                                      (string-append
+                                       %savannah-guix-git-repo-push-url
+                                       " (push)"))
+                                 remotes)))
+    (first (string-split origin-entry #\tab))))
 
-           ;; 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))
+(define (commit-already-pushed? remote commit)
+  "True if COMMIT is found in the REMOTE repository."
+  (not (string-null? (with-input-pipe-to-string
+                      "git" "branch" "-r" "--contains" commit
+                      (string-append remote "/master")))))
 
-           (format #t "source code for commit ~a: ~a (GC root: ~a)~%"
-                   commit source root)))))
+\f
+(define (main . args)
+  (match args
+    ((commit version)
+     (with-directory-excursion %top-srcdir
+       (or (getenv "GUIX_ALLOW_ME_TO_USE_PRIVATE_COMMIT")
+           (commit-already-pushed? (find-origin-remote) commit)
+           (leave (G_ "Commit ~a is not pushed upstream.  Aborting.~%") commit))
+       (dynamic-wind
+         (lambda ()
+           #t)
+         (lambda ()
+           (call-with-temporary-directory
+            (lambda (tmp-directory)
+              (let* ((dummy (git-add-worktree tmp-directory commit))
+                     (hash (nix-base32-string->bytevector
+                            (string-trim-both
+                             (with-output-to-string
+		               (lambda ()
+		                 (guix-hash "-rx" tmp-directory))))))
+                     (location (package-definition-location))
+                     (old-hash (content-hash-value
+                                (origin-hash (package-source guix)))))
+                (edit-expression location
+                                 (update-definition commit hash
+                                                    #:old-hash old-hash
+                                                    #:version version))))))
+         (lambda ()
+           (invoke "git" "worktree" "prune")))))
     ((commit)
      ;; Automatically deduce the version and revision numbers.
      (main commit #f))))
diff --git a/doc/contributing.texi b/doc/contributing.texi
index af3601442e..11a932a9bf 100644
--- a/doc/contributing.texi
+++ b/doc/contributing.texi
@@ -28,6 +28,7 @@ choice.
 * Submitting Patches::          Share your work.
 * Tracking Bugs and Patches::   Using Debbugs.
 * Commit Access::               Pushing to the official repository.
+* Updating the Guix Package::   Updating the Guix package definition.
 @end menu
 
 @node Building from Git
@@ -1323,3 +1324,45 @@ only push their own awesome changes, but also offer some of their time
 @emph{reviewing} and pushing other people's changes.  As a committer,
 you're welcome to use your expertise and commit rights to help other
 contributors, too!
+
+@node Updating the Guix Package
+@section Updating the Guix Package
+
+@cindex update-guix-package, updating the guix package
+It is sometimes desirable to update the @code{guix} package itself (the
+package defined in @code{(gnu packages package-management)}), for
+example to make new daemon features available for use by the
+@code{guix-service-type} service type.  In order to simplify this task,
+the following command can be used:
+
+@example
+make update-guix-package
+@end example
+
+The @code{update-guix-package} make target will use the last known
+@emph{commit} corresponding to @code{HEAD} in your Guix checkout,
+compute the hash of the Guix sources corresponding to that commit and
+update the @code{commit}, @code{revision} and hash of the @code{guix}
+package definition.
+
+To validate that the updated @code{guix} package hashes are correct and
+that it can be built successfully, the following command can be run from
+the directory of your Guix checkout:
+
+@example
+./pre-inst-env guix build guix
+@end example
+
+To guard against accidentally updating the @code{guix} package to a
+commit that others can't refer to, a check is made that the commit used
+has already been pushed to the Savannah-hosted Guix git repository.
+
+This check can be disabled, @emph{at your own peril}, by setting the
+@code{GUIX_ALLOW_ME_TO_USE_PRIVATE_COMMIT} environment variable.
+
+To build the resulting 'guix' package when using a private commit, the
+following command can be used:
+
+@example
+./pre-inst-env guix build guix --with-git-url=guix=$PWD
+@end example
-- 
2.28.0





^ permalink raw reply	[flat|nested] 31+ messages in thread

* bug#43893: [PATCH] maint: update-guix-package: Ensure sources are clean.
  2020-10-10 21:14     ` Danny Milosavljevic
@ 2020-10-12  4:40       ` Maxim Cournoyer
  2020-10-12  9:40       ` Ludovic Courtès
  1 sibling, 0 replies; 31+ messages in thread
From: Maxim Cournoyer @ 2020-10-12  4:40 UTC (permalink / raw)
  To: Danny Milosavljevic; +Cc: 43893

Hello Danny,

Danny Milosavljevic <dannym@scratchpost.org> writes:

> Hi Ludo,

[...]

> Doesn't work for me on x86_64, using a checkout of guix at commit 93d3cfec32bbbe1dfbe0be686b371973545b35b8.
>
> $ guix environment --pure guix --ad-hoc git guile-readline guile-json
> nano guile-zlib guile-lzlib bash -- ./pre-inst-env guix build guix
> --with-git-url=guix=file://$PWD
>
> I get:
>
> [...]
> updating checkout of 'file:///home/dannym/src/guix-master/guix'...
> retrieved commit 93d3cfec32bbbe1dfbe0be686b371973545b35b8
> [...]
> test-name: channel-instances->manifest
> location: /tmp/guix-build-guix-1.1.0-30.5918cb5.drv-0/source/tests/channels.scm:233
> source:
> + (test-assert
> +   "channel-instances->manifest"

[...]

> actual-value: #f
> actual-error:
> + (wrong-type-arg
> +   "struct-vtable"
> +   "Wrong type argument in position 1 (expecting struct): ~S"
> +   (#f)
> +   (#f))
> result: FAIL
> [...]
> command "make" "check" failed with status 2
> builder for `/gnu/store/csagsyh01rq7ilqkcpaa2d7vp1bn41w3-guix-1.1.0-30.5918cb5.drv' failed with exit code 1
> build of /gnu/store/csagsyh01rq7ilqkcpaa2d7vp1bn41w3-guix-1.1.0-30.5918cb5.drv failed
> View build log at '/var/log/guix/drvs/cs/agsyh01rq7ilqkcpaa2d7vp1bn41w3-guix-1.1.0-30.5918cb5.drv.bz2'.
> guix build: error: build of `/gnu/store/csagsyh01rq7ilqkcpaa2d7vp1bn41w3-guix-1.1.0-30.5918cb5.drv' failed
>
> I didn't even RUN update-guix-package or apply this patch--and the above still fails.
>
> This is reproducible every time--both on my laptop and on a x86_64 build host.
>
> So I'm confused how there's a substitute for the package "guix" available.  How was it built? O_o.

You found a bug!  I've reported and investigated it at
https://issues.guix.gnu.org/43940 and marked the test as expected to
fail for now so that we can continue to update our Guix package.

The current guix package was using
4e3ed9bad9ed5758cdee6e636805f65e9ab710eb, which didn't had that problem.

Thanks,

Maxim




^ permalink raw reply	[flat|nested] 31+ messages in thread

* bug#43893: [PATCH] maint: update-guix-package: Ensure sources are clean.
  2020-10-10 21:14     ` Danny Milosavljevic
  2020-10-12  4:40       ` Maxim Cournoyer
@ 2020-10-12  9:40       ` Ludovic Courtès
  2020-10-12 14:18         ` Danny Milosavljevic
  1 sibling, 1 reply; 31+ messages in thread
From: Ludovic Courtès @ 2020-10-12  9:40 UTC (permalink / raw)
  To: Danny Milosavljevic; +Cc: Maxim Cournoyer, 43893

Danny Milosavljevic <dannym@scratchpost.org> skribis:

> Doesn't work for me on x86_64, using a checkout of guix at commit 93d3cfec32bbbe1dfbe0be686b371973545b35b8.

Oh I see, you’re hitting a test failure; I thought you were saying that
‘--with-git-url’ didn’t work.

Ludo’.




^ permalink raw reply	[flat|nested] 31+ messages in thread

* bug#43893: [PATCH] maint: update-guix-package: Ensure sources are clean.
  2020-10-11 19:43     ` Maxim Cournoyer
@ 2020-10-12  9:43       ` Ludovic Courtès
  2020-10-13  1:33         ` Maxim Cournoyer
  0 siblings, 1 reply; 31+ messages in thread
From: Ludovic Courtès @ 2020-10-12  9:43 UTC (permalink / raw)
  To: Maxim Cournoyer; +Cc: 43893

Hi,

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

> Ludovic Courtès <ludo@gnu.org> writes:
>
> [...]
>
>>> * build-aux/update-guix-package.scm (git-add-worktree): New procedure.
>>> (main): Use it to checkout a clean copy of the used commit, and compute the
>>> hash from it.  Print a user warning after completion.
>>
>> I’m not quite enthusiastic about the tool creating a worktree behind my
>> back.
>
> May I ask why?  It's not something you'd (need to) be aware of (it
> doesn't leave traces in my v2 as 'git worktree prune' clears up the
> temporary worktree entries), and it's quite cheap to create.

To me, this is all my workspace, and I generally assume I’m the only one
touching it; it’s more about the mental model, I guess.

Thanks,
Ludo’.




^ permalink raw reply	[flat|nested] 31+ messages in thread

* bug#43893: [PATCH] maint: update-guix-package: Ensure sources are clean.
  2020-10-12  9:40       ` Ludovic Courtès
@ 2020-10-12 14:18         ` Danny Milosavljevic
  0 siblings, 0 replies; 31+ messages in thread
From: Danny Milosavljevic @ 2020-10-12 14:18 UTC (permalink / raw)
  To: Ludovic Courtès; +Cc: Maxim Cournoyer, 43893

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

Hi Ludo,

On Mon, 12 Oct 2020 11:40:56 +0200
Ludovic Courtès <ludo@gnu.org> wrote:

> Danny Milosavljevic <dannym@scratchpost.org> skribis:
> 
> > Doesn't work for me on x86_64, using a checkout of guix at commit 93d3cfec32bbbe1dfbe0be686b371973545b35b8.  
> 
> Oh I see, you’re hitting a test failure; I thought you were saying that
> ‘--with-git-url’ didn’t work.

I wasn't saying the latter.  That was Maxim.

It just happened that I also had a similar problem, and provided more detail.
But whether it was the same I don't know (it probably was, though).

(also, it was not possible to distinguish whether the test failure was caused
by "--with-git-url")

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 31+ messages in thread

* bug#43893: [PATCH] maint: update-guix-package: Ensure sources are clean.
  2020-10-12  9:43       ` Ludovic Courtès
@ 2020-10-13  1:33         ` Maxim Cournoyer
  0 siblings, 0 replies; 31+ messages in thread
From: Maxim Cournoyer @ 2020-10-13  1:33 UTC (permalink / raw)
  To: Ludovic Courtès; +Cc: 43893

Hi Ludovic,

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

> Hi,
>
> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
>
>> Ludovic Courtès <ludo@gnu.org> writes:
>>
>> [...]
>>
>>>> * build-aux/update-guix-package.scm (git-add-worktree): New procedure.
>>>> (main): Use it to checkout a clean copy of the used commit, and compute the
>>>> hash from it.  Print a user warning after completion.
>>>
>>> I’m not quite enthusiastic about the tool creating a worktree behind my
>>> back.
>>
>> May I ask why?  It's not something you'd (need to) be aware of (it
>> doesn't leave traces in my v2 as 'git worktree prune' clears up the
>> temporary worktree entries), and it's quite cheap to create.
>
> To me, this is all my workspace, and I generally assume I’m the only one
> touching it; it’s more about the mental model, I guess.

I see. I understand your position, but also feel it's not a big deal
here, given you wouldn't be able to notice a new worktree unless you
watch the output of 'git worktree list' in a loop while updating the
guix package ;-).

I think the change brings good guarantees to guard against breaking
'guix pull' with a 'make update-guix-package' as it is, and also
documents the thing.  If it's OK with you I'd like to merge it in the
coming days.

Thanks,

Maxim




^ permalink raw reply	[flat|nested] 31+ messages in thread

* bug#43893: [PATCH v2] maint: update-guix-package: Prevent accidentally breaking guix pull.
  2020-10-11 19:57 ` bug#43893: [PATCH v2] maint: update-guix-package: Prevent accidentally breaking guix pull Maxim Cournoyer
@ 2020-10-13 16:00   ` Marius Bakke
  2020-10-14  3:17     ` bug#43893: [PATCH v3] " Maxim Cournoyer
  2020-10-14  4:10     ` bug#43893: [PATCH v2] " Maxim Cournoyer
  0 siblings, 2 replies; 31+ messages in thread
From: Marius Bakke @ 2020-10-13 16:00 UTC (permalink / raw)
  To: Maxim Cournoyer, 43893; +Cc: Maxim Cournoyer

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

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

> Fixes <https://issues.guix.gnu.org/43893>.
>
> This changes the 'update-guix-package' tool so that it:
>
> 1. Always uses a clean checkout to compute the hash of the updated 'guix'
> package.
> 2. Ensures the commit used in the updated 'guix' package definition has already
> been pushed upstream.
>
> * build-aux/update-guix-package.scm (%savannah-guix-git-repo-push-url): New
> variable.
> (with-input-pipe-to-string): New syntax.
> (find-origin-remote, git-add-worktree): New procedures.
> (commit-already-pushed?): New predicate.
> (main): Check the commit used has already been pushed upstream and compute the
> hash from a clean checkout.
> * doc/contributing.texi (Updating the Guix Package): Document it.

[...]
  
>  (define %top-srcdir
>    (string-append (current-source-directory) "/.."))
> @@ -101,44 +109,69 @@ COMMIT."
>        (exp
>         (error "'guix' package definition is not as expected" exp)))))
>  
> -\f
> -(define (main . args)
> -  (match args
> -    ((commit version)
> -     (with-store store
> -       (let* ((source   (add-to-store store
> -                                      "guix-checkout" ;dummy name
> -                                      #t "sha256" %top-srcdir
> -                                      #:select? version-controlled?))
> -              (hash     (query-path-hash store source))
> -              (location (package-definition-location))
> -              (old-hash (content-hash-value
> -                          (origin-hash (package-source guix)))))
> -         (edit-expression location
> -                          (update-definition commit hash
> -                                             #:old-hash old-hash
> -                                             #:version version))
> +(define (git-add-worktree directory commit-ish)
> +  "Create a new git worktree at DIRECTORY, detached on commit COMMIT-ISH."
> +  (invoke "git" "worktree" "add" "--detach" directory commit-ish))

Is it feasible to use Guile-Git here (given appropriate bindings)?

> +(define %savannah-guix-git-repo-push-url
> +  "git.savannah.gnu.org/srv/git/guix.git")
>  
> -         ;; 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)))
> +(define-syntax-rule (with-input-pipe-to-string prog arg ...)
> +  (let* ((input-pipe (open-pipe* OPEN_READ prog arg ...))
> +	 (output (get-string-all input-pipe))
> +	 (exit-val (status:exit-val (close-pipe input-pipe))))
> +    (unless (zero? exit-val)
> +      (error (format #f "Command ~s exited with non-zero exit status: ~s"
> +                     (string-join (list prog arg ...)) exit-val)))
> +    (string-trim-both output)))
>  
> -         (let* ((source (add-to-store store
> -                                      (origin-file-name (package-source guix))
> -                                      #t "sha256" source))
> -                (root   (store-path-package-name source)))
> +(define (find-origin-remote)
> +  "Find the name of the git remote with the Savannah Guix git repo URL."
> +  (and-let* ((remotes (string-split (with-input-pipe-to-string
> +                                     "git" "remote" "-v")
> +                                    #\newline))
> +             (origin-entry (find (cut string-contains <>
> +                                      (string-append
> +                                       %savannah-guix-git-repo-push-url
> +                                       " (push)"))
> +                                 remotes)))
> +    (first (string-split origin-entry #\tab))))
>  
> -           ;; 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))
> +(define (commit-already-pushed? remote commit)
> +  "True if COMMIT is found in the REMOTE repository."
> +  (not (string-null? (with-input-pipe-to-string
> +                      "git" "branch" "-r" "--contains" commit
> +                      (string-append remote "/master")))))

...because parsing git CLI output is error-prone and "ugly" (IMO).  But
not a strong opinion.

> -           (format #t "source code for commit ~a: ~a (GC root: ~a)~%"
> -                   commit source root)))))
> +\f
> +(define (main . args)
> +  (match args
> +    ((commit version)
> +     (with-directory-excursion %top-srcdir
> +       (or (getenv "GUIX_ALLOW_ME_TO_USE_PRIVATE_COMMIT")
> +           (commit-already-pushed? (find-origin-remote) commit)
> +           (leave (G_ "Commit ~a is not pushed upstream.  Aborting.~%") commit))
> +       (dynamic-wind
> +         (lambda ()
> +           #t)
> +         (lambda ()
> +           (call-with-temporary-directory
> +            (lambda (tmp-directory)
> +              (let* ((dummy (git-add-worktree tmp-directory commit))
> +                     (hash (nix-base32-string->bytevector
> +                            (string-trim-both
> +                             (with-output-to-string
> +		               (lambda ()
> +		                 (guix-hash "-rx" tmp-directory))))))
> +                     (location (package-definition-location))
> +                     (old-hash (content-hash-value
> +                                (origin-hash (package-source guix)))))
> +                (edit-expression location
> +                                 (update-definition commit hash
> +                                                    #:old-hash old-hash
> +                                                    #:version version))))))
> +         (lambda ()
> +           (invoke "git" "worktree" "prune")))))

This is not great, because users (well, developers who run this script)
may have worktrees that are temporarily inaccessible (e.g. on a USB
drive or whatever).  Better to just leave the stale reference instead of
potentially destroying users worktrees.

Perhaps the script could 'git clone --maxdepth=1' instead of creating a
worktree?

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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* bug#43893: [PATCH v3] maint: update-guix-package: Prevent accidentally breaking guix pull.
  2020-10-13 16:00   ` Marius Bakke
@ 2020-10-14  3:17     ` Maxim Cournoyer
  2020-10-20 21:06       ` Ludovic Courtès
  2020-10-25 14:41       ` Ludovic Courtès
  2020-10-14  4:10     ` bug#43893: [PATCH v2] " Maxim Cournoyer
  1 sibling, 2 replies; 31+ messages in thread
From: Maxim Cournoyer @ 2020-10-14  3:17 UTC (permalink / raw)
  To: 43893; +Cc: Maxim Cournoyer

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=yes, Size: 9663 bytes --]

Fixes <https://issues.guix.gnu.org/43893>.

This changes the 'update-guix-package' tool so that it:

1. Always uses a clean checkout to compute the hash of the updated 'guix'
package.
2. Ensures the commit used in the updated 'guix' package definition has already
been pushed upstream.

* build-aux/update-guix-package.scm (%savannah-guix-git-repo-push-url): New
variable.
(with-input-pipe-to-string, with-temporary-git-worktree): New syntaxes.
(find-origin-remote, git-add-worktree): New procedures.
(commit-already-pushed?): New predicate.
(main): Check the commit used has already been pushed upstream and compute the
hash from a clean checkout.
* doc/contributing.texi (Updating the Guix Package): Document it.
* .dir-locals.el (scheme-mode): Fix indentation of with-temporary-git-worktree.
---
 .dir-locals.el                    |  1 +
 build-aux/update-guix-package.scm | 98 +++++++++++++++++++++----------
 doc/contributing.texi             | 43 ++++++++++++++
 3 files changed, 112 insertions(+), 30 deletions(-)

diff --git a/.dir-locals.el b/.dir-locals.el
index 7f310d2612..19f15b3e1a 100644
--- a/.dir-locals.el
+++ b/.dir-locals.el
@@ -103,6 +103,7 @@
    (eval . (put 'call-with-progress-reporter 'scheme-indent-function 1))
    (eval . (put 'with-repository 'scheme-indent-function 2))
    (eval . (put 'with-temporary-git-repository 'scheme-indent-function 2))
+   (eval . (put 'with-temporary-git-worktree 'scheme-indent-function 2))
    (eval . (put 'with-environment-variables 'scheme-indent-function 1))
    (eval . (put 'with-fresh-gnupg-setup 'scheme-indent-function 1))
 
diff --git a/build-aux/update-guix-package.scm b/build-aux/update-guix-package.scm
index f695e91cfd..9b03b06c7c 100644
--- a/build-aux/update-guix-package.scm
+++ b/build-aux/update-guix-package.scm
@@ -1,5 +1,6 @@
 ;;; GNU Guix --- Functional package management for GNU
 ;;; Copyright © 2017, 2018 Ludovic Courtès <ludo@gnu.org>
+;;; Copyright © 2020 Maxim Cournoyer <maxim.cournoyer@gmail.com>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -24,13 +25,20 @@
 ;;; Code:
 
 (use-modules (guix)
+             (guix ui)
              (guix git-download)
              (guix upstream)
              (guix utils)
              (guix base32)
              (guix build utils)
+             (guix scripts hash)
              (gnu packages package-management)
-             (ice-9 match))
+             (ice-9 match)
+             (ice-9 popen)
+             (ice-9 textual-ports)
+             (srfi srfi-1)
+             (srfi srfi-2)
+             (srfi srfi-26))
 
 (define %top-srcdir
   (string-append (current-source-directory) "/.."))
@@ -101,44 +109,74 @@ COMMIT."
       (exp
        (error "'guix' package definition is not as expected" exp)))))
 
+(define (git-add-worktree directory commit)
+  "Create a new git worktree at DIRECTORY, detached on commit COMMIT."
+  (invoke "git" "worktree" "add" "--detach" directory commit))
+
+(define-syntax-rule (with-temporary-git-worktree commit body ...)
+  "Execute BODY in the context of a temporary git worktree created from COMMIT."
+  (call-with-temporary-directory
+   (lambda (tmp-directory)
+     (dynamic-wind
+       (lambda ()
+         #t)
+       (lambda ()
+         (git-add-worktree tmp-directory commit)
+         (with-directory-excursion tmp-directory body ...))
+       (lambda ()
+         (invoke "git" "worktree" "remove" "--force" tmp-directory))))))
+
+(define %savannah-guix-git-repo-push-url
+  "git.savannah.gnu.org/srv/git/guix.git")
+
+(define-syntax-rule (with-input-pipe-to-string prog arg ...)
+  (let* ((input-pipe (open-pipe* OPEN_READ prog arg ...))
+	 (output (get-string-all input-pipe))
+	 (exit-val (status:exit-val (close-pipe input-pipe))))
+    (unless (zero? exit-val)
+      (error (format #f "Command ~s exited with non-zero exit status: ~s"
+                     (string-join (list prog arg ...)) exit-val)))
+    (string-trim-both output)))
+
+(define (find-origin-remote)
+  "Find the name of the git remote with the Savannah Guix git repo URL."
+  (and-let* ((remotes (string-split (with-input-pipe-to-string
+                                     "git" "remote" "-v")
+                                    #\newline))
+             (origin-entry (find (cut string-contains <>
+                                      (string-append
+                                       %savannah-guix-git-repo-push-url
+                                       " (push)"))
+                                 remotes)))
+    (first (string-split origin-entry #\tab))))
+
+(define (commit-already-pushed? remote commit)
+  "True if COMMIT is found in the REMOTE repository."
+  (not (string-null? (with-input-pipe-to-string
+                      "git" "branch" "-r" "--contains" commit
+                      (string-append remote "/master")))))
+
 \f
 (define (main . args)
   (match args
     ((commit version)
-     (with-store store
-       (let* ((source   (add-to-store store
-                                      "guix-checkout" ;dummy name
-                                      #t "sha256" %top-srcdir
-                                      #:select? version-controlled?))
-              (hash     (query-path-hash store source))
+     (with-directory-excursion %top-srcdir
+       (or (getenv "GUIX_ALLOW_ME_TO_USE_PRIVATE_COMMIT")
+           (commit-already-pushed? (find-origin-remote) commit)
+           (leave (G_ "Commit ~a is not pushed upstream.  Aborting.~%") commit))
+       (let* ((hash (with-temporary-git-worktree commit
+                        (nix-base32-string->bytevector
+                         (string-trim-both
+                          (with-output-to-string
+		            (lambda ()
+		              (guix-hash "-rx" ".")))))))
               (location (package-definition-location))
               (old-hash (content-hash-value
-                          (origin-hash (package-source guix)))))
+                         (origin-hash (package-source guix)))))
          (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)))))
+                                             #:version version)))))
     ((commit)
      ;; Automatically deduce the version and revision numbers.
      (main commit #f))))
diff --git a/doc/contributing.texi b/doc/contributing.texi
index af3601442e..11a932a9bf 100644
--- a/doc/contributing.texi
+++ b/doc/contributing.texi
@@ -28,6 +28,7 @@ choice.
 * Submitting Patches::          Share your work.
 * Tracking Bugs and Patches::   Using Debbugs.
 * Commit Access::               Pushing to the official repository.
+* Updating the Guix Package::   Updating the Guix package definition.
 @end menu
 
 @node Building from Git
@@ -1323,3 +1324,45 @@ only push their own awesome changes, but also offer some of their time
 @emph{reviewing} and pushing other people's changes.  As a committer,
 you're welcome to use your expertise and commit rights to help other
 contributors, too!
+
+@node Updating the Guix Package
+@section Updating the Guix Package
+
+@cindex update-guix-package, updating the guix package
+It is sometimes desirable to update the @code{guix} package itself (the
+package defined in @code{(gnu packages package-management)}), for
+example to make new daemon features available for use by the
+@code{guix-service-type} service type.  In order to simplify this task,
+the following command can be used:
+
+@example
+make update-guix-package
+@end example
+
+The @code{update-guix-package} make target will use the last known
+@emph{commit} corresponding to @code{HEAD} in your Guix checkout,
+compute the hash of the Guix sources corresponding to that commit and
+update the @code{commit}, @code{revision} and hash of the @code{guix}
+package definition.
+
+To validate that the updated @code{guix} package hashes are correct and
+that it can be built successfully, the following command can be run from
+the directory of your Guix checkout:
+
+@example
+./pre-inst-env guix build guix
+@end example
+
+To guard against accidentally updating the @code{guix} package to a
+commit that others can't refer to, a check is made that the commit used
+has already been pushed to the Savannah-hosted Guix git repository.
+
+This check can be disabled, @emph{at your own peril}, by setting the
+@code{GUIX_ALLOW_ME_TO_USE_PRIVATE_COMMIT} environment variable.
+
+To build the resulting 'guix' package when using a private commit, the
+following command can be used:
+
+@example
+./pre-inst-env guix build guix --with-git-url=guix=$PWD
+@end example
-- 
2.28.0





^ permalink raw reply	[flat|nested] 31+ messages in thread

* bug#43893: [PATCH v2] maint: update-guix-package: Prevent accidentally breaking guix pull.
  2020-10-13 16:00   ` Marius Bakke
  2020-10-14  3:17     ` bug#43893: [PATCH v3] " Maxim Cournoyer
@ 2020-10-14  4:10     ` Maxim Cournoyer
  2020-10-19 18:04       ` Maxim Cournoyer
  1 sibling, 1 reply; 31+ messages in thread
From: Maxim Cournoyer @ 2020-10-14  4:10 UTC (permalink / raw)
  To: Marius Bakke; +Cc: 43893

Hello Marius,

And thanks for the review!

[...]

>> +(define (git-add-worktree directory commit-ish)
>> +  "Create a new git worktree at DIRECTORY, detached on commit COMMIT-ISH."
>> +  (invoke "git" "worktree" "add" "--detach" directory commit-ish))
>
> Is it feasible to use Guile-Git here (given appropriate bindings)?

I had a cursory look at the guile-git sources, and it seems to miss at
least bindings for manipulating worktrees, and I'm not sure how we could
get a list of all the remotes and check their URLs to find the remote
used to push to Savannah.  Supposing we'd be able to get that remote, I
also don't know how we could query if its master branch contains a given
commit (I imagine it's doable, but it's not documented so it takes time
to figure it out :-).

[...]

>> -                              (string-append (getcwd) "/" root))
>> +(define (commit-already-pushed? remote commit)
>> +  "True if COMMIT is found in the REMOTE repository."
>> +  (not (string-null? (with-input-pipe-to-string
>> +                      "git" "branch" "-r" "--contains" commit
>> +                      (string-append remote "/master")))))
>
> ...because parsing git CLI output is error-prone and "ugly" (IMO).  But
> not a strong opinion.

I agree; but for the time being we don't have an another option.  I'd be
happy to be proven wrong.

[...]

>> +         (lambda ()
>> +           (invoke "git" "worktree" "prune")))))
>
> This is not great, because users (well, developers who run this script)
> may have worktrees that are temporarily inaccessible (e.g. on a USB
> drive or whatever).  Better to just leave the stale reference instead of
> potentially destroying users worktrees.

That's a good point.  I've improved the cleanup in v3 to only remove the
worktree it creates and no other.

> Perhaps the script could 'git clone --maxdepth=1' instead of creating
> a worktree?

I think you meant something like:

--8<---------------cut here---------------start------------->8---
$ git clone --branch the-branch --depth -1 %top-srcdir
--8<---------------cut here---------------end--------------->8---

That could work, but it's about 2x slower and more expensive than
creating a worktree (15975 syscalls vs 647, according to 'strace -c').

Thank you,

Maxim




^ permalink raw reply	[flat|nested] 31+ messages in thread

* bug#43893: [PATCH v2] maint: update-guix-package: Prevent accidentally breaking guix pull.
  2020-10-14  4:10     ` bug#43893: [PATCH v2] " Maxim Cournoyer
@ 2020-10-19 18:04       ` Maxim Cournoyer
  0 siblings, 0 replies; 31+ messages in thread
From: Maxim Cournoyer @ 2020-10-19 18:04 UTC (permalink / raw)
  To: Marius Bakke, ludo; +Cc: 43893-done

Hello!

I went ahead and merged this with commit 5800d2aae2.  I think it
provides more benefits (breaking 'guix pull' is quite bad) than
drawbacks.

Thanks for your input!

Closing,

Maxim




^ permalink raw reply	[flat|nested] 31+ messages in thread

* bug#43893: [PATCH v3] maint: update-guix-package: Prevent accidentally breaking guix pull.
  2020-10-14  3:17     ` bug#43893: [PATCH v3] " Maxim Cournoyer
@ 2020-10-20 21:06       ` Ludovic Courtès
  2020-10-21  2:36         ` Maxim Cournoyer
  2020-10-25 14:41       ` Ludovic Courtès
  1 sibling, 1 reply; 31+ messages in thread
From: Ludovic Courtès @ 2020-10-20 21:06 UTC (permalink / raw)
  To: Maxim Cournoyer; +Cc: 43893

Hi Maxim,

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

>  (define (main . args)
>    (match args
>      ((commit version)
> -     (with-store store
> -       (let* ((source   (add-to-store store
> -                                      "guix-checkout" ;dummy name
> -                                      #t "sha256" %top-srcdir
> -                                      #:select? version-controlled?))
> -              (hash     (query-path-hash store source))
> +     (with-directory-excursion %top-srcdir
> +       (or (getenv "GUIX_ALLOW_ME_TO_USE_PRIVATE_COMMIT")
> +           (commit-already-pushed? (find-origin-remote) commit)
> +           (leave (G_ "Commit ~a is not pushed upstream.  Aborting.~%") commit))
> +       (let* ((hash (with-temporary-git-worktree commit
> +                        (nix-base32-string->bytevector
> +                         (string-trim-both
> +                          (with-output-to-string
> +		            (lambda ()
> +		              (guix-hash "-rx" ".")))))))
>                (location (package-definition-location))
>                (old-hash (content-hash-value
> -                          (origin-hash (package-source guix)))))
> +                         (origin-hash (package-source guix)))))
>           (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)))))

I realize it was maybe enough to wrap this whole portion (starting from
“Re-add SOURCE”) in (unless (getenv
"GUIX_ALLOW_ME_TO_USE_PRIVATE_COMMIT") …)?  Running ‘guix build guix’
would have forced the source derivation to be built.

Anyhow, thanks for working on it!

Ludo’, who’s always wary of shelling out.




^ permalink raw reply	[flat|nested] 31+ messages in thread

* bug#43893: [PATCH v3] maint: update-guix-package: Prevent accidentally breaking guix pull.
  2020-10-20 21:06       ` Ludovic Courtès
@ 2020-10-21  2:36         ` Maxim Cournoyer
  2020-10-21  8:53           ` Ludovic Courtès
  0 siblings, 1 reply; 31+ messages in thread
From: Maxim Cournoyer @ 2020-10-21  2:36 UTC (permalink / raw)
  To: Ludovic Courtès; +Cc: 43893

Hello Ludovic,

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

> Hi Maxim,
>
> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
>
>>  (define (main . args)
>>    (match args
>>      ((commit version)
>> -     (with-store store
>> -       (let* ((source   (add-to-store store
>> -                                      "guix-checkout" ;dummy name
>> -                                      #t "sha256" %top-srcdir
>> -                                      #:select? version-controlled?))
>> -              (hash     (query-path-hash store source))
>> +     (with-directory-excursion %top-srcdir
>> +       (or (getenv "GUIX_ALLOW_ME_TO_USE_PRIVATE_COMMIT")
>> +           (commit-already-pushed? (find-origin-remote) commit)
>> +           (leave (G_ "Commit ~a is not pushed upstream.  Aborting.~%") commit))
>> +       (let* ((hash (with-temporary-git-worktree commit
>> +                        (nix-base32-string->bytevector
>> +                         (string-trim-both
>> +                          (with-output-to-string
>> +		            (lambda ()
>> +		              (guix-hash "-rx" ".")))))))
>>                (location (package-definition-location))
>>                (old-hash (content-hash-value
>> -                          (origin-hash (package-source guix)))))
>> +                         (origin-hash (package-source guix)))))
>>           (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)))))
>
> I realize it was maybe enough to wrap this whole portion (starting from
> “Re-add SOURCE”) in (unless (getenv
> "GUIX_ALLOW_ME_TO_USE_PRIVATE_COMMIT") …)?  Running ‘guix build guix’
> would have forced the source derivation to be built.
>
> Anyhow, thanks for working on it!

Sorry, I'm a bit lost.  Are you suggesting that we should restore the
code following the ;; Re-add SOURCE [...], but wrapped with unless to
make it conditional to GUIX_ALLOW_ME_TO_USE_PRIVATE_COMMIT being
defined?  That part depends on SOURCE, a store file name, which we don't
have anymore since we no longer add the sources to the store to compute
the hash.

We could add the sources to the store from the clean checkout, but I
thought one great thing about the patch was that it removed interactions
with the store, allowing for the source derivations to happen normally
when testing with 'guix build guix' (previously you'd have had to 'guix
build guix --check', as the store had silently been pre-populated with
the sources).  I see value in using the usual mechanism to get the
source rather than a side-channel, optimization hack, as it will help
ensuring correctness.

Perhaps I misunderstood your point?

Thank you,

Maxim




^ permalink raw reply	[flat|nested] 31+ messages in thread

* bug#43893: [PATCH v3] maint: update-guix-package: Prevent accidentally breaking guix pull.
  2020-10-21  2:36         ` Maxim Cournoyer
@ 2020-10-21  8:53           ` Ludovic Courtès
  2020-10-23  4:38             ` Maxim Cournoyer
  0 siblings, 1 reply; 31+ messages in thread
From: Ludovic Courtès @ 2020-10-21  8:53 UTC (permalink / raw)
  To: Maxim Cournoyer; +Cc: 43893

Hi,

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

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

[...]

>>> -         ;; 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)))))
>>
>> I realize it was maybe enough to wrap this whole portion (starting from
>> “Re-add SOURCE”) in (unless (getenv
>> "GUIX_ALLOW_ME_TO_USE_PRIVATE_COMMIT") …)?  Running ‘guix build guix’
>> would have forced the source derivation to be built.
>>
>> Anyhow, thanks for working on it!
>
> Sorry, I'm a bit lost.  Are you suggesting that we should restore the
> code following the ;; Re-add SOURCE [...], but wrapped with unless to
> make it conditional to GUIX_ALLOW_ME_TO_USE_PRIVATE_COMMIT being
> defined?  That part depends on SOURCE, a store file name, which we don't
> have anymore since we no longer add the sources to the store to compute
> the hash.

I’m saying that the solution to the initial problem (that if you don’t
run ‘guix build guix -S --check’ you’re not sure ‘guix’ is referring to
a valid upstream commit) could have been to simply not do the trick
above.  The trick at “Re-add SOURCE” is here precisely to prevent
attempts to re-download, as the comment explains, and this is what has
been causing these troubles.

The advantages (to me) would have been simpler code, no shelling out to
‘git’, and no fiddling with files under $PWD.

BTW, in ‘make release’ does ‘make update-guix-package’ and expects it to
work with a not-pushed-yet commit.  So it’s a case where we need
GUIX_ALLOW_ME_TO_USE_PRIVATE_COMMIT=yes.

Thanks,
Ludo’.




^ permalink raw reply	[flat|nested] 31+ messages in thread

* bug#43893: [PATCH v3] maint: update-guix-package: Prevent accidentally breaking guix pull.
  2020-10-21  8:53           ` Ludovic Courtès
@ 2020-10-23  4:38             ` Maxim Cournoyer
  2020-10-23 15:01               ` Ludovic Courtès
  0 siblings, 1 reply; 31+ messages in thread
From: Maxim Cournoyer @ 2020-10-23  4:38 UTC (permalink / raw)
  To: Ludovic Courtès; +Cc: 43893

Hi Ludovic,

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

> Hi,
>
> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
>
>> Ludovic Courtès <ludo@gnu.org> writes:
>

[...]

>> Sorry, I'm a bit lost.  Are you suggesting that we should restore the
>> code following the ;; Re-add SOURCE [...], but wrapped with unless to
>> make it conditional to GUIX_ALLOW_ME_TO_USE_PRIVATE_COMMIT being
>> defined?  That part depends on SOURCE, a store file name, which we don't
>> have anymore since we no longer add the sources to the store to compute
>> the hash.
>
> I’m saying that the solution to the initial problem (that if you don’t
> run ‘guix build guix -S --check’ you’re not sure ‘guix’ is referring to
> a valid upstream commit)

The original problem was about the updated Guix package containing a
faulty hash (due to being computed from a uncontrolled checkout that
could be dirty).  The other concern about preventing the use of a not
yet published commit was added based on earlier feedback.

> above.  The trick at “Re-add SOURCE” is here precisely to prevent
> attempts to re-download, as the comment explains, and this is what has
> been causing these troubles.
>
> The advantages (to me) would have been simpler code, no shelling out to
> ‘git’, and no fiddling with files under $PWD.

Less code would also mean poorer diagnostics:

time ./pre-inst-env guix build guix -S
The following derivation will be built:
   /gnu/store/zhfchz831mncd2kyqmh5d2q0r2rpg57s-guix-1.1.0-31.07c13ae-checkout.drv
building /gnu/store/zhfchz831mncd2kyqmh5d2q0r2rpg57s-guix-1.1.0-31.07c13ae-checkout.drv...
guile: warning: failed to install locale
environment variable `PATH' set to `/gnu/store/378zjf2kgajcfd7mfr98jn5xyc5wa3qv-gzip-1.10/bin:/gnu/store/sf3rbvb6iqcphgm1afbplcs72hsywg25-tar-1.32/bin'
Initialized empty Git repository in /gnu/store/02da8jb3wzzi3bqvrl214gdg0kkxmaf8-guix-1.1.0-31.07c13ae-checkout/.git/
error: Server does not allow request for unadvertised object 07c13aeb5abb1a5bc3cabffb9b2212993a0d5a0e
Failed to do a shallow fetch; retrying a full fetch...
From https://git.savannah.gnu.org/git/guix
 * [new branch]      core-updates            -> origin/core-updates
[...]
 * [new tag]               v1.1.0rc2               -> v1.1.0rc2
fatal: reference is not a tree: 07c13aeb5abb1a5bc3cabffb9b2212993a0d5a0e
git-fetch: '/gnu/store/i5b1vv7qc6l2gi4xwa9mqzjy3shvgk30-git-minimal-2.28.0/bin/git checkout 07c13aeb5abb1a5bc3cabffb9b2212993a0d5a0e' failed with exit code 128
Trying content-addressed mirror at berlin.guix.gnu.org...
Trying content-addressed mirror at berlin.guix.gnu.org...
Trying to download from Software Heritage...
builder for `/gnu/store/zhfchz831mncd2kyqmh5d2q0r2rpg57s-guix-1.1.0-31.07c13ae-checkout.drv' failed to produce output path `/gnu/store/02da8jb3wzzi3bqvrl214gdg0kkxmaf8-guix-1.1.0-31.07c13ae-checkout'
build of /gnu/store/zhfchz831mncd2kyqmh5d2q0r2rpg57s-guix-1.1.0-31.07c13ae-checkout.drv failed
View build log at '/var/log/guix/drvs/zh/fchz831mncd2kyqmh5d2q0r2rpg57s-guix-1.1.0-31.07c13ae-checkout.drv.bz2'.
guix build: error: build of `/gnu/store/zhfchz831mncd2kyqmh5d2q0r2rpg57s-guix-1.1.0-31.07c13ae-checkout.drv' failed

real    1m42.825s
user    0m2.191s
sys     0m0.189s

This took nearly 2 minutes, fetching the full Guix git repo just to tell
me that *something* is not right.

Currently, we have:

time make update-guix-package
git rev-parse HEAD
4893a1394e2eb8b97995b491f2f37ed85513a20f
./pre-inst-env "/gnu/store/i7z4pfa0c22q0qkxyl7fy2nlp3w658yg-profile/bin/guile"                  \
   ./build-aux/update-guix-package.scm  \
   "`git rev-parse HEAD`"
error: Commit 4893a1394e2eb8b97995b491f2f37ed85513a20f is not pushed upstream.  Aborting.
make: *** [Makefile:6507: update-guix-package] Error 1

real    0m1.135s
user    0m1.066s
sys     0m0.199s

> BTW, in ‘make release’ does ‘make update-guix-package’ and expects it to
> work with a not-pushed-yet commit.  So it’s a case where we need
> GUIX_ALLOW_ME_TO_USE_PRIVATE_COMMIT=yes.

Ah, good point.  I'd like to fix this, but running 'make release', it
fails on:

make[3]: *** No rule to make target 'po/doc/guix-manual.pot', needed by
'distdir-am'.  Stop.

What did I miss?

Thank you!

Maxim




^ permalink raw reply	[flat|nested] 31+ messages in thread

* bug#43893: [PATCH v3] maint: update-guix-package: Prevent accidentally breaking guix pull.
  2020-10-23  4:38             ` Maxim Cournoyer
@ 2020-10-23 15:01               ` Ludovic Courtès
  2020-10-25  4:32                 ` Maxim Cournoyer
  0 siblings, 1 reply; 31+ messages in thread
From: Ludovic Courtès @ 2020-10-23 15:01 UTC (permalink / raw)
  To: Maxim Cournoyer; +Cc: 43893

Hi Maxim,

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

> The original problem was about the updated Guix package containing a
> faulty hash (due to being computed from a uncontrolled checkout that
> could be dirty).  The other concern about preventing the use of a not
> yet published commit was added based on earlier feedback.
>
>> above.  The trick at “Re-add SOURCE” is here precisely to prevent
>> attempts to re-download, as the comment explains, and this is what has
>> been causing these troubles.
>>
>> The advantages (to me) would have been simpler code, no shelling out to
>> ‘git’, and no fiddling with files under $PWD.
>
> Less code would also mean poorer diagnostics:
>
> time ./pre-inst-env guix build guix -S
> The following derivation will be built:
>    /gnu/store/zhfchz831mncd2kyqmh5d2q0r2rpg57s-guix-1.1.0-31.07c13ae-checkout.drv
> building /gnu/store/zhfchz831mncd2kyqmh5d2q0r2rpg57s-guix-1.1.0-31.07c13ae-checkout.drv...
> guile: warning: failed to install locale
> environment variable `PATH' set to `/gnu/store/378zjf2kgajcfd7mfr98jn5xyc5wa3qv-gzip-1.10/bin:/gnu/store/sf3rbvb6iqcphgm1afbplcs72hsywg25-tar-1.32/bin'
> Initialized empty Git repository in /gnu/store/02da8jb3wzzi3bqvrl214gdg0kkxmaf8-guix-1.1.0-31.07c13ae-checkout/.git/
> error: Server does not allow request for unadvertised object 07c13aeb5abb1a5bc3cabffb9b2212993a0d5a0e
> Failed to do a shallow fetch; retrying a full fetch...
> From https://git.savannah.gnu.org/git/guix
>  * [new branch]      core-updates            -> origin/core-updates
> [...]
>  * [new tag]               v1.1.0rc2               -> v1.1.0rc2
> fatal: reference is not a tree: 07c13aeb5abb1a5bc3cabffb9b2212993a0d5a0e
> git-fetch: '/gnu/store/i5b1vv7qc6l2gi4xwa9mqzjy3shvgk30-git-minimal-2.28.0/bin/git checkout 07c13aeb5abb1a5bc3cabffb9b2212993a0d5a0e' failed with exit code 128
> Trying content-addressed mirror at berlin.guix.gnu.org...
> Trying content-addressed mirror at berlin.guix.gnu.org...
> Trying to download from Software Heritage...
> builder for `/gnu/store/zhfchz831mncd2kyqmh5d2q0r2rpg57s-guix-1.1.0-31.07c13ae-checkout.drv' failed to produce output path `/gnu/store/02da8jb3wzzi3bqvrl214gdg0kkxmaf8-guix-1.1.0-31.07c13ae-checkout'
> build of /gnu/store/zhfchz831mncd2kyqmh5d2q0r2rpg57s-guix-1.1.0-31.07c13ae-checkout.drv failed
> View build log at '/var/log/guix/drvs/zh/fchz831mncd2kyqmh5d2q0r2rpg57s-guix-1.1.0-31.07c13ae-checkout.drv.bz2'.
> guix build: error: build of `/gnu/store/zhfchz831mncd2kyqmh5d2q0r2rpg57s-guix-1.1.0-31.07c13ae-checkout.drv' failed
>
> real    1m42.825s
> user    0m2.191s
> sys     0m0.189s
>
> This took nearly 2 minutes, fetching the full Guix git repo just to tell
> me that *something* is not right.
>
> Currently, we have:
>
> time make update-guix-package
> git rev-parse HEAD
> 4893a1394e2eb8b97995b491f2f37ed85513a20f
> ./pre-inst-env "/gnu/store/i7z4pfa0c22q0qkxyl7fy2nlp3w658yg-profile/bin/guile"                  \
>    ./build-aux/update-guix-package.scm  \
>    "`git rev-parse HEAD`"
> error: Commit 4893a1394e2eb8b97995b491f2f37ed85513a20f is not pushed upstream.  Aborting.
> make: *** [Makefile:6507: update-guix-package] Error 1

I agree that the better diagnostic is nice.  Though it’s a script that’s
essentially for a handful of people, who can certainly cope with the
ugly error.

Anyway, I think we didn’t analyze the initial situation well enough
(myself included, by not commenting early and accurately).  I’m also not
fond of the added complexity and the risk of surprises when we make the
release, but OTOH, it’s no big deal in the big picture!

>> BTW, in ‘make release’ does ‘make update-guix-package’ and expects it to
>> work with a not-pushed-yet commit.  So it’s a case where we need
>> GUIX_ALLOW_ME_TO_USE_PRIVATE_COMMIT=yes.
>
> Ah, good point.  I'd like to fix this,

It’s mostly about setting this variable at the right place in
Makefile.am.

> but running 'make release', it fails on:
>
> make[3]: *** No rule to make target 'po/doc/guix-manual.pot', needed by
> 'distdir-am'.  Stop.
>
> What did I miss?

Probably you need ./bootstrap to generate the POT files.

Thanks,
Ludo’.




^ permalink raw reply	[flat|nested] 31+ messages in thread

* bug#43893: [PATCH v3] maint: update-guix-package: Prevent accidentally breaking guix pull.
  2020-10-23 15:01               ` Ludovic Courtès
@ 2020-10-25  4:32                 ` Maxim Cournoyer
  2020-10-25 14:50                   ` Ludovic Courtès
  0 siblings, 1 reply; 31+ messages in thread
From: Maxim Cournoyer @ 2020-10-25  4:32 UTC (permalink / raw)
  To: Ludovic Courtès; +Cc: 43893

Hi Ludovic,

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

[...]

>> Currently, we have:
>>
>> time make update-guix-package
>> git rev-parse HEAD
>> 4893a1394e2eb8b97995b491f2f37ed85513a20f
>> ./pre-inst-env "/gnu/store/i7z4pfa0c22q0qkxyl7fy2nlp3w658yg-profile/bin/guile"                  \
>>    ./build-aux/update-guix-package.scm  \
>>    "`git rev-parse HEAD`"
>> error: Commit 4893a1394e2eb8b97995b491f2f37ed85513a20f is not pushed upstream.  Aborting.
>> make: *** [Makefile:6507: update-guix-package] Error 1
>
> I agree that the better diagnostic is nice.  Though it’s a script that’s
> essentially for a handful of people, who can certainly cope with the
> ugly error.
>
> Anyway, I think we didn’t analyze the initial situation well enough
> (myself included, by not commenting early and accurately).  I’m also not
> fond of the added complexity and the risk of surprises when we make the
> release, but OTOH, it’s no big deal in the big picture!

I'm sorry but I don't agree with the "we didn't analyze the initial
situation well enough"; if I had to think about the best way to solve
this problem now, I'd still choose the way that was chosen then, as it
provides the best guarantee against producing a broken Guix package,
something that happened a couple times in the past, judging from git
log.  About complexity, I'd much rather the tool break on me than
breaking 'guix pull' for everyone :-).

It seems we'll have to disagree on this one; but as you said, it's a
tiny part of the bigger landscape!

>>> BTW, in ‘make release’ does ‘make update-guix-package’ and expects it to
>>> work with a not-pushed-yet commit.  So it’s a case where we need
>>> GUIX_ALLOW_ME_TO_USE_PRIVATE_COMMIT=yes.

I want to be able to run 'make release' first to test this works
correctly, but even after rebuilding my source tree from scratch
(following a 'make distclean'), and also attempting 'make download-po',
and following release.org from guix-maintenance, I still get:

make[3]: *** No rule to make target 'po/doc/guix-manual.pot', needed by 'distdir-am'.  Stop.
make[3]: Leaving directory '/home/maxim/src/guix'
make[2]: *** [Makefile:5521: distdir] Error 2
make[2]: Leaving directory '/home/maxim/src/guix'
make[1]: *** [Makefile:5630: dist] Error 2
make[1]: Leaving directory '/home/maxim/src/guix'
make: *** [Makefile:6410: dist-with-updated-version] Error 2

Can you reproduce this problem?

Thank you,

Maxim




^ permalink raw reply	[flat|nested] 31+ messages in thread

* bug#43893: [PATCH v3] maint: update-guix-package: Prevent accidentally breaking guix pull.
  2020-10-14  3:17     ` bug#43893: [PATCH v3] " Maxim Cournoyer
  2020-10-20 21:06       ` Ludovic Courtès
@ 2020-10-25 14:41       ` Ludovic Courtès
  2020-10-25 19:17         ` Maxim Cournoyer
  1 sibling, 1 reply; 31+ messages in thread
From: Ludovic Courtès @ 2020-10-25 14:41 UTC (permalink / raw)
  To: Maxim Cournoyer; +Cc: 43893

Hi,

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

> +(define %savannah-guix-git-repo-push-url
> +  "git.savannah.gnu.org/srv/git/guix.git")

[...]

> +(define (find-origin-remote)
> +  "Find the name of the git remote with the Savannah Guix git repo URL."
> +  (and-let* ((remotes (string-split (with-input-pipe-to-string
> +                                     "git" "remote" "-v")
> +                                    #\newline))
> +             (origin-entry (find (cut string-contains <>
> +                                      (string-append
> +                                       %savannah-guix-git-repo-push-url
> +                                       " (push)"))
> +                                 remotes)))
> +    (first (string-split origin-entry #\tab))))

I noticed that this returns #f for me because I’m using git.sv.gnu.org,
not git.savannah.gnu.org.

Initially I thought it would break due to i18n, but it seems that the
string “push” is not translated (currently).

Ludo’.




^ permalink raw reply	[flat|nested] 31+ messages in thread

* bug#43893: [PATCH v3] maint: update-guix-package: Prevent accidentally breaking guix pull.
  2020-10-25  4:32                 ` Maxim Cournoyer
@ 2020-10-25 14:50                   ` Ludovic Courtès
  2020-10-25 15:29                     ` Ludovic Courtès
  0 siblings, 1 reply; 31+ messages in thread
From: Ludovic Courtès @ 2020-10-25 14:50 UTC (permalink / raw)
  To: Maxim Cournoyer; +Cc: 43893

Hi Maxim,

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

> Ludovic Courtès <ludo@gnu.org> writes:
>
> [...]
>
>>> Currently, we have:
>>>
>>> time make update-guix-package
>>> git rev-parse HEAD
>>> 4893a1394e2eb8b97995b491f2f37ed85513a20f
>>> ./pre-inst-env "/gnu/store/i7z4pfa0c22q0qkxyl7fy2nlp3w658yg-profile/bin/guile"                  \
>>>    ./build-aux/update-guix-package.scm  \
>>>    "`git rev-parse HEAD`"
>>> error: Commit 4893a1394e2eb8b97995b491f2f37ed85513a20f is not pushed upstream.  Aborting.
>>> make: *** [Makefile:6507: update-guix-package] Error 1
>>
>> I agree that the better diagnostic is nice.  Though it’s a script that’s
>> essentially for a handful of people, who can certainly cope with the
>> ugly error.
>>
>> Anyway, I think we didn’t analyze the initial situation well enough
>> (myself included, by not commenting early and accurately).  I’m also not
>> fond of the added complexity and the risk of surprises when we make the
>> release, but OTOH, it’s no big deal in the big picture!
>
> I'm sorry but I don't agree with the "we didn't analyze the initial
> situation well enough";

The reason I wrote that is that we had overlooked the fact that
‘update-guix-package’ purposefully allows non-upstream commits, and
that’s what allows ‘make release’ to work.

It didn’t occur to me at the time, but the simplest path would have been
to conditionalize the bit that makes it possible to refer to
non-upstream commits.

> if I had to think about the best way to solve this problem now, I'd
> still choose the way that was chosen then, as it provides the best
> guarantee against producing a broken Guix package, something that
> happened a couple times in the past, judging from git log.  About
> complexity, I'd much rather the tool break on me than breaking 'guix
> pull' for everyone :-).

I agree that addressing this problem was in order.  :-)  The added
complexity brings its own set of (less serious) issues though.

>>>> BTW, in ‘make release’ does ‘make update-guix-package’ and expects it to
>>>> work with a not-pushed-yet commit.  So it’s a case where we need
>>>> GUIX_ALLOW_ME_TO_USE_PRIVATE_COMMIT=yes.
>
> I want to be able to run 'make release' first to test this works
> correctly, but even after rebuilding my source tree from scratch
> (following a 'make distclean'), and also attempting 'make download-po',
> and following release.org from guix-maintenance, I still get:
>
> make[3]: *** No rule to make target 'po/doc/guix-manual.pot', needed by 'distdir-am'.  Stop.

Oh my bad; the solution appears to be:

  make doc-pot-update

Lemme know how it goes!

Ludo’.




^ permalink raw reply	[flat|nested] 31+ messages in thread

* bug#43893: [PATCH v3] maint: update-guix-package: Prevent accidentally breaking guix pull.
  2020-10-25 14:50                   ` Ludovic Courtès
@ 2020-10-25 15:29                     ` Ludovic Courtès
  2020-10-31  3:56                       ` Maxim Cournoyer
  0 siblings, 1 reply; 31+ messages in thread
From: Ludovic Courtès @ 2020-10-25 15:29 UTC (permalink / raw)
  To: Maxim Cournoyer; +Cc: 43893

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

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


[-- Attachment #2: the code --]
[-- Type: text/plain, Size: 6871 bytes --]

;;; GNU Guix --- Functional package management for GNU
;;; Copyright © 2017, 2018 Ludovic Courtès <ludo@gnu.org>
;;; Copyright © 2020 Maxim Cournoyer <maxim.cournoyer@gmail.com>
;;;
;;; This file is part of GNU Guix.
;;;
;;; GNU Guix is free software; you can redistribute it and/or modify it
;;; under the terms of the GNU General Public License as published by
;;; the Free Software Foundation; either version 3 of the License, or (at
;;; your option) any later version.
;;;
;;; GNU Guix is distributed in the hope that it will be useful, but
;;; WITHOUT ANY WARRANTY; without even the implied warranty of
;;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
;;; GNU General Public License for more details.
;;;
;;; You should have received a copy of the GNU General Public License
;;; along with GNU Guix.  If not, see <http://www.gnu.org/licenses/>.

;;; Commentary:
;;;
;;; This scripts updates the definition of the 'guix' package in Guix for the
;;; current commit.  It requires Git to be installed.
;;;
;;; Code:

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

(define %top-srcdir
  (string-append (current-source-directory) "/.."))

(define version-controlled?
  (git-predicate %top-srcdir))

(define (package-definition-location)
  "Return the source properties of the definition of the 'guix' package."
  (call-with-input-file (location-file (package-location guix))
    (lambda (port)
      (let loop ()
        (match (read port)
          ((? eof-object?)
           (error "definition of 'guix' package could not be found"
                  (port-filename port)))
          (('define-public 'guix value)
           (source-properties value))
          (_
           (loop)))))))

(define* (update-definition commit hash
                            #:key version old-hash)
  "Return a one-argument procedure that takes a string, the definition of the
'guix' package, and returns a string, the update definition for VERSION,
COMMIT."
  (define (linear-offset str line column)
    ;; Return the offset in characters to reach LINE and COLUMN (both
    ;; zero-indexed) in STR.
    (call-with-input-string str
      (lambda (port)
        (let loop ((offset 0))
          (cond ((and (= (port-column port) column)
                      (= (port-line port) line))
                 offset)
                ((eof-object? (read-char port))
                 (error "line and column not reached!"
                        str))
                (else
                 (loop (+ 1 offset))))))))

  (define (update-hash str)
    ;; Replace OLD-HASH with HASH in STR.
    (string-replace-substring str
                              (bytevector->nix-base32-string old-hash)
                              (bytevector->nix-base32-string hash)))

  (lambda (str)
    (match (call-with-input-string str read)
      (('let (('version old-version)
              ('commit old-commit)
              ('revision old-revision))
         defn)
       (let* ((location (source-properties defn))
              (line     (assq-ref location 'line))
              (column   0)
              (offset   (linear-offset str line column)))
         (string-append (format #f "(let ((version \"~a\")
        (commit \"~a\")
        (revision ~a))\n"
                                (or version old-version)
                                commit
                                (if (and version
                                         (not (string=? version old-version)))
                                    0
                                    (+ 1 old-revision)))
                        (string-drop (update-hash str) offset))))
      (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))

\f
(define (main . args)
  (match args
    ((commit version)
     (with-store store
       (let* ((source   (add-to-store store
                                      "guix-checkout" ;dummy name
                                      #t "sha256" %top-srcdir
                                      #:select? version-controlled?))
              (hash     (query-path-hash store source))
              (location (package-definition-location))
              (old-hash (content-hash-value
                         (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))

         (when (getenv "GUIX_ALLOW_ME_TO_USE_PRIVATE_COMMIT")
           (keep-source-in-store store source)))))
    ((commit)
     ;; Automatically deduce the version and revision numbers.
     (main commit #f))))

(apply main (cdr (command-line)))

^ permalink raw reply	[flat|nested] 31+ messages in thread

* bug#43893: [PATCH v3] maint: update-guix-package: Prevent accidentally breaking guix pull.
  2020-10-25 14:41       ` Ludovic Courtès
@ 2020-10-25 19:17         ` Maxim Cournoyer
  0 siblings, 0 replies; 31+ messages in thread
From: Maxim Cournoyer @ 2020-10-25 19:17 UTC (permalink / raw)
  To: Ludovic Courtès; +Cc: 43893

Hey Ludo,

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

> Hi,
>
> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
>
>> +(define %savannah-guix-git-repo-push-url
>> +  "git.savannah.gnu.org/srv/git/guix.git")
>
> [...]
>
>> +(define (find-origin-remote)
>> +  "Find the name of the git remote with the Savannah Guix git repo URL."
>> +  (and-let* ((remotes (string-split (with-input-pipe-to-string
>> +                                     "git" "remote" "-v")
>> +                                    #\newline))
>> +             (origin-entry (find (cut string-contains <>
>> +                                      (string-append
>> +                                       %savannah-guix-git-repo-push-url
>> +                                       " (push)"))
>> +                                 remotes)))
>> +    (first (string-split origin-entry #\tab))))
>
> I noticed that this returns #f for me because I’m using git.sv.gnu.org,
> not git.savannah.gnu.org.

Thank for the report!  It should be fixed with
13a3b9c748a80c0d4c79450e479416480273d2f7.  Feel free to edit the
%savannah-guix-git-repo-push-url-regexp regexp if there are more
variants that we're still missing.

Maxim




^ permalink raw reply	[flat|nested] 31+ messages in thread

* bug#43893: [PATCH v3] maint: update-guix-package: Prevent accidentally breaking guix pull.
  2020-10-25 15:29                     ` Ludovic Courtès
@ 2020-10-31  3:56                       ` Maxim Cournoyer
  2020-10-31 10:42                         ` Ludovic Courtès
  0 siblings, 1 reply; 31+ messages in thread
From: Maxim Cournoyer @ 2020-10-31  3:56 UTC (permalink / raw)
  To: Ludovic Courtès; +Cc: 43893

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

> -\f
> +(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




^ permalink raw reply	[flat|nested] 31+ messages in thread

* bug#43893: [PATCH v3] maint: update-guix-package: Prevent accidentally breaking guix pull.
  2020-10-31  3:56                       ` Maxim Cournoyer
@ 2020-10-31 10:42                         ` Ludovic Courtès
  0 siblings, 0 replies; 31+ messages in thread
From: Ludovic Courtès @ 2020-10-31 10:42 UTC (permalink / raw)
  To: Maxim Cournoyer; +Cc: 43893

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




^ permalink raw reply	[flat|nested] 31+ messages in thread

end of thread, other threads:[~2020-10-31 10:44 UTC | newest]

Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-09 21:58 bug#43893: make update-guix-package produced an incorrect hash Maxim Cournoyer
2020-10-10  0:04 ` Danny Milosavljevic
2020-10-10  5:08   ` Maxim Cournoyer
2020-10-10  5:08 ` bug#43893: [PATCH] maint: update-guix-package: Ensure sources are clean Maxim Cournoyer
2020-10-10 11:59   ` Danny Milosavljevic
2020-10-11  2:35     ` Maxim Cournoyer
2020-10-10 20:08   ` Ludovic Courtès
2020-10-10 21:14     ` Danny Milosavljevic
2020-10-12  4:40       ` Maxim Cournoyer
2020-10-12  9:40       ` Ludovic Courtès
2020-10-12 14:18         ` Danny Milosavljevic
2020-10-11 19:43     ` Maxim Cournoyer
2020-10-12  9:43       ` Ludovic Courtès
2020-10-13  1:33         ` Maxim Cournoyer
2020-10-11 19:57 ` bug#43893: [PATCH v2] maint: update-guix-package: Prevent accidentally breaking guix pull Maxim Cournoyer
2020-10-13 16:00   ` Marius Bakke
2020-10-14  3:17     ` bug#43893: [PATCH v3] " Maxim Cournoyer
2020-10-20 21:06       ` Ludovic Courtès
2020-10-21  2:36         ` Maxim Cournoyer
2020-10-21  8:53           ` Ludovic Courtès
2020-10-23  4:38             ` Maxim Cournoyer
2020-10-23 15:01               ` Ludovic Courtès
2020-10-25  4:32                 ` Maxim Cournoyer
2020-10-25 14:50                   ` Ludovic Courtès
2020-10-25 15:29                     ` Ludovic Courtès
2020-10-31  3:56                       ` Maxim Cournoyer
2020-10-31 10:42                         ` Ludovic Courtès
2020-10-25 14:41       ` Ludovic Courtès
2020-10-25 19:17         ` Maxim Cournoyer
2020-10-14  4:10     ` bug#43893: [PATCH v2] " Maxim Cournoyer
2020-10-19 18:04       ` Maxim Cournoyer

unofficial mirror of bug-guix@gnu.org 

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://yhetil.org/guix-bugs/0 guix-bugs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 guix-bugs guix-bugs/ https://yhetil.org/guix-bugs \
		bug-guix@gnu.org
	public-inbox-index guix-bugs

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.yhetil.org/yhetil.gnu.guix.bugs
	nntp://news.gmane.io/gmane.comp.gnu.guix.bugs


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git