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

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 43193 in the body.
You can then email your comments to 43193 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Jesse Gibbons <jgibbons2357 <at> gmail.com>
To: Guix Patches <guix-patches <at> gnu.org>
Subject: [PATCH] guix: Add --with-dependency-source option
Date: Thu, 3 Sep 2020 22:30:02 -0600

[Message part 1 (text/plain, inline)]

    The attached patch adds support for the --with-dependency-source 
common build option. It can be used to specify the sources of 
dependencies of specified packages. With this step, we can close #42155. 
This is also a step in closing #43061.

Note that I suggested making a new 
--with-dependency-source=package=source build option in response to 
#42155 and nobody responded with an objection. So I am sending this 
patch with the assumption that there are no objections to this new build 
option, and that if a member of the community wants to completely 
replace the behavior of --with-source with the behavior of the new 
option, that person can do the work required to not break 
--with-source=source.

-Jesse

[v1-0001-guix-Add-with-dependency-source-option.patch (text/x-patch, attachment)]

Message #8 received at 43193 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Jesse Gibbons <jgibbons2357 <at> gmail.com>
Cc: 43193 <at> debbugs.gnu.org
Subject: Re: [bug#43193] [PATCH] guix: Add --with-dependency-source option
Date: Thu, 10 Sep 2020 11:43:50 +0200

Hi Jesse,

Jesse Gibbons <jgibbons2357 <at> gmail.com> skribis:

>     The attached patch adds support for the --with-dependency-source
> common build option. It can be used to specify the sources of
> dependencies of specified packages. With this step, we can close
> #42155. This is also a step in closing #43061.

Excellent!

> Note that I suggested making a new
> --with-dependency-source=package=source build option in response to
> #42155 and nobody responded with an objection. So I am sending this
> patch with the assumption that there are no objections to this new
> build option, and that if a member of the community wants to
> completely replace the behavior of --with-source with the behavior of
> the new option, that person can do the work required to not break
> --with-source=source.

OK.  Like I wrote at <https://issues.guix.gnu.org/42155#3>, I wouldn’t
mind simply calling this new option ‘--with-source’.  What we’d lose by
doing so is the warning you get if you do
‘--with-source=does-not-exist=whatever’, saying the option had no
effect, but that’s about it.  The new ‘--with-source’ behavior
(recursive) would be consistent with the other options, which, to me, is
more important.

WDYT?

>>From 91a89277067fd454ad77edb3a09ed06382f3694c 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 v1 1/1] guix: Add --with-dependency-source option
>
> * guix/scripts/build.scm: (transform-package-inputs/source): new
>   function
> (evaluate-source-replacement-specs): new function
> (%transformations): add with-dependency-source option
> (%transformation-options): add with-dependency-source-option
> (show-transformation-options-help): document --with-dependency-source

[...]

> +(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."
> +  (map (lambda (spec)
> +         (match (string-tokenize spec %not-equal)
> +           ((spec url)
> +            (define (replace old)
> +              (proc old url))
> +            (cons spec replace))
> +           (x
> +            (leave (G_ "invalid replacement specification: ~s~%") spec))))
                                  ^
Add “source” here.  It’s always a good idea to not have the exact same
error message in several places.  :-)

Could you:

  1. adjust doc/guix.texi accordingly?  That is, if we rename this new
     option to ‘--with-source’, simply add a note stating that it’s
     recursive.

  2. add a test to tests/guix-build.sh?  There are already --with-source
     tests in other files.  You can mimic them, essentially to make sure
     the option has an effect.

  3. optionally add an entry as a separate commit to
     etc/news.scm.  I can do that for you if you want.

Thanks!

Ludo’.

Message #11 received at 43193 <at> debbugs.gnu.org (full text, mbox):

From: Jesse Gibbons <jgibbons2357 <at> gmail.com>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 43193 <at> debbugs.gnu.org
Subject: Re: [bug#43193] [PATCH] guix: Add --with-dependency-source option
Date: Thu, 10 Sep 2020 23:16:28 -0600

[Message part 1 (text/plain, inline)]

On 9/10/20 3:43 AM, Ludovic Courtès wrote:
> Hi Jesse,
>
> Jesse Gibbons <jgibbons2357 <at> gmail.com> skribis:
>
>>      The attached patch adds support for the --with-dependency-source
>> common build option. It can be used to specify the sources of
>> dependencies of specified packages. With this step, we can close
>> #42155. This is also a step in closing #43061.
> Excellent!
>
>> Note that I suggested making a new
>> --with-dependency-source=package=source build option in response to
>> #42155 and nobody responded with an objection. So I am sending this
>> patch with the assumption that there are no objections to this new
>> build option, and that if a member of the community wants to
>> completely replace the behavior of --with-source with the behavior of
>> the new option, that person can do the work required to not break
>> --with-source=source.
> OK.  Like I wrote at <https://issues.guix.gnu.org/42155#3>, I wouldn’t
> mind simply calling this new option ‘--with-source’.  What we’d lose by
> doing so is the warning you get if you do
> ‘--with-source=does-not-exist=whatever’, saying the option had no
> effect, but that’s about it.  The new ‘--with-source’ behavior
> (recursive) would be consistent with the other options, which, to me, is
> more important.
I agree that '--with-source' is a better name for the option, but I 
don't want to lose that particular functionality. I worked a little more 
to alter "--with-source" while still preserving the simple 
'--with-source=source' option, because once it's committed to master, it 
will be difficult to turn back and get the ideal implementation. The 
result is a bit dirty and should be refactored and cleaned, but at least 
it works. Attached is the updated patch.
>
> WDYT?
>
>> >From 91a89277067fd454ad77edb3a09ed06382f3694c 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 v1 1/1] guix: Add --with-dependency-source option
>>
>> * guix/scripts/build.scm: (transform-package-inputs/source): new
>>    function
>> (evaluate-source-replacement-specs): new function
>> (%transformations): add with-dependency-source option
>> (%transformation-options): add with-dependency-source-option
>> (show-transformation-options-help): document --with-dependency-source
> [...]
>
>> +(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."
>> +  (map (lambda (spec)
>> +         (match (string-tokenize spec %not-equal)
>> +           ((spec url)
>> +            (define (replace old)
>> +              (proc old url))
>> +            (cons spec replace))
>> +           (x
>> +            (leave (G_ "invalid replacement specification: ~s~%") spec))))
>                                    ^
> Add “source” here.  It’s always a good idea to not have the exact same
> error message in several places.  :-)
Fixed.
> Could you:
>
>    1. adjust doc/guix.texi accordingly?  That is, if we rename this new
>       option to ‘--with-source’, simply add a note stating that it’s
>       recursive.
I included this in the attached patch.
>    2. add a test to tests/guix-build.sh?  There are already --with-source
>       tests in other files.  You can mimic them, essentially to make sure
>       the option has an effect.
>    3. optionally add an entry as a separate commit to
>       etc/news.scm.  I can do that for you if you want.
>
Do you still think the tests should be updated and this change should be 
announced in the news file? I'm willing to do these.
> Thanks!
>
> Ludo’.
-Jesse

[0001-guix-Make-with-source-option-recursive.patch (text/x-patch, attachment)]

Message #14 received at 43193 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Jesse Gibbons <jgibbons2357 <at> gmail.com>
Cc: 43193 <at> debbugs.gnu.org
Subject: Re: [bug#43193] [PATCH] guix: Add --with-dependency-source option
Date: Fri, 11 Sep 2020 17:43:09 +0200

Hi,

Jesse Gibbons <jgibbons2357 <at> gmail.com> skribis:

>> Could you:
>>
>>    1. adjust doc/guix.texi accordingly?  That is, if we rename this new
>>       option to ‘--with-source’, simply add a note stating that it’s
>>       recursive.
> I included this in the attached patch.
>>    2. add a test to tests/guix-build.sh?  There are already --with-source
>>       tests in other files.  You can mimic them, essentially to make sure
>>       the option has an effect.
>>    3. optionally add an entry as a separate commit to
>>       etc/news.scm.  I can do that for you if you want.
>>
> Do you still think the tests should be updated and this change should
> be announced in the news file? I'm willing to do these.

Yes, please.  There’s should be a test that checks that --with-source
works for non-leaf packages.  A new entry would also be nice.

> From 2786da1e7011c59f08fc150dfa284f35bc0ed093 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/1] 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.

Nitpick: There are spacing and capitalization issues.  Please see ‘git
log’ for examples.

> +++ b/doc/guix.texi
> @@ -9129,7 +9129,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.
> +its version number.  This replacement is applied recursively on all
> +dependencies only if PACKAGE is specified.

s/PACKAGE/@var{package}/

However, the semantics are a bit “weird”.  I would just do the recursive
replacement thing unconditionally.  WDYT?

> +(define (transform-package-inputs/source replacement-specs)
> +  "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."
> +  (match (string-tokenize (car replacement-specs) %not-equal)
> +    ((spec url)

s/url/file/ since it’s a file name.

> +     (lambda (store obj)
> +       (let* ((replacements (evaluate-source-replacement-specs replacement-specs
> +                                                               (lambda (old url)
> +                                                                 (package-with-source store old url))))
> +              (rewrite (package-input-rewriting/spec replacements))
> +              (rewrite* (lambda (obj)
> +                          (rewrite obj))))
> +         (if (package? obj)
> +             (rewrite* obj)
> +             obj))))
> +    ((url)
> +     (transform-package-source replacement-specs))))

So maybe drop the second clause for non-recursive replacement, and drop
‘transform-package-source’ as well.

Thanks!

Ludo’.

Message #17 received at 43193 <at> debbugs.gnu.org (full text, mbox):

From: Jesse Gibbons <jgibbons2357 <at> gmail.com>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 43193 <at> debbugs.gnu.org
Subject: Re: [bug#43193] [PATCH] guix: Add --with-dependency-source option
Date: Fri, 11 Sep 2020 12:28:31 -0600

On 9/11/20 9:43 AM, Ludovic Courtès wrote:
> Hi,
>
> Jesse Gibbons <jgibbons2357 <at> gmail.com> skribis:
>
>>> Could you:
>>>
>>>     1. adjust doc/guix.texi accordingly?  That is, if we rename this new
>>>        option to ‘--with-source’, simply add a note stating that it’s
>>>        recursive.
>> I included this in the attached patch.
>>>     2. add a test to tests/guix-build.sh?  There are already --with-source
>>>        tests in other files.  You can mimic them, essentially to make sure
>>>        the option has an effect.
>>>     3. optionally add an entry as a separate commit to
>>>        etc/news.scm.  I can do that for you if you want.
>>>
>> Do you still think the tests should be updated and this change should
>> be announced in the news file? I'm willing to do these.
> Yes, please.  There’s should be a test that checks that --with-source
> works for non-leaf packages.  A new entry would also be nice.
I will work on that then.
>
>>  From 2786da1e7011c59f08fc150dfa284f35bc0ed093 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/1] 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.
> Nitpick: There are spacing and capitalization issues.  Please see ‘git
> log’ for examples.
I'll fix this.
>> +++ b/doc/guix.texi
>> @@ -9129,7 +9129,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.
>> +its version number.  This replacement is applied recursively on all
>> +dependencies only if PACKAGE is specified.
> s/PACKAGE/@var{package}/
>
> However, the semantics are a bit “weird”.  I would just do the recursive
> replacement thing unconditionally.  WDYT?
>
>> +(define (transform-package-inputs/source replacement-specs)
>> +  "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."
>> +  (match (string-tokenize (car replacement-specs) %not-equal)
>> +    ((spec url)
> s/url/file/ since it’s a file name.
>
>> +     (lambda (store obj)
>> +       (let* ((replacements (evaluate-source-replacement-specs replacement-specs
>> +                                                               (lambda (old url)
>> +                                                                 (package-with-source store old url))))
>> +              (rewrite (package-input-rewriting/spec replacements))
>> +              (rewrite* (lambda (obj)
>> +                          (rewrite obj))))
>> +         (if (package? obj)
>> +             (rewrite* obj)
>> +             obj))))
>> +    ((url)
>> +     (transform-package-source replacement-specs))))
> 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. 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.

>
> Thanks!
>
> Ludo’.

Message #20 received at 43193 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Jesse Gibbons <jgibbons2357 <at> gmail.com>
Cc: 43193 <at> debbugs.gnu.org
Subject: Re: [bug#43193] [PATCH] guix: Add --with-dependency-source option
Date: Sun, 13 Sep 2020 14:55:28 +0200

Hi,

Jesse Gibbons <jgibbons2357 <at> 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’.

Message #23 received at 43193 <at> debbugs.gnu.org (full text, mbox):

From: Jesse Gibbons <jgibbons2357 <at> gmail.com>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 43193 <at> debbugs.gnu.org
Subject: Re: [bug#43193] [PATCH] guix: Add --with-dependency-source option
Date: Sun, 13 Sep 2020 08:28:50 -0600

On 9/13/20 6:55 AM, Ludovic Courtès wrote:
> Hi,
>
> Jesse Gibbons <jgibbons2357 <at> 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 was able to get this working nicely.
>> 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!

I wrote a new test that checks non-leafs. The following tests fail:

test-name: options->transformation, no transformations
test-name: options->transformation, with-source
test-name: options->transformation, with-source, replacement
test-name: options->transformation, with-source, with version
test-name: options->transformation, with-source, no matches
test-name: options->transformation, with-source, PKG <at> VER=URI

Some of these tests raise a similar error, some only have a false actual 
value, and "options->transformation, with-source, no matches" will need 
to be changed at the test level because it checks for a warning in the 
output. I've been trying to debug, but the tests don't even give a stack 
trace. I will do what I can to fix the tests and follow up at the end of 
the week.

-Jesse

Message #26 received at 43193 <at> debbugs.gnu.org (full text, mbox):

From: Jesse Gibbons <jgibbons2357 <at> gmail.com>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 43193 <at> debbugs.gnu.org
Subject: Re: [bug#43193] [PATCH] guix: Add --with-dependency-source option
Date: Sat, 26 Sep 2020 16:46:16 -0600

[Message part 1 (text/plain, inline)]

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

[0001-guix-Make-with-source-option-recursive.patch (text/x-patch, attachment)]

[0002-news-Add-entry-for-with-source.patch (text/x-patch, attachment)]

Message #29 received at 43193 <at> debbugs.gnu.org (full text, mbox):

From: Jesse Gibbons <jgibbons2357 <at> gmail.com>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 43193 <at> debbugs.gnu.org
Subject: Re: [bug#43193] [PATCH] guix: Add --with-dependency-source option
Date: Wed, 7 Oct 2020 21:43:34 -0600

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

Message #32 received at 43193 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Jesse Gibbons <jgibbons2357 <at> gmail.com>
Cc: 43193 <at> debbugs.gnu.org
Subject: Re: [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’.

Message #35 received at 43193 <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 58032 <at> debbugs.gnu.org,
 Ludovic Courtès <ludovic.courtes <at> inria.fr>,
 43193 <at> debbugs.gnu.org, philippe.swartvagher <at> inria.fr
Subject: Re: bug#58032: [PATCH] transformations: '--with-source' now
 operates in depth.
Date: Wed, 28 Sep 2022 12:46:39 -0400

Hi,

Ludovic Courtès <ludo <at> gnu.org> writes:

> From: Ludovic Courtès <ludovic.courtes <at> inria.fr>
>
> The '--with-source' option is the first one that was implemented, and
> it's the only one that would operate only on leaf packages rather than
> traversing the dependency graph.  This change makes it consistent with
> the rest of the transformation options.

Good idea!  I needed to workaround the lack of recursion at least once.

[...]

> diff --git a/guix/transformations.scm b/guix/transformations.scm
> index 411c4014cb..be2d31b8c7 100644
> --- a/guix/transformations.scm
> +++ b/guix/transformations.scm
> @@ -129,42 +129,46 @@ (define* (package-with-source p uri #:optional version)
>  ;;; Transformations.
>  ;;;
>  
> -(define (transform-package-source sources)
> +(define (evaluate-source-replacement-specs specs)
> +  "Parse SPECS, a list of strings like \"guile=/tmp/guile-4.2.tar.gz\" or just
> +\"/tmp/guile-4.2.tar.gz\" and return a list of package spec/procedure pairs as
> +expected by 'package-input-rewriting/spec'.  Raise an error if an element of
> +SPECS uses invalid syntax."
> +  (define not-equal
> +    (char-set-complement (char-set #\=)))
> +
> +  (map (lambda (spec)
> +         (match (string-tokenize spec not-equal)
> +           ((uri)
> +            (let* ((base (tarball-base-name (basename uri)))

Unrelated, but 'tarball-base-name' is a bit of a misnomer since the file
could be a single, non-tarball archive (or plain file).

> +                   (name (hyphen-package-name->name+version base)))
> +              (cons name
> +                    (lambda (old)
> +                      (package-with-source old uri)))))
> +           ((spec uri)
> +            (let-values (((name version)
> +                          (package-name->name+version spec)))

You usually recommend (srfi srfi-71) when using multiple values; why not
use it here?  I don't have a preference myself (I find srfi-71
surprising for the non-initiated (I was), although I like its simple
interface! :-)).
> +              ;; Note: Here VERSION is used as the version string of the new
> +              ;; package rather than as part of the spec of the package being
> +              ;; targeted.
> +              (cons name
> +                    (lambda (old)
> +                      (package-with-source old uri version)))))
> +           (_
> +            (raise (formatted-message
> +                    (G_ "invalid source replacement specification: ~s")
> +                    spec)))))
> +       specs))
> +
> +(define (transform-package-source replacement-specs)
>    "Return a transformation procedure that replaces package sources with the
> -matching URIs given in SOURCES."
> -  (define new-sources
> -    (map (lambda (uri)
> -           (match (string-index uri #\=)
> -             (#f
> -              ;; Determine the package name and version from URI.
> -              (call-with-values
> -                  (lambda ()
> -                    (hyphen-package-name->name+version
> -                     (tarball-base-name (basename uri))))
> -                (lambda (name version)
> -                  (list name version uri))))
> -             (index
> -              ;; What's before INDEX is a "PKG <at> VER" or "PKG" spec.
> -              (call-with-values
> -                  (lambda ()
> -                    (package-name->name+version (string-take uri index)))
> -                (lambda (name version)
> -                  (list name version
> -                        (string-drop uri (+ 1 index))))))))
> -         sources))
> -
> -  (lambda (obj)
> -    (let loop ((sources  new-sources)
> -               (result   '()))
> -      (match obj
> -        ((? package? p)
> -         (match (assoc-ref sources (package-name p))
> -           ((version source)
> -            (package-with-source p source version))
> -           (#f
> -            p)))
> -        (_
> -         obj)))))
> +matching URIs given in REPLACEMENT-SPECS."
> +  (let* ((replacements (evaluate-source-replacement-specs replacement-specs))
> +         (rewrite      (package-input-rewriting/spec replacements)))
> +    (lambda (obj)
> +      (if (package? obj)
> +          (rewrite obj)
> +          obj))))
>  
>  (define (evaluate-replacement-specs specs proc)
>    "Parse SPECS, a list of strings like \"guile=guile <at> 2.1\" and return a list
> diff --git a/tests/transformations.scm b/tests/transformations.scm
> index dbfe523518..47b1fc650d 100644
> --- a/tests/transformations.scm
> +++ b/tests/transformations.scm
> @@ -103,16 +103,11 @@ (define-module (test-transformations)
>                                            "sha256" f))))))))))
>  
>  (test-assert "options->transformation, with-source, no matches"
> -  ;; When a transformation in not applicable, a warning must be raised.
>    (let* ((p (dummy-package "foobar"))
>           (s (search-path %load-path "guix.scm"))
>           (t (options->transformation `((with-source . ,s)))))
> -    (let* ((port (open-output-string))
> -           (new  (parameterize ((guix-warning-port port))
> -                   (t p))))
> -      (and (eq? new p)
> -           (string-contains (get-output-string port)
> -                            "had no effect")))))
> +    (eq? (package-source (t p))
> +         (package-source p))))

I'd personally find it a better interface if it failed noisily when
--with-source doesn't have any effect.  The warning was of little use
because it got lost in the other outputs; now it would be totally
silent, right?

>  (test-assert "options->transformation, with-source, PKG=URI"
>    (let* ((p (dummy-package "foo"))
> @@ -147,6 +142,29 @@ (define-module (test-transformations)
>                         (add-to-store store (basename s) #t
>                                       "sha256" s)))))))
>  
> +(test-assert "options->transformation, with-source, in depth"
> +  (let* ((p0 (dummy-package "foo" (version "0.0")))
> +         (s  (search-path %load-path "guix.scm"))
> +         (f  (string-append "foo <at> 42.0=" s))
> +         (t  (options->transformation `((with-source . ,f))))
> +         (p1 (dummy-package "bar" (inputs (list p0))))
> +         (p2 (dummy-package "baz" (inputs (list p1)))))
> +    (with-store store
> +      (let ((new (t p2)))
> +        (and (not (eq? new p2))
> +             (match (package-inputs new)
> +               ((("bar" p1*))
> +                (match (package-inputs p1*)
> +                  ((("foo" p0*))
> +                   (and (not (eq? p0* p0))
> +                        (string=? (package-name p0*) (package-name p0))
> +                        (string=? (package-version p0*) "42.0")
> +                        (string=? (add-to-store store (basename s) #t
> +                                                "sha256" s)
> +                                  (run-with-store store
> +                                    (lower-object
> +                                     (package-source p0*))))))))))))))
> +

The recursive? option should probably be #f in the add-store above,
since the "dummy" source is a single file.  It may be better to create
the dummy file ourselves instead of relying on the existence of a
'guix.scm' one (it'd help clarify the test too, that bit was a bit
mysterious at first).

Other than that, LGTM!

Thanks,

Maxim

Message #38 received at 43193 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludovic.courtes <at> inria.fr>
To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Cc: 58032 <at> debbugs.gnu.org, 43193 <at> debbugs.gnu.org, philippe.swartvagher <at> inria.fr
Subject: Re: bug#58032: [PATCH] transformations: '--with-source' now
 operates in depth.
Date: Thu, 29 Sep 2022 13:00:04 +0200

Hi Maxim,

Maxim Cournoyer <maxim.cournoyer <at> gmail.com> skribis:

>> +                   (name (hyphen-package-name->name+version base)))
>> +              (cons name
>> +                    (lambda (old)
>> +                      (package-with-source old uri)))))
>> +           ((spec uri)
>> +            (let-values (((name version)
>> +                          (package-name->name+version spec)))
>
> You usually recommend (srfi srfi-71) when using multiple values; why not
> use it here?  I don't have a preference myself (I find srfi-71
> surprising for the non-initiated (I was), although I like its simple
> interface! :-)).

This file is still on SRFI-11 so I followed that, but now that you
mention it, I’ll switch it to SRFI-71 in a subsequent commit.  :-)

>>  (test-assert "options->transformation, with-source, no matches"
>> -  ;; When a transformation in not applicable, a warning must be raised.
>>    (let* ((p (dummy-package "foobar"))
>>           (s (search-path %load-path "guix.scm"))
>>           (t (options->transformation `((with-source . ,s)))))
>> -    (let* ((port (open-output-string))
>> -           (new  (parameterize ((guix-warning-port port))
>> -                   (t p))))
>> -      (and (eq? new p)
>> -           (string-contains (get-output-string port)
>> -                            "had no effect")))))
>> +    (eq? (package-source (t p))
>> +         (package-source p))))
>
> I'd personally find it a better interface if it failed noisily when
> --with-source doesn't have any effect.  The warning was of little use
> because it got lost in the other outputs; now it would be totally
> silent, right?

Yes, it’ll be silent now, as with the other options.

That’s not great; the problem is that it’s impossible to tell whether a
package variant differs from the original one until they’ve been lowered
to a bag (or a derivation).

>> +(test-assert "options->transformation, with-source, in depth"
>> +  (let* ((p0 (dummy-package "foo" (version "0.0")))
>> +         (s  (search-path %load-path "guix.scm"))
>> +         (f  (string-append "foo <at> 42.0=" s))
>> +         (t  (options->transformation `((with-source . ,f))))
>> +         (p1 (dummy-package "bar" (inputs (list p0))))
>> +         (p2 (dummy-package "baz" (inputs (list p1)))))
>> +    (with-store store
>> +      (let ((new (t p2)))
>> +        (and (not (eq? new p2))
>> +             (match (package-inputs new)
>> +               ((("bar" p1*))
>> +                (match (package-inputs p1*)
>> +                  ((("foo" p0*))
>> +                   (and (not (eq? p0* p0))
>> +                        (string=? (package-name p0*) (package-name p0))
>> +                        (string=? (package-version p0*) "42.0")
>> +                        (string=? (add-to-store store (basename s) #t
>> +                                                "sha256" s)
>> +                                  (run-with-store store
>> +                                    (lower-object
>> +                                     (package-source p0*))))))))))))))
>> +
>
> The recursive? option should probably be #f in the add-store above,
> since the "dummy" source is a single file.

No, it must be #t because that’s also how the transformation interns the
file (the option is called “recursive?” but it has little to do with
recursive directory traversal and more to do with the serialization
format.)

> It may be better to create the dummy file ourselves instead of relying
> on the existence of a 'guix.scm' one (it'd help clarify the test too,
> that bit was a bit mysterious at first).

Right, we could do that but that’s a tiny bit more verbose.  Here I
followed what existing tests do; perhaps we should plan for a face lift
of some of these tests.

Thanks for your feedback!

Ludo’.

Message #43 received at 43193-done <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Cc: 58032-done <at> debbugs.gnu.org, 43193-done <at> debbugs.gnu.org,
 philippe.swartvagher <at> inria.fr
Subject: Re: bug#58032: [PATCH] transformations: '--with-source' now
 operates in depth.
Date: Thu, 29 Sep 2022 23:11:46 +0200

Pushed!

  4244f5e9a7 news: Add entry for '--with-source'.
  28ade1bab2 transformations: '--with-source' now operates in depth.

Ludo'.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.