make update-guix-package produced an incorrect hash

DoneSubmitted by Maxim Cournoyer.
Details
4 participants
  • Danny Milosavljevic
  • Ludovic Courtès
  • Marius Bakke
  • Maxim Cournoyer
Owner
unassigned
Severity
normal
M
M
Maxim Cournoyer wrote on 9 Oct 23:58 +0200
(name . bug-guix)(address . bug-guix@gnu.org)
87eem7qcxc.fsf@gmail.com
Hello,
Today, I used 'make update-guix-package' and pushed the result (commitf08587682a). Users later reported being unable to pull because of ahash mismatch on the newly updated Guix package :-/.
I haven't investigated why yet, and simply reverted the faulty updatefor now in commit a279f7c61c.
Thank you,
Maxim
D
D
Danny Milosavljevic wrote on 10 Oct 02:04 +0200
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)(address . 43893@debbugs.gnu.org)
20201010020410.3a301654@scratchpost.org
I'm guessing it has something to do with update-guix-package using git-predicateto add only git-known (but not necessarily committed) files to the store and thencalculating the checksum of that--but the git-fetch for the guix package notnecessarily doing the same.
Then update-guix-package.scm does one worse and actively prevents guix fromdoing the checkout from git when building that "guix" package. That means theperson invoking update-guix-package.scm can't notice even when the sha256 hashis definitely wrong--because guix will have the source for package "guix" inthe store already (a faked entry added by update-guix-package.scm) and thuswon'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 thepackage "guix" to commit A, at that point you can't know what commit hashcommit A will have (since you haven't committed it yet) and yet you haveto know the commit hash of commit A in order to write it into the packagedefinition 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" packageinside, 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 aUSER-SPECIFIED COMMIT HASH (the latter is determined by the user viagit rev-parse HEAD).(3) Then, it commits that changed working copy as commit B. Commit B isessentially not referred-to by anyone--it's just to make it to thegit 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) actuallyrefers 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 actuallycommit that) before invoking
make update-guix-package
the commit it refers to in the "guix" package will be one which cannot beresolved by users.
Worse, if you change anything but not commit it (even locally), then thatsurely counts as "part of the checkout" for make update-guix-package, sothe hash will be calculated including those change--but the changes arenot committed, so no one can build the resulting guix package (becauseof 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 sha256hash will include those, but the commit id won't.
Long story short, we should make "make update-guix-package" check foruncommitted changes in the working copy, and fail if any such exist[1].There are no downsides that I can see. Even building from local workingcopy still works then.
Also, let's please document update-guix-package.
[1] git diff-index --quiet HEAD || echo fail
-----BEGIN PGP SIGNATURE-----
iQEzBAEBCgAdFiEEds7GsXJ0tGXALbPZ5xo1VCwwuqUFAl+A+noACgkQ5xo1VCwwuqXeZgf+MqjmvegOlqH7NPQKPtzT5Yyc5EqplXtTY9pYnKKV5wZj1f2MHpTCLoTqvQI1aiIEMYkLhmE+AXgh1nM0QZ+ENE7n8Gh/QpdY52Wg4IJ88xJzT2ifQ8z9qURmVyxwhkGWb/XschF/GhQ6nX4pl2Tu7KR/fTDzS0ROtpcBhKTD7ckIKTAGUYWTRQ4Su9Ungnkm7rfA7I8oj2cPjiIdj6gRniGLrdAfIDhQglYBtHSUabte2fO3crploVOjy9FAxrG+ZXF3Er51+k95yJ28sz8gnsWKDCzs8GKxwyJYKOjllwq5oUkdJW+X9UpPj3RxVrqqPVYsbIoZfxj/jKUvYqRehg===EbQx-----END PGP SIGNATURE-----

M
M
Maxim Cournoyer wrote on 10 Oct 07:08 +0200
(name . Danny Milosavljevic)(address . dannym@scratchpost.org)(address . 43893@debbugs.gnu.org)
874kn2r7lf.fsf@gmail.com
Hello Danny,
Thanks for the quick investigation.
Danny Milosavljevic <dannym@scratchpost.org> writes:
Toggle quote (5 lines)> 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 treelooks clean (no modified git-tracked files), but I had in factmodifications made to .dir-locals that I've been testing and these werehidden 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 pickedup with my invisible changes, causing the hash mismatch.
Toggle quote (27 lines)> 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 haveto 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'dchange the hashes, breaking the computed Guix hash). That's how I broke'guix pull' the first time I used 'make update-guix-package' :-). But Ithink it's inevitable, so perhaps the best we can do is documement itwell and print a warning when running the target.
Toggle quote (3 lines)> 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 athing?
Toggle quote (15 lines)> @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.
[...]
Toggle quote (5 lines)> 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 cleanworktree to do the computation, because that's even safer as it preventssubtle things like "git update-index --skip-worktree some/path" frominteracting with the computed hash too.
Toggle quote (2 lines)> Also, let's please document update-guix-package.
I'll send a first commit. I haven't found a way to build it locallywith the command in the message; it seems to create a cycle. Let meknow what you think.
Maxim
D
D
Danny Milosavljevic wrote on 10 Oct 13:59 +0200
Re: bug#43893: [PATCH] maint: update-guix-package: Ensure sources are clean.
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)(address . 43893@debbugs.gnu.org)
20201010135913.6d0e02d5@scratchpost.org
Hi Maxim,
hmm, git worktree can fail if the commit already is checked out somewhere (forexample if you invoke make update-guix-package twice in a row), or if the userused git worktree on that repo for that commit for other purposes. That wouldmean that
make update-guix-package
would fail in weird undocumented ways again. Please please let's documentstuff 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 aguix package of committed stuff only (which is NOT what your working copy isactually 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 areuncommitted changes that are now not included in the "guix" package.
Other than that, okay.
Toggle quote (3 lines)>#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 meansyou can't test with local-only commits, you'd always have to push. Weshould find out why this doesn't work and fix it.
-----BEGIN PGP SIGNATURE-----
iQEzBAEBCgAdFiEEds7GsXJ0tGXALbPZ5xo1VCwwuqUFAl+BohEACgkQ5xo1VCwwuqWhjQf/csexxtAxHPK5c56adbbkonAjGkOTJ+D9THjRt14TPiUH84HMYhhT02SdpLpl1I5ZQX/5sxsoXj5xWcMC2LEZGVP9wgqieXVTx9GA8i+HDatco7YVKKKW/22nZAAW49MFf2zT3rlOEedWeCFXWOk1B+jy+odSDp7KHceIsDvl7uBYTTvqqU5J4QpcNPmXXv4Cr/Itg84VlGkjfqmLUwbY4VSk3diexP/kApcb6p/e4d44Ld06LoNGGECR+i5rRLvjYQ+4dGM5Xp61juNoAIciqcMBRUTUmGT2+YpIcJG8WfGIWx946lV5XPfhi2EDgAMc9WJmwRczooyXR6jM7VIWcA===q0DZ-----END PGP SIGNATURE-----

L
L
Ludovic Courtès wrote on 10 Oct 22:08 +0200
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)(address . 43893@debbugs.gnu.org)
871ri5j13g.fsf@gnu.org
Hi,
Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
Toggle quote (8 lines)> 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. :-)
Toggle quote (4 lines)> * 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 myback.
Toggle quote (5 lines)> - ;; 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?
Toggle quote (4 lines)> + (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 useGuile-Git to (1) check whether we’re on a dirty tree and stop right awayif we are, and (2) check whether the commit exists in the official Gitrepo and error out if it doesn’t, unlessGUIX_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 needsto be addressed!
Ludo’.
D
D
Danny Milosavljevic wrote on 10 Oct 23:14 +0200
(name . Ludovic Courtès)(address . ludo@gnu.org)
20201010231416.6322dea1@scratchpost.org
Hi Ludo,
On Sat, 10 Oct 2020 22:08:19 +0200Ludovic Courtès <ludo@gnu.org> wrote:
Toggle quote (7 lines)> 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->manifestlocation: /tmp/guix-build-guix-1.1.0-30.5918cb5.drv-0/source/tests/channels.scm:233source:+ (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: #factual-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 2builder for `/gnu/store/csagsyh01rq7ilqkcpaa2d7vp1bn41w3-guix-1.1.0-30.5918cb5.drv' failed with exit code 1build of /gnu/store/csagsyh01rq7ilqkcpaa2d7vp1bn41w3-guix-1.1.0-30.5918cb5.drv failedView 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.
-----BEGIN PGP SIGNATURE-----
iQEzBAEBCgAdFiEEds7GsXJ0tGXALbPZ5xo1VCwwuqUFAl+CJCgACgkQ5xo1VCwwuqUvGgf9EY0PMR+deakwHrEkZprVMt2c5E2/robHJBxFn+FtYBCEw/SfUrkee/NrTbr9jx1bZpsuP/mqZYgJ+ZJD51A067VM6gnGVs/02nsJmB8QgunyXDDL7BAjHN4CeF/fglRcrhRF34/t6BA1ARdq2O2re+l09vuKiHgxXEcvkNacw9uMD5QkKrPJ7gaVfLk0PI+34JjTd0FM+zz5nmrkDf9VW60qwS7kffP0hSyISJxBRNNGWny+xk9VGZYDQjMctH9o+Y9jAKchyz+ex5mTrktdhSuYGQsio3UUHkdIy9SCu5cWdJyJiD6L2FHb2wN2V8hUnl4coL4RKDqEarQ0gnoOgQ===fgYz-----END PGP SIGNATURE-----

M
M
Maxim Cournoyer wrote on 11 Oct 04:35 +0200
(address . 43893@debbugs.gnu.org)
87eem5trpx.fsf@gmail.com
Hi Danny,
Danny Milosavljevic <dannym@scratchpost.org> writes:
Toggle quote (9 lines)> 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 ofthe same commit.
Toggle quote (5 lines)> 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 tocommit X, which is the latest commit in your tree, and doesn't havetechnical reasons preventing it from doing so successfully, why enforcea clean tree?
Using a worktree to setup a pristine checkout also covers for uses suchas 'git update-index --skip-worktree some-file-or-path' or 'gitupdate-index --assume-unchanged some-file-or-path', which would goundetected by checking for modifications with 'git diff-index --quietHEAD'. We'd also have to check for the output of
Toggle quote (4 lines)> 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 upgradingthe guix package definition to a state that doesn't exist as a commit.
Toggle quote (3 lines)> 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 thehash, which is problematic as we saw.
Toggle quote (9 lines)> 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 isthat the only output I had for multiple minutes was:
Toggle snippet (3 lines)updating checkout of 'file:///home/maxim/src/guix'...
And the CPU peaked and the RAM stayed around 1 GiB while it was doingits magic.
Maxim
M
M
Maxim Cournoyer wrote on 11 Oct 21:43 +0200
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 43893@debbugs.gnu.org)
875z7gtupi.fsf@gmail.com
Hello!
Ludovic Courtès <ludo@gnu.org> writes:
[...]
Toggle quote (7 lines)>> * 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 (itdoesn't leave traces in my v2 as 'git worktree prune' clears up thetemporary worktree entries), and it's quite cheap to create.
Toggle quote (7 lines)>> - ;; 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.
[...]
Toggle quote (6 lines)> 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 madeunnecessary; as long as the tool clearly says which commit will be usedfor the updated Guix package, I don't see why we should force thedeveloper to stash their changes (in the same way git checkout doesn'tforces you to do so in the absence of conflicts).
Also, this bug reported was triggered by something like:
Toggle snippet (5 lines)echo ";;some comment" >> .dir-locals \ && git update-index --skip-worktree .dir-locals \ && make update-guix-package)
leading 'update-guix-package' to produce a wrong hash. Git has beentold to ignore changes on .dir-locals so checking for changes would nothave been sufficient (you'd also need to 'git ls-files -v' and interpretthe result).
Toggle quote (4 lines)> 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
M
M
Maxim Cournoyer wrote on 12 Oct 06:40 +0200
(name . Danny Milosavljevic)(address . dannym@scratchpost.org)
87r1q4rr93.fsf@gmail.com
Hello Danny,
Danny Milosavljevic <dannym@scratchpost.org> writes:
Toggle quote (2 lines)> Hi Ludo,
[...]
Toggle quote (18 lines)> 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"
[...]
Toggle quote (21 lines)> 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 athttps://issues.guix.gnu.org/43940and marked the test as expected tofail for now so that we can continue to update our Guix package.
The current guix package was using4e3ed9bad9ed5758cdee6e636805f65e9ab710eb, which didn't had that problem.
Thanks,
Maxim
M
M
Maxim Cournoyer wrote on 12 Oct 06:41 +0200
control message for bug #43893
(address . control@debbugs.gnu.org)
87pn5orr8f.fsf@gmail.com
tags 43893 + patchquit
L
L
Ludovic Courtès wrote on 12 Oct 11:40 +0200
Re: bug#43893: [PATCH] maint: update-guix-package: Ensure sources are clean.
(name . Danny Milosavljevic)(address . dannym@scratchpost.org)
87r1q3vl1z.fsf@gnu.org
Danny Milosavljevic <dannym@scratchpost.org> skribis:
Toggle quote (2 lines)> 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’.
L
L
Ludovic Courtès wrote on 12 Oct 11:43 +0200
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)(address . 43893@debbugs.gnu.org)
87lfgbvky6.fsf@gnu.org
Hi,
Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
Toggle quote (15 lines)> 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 onetouching it; it’s more about the mental model, I guess.
Thanks,Ludo’.
D
D
Danny Milosavljevic wrote on 12 Oct 16:18 +0200
(name . Ludovic Courtès)(address . ludo@gnu.org)
20201012161853.0ae2308f@scratchpost.org
Hi Ludo,
On Mon, 12 Oct 2020 11:40:56 +0200Ludovic Courtès <ludo@gnu.org> wrote:
Toggle quote (7 lines)> 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 causedby "--with-git-url")
-----BEGIN PGP SIGNATURE-----
iQEzBAEBCgAdFiEEds7GsXJ0tGXALbPZ5xo1VCwwuqUFAl+EZc0ACgkQ5xo1VCwwuqU39Af5AZ8Gy4GNsL/+/4W+jV+jVCMXU2EX5+hWhA6Pe0QfmT1Dw+WoKPRE28Be2sEbnpHLm6GNLaRxugZRLwMFvLHShd+aO0vQS5/q+WLIO6tEufQMOeVPsqSoGQJz9XlR8nl/uYcVsS0aqYiD+fKpUmfOy3fv3XySZgoQyVXT3+r44INmpHKHLfef0hA68ffCu/46j5OAeEFJo7KokNZXf4XqIVQ3IuGM9Pwsexs7w4YfC1CI+j0OvKdBejE+IxnHhnJdkF8gb4rdqVvNvjh+aYK7jxmPoRt1X4W3RGRmXCHsmH/4obrUYiFEW7F/A9GgbCyF3/n8YydlsI0MgzuMFISurg===FKY2-----END PGP SIGNATURE-----

M
M
Maxim Cournoyer wrote on 13 Oct 03:33 +0200
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 43893@debbugs.gnu.org)
87blh69aft.fsf@gmail.com
Hi Ludovic,
Ludovic Courtès <ludo@gnu.org> writes:
Toggle quote (22 lines)> 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 dealhere, given you wouldn't be able to notice a new worktree unless youwatch the output of 'git worktree list' in a loop while updating theguix package ;-).
I think the change brings good guarantees to guard against breaking'guix pull' with a 'make update-guix-package' as it is, and alsodocuments the thing. If it's OK with you I'd like to merge it in thecoming days.
Thanks,
Maxim
M
M
Marius Bakke wrote on 13 Oct 18:00 +0200
Re: bug#43893: [PATCH v2] maint: update-guix-package: Prevent accidentally breaking guix pull.
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
87imbedsko.fsf@gnu.org
Maxim Cournoyer <maxim.cournoyer@gmail.com> writes:
Toggle quote (18 lines)> 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.
[...]
Toggle quote (27 lines)> (define %top-srcdir> (string-append (current-source-directory) "/.."))> @@ -101,44 +109,69 @@ COMMIT."> (exp> (error "'guix' package definition is not as expected" exp)))))> > - > -(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)?
Toggle quote (45 lines)> +(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). Butnot a strong opinion.
Toggle quote (32 lines)> - (format #t "source code for commit ~a: ~a (GC root: ~a)~%"> - commit source root)))))> + > +(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 USBdrive or whatever). Better to just leave the stale reference instead ofpotentially destroying users worktrees.
Perhaps the script could 'git clone --maxdepth=1' instead of creating aworktree?
-----BEGIN PGP SIGNATURE-----
iQEzBAEBCgAdFiEEu7At3yzq9qgNHeZDoqBt8qM6VPoFAl+FzxcACgkQoqBt8qM6VPr3UAf6Aq3Pf6Va1zEAP3DCV5RUFoYQSH2mnns+qI0cULZK1iShIvcwdC6X7u1J1LMaerMRmzSTtjtO3csi62AMvXAr6rI5Ld4aON9NwJkmbnR0dKAvLfC/34Igcm9hcJO2vp5L6tQag0xcKmQYdI6FcI+xKdh+uBPczVTofyyAl225CENMsyqdAUEmnVHRKHo8v1Uyy+r4bvnQZnf+sJMGtv/RgbEmPMCk36tCrQYMvQ+2atdx/A5lQ5LpfDPvOJLL9F06930dyJBxMEC2JKNbplKgVujue7Vyy9n9Mog80jM2Ok2nb69IcYqlm1J+Gpw9LOmiueEiqldEJ5FeuR2cmu8Q8g===rNVH-----END PGP SIGNATURE-----
M
M
Maxim Cournoyer wrote on 14 Oct 06:10 +0200
(name . Marius Bakke)(address . marius@gnu.org)(address . 43893@debbugs.gnu.org)
87imbd5ty8.fsf@gmail.com
Hello Marius,
And thanks for the review!
[...]
Toggle quote (6 lines)>> +(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 atleast bindings for manipulating worktrees, and I'm not sure how we couldget a list of all the remotes and check their URLs to find the remoteused to push to Savannah. Supposing we'd be able to get that remote, Ialso don't know how we could query if its master branch contains a givencommit (I imagine it's doable, but it's not documented so it takes timeto figure it out :-).
[...]
Toggle quote (10 lines)>> - (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 behappy to be proven wrong.
[...]
Toggle quote (8 lines)>> + (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 theworktree it creates and no other.
Toggle quote (3 lines)> Perhaps the script could 'git clone --maxdepth=1' instead of creating> a worktree?
I think you meant something like:
Toggle snippet (3 lines)$ git clone --branch the-branch --depth -1 %top-srcdir
That could work, but it's about 2x slower and more expensive thancreating a worktree (15975 syscalls vs 647, according to 'strace -c').
Thank you,
Maxim
M
M
Maxim Cournoyer wrote on 19 Oct 20:04 +0200
(address . 43893-done@debbugs.gnu.org)
87ft6a5bz1.fsf@gmail.com
Hello!
I went ahead and merged this with commit 5800d2aae2. I think itprovides more benefits (breaking 'guix pull' is quite bad) thandrawbacks.
Thanks for your input!
Closing,
Maxim
Closed
L
L
Ludovic Courtès wrote on 20 Oct 23:06 +0200
Re: bug#43893: [PATCH v3] maint: update-guix-package: Prevent accidentally breaking guix pull.
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
87lfg0bo9m.fsf@gnu.org
Hi Maxim,
Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
Toggle quote (49 lines)> (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.
M
M
Maxim Cournoyer wrote on 21 Oct 04:36 +0200
(name . Ludovic Courtès)(address . ludo@gnu.org)
87v9f44852.fsf@gmail.com
Hello Ludovic,
Ludovic Courtès <ludo@gnu.org> writes:
Toggle quote (60 lines)> 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 thecode following the ;; Re-add SOURCE [...], but wrapped with unless tomake it conditional to GUIX_ALLOW_ME_TO_USE_PRIVATE_COMMIT beingdefined? That part depends on SOURCE, a store file name, which we don'thave anymore since we no longer add the sources to the store to computethe hash.
We could add the sources to the store from the clean checkout, but Ithought one great thing about the patch was that it removed interactionswith the store, allowing for the source derivations to happen normallywhen testing with 'guix build guix' (previously you'd have had to 'guixbuild guix --check', as the store had silently been pre-populated withthe sources). I see value in using the usual mechanism to get thesource rather than a side-channel, optimization hack, as it will helpensuring correctness.
Perhaps I misunderstood your point?
Thank you,
Maxim
L
L
Ludovic Courtès wrote on 21 Oct 10:53 +0200
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
875z749czt.fsf@gnu.org
Hi,
Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
Toggle quote (2 lines)> Ludovic Courtès <ludo@gnu.org> writes:
[...]
Toggle quote (35 lines)>>> - ;; 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’trun ‘guix build guix -S --check’ you’re not sure ‘guix’ is referring toa valid upstream commit) could have been to simply not do the trickabove. The trick at “Re-add SOURCE” is here precisely to preventattempts to re-download, as the comment explains, and this is what hasbeen 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 towork with a not-pushed-yet commit. So it’s a case where we needGUIX_ALLOW_ME_TO_USE_PRIVATE_COMMIT=yes.
Thanks,Ludo’.
M
M
Maxim Cournoyer wrote on 23 Oct 06:38 +0200
(name . Ludovic Courtès)(address . ludo@gnu.org)
87eelpd0af.fsf@gmail.com
Hi Ludovic,
Ludovic Courtès <ludo@gnu.org> writes:
Toggle quote (7 lines)> Hi,>> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:>>> Ludovic Courtès <ludo@gnu.org> writes:>
[...]
Toggle quote (11 lines)>> 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 afaulty hash (due to being computed from a uncontrolled checkout thatcould be dirty). The other concern about preventing the use of a notyet published commit was added based on earlier feedback.
Toggle quote (7 lines)> 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 -SThe following derivation will be built: /gnu/store/zhfchz831mncd2kyqmh5d2q0r2rpg57s-guix-1.1.0-31.07c13ae-checkout.drvbuilding /gnu/store/zhfchz831mncd2kyqmh5d2q0r2rpg57s-guix-1.1.0-31.07c13ae-checkout.drv...guile: warning: failed to install localeenvironment 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 07c13aeb5abb1a5bc3cabffb9b2212993a0d5a0eFailed 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.0rc2fatal: reference is not a tree: 07c13aeb5abb1a5bc3cabffb9b2212993a0d5a0egit-fetch: '/gnu/store/i5b1vv7qc6l2gi4xwa9mqzjy3shvgk30-git-minimal-2.28.0/bin/git checkout 07c13aeb5abb1a5bc3cabffb9b2212993a0d5a0e' failed with exit code 128Trying 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 failedView 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.825suser 0m2.191ssys 0m0.189s
This took nearly 2 minutes, fetching the full Guix git repo just to tellme that *something* is not right.
Currently, we have:
time make update-guix-packagegit rev-parse HEAD4893a1394e2eb8b97995b491f2f37ed85513a20f./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.135suser 0m1.066ssys 0m0.199s
Toggle quote (4 lines)> 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', itfails 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
L
L
Ludovic Courtès wrote on 23 Oct 17:01 +0200
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
87k0vhm1fg.fsf@gnu.org
Hi Maxim,
Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
Toggle quote (55 lines)> 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’sessentially for a handful of people, who can certainly cope with theugly error.
Anyway, I think we didn’t analyze the initial situation well enough(myself included, by not commenting early and accurately). I’m also notfond of the added complexity and the risk of surprises when we make therelease, but OTOH, it’s no big deal in the big picture!
Toggle quote (6 lines)>> 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 inMakefile.am.
Toggle quote (7 lines)> 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’.
M
M
Maxim Cournoyer wrote on 25 Oct 05:32 +0100
(name . Ludovic Courtès)(address . ludo@gnu.org)
87d0172adj.fsf@gmail.com
Hi Ludovic,
Ludovic Courtès <ludo@gnu.org> writes:
[...]
Toggle quote (20 lines)>> 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 initialsituation well enough"; if I had to think about the best way to solvethis problem now, I'd still choose the way that was chosen then, as itprovides the best guarantee against producing a broken Guix package,something that happened a couple times in the past, judging from gitlog. About complexity, I'd much rather the tool break on me thanbreaking 'guix pull' for everyone :-).
It seems we'll have to disagree on this one; but as you said, it's atiny part of the bigger landscape!
Toggle quote (4 lines)>>> 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 workscorrectly, 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 2make[2]: Leaving directory '/home/maxim/src/guix'make[1]: *** [Makefile:5630: dist] Error 2make[1]: Leaving directory '/home/maxim/src/guix'make: *** [Makefile:6410: dist-with-updated-version] Error 2
Can you reproduce this problem?
Thank you,
Maxim
L
L
Ludovic Courtès wrote on 25 Oct 15:41 +0100
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
873622gyfz.fsf@gnu.org
Hi,
Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
Toggle quote (3 lines)> +(define %savannah-guix-git-repo-push-url> + "git.savannah.gnu.org/srv/git/guix.git")
[...]
Toggle quote (12 lines)> +(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 thestring “push” is not translated (currently).
Ludo’.
L
L
Ludovic Courtès wrote on 25 Oct 15:50 +0100
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
87k0vefjgv.fsf@gnu.org
Hi Maxim,
Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
Toggle quote (27 lines)> 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, andthat’s what allows ‘make release’ to work.
It didn’t occur to me at the time, but the simplest path would have beento conditionalize the bit that makes it possible to refer tonon-upstream commits.
Toggle quote (7 lines)> 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 addedcomplexity brings its own set of (less serious) issues though.
Toggle quote (11 lines)>>>> 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’.
L
L
Ludovic Courtès wrote on 25 Oct 16:29 +0100
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
874kmifhn2.fsf@gnu.org
Hi again,
How about this variant of the initial script? I think it addresses themain 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 dothink we’re making progress anyway!
Thanks,Ludo’.
;;; 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)) (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)))
M
M
Maxim Cournoyer wrote on 25 Oct 20:17 +0100
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 43893@debbugs.gnu.org)
87pn56xghz.fsf@gmail.com
Hey Ludo,
Ludovic Courtès <ludo@gnu.org> writes:
Toggle quote (24 lines)> 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 with13a3b9c748a80c0d4c79450e479416480273d2f7. Feel free to edit the%savannah-guix-git-repo-push-url-regexp regexp if there are morevariants that we're still missing.
Maxim
M
M
Maxim Cournoyer wrote on 31 Oct 04:56 +0100
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 43893@debbugs.gnu.org)
87d00zyrom.fsf@gmail.com
Hi Ludovic,
Ludovic Courtès <ludo@gnu.org> writes:
Toggle quote (20 lines)> 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:
Toggle quote (59 lines)> @@ -23,12 +24,15 @@> ;;;> ;;; Code:
> -(use-modules (guix)> +(use-modules (git)> + (guix)> (guix git-download)> (guix upstream)> (guix utils)> (guix base32)> (guix build utils)> + (guix i18n)> + (guix diagnostics)> (gnu packages package-management)> (ice-9 match))
> @@ -101,7 +105,43 @@ COMMIT."> (exp> (error "'guix' package definition is not as expected" exp)))))
> - > +(define (keep-source-in-store store source)> + "Add SOURCE to the store under the name that the 'guix' package expects."> +> + ;; Add SOURCE to the store, but this time under the real name used in the> + ;; 'origin'. This allows us to build the package without having to make a> + ;; real checkout; thus, it also works when working on a private branch.> + (reload-module> + (resolve-module '(gnu packages package-management)))> +> + (let* ((source (add-to-store store> + (origin-file-name (package-source guix))> + #t "sha256" source))> + (root (store-path-package-name source)))> +> + ;; Add an indirect GC root for SOURCE in the current directory.> + (false-if-exception (delete-file root))> + (symlink source root)> + (add-indirect-root store> + (string-append (getcwd) "/" root))> +> + (info (G_ "source code kept in ~a (GC root: ~a)~%")> + source root)))> +> +(define (assert-clean-checkout repository)> + "Error out if the working directory at REPOSITORY contains local> +modifications."> + (define description> + (let ((format-options (make-describe-format-options> + #:dirty-suffix "-dirty")))> + (describe-format (describe-workdir repository) format-options)))> +> + (when (string-suffix? "-dirty" description)> + (leave (G_ "attempt to update 'guix' package from a dirty tree (~a)~%")> + description))> +> + (info (G_ "updating 'guix' package to '~a'~%") description))
Unfortunately this doesn't catch the case where git has explicitly beentold to '--skip-worktree' on a path or file (the original cause of thisbug report). Seehttps://debbugs.gnu.org/cgi/bugreport.cgi?bug=43893#11.
Toggle quote (43 lines)> (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'dprefer to keep the behavior consistent in both modes, unless there's avery good reason not too?
Thanks,
Maxim
L
L
Ludovic Courtès wrote on 31 Oct 11:42 +0100
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)(address . 43893@debbugs.gnu.org)
871rhevfpp.fsf@gnu.org
Hi Maxim,
Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
Toggle quote (19 lines)>> +(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 wayto catch this common mistake early and report it nicely.
Toggle quote (47 lines)>> (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, isa prerequisite for use cases like ‘make release’: there you not onlywant to update the package definition to refer to your private commitand corresponding hash, you also want to be able to build it. If thesource isn’t already in the store, ‘guix build guix’ tries to look it upon Savannah, which fails.
Conversely, when GUIX_ALLOW_ME_TO_USE_PRIVATE_COMMIT is unset, we don’tadd the source to the store: that way, ‘guix build guix’ is forced toclone from Savannah, which fails if for some reason the commit or hashis incorrect.
This catches the kinds of mistakes that we previously made, where wesometimes unwillingly ended up updating to the wrong commit/hash.
I hope that makes sense.
Thanks for your time!
Ludo’.
M
M
Maxim Cournoyer wrote on 9 Nov 20:28 +0100
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 43893@debbugs.gnu.org)
877dquib3z.fsf@gmail.com
Hello Ludovic!
Ludovic Courtès <ludo@gnu.org> writes:
Toggle quote (29 lines)> Hi Maxim,>> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:>>>> +(define (assert-clean-checkout repository)>>> + "Error out if the working directory at REPOSITORY contains local>>> +modifications.">>> + (define description>>> + (let ((format-options (make-describe-format-options>>> + #:dirty-suffix "-dirty")))>>> + (describe-format (describe-workdir repository) format-options)))>>> +>>> + (when (string-suffix? "-dirty" description)>>> + (leave (G_ "attempt to update 'guix' package from a dirty tree (~a)~%")>>> + description))>>> +>>> + (info (G_ "updating 'guix' package to '~a'~%") description))>>>> Unfortunately this doesn't catch the case where git has explicitly been>> told to '--skip-worktree' on a path or file (the original cause of this>> bug report). See>> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=43893#11.>> Any such issue is caught when one eventually runs ‘guix build guix’> (wrong commit ID, wrong hash, etc.).>> But you’re right that the above test isn’t fool-proof: it’s just a way> to catch this common mistake early and report it nicely.
Right. I still don't like that it wouldn't work from a checkout wheresomeone would have modified, e.g., the .dir-locals file locally andmarked it with 'git --skip-worktree'. Having to revert this kind of'developer setup' is painful. The current approach makes it unnecessary(only committed changes are taken into account, not just git trackedfiles).
Toggle quote (54 lines)>>> (define (main . args)>>> (match args>>> ((commit version)>>> @@ -113,32 +153,20 @@ COMMIT.">>> (hash (query-path-hash store source))>>> (location (package-definition-location))>>> (old-hash (content-hash-value>>> - (origin-hash (package-source guix)))))>>> + (origin-hash (package-source guix)))))>>> +>>> + (unless (getenv "GUIX_ALLOW_ME_TO_USE_PRIVATE_COMMIT")>>> + (let ((repository (repository-open ".")))>>> + (assert-clean-checkout repository)>>> + (repository-close! repository)))>>> +>>> (edit-expression location>>> (update-definition commit hash>>> #:old-hash old-hash>>> #:version version))>>>>> - ;; Re-add SOURCE to the store, but this time under the real name used>>> - ;; in the 'origin'. This allows us to build the package without>>> - ;; having to make a real checkout; thus, it also works when working>>> - ;; on a private branch.>>> - (reload-module>>> - (resolve-module '(gnu packages package-management)))>>> ->>> - (let* ((source (add-to-store store>>> - (origin-file-name (package-source guix))>>> - #t "sha256" source))>>> - (root (store-path-package-name source)))>>> ->>> - ;; Add an indirect GC root for SOURCE in the current directory.>>> - (false-if-exception (delete-file root))>>> - (symlink source root)>>> - (add-indirect-root store>>> - (string-append (getcwd) "/" root))>>> ->>> - (format #t "source code for commit ~a: ~a (GC root: ~a)~%">>> - commit source root)))))>>> + (when (getenv "GUIX_ALLOW_ME_TO_USE_PRIVATE_COMMIT")>>> + (keep-source-in-store store source)))))>>>> That environment variable would now do more than it advertises. I'd>> prefer to keep the behavior consistent in both modes, unless there's a>> very good reason not too?>> Adding the source to the store, under the right name, with a GC root, is> a prerequisite for use cases like ‘make release’: there you not only> want to update the package definition to refer to your private commit> and corresponding hash, you also want to be able to build it. If the> source isn’t already in the store, ‘guix build guix’ tries to look it up> on Savannah, which fails.
Thanks for providing a rational for it. I'll git-send a new patch whichadds the source to the store using the procedure you shared above, butotherwise keeps the existing mechanism intact.
Thank you for you patience!
Maxim
M
M
Maxim Cournoyer wrote on 9 Nov 20:29 +0100
[PATCH] maint: update-guix-package: Optionally add sources to store.
(address . 43893@debbugs.gnu.org)
20201109192923.29580-1-maxim.cournoyer@gmail.com
Following discussions in https://issues.guix.gnu.org/43893, keeping a copyof the updated package source is desirable when generating a release.
* build-aux/update-guix-package.scm (version-controlled?): Remove variable.(call-with-temporary-git-worktree): Renamed from'with-temporary-git-worktree'. Update doc. Do not change directoryimplicitly.(keep-source-in-store): New procedure.(main): Adjust to use with call-with-temporary-git-worktree. Add the sourcesto the store when GUIX_ALLOW_ME_TO_USE_PRIVATE_COMMIT is set.* .dir-locals.el (scheme-mode): Update.* doc/contributing.texi (Updating the Guix Package): Update doc.
Co-authored-by: Ludovic Courtès <ludo@gnu.org>--- .dir-locals.el | 2 +- build-aux/update-guix-package.scm | 66 ++++++++++++++++++++++--------- doc/contributing.texi | 11 ++---- 3 files changed, 51 insertions(+), 28 deletions(-)
Toggle diff (139 lines)diff --git a/.dir-locals.el b/.dir-locals.elindex 8e5d3902e3..38bb3af344 100644--- a/.dir-locals.el+++ b/.dir-locals.el@@ -123,7 +123,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 'call-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.scmindex ff6b105468..f197bc7e2a 100644--- a/build-aux/update-guix-package.scm+++ b/build-aux/update-guix-package.scm@@ -44,9 +44,6 @@ (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))@@ -114,8 +111,9 @@ 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."+(define-syntax-rule (call-with-temporary-git-worktree commit proc)+ "Execute PROC in the context of a temporary git worktree created from+COMMIT. PROC receives the temporary directory file name as an argument." (call-with-temporary-directory (lambda (tmp-directory) (dynamic-wind@@ -123,7 +121,7 @@ COMMIT." #t) (lambda () (git-add-worktree tmp-directory commit)- (with-directory-excursion tmp-directory body ...))+ (proc tmp-directory)) (lambda () (invoke "git" "worktree" "remove" "--force" tmp-directory)))))) @@ -156,6 +154,30 @@ COMMIT." "git" "branch" "-r" "--contains" commit (string-append remote "/master"))))) +(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+ #:select? (git-predicate 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 (main . args) (match args@@ -164,19 +186,25 @@ COMMIT." (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)))))- (edit-expression location- (update-definition commit hash- #:old-hash old-hash- #:version version)))))+ (call-with-temporary-git-worktree commit+ (lambda (tmp-directory)+ (let* ((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))+ ;; When GUIX_ALLOW_ME_TO_USE_PRIVATE_COMMIT is set, the sources are+ ;; added to the store. This is used as part of 'make release'.+ (when (getenv "GUIX_ALLOW_ME_TO_USE_PRIVATE_COMMIT")+ (with-store store+ (keep-source-in-store store tmp-directory)))))))) ((commit) ;; Automatically deduce the version and revision numbers. (main commit #f))))diff --git a/doc/contributing.texi b/doc/contributing.texiindex d3f6325c3f..d8de71055a 100644--- a/doc/contributing.texi+++ b/doc/contributing.texi@@ -1368,11 +1368,6 @@ 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+@code{GUIX_ALLOW_ME_TO_USE_PRIVATE_COMMIT} environment variable. When+this variable is set, the updated package source is also added to the+store. This is used as part of the release process of Guix.-- 2.28.0
L
L
Ludovic Courtès wrote on 9 Nov 23:03 +0100
Re: bug#43893: [PATCH v3] maint: update-guix-package: Prevent accidentally breaking guix pull.
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)(address . 43893@debbugs.gnu.org)
878sbarxvq.fsf@gnu.org
Hi!
Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
Toggle quote (34 lines)>> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:>>>>>> +(define (assert-clean-checkout repository)>>>> + "Error out if the working directory at REPOSITORY contains local>>>> +modifications.">>>> + (define description>>>> + (let ((format-options (make-describe-format-options>>>> + #:dirty-suffix "-dirty")))>>>> + (describe-format (describe-workdir repository) format-options)))>>>> +>>>> + (when (string-suffix? "-dirty" description)>>>> + (leave (G_ "attempt to update 'guix' package from a dirty tree (~a)~%")>>>> + description))>>>> +>>>> + (info (G_ "updating 'guix' package to '~a'~%") description))>>>>>> Unfortunately this doesn't catch the case where git has explicitly been>>> told to '--skip-worktree' on a path or file (the original cause of this>>> bug report). See>>> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=43893#11.>>>> Any such issue is caught when one eventually runs ‘guix build guix’>> (wrong commit ID, wrong hash, etc.).>>>> But you’re right that the above test isn’t fool-proof: it’s just a way>> to catch this common mistake early and report it nicely.>> Right. I still don't like that it wouldn't work from a checkout where> someone would have modified, e.g., the .dir-locals file locally and> marked it with 'git --skip-worktree'. Having to revert this kind of> 'developer setup' is painful. The current approach makes it unnecessary> (only committed changes are taken into account, not just git tracked> files).
“Wouldn’t work” is a strong statement: like I wrote, mistakes wouldalways be caught when you try to build ‘guix’, as with any other package(we don’t have special support for other packages, why would this onehave to be different?).
Thanks,Ludo’.
L
L
Ludovic Courtès wrote on 9 Nov 23:18 +0100
Re: [PATCH] maint: update-guix-package: Optionally add sources to store.
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)(address . 43893@debbugs.gnu.org)
87pn4mqime.fsf@gnu.org
Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
Toggle quote (3 lines)> Following discussions in https://issues.guix.gnu.org/43893, keeping a copy> of the updated package source is desirable when generating a release.
Not just when generating a release: anytime you want to refer to aprivate commit, which could be for mere testing.
Toggle quote (12 lines)> * build-aux/update-guix-package.scm (version-controlled?): Remove variable.> (call-with-temporary-git-worktree): Renamed from> 'with-temporary-git-worktree'. Update doc. Do not change directory> implicitly.> (keep-source-in-store): New procedure.> (main): Adjust to use with call-with-temporary-git-worktree. Add the sources> to the store when GUIX_ALLOW_ME_TO_USE_PRIVATE_COMMIT is set.> * .dir-locals.el (scheme-mode): Update.> * doc/contributing.texi (Updating the Guix Package): Update doc.>> Co-authored-by: Ludovic Courtès <ludo@gnu.org>
[...]
Toggle quote (6 lines)> -(define-syntax-rule (with-temporary-git-worktree commit body ...)> - "Execute BODY in the context of a temporary git worktree created from COMMIT."> +(define-syntax-rule (call-with-temporary-git-worktree commit proc)> + "Execute PROC in the context of a temporary git worktree created from> +COMMIT. PROC receives the temporary directory file name as an argument."
This could be a procedure rather a macro now.
[...]
Toggle quote (20 lines)> + (call-with-temporary-git-worktree commit> + (lambda (tmp-directory)> + (let* ((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))> + ;; When GUIX_ALLOW_ME_TO_USE_PRIVATE_COMMIT is set, the sources are> + ;; added to the store. This is used as part of 'make release'.> + (when (getenv "GUIX_ALLOW_ME_TO_USE_PRIVATE_COMMIT")> + (with-store store> + (keep-source-in-store store tmp-directory))))))))
OK, that should do the job.
Thanks for the patch, that should break the deadlock and allow us toproceed with the release!
Next we need to update the ‘release’ target soGUIX_ALLOW_ME_TO_USE_PRIVATE_COMMIT is set.
I like that the initial issue is fixed, but I still don’t buy the extradependency on Git, the extra copies of the whole tree, the extra code,and the shell pipelines, something avoided in the rest of Guix. Perhapsthat suggests there are unwritten coding guidelines we should eventuallydiscuss and write. We’ll see!
Thanks,Ludo’.
M
M
Maxim Cournoyer wrote on 9 Nov 23:44 +0100
[PATCH v5] maint: update-guix-package: Optionally add sources to store.
(address . 43893@debbugs.gnu.org)
20201109224443.27495-1-maxim.cournoyer@gmail.com
Following discussions in https://issues.guix.gnu.org/43893, keeping a copyof the updated package source is desirable when generating a release.
* build-aux/update-guix-package.scm (version-controlled?): Remove variable.(call-with-temporary-git-worktree): Renamed from'with-temporary-git-worktree'. Update doc. Do not change directoryimplicitly.(keep-source-in-store): New procedure.(main): Adjust to use with call-with-temporary-git-worktree. Add the sourcesto the store when GUIX_ALLOW_ME_TO_USE_PRIVATE_COMMIT is set. Exit gracefullywhen FIND-ORIGIN-REMOTE returns #f.(%savannah-guix-git-repo-push-url-regexp): Adjust match for a potential colonseparator.* Makefile.am (GUIX_ALLOW_ME_TO_USE_PRIVATE_COMMIT): Adjust.* .dir-locals.el (scheme-mode): Update.* doc/contributing.texi (Updating the Guix Package): Update doc.
Co-authored-by: Ludovic Courtès <ludo@gnu.org>--- .dir-locals.el | 2 +- Makefile.am | 14 +++--- build-aux/update-guix-package.scm | 73 ++++++++++++++++++++++--------- doc/contributing.texi | 11 ++--- 4 files changed, 64 insertions(+), 36 deletions(-)
Toggle diff (182 lines)diff --git a/.dir-locals.el b/.dir-locals.elindex 8e5d3902e3..38bb3af344 100644--- a/.dir-locals.el+++ b/.dir-locals.el@@ -123,7 +123,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 'call-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/Makefile.am b/Makefile.amindex e7053ee4f4..6faf8c9349 100644--- a/Makefile.am+++ b/Makefile.am@@ -826,9 +826,10 @@ release: dist-with-updated-version $(MKDIR_P) "$(releasedir)" rm -f "$(releasedir)"/* mv $(SOURCE_TARBALLS) "$(releasedir)"- $(top_builddir)/pre-inst-env "$(GUILE)" \- $(top_srcdir)/build-aux/update-guix-package.scm \- "`git rev-parse HEAD`" "$(PACKAGE_VERSION)"+ GUIX_ALLOW_ME_TO_USE_PRIVATE_COMMIT=yes \+ $(top_builddir)/pre-inst-env "$(GUILE)" \+ $(top_srcdir)/build-aux/update-guix-package.scm \+ "`git rev-parse HEAD`" "$(PACKAGE_VERSION)" git add $(top_srcdir)/gnu/packages/package-management.scm git commit -m "gnu: guix: Update to $(PACKAGE_VERSION)." $(top_builddir)/pre-inst-env guix build $(GUIX_FOR_BINARY_TARBALL) \@@ -840,9 +841,10 @@ release: dist-with-updated-version mv "guix-binary.$$system.tar.xz" \ "$(releasedir)/guix-binary-$(PACKAGE_VERSION).$$system.tar.xz" ; \ done- $(top_builddir)/pre-inst-env "$(GUILE)" \- $(top_srcdir)/build-aux/update-guix-package.scm \- "`git rev-parse HEAD`"+ GUIX_ALLOW_ME_TO_USE_PRIVATE_COMMIT=yes \+ $(top_builddir)/pre-inst-env "$(GUILE)" \+ $(top_srcdir)/build-aux/update-guix-package.scm \+ "`git rev-parse HEAD`" git add $(top_srcdir)/gnu/packages/package-management.scm git commit -m "gnu: guix: Update to `git rev-parse HEAD | cut -c1-7`." $(top_builddir)/pre-inst-env guix build guix \diff --git a/build-aux/update-guix-package.scm b/build-aux/update-guix-package.scmindex ff6b105468..b79e96eb96 100644--- a/build-aux/update-guix-package.scm+++ b/build-aux/update-guix-package.scm@@ -44,9 +44,6 @@ (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))@@ -114,8 +111,9 @@ 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."+(define-syntax-rule (call-with-temporary-git-worktree commit proc)+ "Execute PROC in the context of a temporary git worktree created from+COMMIT. PROC receives the temporary directory file name as an argument." (call-with-temporary-directory (lambda (tmp-directory) (dynamic-wind@@ -123,12 +121,12 @@ COMMIT." #t) (lambda () (git-add-worktree tmp-directory commit)- (with-directory-excursion tmp-directory body ...))+ (proc tmp-directory)) (lambda () (invoke "git" "worktree" "remove" "--force" tmp-directory)))))) (define %savannah-guix-git-repo-push-url-regexp- "git.(savannah|sv).gnu.org/srv/git/guix.git \\(push\\)")+ "git.(savannah|sv).gnu.org:?/srv/git/guix.git \\(push\\)") (define-syntax-rule (with-input-pipe-to-string prog arg ...) (let* ((input-pipe (open-pipe* OPEN_READ prog arg ...))@@ -156,27 +154,60 @@ COMMIT." "git" "branch" "-r" "--contains" commit (string-append remote "/master"))))) +(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+ #:select? (git-predicate 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 (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)+ (let ((remote (find-origin-remote)))+ (unless remote+ (leave (G_ "Failed to find the origin git remote.~%")))+ (commit-already-pushed? 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)))))- (edit-expression location- (update-definition commit hash- #:old-hash old-hash- #:version version)))))+ (call-with-temporary-git-worktree commit+ (lambda (tmp-directory)+ (let* ((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))+ ;; When GUIX_ALLOW_ME_TO_USE_PRIVATE_COMMIT is set, the sources are+ ;; added to the store. This is used as part of 'make release'.+ (when (getenv "GUIX_ALLOW_ME_TO_USE_PRIVATE_COMMIT")+ (with-store store+ (keep-source-in-store store tmp-directory)))))))) ((commit) ;; Automatically deduce the version and revision numbers. (main commit #f))))diff --git a/doc/contributing.texi b/doc/contributing.texiindex d3f6325c3f..d8de71055a 100644--- a/doc/contributing.texi+++ b/doc/contributing.texi@@ -1368,11 +1368,6 @@ 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+@code{GUIX_ALLOW_ME_TO_USE_PRIVATE_COMMIT} environment variable. When+this variable is set, the updated package source is also added to the+store. This is used as part of the release process of Guix.-- 2.28.0
L
L
Ludovic Courtès wrote on 10 Nov 10:32 +0100
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)(address . 43893@debbugs.gnu.org)
871rh1r1yy.fsf@gnu.org
Hi Maxim,
Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
Toggle quote (19 lines)> Following discussions in https://issues.guix.gnu.org/43893, keeping a copy> of the updated package source is desirable when generating a release.>> * build-aux/update-guix-package.scm (version-controlled?): Remove variable.> (call-with-temporary-git-worktree): Renamed from> 'with-temporary-git-worktree'. Update doc. Do not change directory> implicitly.> (keep-source-in-store): New procedure.> (main): Adjust to use with call-with-temporary-git-worktree. Add the sources> to the store when GUIX_ALLOW_ME_TO_USE_PRIVATE_COMMIT is set. Exit gracefully> when FIND-ORIGIN-REMOTE returns #f.> (%savannah-guix-git-repo-push-url-regexp): Adjust match for a potential colon> separator.> * Makefile.am (GUIX_ALLOW_ME_TO_USE_PRIVATE_COMMIT): Adjust.> * .dir-locals.el (scheme-mode): Update.> * doc/contributing.texi (Updating the Guix Package): Update doc.>> Co-authored-by: Ludovic Courtès <ludo@gnu.org>
Go for it, thanks!
Ludo’.
M
M
Maxim Cournoyer wrote on 10 Nov 15:02 +0100
Re: [PATCH] maint: update-guix-package: Optionally add sources to store.
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 43893@debbugs.gnu.org)
87zh3pgvhp.fsf@gmail.com
Hello Ludovic,
Ludovic Courtès <ludo@gnu.org> writes:
Toggle quote (2 lines)> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
[...]
Toggle quote (8 lines)>> -(define-syntax-rule (with-temporary-git-worktree commit body ...)>> - "Execute BODY in the context of a temporary git worktree created from COMMIT.">> +(define-syntax-rule (call-with-temporary-git-worktree commit proc)>> + "Execute PROC in the context of a temporary git worktree created from>> +COMMIT. PROC receives the temporary directory file name as an argument.">> This could be a procedure rather a macro now.
I've changed it to a plain define in the latest version (now merged).
Toggle quote (30 lines)> [...]>>> + (call-with-temporary-git-worktree commit>> + (lambda (tmp-directory)>> + (let* ((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))>> + ;; When GUIX_ALLOW_ME_TO_USE_PRIVATE_COMMIT is set, the sources are>> + ;; added to the store. This is used as part of 'make release'.>> + (when (getenv "GUIX_ALLOW_ME_TO_USE_PRIVATE_COMMIT")>> + (with-store store>> + (keep-source-in-store store tmp-directory))))))))>> OK, that should do the job.>> Thanks for the patch, that should break the deadlock and allow us to> proceed with the release!>> Next we need to update the ‘release’ target so> GUIX_ALLOW_ME_TO_USE_PRIVATE_COMMIT is set.
Done!
Toggle quote (2 lines)> I like that the initial issue is fixed, but I still don’t buy
[...]
Hehe. Last round of weighing the + and - of this change
Toggle quote (2 lines)> the extra dependency on Git,
To me, this script (update-guix-package), is an extension of theMake-based build system (that's currently it's sole entry-point). Thereare already calls to git in this build system (for example, to get thecommit corresponding to HEAD), so I don't perceive it as nasty in thiscontext. It can also be used as a reminder of things that are missingin (guile git), for the purists ;-).
Toggle quote (2 lines)> the extra copies of the whole tree,
There used to be 3 copies required in total (the Guix checkout, a firstdummy copy in the store, and a final copy in the store).
Now, we have 2 copies unless GUIX_ALLOW_ME_TO_USE_PRIVATE_COMMIT is set,in which case we get a third one in the store. Seems pretty even to me!
Toggle quote (2 lines)> the extra code
The extra code make things smoother (better/faster diagnostic), causesless friction in the workflow (I don't need to go paranoid about my treebeing in pristine condition before 'make update-guix-package' -- I canrely on Guix computing it deterministically from the last commit).
Toggle quote (2 lines)> and the shell pipelines, something avoided in the rest of Guix.
Again, to me this script is a standalone extension of the build system.It's not defined as a module, cannot be used in the rest of the codebase, so that it does things a bit differently doesn't strike me as bad.
Toggle quote (3 lines)> Perhaps that suggests there are unwritten coding guidelines we should> eventually discuss and write. We’ll see!
That could be nice. Although a linter included with Guile (ala Rust orGo) and configurable through a config file could have even more impact,in my opinion. In any case I'd be honored that my code got to be thespark behind such guidelines/tool, eh :-).
Thank you,
Maxim
M
M
Maxim Cournoyer wrote on 10 Nov 15:31 +0100
Re: bug#43893: [PATCH v3] maint: update-guix-package: Prevent accidentally breaking guix pull.
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 43893@debbugs.gnu.org)
87v9edgu6i.fsf@gmail.com
Hey,
Ludovic Courtès <ludo@gnu.org> writes:
Toggle quote (43 lines)> Hi!>> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:>>>> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:>>>>>>>> +(define (assert-clean-checkout repository)>>>>> + "Error out if the working directory at REPOSITORY contains local>>>>> +modifications.">>>>> + (define description>>>>> + (let ((format-options (make-describe-format-options>>>>> + #:dirty-suffix "-dirty")))>>>>> + (describe-format (describe-workdir repository) format-options)))>>>>> +>>>>> + (when (string-suffix? "-dirty" description)>>>>> + (leave (G_ "attempt to update 'guix' package from a dirty tree (~a)~%")>>>>> + description))>>>>> +>>>>> + (info (G_ "updating 'guix' package to '~a'~%") description))>>>>>>>> Unfortunately this doesn't catch the case where git has explicitly been>>>> told to '--skip-worktree' on a path or file (the original cause of this>>>> bug report). See>>>> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=43893#11.>>>>>> Any such issue is caught when one eventually runs ‘guix build guix’>>> (wrong commit ID, wrong hash, etc.).>>>>>> But you’re right that the above test isn’t fool-proof: it’s just a way>>> to catch this common mistake early and report it nicely.>>>> Right. I still don't like that it wouldn't work from a checkout where>> someone would have modified, e.g., the .dir-locals file locally and>> marked it with 'git --skip-worktree'. Having to revert this kind of>> 'developer setup' is painful. The current approach makes it unnecessary>> (only committed changes are taken into account, not just git tracked>> files).>> “Wouldn’t work” is a strong statement: like I wrote, mistakes would> always be caught when you try to build ‘guix’, as with any other package> (we don’t have special support for other packages, why would this one> have to be different?).
True. I meant it in the sense "I wouldn't be able to update the Guixpackage before manually ensuring that the HEAD of my tree was in apristine condition, that is, equivalent to the last commit".
Since Guix is the focus of Guix developers, it's much more likely tohave its sources in flux compared to the other packages we update inGuix. To me it seems useful to automate the 'cleanliness' part of thetree rather than force it on developers, since it can be.
We also don't update the package in place from a Git checkout whenupdating other packages. It's a manual work of 'git clone', 'guix hash-rx' and editing the source manually, which is different from 'makeupdate-guix-package', which strives to automate the process.
My 2 cents! Time will tell if this is a viable route. If it breaksevery time we use it, we can fallback to the simpler scheme.
Maxim
L
L
Ludovic Courtès wrote on 10 Nov 15:48 +0100
Re: [PATCH] maint: update-guix-package: Optionally add sources to store.
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)(address . 43893@debbugs.gnu.org)
87y2j9nu8f.fsf@gnu.org
Hi,
Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
Toggle quote (8 lines)>> Thanks for the patch, that should break the deadlock and allow us to>> proceed with the release!>>>> Next we need to update the ‘release’ target so>> GUIX_ALLOW_ME_TO_USE_PRIVATE_COMMIT is set.>> Done!
Thanks!
Toggle quote (9 lines)>> the extra dependency on Git,>> To me, this script (update-guix-package), is an extension of the> Make-based build system (that's currently it's sole entry-point). There> are already calls to git in this build system (for example, to get the> commit corresponding to HEAD), so I don't perceive it as nasty in this> context. It can also be used as a reminder of things that are missing> in (guile git), for the purists ;-).
I think we had everything needed in Guile-Git.
Toggle quote (8 lines)>> the extra copies of the whole tree,>> There used to be 3 copies required in total (the Guix checkout, a first> dummy copy in the store, and a final copy in the store).>> Now, we have 2 copies unless GUIX_ALLOW_ME_TO_USE_PRIVATE_COMMIT is set,> in which case we get a third one in the store. Seems pretty even to me!
Right, I stand corrected.
Toggle quote (13 lines)>> the extra code>> The extra code make things smoother (better/faster diagnostic), causes> less friction in the workflow (I don't need to go paranoid about my tree> being in pristine condition before 'make update-guix-package' -- I can> rely on Guix computing it deterministically from the last commit).>>> and the shell pipelines, something avoided in the rest of Guix.>> Again, to me this script is a standalone extension of the build system.> It's not defined as a module, cannot be used in the rest of the code> base, so that it does things a bit differently doesn't strike me as bad.
Note that quite a few modules started their life under build-aux/.
Toggle quote (8 lines)>> Perhaps that suggests there are unwritten coding guidelines we should>> eventually discuss and write. We’ll see!>> That could be nice. Although a linter included with Guile (ala Rust or> Go) and configurable through a config file could have even more impact,> in my opinion. In any case I'd be honored that my code got to be the> spark behind such guidelines/tool, eh :-).
I don’t think a linter could flag high-level patterns like the oneswe’re talking about, but human-written text could.
I hope this discussion can at least help improve mutual understanding onfuture patches and review processes.
Thanks again for your time and patience!
Ludo’.
M
M
Maxim Cournoyer wrote on 10 Nov 16:18 +0100
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 43893@debbugs.gnu.org)
87r1p1gs08.fsf@gmail.com
Hi again!
Ludovic Courtès <ludo@gnu.org> writes:
Toggle quote (25 lines)> Hi,>> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:>>>> Thanks for the patch, that should break the deadlock and allow us to>>> proceed with the release!>>>>>> Next we need to update the ‘release’ target so>>> GUIX_ALLOW_ME_TO_USE_PRIVATE_COMMIT is set.>>>> Done!>> Thanks!>>>> the extra dependency on Git,>>>> To me, this script (update-guix-package), is an extension of the>> Make-based build system (that's currently it's sole entry-point). There>> are already calls to git in this build system (for example, to get the>> commit corresponding to HEAD), so I don't perceive it as nasty in this>> context. It can also be used as a reminder of things that are missing>> in (guile git), for the purists ;-).>> I think we had everything needed in Guile-Git.
Not for the current implementation, that uses git worktrees and searchfor the presence of the commit corresponding to HEAD in the upstreamremote. If my (summary) analysis of the current state of (guile git) iswrong, I'd be happy to migrate the bits shelling out to git to actuallibrary calls via (guile git).
[...]
Toggle quote (5 lines)> I hope this discussion can at least help improve mutual understanding on> future patches and review processes.>> Thanks again for your time and patience!
I am equally grateful for yours. This is a long thread!
Thanks,
Maxim
?