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

On 9/10/20 3:43 AM, Ludovic Courtès wrote:
> Hi Jesse,
>
> Jesse Gibbons <jgibbons2357@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@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

Hi,

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

On 9/11/20 9:43 AM, Ludovic Courtès wrote:
> Hi,
>
> Jesse Gibbons <jgibbons2357@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@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’.

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

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 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@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