| Message ID | b0a3a92d6adda6582cd3aa61d3be30a90d91c96b.1695421391.git.ludo@gnu.org |
|---|---|
| State | New |
| Headers | show |
| Series | Add built-in builder for Git checkouts | expand |
Hi Ludo, On Sat, 23 Sep 2023 at 00:28, Ludovic Courtès <ludo@gnu.org> wrote: > +(define* (git-fetch/built-in ref hash-algo hash > + #:optional name > + #:key (system (%current-system))) > + "Return a fixed-output derivation without any dependency that performs a Git > +checkout of REF, using the \"builtin:git-download\" derivation builder." I do not understand what means “without any dependency” here. I would drop it. Cheers, simon
Simon Tournier <zimon.toutoune@gmail.com> skribis: > On Sat, 23 Sep 2023 at 00:28, Ludovic Courtès <ludo@gnu.org> wrote: > >> +(define* (git-fetch/built-in ref hash-algo hash >> + #:optional name >> + #:key (system (%current-system))) >> + "Return a fixed-output derivation without any dependency that performs a Git >> +checkout of REF, using the \"builtin:git-download\" derivation builder." > > I do not understand what means “without any dependency” here. It means the derivation does not depend on anything (it has zero “sources” and zero “input derivations”). That’s fundamental here, which is why I made it explicit. Ludo’.
Hi, On Mon, 25 Sep 2023 at 11:23, Ludovic Courtès <ludo@gnu.org> wrote: > Simon Tournier <zimon.toutoune@gmail.com> skribis: > >> On Sat, 23 Sep 2023 at 00:28, Ludovic Courtès <ludo@gnu.org> wrote: >> >>> +(define* (git-fetch/built-in ref hash-algo hash >>> + #:optional name >>> + #:key (system (%current-system))) >>> + "Return a fixed-output derivation without any dependency that performs a Git >>> +checkout of REF, using the \"builtin:git-download\" derivation builder." >> >> I do not understand what means “without any dependency” here. > > It means the derivation does not depend on anything (it has zero > “sources” and zero “input derivations”). That’s fundamental here, which > is why I made it explicit. Aaah :-) Yeah that makes sense. Well, from my understanding, « fixed-output derivation without any dependency » is not clear at first (we are always dependent on something else ;-)) and I am sure that weeks or months later, I will not remember that it means: « zero “sources” and zero “input derivations” ». Hum, once it is known, it is hard to find a better wording though. ;-) I do not know if it is better, at least, it appears clearer from my point of view: "Return a fixed-output derivation that performs a Git checkout of REF, using the \"builtin:git-download\" derivation builder, thus without any dependency other than from builtin." Cheers, simon
Re, On Mon, 25 Sep 2023 at 11:23, Ludovic Courtès <ludo@gnu.org> wrote: >>> + "Return a fixed-output derivation without any dependency that performs a Git >>> +checkout of REF, using the \"builtin:git-download\" derivation builder." >> >> I do not understand what means “without any dependency” here. > > It means the derivation does not depend on anything (it has zero > “sources” and zero “input derivations”). That’s fundamental here, which > is why I made it explicit. For instance, in ’built-in-download’, the docstring reads: --8<---------------cut here---------------start------------->8--- This is an \"out-of-band\" download in that the returned derivation does not explicitly depend on Guile, GnuTLS, etc. Instead, the daemon performs the download by itself using its own dependencies. --8<---------------cut here---------------end--------------->8--- which I find “clearer” than “without any dependency”. Well, enough for nitpicking. :-) Cheers, simon
Hi, Simon Tournier <zimon.toutoune@gmail.com> writes: > Re, > > On Mon, 25 Sep 2023 at 11:23, Ludovic Courtès <ludo@gnu.org> wrote: > >>>> + "Return a fixed-output derivation without any dependency that performs a Git >>>> +checkout of REF, using the \"builtin:git-download\" derivation builder." >>> >>> I do not understand what means “without any dependency” here. >> >> It means the derivation does not depend on anything (it has zero >> “sources” and zero “input derivations”). That’s fundamental here, which >> is why I made it explicit. > > For instance, in ’built-in-download’, the docstring reads: > > This is an \"out-of-band\" download in that the returned derivation does not > explicitly depend on Guile, GnuTLS, etc. Instead, the daemon performs the > download by itself using its own dependencies. > > which I find “clearer” than “without any dependency”. > > Well, enough for nitpicking. :-) I also think the built-in-download's docstring explicitness is nicer.
Hi, Simon Tournier <zimon.toutoune@gmail.com> skribis: > Re, > > On Mon, 25 Sep 2023 at 11:23, Ludovic Courtès <ludo@gnu.org> wrote: > >>>> + "Return a fixed-output derivation without any dependency that performs a Git >>>> +checkout of REF, using the \"builtin:git-download\" derivation builder." >>> >>> I do not understand what means “without any dependency” here. >> >> It means the derivation does not depend on anything (it has zero >> “sources” and zero “input derivations”). That’s fundamental here, which >> is why I made it explicit. > > For instance, in ’built-in-download’, the docstring reads: > > This is an \"out-of-band\" download in that the returned derivation does not > explicitly depend on Guile, GnuTLS, etc. Instead, the daemon performs the > download by itself using its own dependencies. > > which I find “clearer” than “without any dependency”. Yes, good idea. I changed the docstring as you suggest and pushed the whole thing: ba21eeb565 tests: Assume ‘git’ is always available. 13b0cf85eb git-download: Use “builtin:git-download” when available. c4a1d69a69 perform-download: Use the ‘git’ command captured at configure time. f651a35969 build: Add dependency on Git. 95f2123135 daemon: Add “git-download” built-in builder. 9d0e2002a5 perform-download: Remove unused one-argument clause. c7ed1e0160 git-download: Honor the ‘GUIX_DOWNLOAD_FALLBACK_TEST’ environment variable. 811b249397 git-download: Move fallback code to (guix build git). I’ll update the ‘guix’ package soon so we can start using the new builtin. Thanks to the two of you! Ludo’.
On Tue, 26 Sept 2023 at 17:44, Ludovic Courtès <ludo@gnu.org> wrote:
> I changed the docstring as you suggest and pushed the whole thing:
I am not convinced that we reached a consensus about this series.
Because enlarging the "Trusting Computing Base" as this series does
cannot be dismissed as "drifting" and had not been discussed at all
before pushing although I raised the concern.
All the best,
simon
Hi, Simon Tournier <zimon.toutoune@gmail.com> skribis: > On Tue, 26 Sept 2023 at 17:44, Ludovic Courtès <ludo@gnu.org> wrote: > >> I changed the docstring as you suggest and pushed the whole thing: > > I am not convinced that we reached a consensus about this series. > Because enlarging the "Trusting Computing Base" as this series does > cannot be dismissed as "drifting" and had not been discussed at all > before pushing although I raised the concern. The reasoning for “builtin:git-download” is the same as for “builtin:download”, which was introduced in 2016¹, and which is itself a logical followup to the notion of fixed-output derivations. None of these increases the TCB because they’re about downloading data whose contents are known in advance. I think it’s important in these discussions to make sure we start from a shared understanding so we can remain focused and productive. Ludo’. ¹ https://issues.guix.gnu.org/22774
Hi Ludo, We are not on the same wavelength about some technical parts. It does not really matter – we could tune our frequency separately on some random occasions. :-) However… On Sun, 1 Oct 2023 at 17:02, Ludovic Courtès <ludo@gnu.org> wrote: > I think it’s important in these discussions to make sure we start from a > shared understanding so we can remain focused and productive. …I am, between sad and upset, by this part. It appears to me unfair or uncalled for. « A group using consensus is committed to finding solutions that everyone actively supports, or at least can live with. » [1]. And I am sorry to say that we have a failure here; at the root (Guile-GnuTLS bug report). And it is a group failure. Elsewhere in this thread,, I have expressed « I am not convinced that we reached a consensus about this series. ». Why no one is asking if I am blocking this patch? Anyway! Thinking more than twice about why it bothers me. If I feel a failure about the consensus, then it is because the process fails from my point of view. Why? Because an implementation detail is missing. It had been expressed several times. Notably: Time for a request-for-comments process? Ludovic Courtès <ludo@gnu.org> Wed, 27 Oct 2021 23:22:50 +0200 id:87cznqb1sl.fsf@inria.fr https://lists.gnu.org/archive/html/guix-devel/2021-10 https://yhetil.org/guix/87cznqb1sl.fsf@inria.fr And this kind of change « Add Git as hard dependency » should have been part of such process, IMHO. Because considering how the current decision process works, it is impossible to get this “shared understanding” if you have not been here at the right moment. How can I acquire this shared understanding? For example, you said that Git or TLS or etc. on daemon side are not part of the TCB because they are used for fixed-output derivations, I disagree and I still think it is incorrect. The problem is not my disagreement – I can live with it, as many others ;-) – no, the problem is that you refer to implicit decision that I cannot digest, question or ask explanations. Somehow, I do not have some gauge for evaluating my own expectations. It appears to me that this patch falls under similar circumstances as an idea expressed here: [bug#61894] [PATCH RFC] Team approval for patches Ludovic Courtès <ludo@gnu.org> Wed, 01 Mar 2023 17:13:28 +0100 id:878rgga1qv.fsf@inria.fr https://lists.gnu.org/archive/html/guix-devel/2023-03 https://yhetil.org/guix/878rgga1qv.fsf@inria.fr Now, Guix is probably reaching a point where we deserve more structure without much burden for making decisions about changes of some category. Therefore, let turn my own frustration here into a concrete proposal, I will send this week a Request-For-Comment process inspired by Rust, Nix and Haskell ones. Cheers, simon 1: https://www.seedsforchange.org.uk/consensus 2: https://guix.gnu.org/blog/2019/gnu-guix-maintainer-collective-expands/
Hi Simon and all, This is again a long message but I’ll try to reply shortly. First, my apologies as I failed to understand that you were rejecting the change altogether—I interpreted your comments on details about the patches as a sign that you were not opposing the idea in itself. Second, the sentence of mine you quoted was poorly worded. I felt with your last messages in the thread that you were questioning “builtin:download” itself, which was going way beyond the scope of this patch. Last, and probably more importantly: I’m all for an RFC process as you propose! I think this is something we desperately need anytime we make non-trivial changes to the core, to command-line interfaces, etc. I’ll be there to support a move in that direction as much as I can. So… thank you for making pushing this proposal. Ludo’.
diff --git a/guix/git-download.scm b/guix/git-download.scm index f1f19397c6..505dff0a89 100644 --- a/guix/git-download.scm +++ b/guix/git-download.scm @@ -27,6 +27,7 @@ (define-module (guix git-download) #:use-module (guix records) #:use-module (guix packages) #:use-module (guix modules) + #:use-module ((guix derivations) #:select (raw-derivation)) #:autoload (guix build-system gnu) (standard-packages) #:autoload (guix download) (%download-fallback-test) #:autoload (git bindings) (libgit2-init!) @@ -78,15 +79,19 @@ (define (git-package) (let ((distro (resolve-interface '(gnu packages version-control)))) (module-ref distro 'git-minimal))) -(define* (git-fetch ref hash-algo hash - #:optional name - #:key (system (%current-system)) (guile (default-guile)) - (git (git-package))) - "Return a fixed-output derivation that fetches REF, a <git-reference> -object. The output is expected to have recursive hash HASH of type -HASH-ALGO (a symbol). Use NAME as the file name, or a generic name if #f." +(define* (git-fetch/in-band ref hash-algo hash + #:optional name + #:key (system (%current-system)) + (guile (default-guile)) + (git (git-package))) + "Return a fixed-output derivation that performs a Git checkout of REF, using +GIT and GUILE (thus, said derivation depends on GIT and GUILE). + +This method is deprecated in favor of the \"builtin:git-download\" builder. +It will be removed when versions of guix-daemon implementing +\"builtin:git-download\" will be sufficiently widespread." (define inputs - `(("git" ,git) + `(("git" ,(or git (git-package))) ;; When doing 'git clone --recursive', we need sed, grep, etc. to be ;; available so that 'git submodule' works. @@ -154,7 +159,8 @@ (define* (git-fetch ref hash-algo hash #:recursive? recursive? #:git-command "git"))))) - (mlet %store-monad ((guile (package->derivation guile system))) + (mlet %store-monad ((guile (package->derivation (or guile (default-guile)) + system))) (gexp->derivation (or name "git-checkout") build ;; Use environment variables and a fixed script name so @@ -181,6 +187,50 @@ (define* (git-fetch ref hash-algo hash #:recursive? #t #:guile-for-build guile))) +(define* (git-fetch/built-in ref hash-algo hash + #:optional name + #:key (system (%current-system))) + "Return a fixed-output derivation without any dependency that performs a Git +checkout of REF, using the \"builtin:git-download\" derivation builder." + (raw-derivation (or name "git-checkout") "builtin:git-download" '() + #:system system + #:hash-algo hash-algo + #:hash hash + #:recursive? #t + #:env-vars + `(("url" . ,(object->string + (match (%download-fallback-test) + ('content-addressed-mirrors + "https://example.org/does-not-exist") + (_ + (git-reference-url ref))))) + ("commit" . ,(git-reference-commit ref)) + ("recursive?" . ,(object->string + (git-reference-recursive? ref)))) + #:leaked-env-vars '("http_proxy" "https_proxy" + "LC_ALL" "LC_MESSAGES" "LANG" + "COLUMNS") + #:local-build? #t)) + +(define built-in-builders* + (store-lift built-in-builders)) + +(define* (git-fetch ref hash-algo hash + #:optional name + #:key (system (%current-system)) + guile git) + "Return a fixed-output derivation that fetches REF, a <git-reference> +object. The output is expected to have recursive hash HASH of type +HASH-ALGO (a symbol). Use NAME as the file name, or a generic name if #f." + (mlet %store-monad ((builtins (built-in-builders*))) + (if (member "git-download" builtins) + (git-fetch/built-in ref hash-algo hash name + #:system system) + (git-fetch/in-band ref hash-algo hash name + #:system system + #:guile guile + #:git git)))) + (define (git-version version revision commit) "Return the version string for packages using git-download." ;; git-version is almost exclusively executed while modules are being loaded. diff --git a/tests/builders.scm b/tests/builders.scm index 0b5577c7a3..619caa5f31 100644 --- a/tests/builders.scm +++ b/tests/builders.scm @@ -1,5 +1,5 @@ ;;; GNU Guix --- Functional package management for GNU -;;; Copyright © 2012, 2013, 2014, 2015, 2018, 2019, 2021 Ludovic Courtès <ludo@gnu.org> +;;; Copyright © 2012-2015, 2018-2019, 2021, 2023 Ludovic Courtès <ludo@gnu.org> ;;; Copyright © 2021 Lars-Dominik Braun <lars@6xq.net> ;;; ;;; This file is part of GNU Guix. @@ -20,6 +20,7 @@ (define-module (tests builders) #:use-module (guix download) + #:use-module (guix git-download) #:use-module (guix build-system) #:use-module (guix build-system gnu) #:use-module (guix build gnu-build-system) @@ -31,9 +32,12 @@ (define-module (tests builders) #:use-module (guix base32) #:use-module (guix derivations) #:use-module (gcrypt hash) + #:use-module ((guix hash) #:select (file-hash*)) #:use-module (guix tests) + #:use-module (guix tests git) #:use-module (guix packages) #:use-module (gnu packages bootstrap) + #:use-module ((ice-9 ftw) #:select (scandir)) #:use-module (ice-9 match) #:use-module (ice-9 textual-ports) #:use-module (srfi srfi-1) @@ -84,6 +88,29 @@ (define url-fetch* (and (file-exists? out) (valid-path? %store out)))) +(test-equal "git-fetch, file URI" + '("." ".." "a.txt" "b.scm") + (let ((nonce (random-text))) + (with-temporary-git-repository directory + `((add "a.txt" ,nonce) + (add "b.scm" "#t") + (commit "Commit.") + (tag "v1.0.0" "The tag.")) + (run-with-store %store + (mlet* %store-monad ((hash + -> (file-hash* directory + #:algorithm (hash-algorithm sha256) + #:recursive? #t)) + (drv (git-fetch + (git-reference + (url (string-append "file://" directory)) + (commit "v1.0.0")) + 'sha256 hash + "git-fetch-test"))) + (mbegin %store-monad + (built-derivations (list drv)) + (return (scandir (derivation->output-path drv))))))))) + (test-assert "gnu-build-system" (build-system? gnu-build-system))
Fixes <https://issues.guix.gnu.org/63331>. Longer-term this will remove Git from the derivation graph when its sole use is to perform a checkout for a fixed-output derivation, thereby breaking dependency cycles that can arise in these situations. * guix/git-download.scm (git-fetch): Rename to… (git-fetch/in-band): … this. Deal with GIT or GUILE being #f. (git-fetch/built-in, built-in-builders*, git-fetch): New procedures. * tests/builders.scm ("git-fetch, file URI"): New test. --- guix/git-download.scm | 68 +++++++++++++++++++++++++++++++++++++------ tests/builders.scm | 29 +++++++++++++++++- 2 files changed, 87 insertions(+), 10 deletions(-)