Message ID | 9c60e5f2-e9fa-55c9-863f-cc3bccc2cf15@gmail.com |
---|---|
State | Accepted |
Headers | show |
Series | [bug#43193] guix: Add --with-dependency-source option | expand |
Context | Check | Description |
---|---|---|
cbaines/comparison | success | View comparision |
cbaines/git branch | success | View Git branch |
cbaines/applying patch | fail | View Laminar job |
This is a friendly bump. On 9/26/20 4:46 PM, Jesse Gibbons wrote: > Attached are the patches that make the --with-source option recursive, > add documentation, add a test, adjust a test, and update the news. > > As expected, the changes I made are incompatible with the test > "options->transformation, with-source, no matches". Since > options->transformation, with-source does a recursive replacement, it > returns a clone of the package in question. I have tried changing the > comparison to eq?, eqv? and equal?, each returning false, so I settled > on a (limited) comparison of the packages' attributes. > > On 9/13/20 6:55 AM, Ludovic Courtès wrote: >> Hi, >> >> Jesse Gibbons <jgibbons2357@gmail.com> skribis: >> >>> On 9/11/20 9:43 AM, Ludovic Courtès wrote: >> [...] >> >>>> So maybe drop the second clause for non-recursive replacement, and >>>> drop >>>> ‘transform-package-source’ as well. >>> I included a fallback to transform-package-source because the >>> following happens: >>> >>> $ ./pre-inst-env guix build --with-source=$(guix build --source >>> hello) hello >>> guix build: error: invalid source replacement specification: >>> "/gnu/store/hbdalsf5lpf01x4dcknwx6xbn6n5km6k-hello-2.10.tar.gz" >>> >>> This does not fail when I fall back to the non-recursive logic. >>> >>> I can drop transform-package-source, but I will need to do some more >>> hacking to figure out how the package name and version are parsed from >>> the file name as described in the manual, and move it to the logic in >>> transform-package-inputs/source. >> Yes, that’d be nice. Namely, if you do: >> >> guix build hello --source=hello-1.2.3.tar.gz >> >> it should work just as now (from the source file name, we assume that >> the source applies to package “hello”). >> >> Conversely, doing: >> >> guix build hello --source=xyz-hello-1.2.3.tar.gz >> >> would have no effect. It would not even emit a warning, unlike now. >> >>> I'm not going to have as much free time starting next week, so I might >>> not be able to do that for a while, but I will try to get it done >>> ASAP. >> Sure, let’s stay in touch, I think we’re almost done! >> >> Thank you, >> Ludo’.
Hi Jesse, Jesse Gibbons <jgibbons2357@gmail.com> skribis: > Attached are the patches that make the --with-source option recursive, > add documentation, add a test, adjust a test, and update the news. Great, and apologies for the delay. >>From 2d2f6c97ad1deeb2fc8a214d992c7894a7c5e293 Mon Sep 17 00:00:00 2001 > From: Jesse Gibbons <jgibbons2357+guix@gmail.com> > Date: Thu, 3 Sep 2020 17:45:08 -0600 > Subject: [PATCH 1/2] guix: Make --with-source option recursive > > * guix/scripts/build.scm: (transform-package-inputs/source): new > function > (evaluate-source-replacement-specs): new function > (%transformations): change with-source to use > evaluate-source-replacement-specs > > * doc/guix.texi (Package Transformation Options): document it. > > * tests/scripts-build.scm: (options->transformation, with-source, no > matches): adjust to new expectations. > (options->transformation, with-source, recursive): new test. [...] > +++ b/doc/guix.texi > @@ -9142,8 +9142,8 @@ without having to type in the definitions of package variants > @itemx --with-source=@var{package}=@var{source} > @itemx --with-source=@var{package}@@@var{version}=@var{source} > Use @var{source} as the source of @var{package}, and @var{version} as > -its version number. > -@var{source} must be a file name or a URL, as for @command{guix > +its version number. This replacement is applied recursively on all > +dependencies. @var{source} must be a file name or a URL, as for @command{guix > download} (@pxref{Invoking guix download}). Maybe s/all dependencies/all matching dependencies/? > +++ b/guix/scripts/build.scm > @@ -201,9 +201,9 @@ matching URIs given in SOURCES." > (#f > ;; Determine the package name and version from URI. > (call-with-values > - (lambda () > - (hyphen-package-name->name+version > - (tarball-base-name (basename uri)))) > + (lambda () > + (hyphen-package-name->name+version > + (tarball-base-name (basename uri)))) Please avoid unrelated whitespace changes like this one. > +(define (transform-package-inputs/source replacement-specs) Maybe call it ‘transform-package-source’ and remove the previous ‘transform-package-source’ procedure. > + "Return a procedure that, when passed a package, replaces its direct > +dependencies according to REPLACEMENT-SPECS. REPLACEMENT-SPECS is a list of > +strings like \"guile=/path/to/source\" or > +\"guile=https://www.example.com/guile-source.tar.gz\" meaning that, any > +dependency on a package called \"guile\" must be replaced with a dependency on a > +\"guile\" built with the source at the specified location. SPECS may also > +simply be a file location, in which case the package name and version are parsed > +from the file name." > + (lambda (store obj) > + (let* ((replacements (evaluate-source-replacement-specs replacement-specs > + (lambda* (old file #:optional version) > + (package-with-source store old file version)))) Please indent like the rest of the code (if you use Emacs, you can hit M-q to have it indent the surrounding expression correctly). Here the second line should be aligned with the first ‘a’ of ‘lambda*’. Also please arrange to keep lines below 80 chars. > + (rewrite (package-input-rewriting/spec replacements)) > + (rewrite* (lambda (obj) > + (rewrite obj)))) You can remove ‘rewrite*’ and use ‘rewrite’ directly. > +(define (evaluate-source-replacement-specs specs proc) > + "Parse SPECS, a list of strings like \"guile=/path/to/source\", and return a > +list of package pairs, where (PROC PACKAGE URL) returns the replacement package. > +Raise an error if an element of SPECS uses invalid syntax, or if a package it > +refers to could not be found." > + (define* (replacement file #:optional version) > + (lambda (old) > + (proc old file version))) > + (map (lambda (spec) > + (match (string-tokenize spec %not-equal) > + ((package-spec file) > + (let* ((spec-list (call-with-values > + (lambda () > + (package-specification->name+version+output package-spec)) > + list)) > + (name (list-ref spec-list 0)) > + (version (list-ref spec-list 1))) Use ‘let-values’ instead: (let-values (((name version) (package-specification->name+version+output spec))) …) Also maybe s/package-spec/spec/; it’s clear enough in this context. > + (cons name (replacement file version)))) > + ((file) > + (let* ((package-spec > + (call-with-values > + (lambda () > + (hyphen-package-name->name+version > + (tarball-base-name (basename file)))) > + (lambda (name version) > + (cons name version)))) > + (name (car package-spec)) > + (version (cdr package-spec))) > + (cons name (replacement file version)))) ‘let-values’ also. > +++ b/tests/scripts-build.scm > @@ -94,9 +94,9 @@ > (let* ((port (open-output-string)) > (new (parameterize ((guix-warning-port port)) > (t store p)))) > - (and (eq? new p) > - (string-contains (get-output-string port) > - "had no effect")))))) > + (and (eq? (package-version new) (package-version p)) > + (eq? (package-name new) (package-name p)) > + (eq? (package-source new) (package-source p))))))) We can probably remove this test since the behavior it was checking no longer exists. > +(test-assert "options->transformation, with-source, recursive" > + (let* ((q (dummy-package "foo")) > + (p (dummy-package "guix.scm" > + (inputs `(("foo" ,q))))) > + (s (search-path %load-path "guix.scm")) > + (f (string-append "foo@42.0=" s)) > + (t (options->transformation `((with-source . ,f))))) > + (with-store store > + (let ((new (t store p))) > + (and (not (eq? new p)) > + (match (package-inputs new) > + ((("foo" dep1)) > + (and > + (string=? (package-name dep1) "foo") > + (string=? (package-version dep1) "42.0") > + (string=? (package-source dep1) > + (add-to-store store (basename s) #t > + "sha256" s)))))))))) Please indent correctly. >>From 738fdb35af1449e245ce2e48c266c82f798d89af Mon Sep 17 00:00:00 2001 > From: Jesse Gibbons <jgibbons2357+guix@gmail.com> > Date: Sat, 26 Sep 2020 16:29:25 -0600 > Subject: [PATCH 2/2] news: Add entry for "--with-source" > > * etc/news,scm: Add entry. [...] > + (entry (commit "2d2f6c97ad1deeb2fc8a214d992c7894a7c5e293") > + (title (en "@option{--with-source} now recursive")) ^ + “package transformation option is” > + (body (en "The @option{--with-source} common option now uses the > +specified source for all matching dependencies of any packages guix is directed > +to work with. This option is useful for all package maintainers, developers, > +and, in general, all users who want guix to facilitate their rights to modify > +their software and share their changes."))) Usually there’s an extra sentence like “Run info …” explaining how to read the relevant part of the manual; it may be a good idea to add it. Could you send an updated patch? Hopefully the last one! Thanks, Ludo’.
From 738fdb35af1449e245ce2e48c266c82f798d89af Mon Sep 17 00:00:00 2001 From: Jesse Gibbons <jgibbons2357+guix@gmail.com> Date: Sat, 26 Sep 2020 16:29:25 -0600 Subject: [PATCH 2/2] news: Add entry for "--with-source" * etc/news,scm: Add entry. --- etc/news.scm | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/etc/news.scm b/etc/news.scm index 1ef238ca2d..fcb195c679 100644 --- a/etc/news.scm +++ b/etc/news.scm @@ -13,6 +13,14 @@ (channel-news (version 0) + (entry (commit "2d2f6c97ad1deeb2fc8a214d992c7894a7c5e293") + (title (en "@option{--with-source} now recursive")) + (body (en "The @option{--with-source} common option now uses the +specified source for all matching dependencies of any packages guix is directed +to work with. This option is useful for all package maintainers, developers, +and, in general, all users who want guix to facilitate their rights to modify +their software and share their changes."))) + (entry (commit "a98712785e0b042a290420fd74e5a4a5da4fc68f") (title (en "New @command{guix git authenticate} command") (de "Neuer Befehl @command{guix git authenticate}") -- 2.28.0