[bug#43193] [PATCH] guix: Add --with-dependency-source option

["Ludovic Courtès" <ludo@gnu.org>]

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