Package: guix-patches;
Reported by: Jesse Gibbons <jgibbons2357 <at> gmail.com>
Date: Fri, 4 Sep 2020 04:31:01 UTC
Severity: normal
Tags: patch
Done: Ludovic Courtès <ludo <at> gnu.org>
Bug is archived. No further changes may be made.
View this message in rfc822 format
From: Ludovic Courtès <ludo <at> gnu.org> To: Jesse Gibbons <jgibbons2357 <at> gmail.com> Cc: 43193 <at> debbugs.gnu.org Subject: [bug#43193] [PATCH] guix: Add --with-dependency-source option Date: Thu, 22 Oct 2020 17:08:10 +0200
Hi Jesse,
Jesse Gibbons <jgibbons2357 <at> 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 <at> 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 <at> 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 <at> 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’.
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.