unofficial mirror of guix-patches@gnu.org 
 help / color / mirror / code / Atom feed
From: "Ludovic Courtès" <ludo@gnu.org>
To: Jesse Gibbons <jgibbons2357@gmail.com>
Cc: 43193@debbugs.gnu.org
Subject: [bug#43193] [PATCH] guix: Add --with-dependency-source option
Date: Thu, 22 Oct 2020 17:08:10 +0200	[thread overview]
Message-ID: <87pn5as3hh.fsf@gnu.org> (raw)
In-Reply-To: <9c60e5f2-e9fa-55c9-863f-cc3bccc2cf15@gmail.com> (Jesse Gibbons's message of "Sat, 26 Sep 2020 16:46:16 -0600")

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




      parent reply	other threads:[~2020-10-22 15:09 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-04  4:30 [bug#43193] [PATCH] guix: Add --with-dependency-source option Jesse Gibbons
2020-09-10  9:43 ` Ludovic Courtès
2020-09-11  5:16   ` Jesse Gibbons
2020-09-11 15:43     ` Ludovic Courtès
2020-09-11 18:28       ` Jesse Gibbons
2020-09-13 12:55         ` Ludovic Courtès
2020-09-13 14:28           ` Jesse Gibbons
2020-09-26 22:46           ` Jesse Gibbons
2020-10-08  3:43             ` Jesse Gibbons
2020-10-22 15:08             ` Ludovic Courtès [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://guix.gnu.org/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87pn5as3hh.fsf@gnu.org \
    --to=ludo@gnu.org \
    --cc=43193@debbugs.gnu.org \
    --cc=jgibbons2357@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/guix.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).