diff mbox series

[bug#65062,core-updates,1/1] packages: Specify output in input label when it's not "out".

Message ID b6c9adca21cc4418219b51532c2f0a9bddb208f0.1691202289.git.hako@ultrarare.space
State New
Headers show
Series Specify output in input label when it's not "out". | expand

Commit Message

Hilton Chain Aug. 5, 2023, 2:53 a.m. UTC
* guix/packages.scm (add-input-label): Specify output when it's not "out".
---
 guix/packages.scm | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Comments

Ludovic Courtès Aug. 22, 2023, 4 p.m. UTC | #1
Hi,

Hilton Chain <hako@ultrarare.space> skribis:

> * guix/packages.scm (add-input-label): Specify output when it's not "out".

[...]

> +         (list (string-append (package-name package) ":" output)
> +               package
> +               output)))

The Grand Plan¹ is to eventually get rid of labels entirely (or almost:
there’d still be input alists on the build side).  As such, I thought we
shouldn’t worry too much about what the actual label is.  But perhaps
you stumbled upon situations where this is a problem?  Could you
describe them?

Thanks,
Ludo’.

¹ https://guix.gnu.org/en/blog/2021/the-big-change/
Hilton Chain Aug. 24, 2023, 3:42 a.m. UTC | #2
Hi Ludo,

On Wed, 23 Aug 2023 00:00:00 +0800,
Ludovic Courtès wrote:
>
> Hi,
>
> Hilton Chain <hako@ultrarare.space> skribis:
>
> > * guix/packages.scm (add-input-label): Specify output when it's not "out".
>
> [...]
>
> > +         (list (string-append (package-name package) ":" output)
> > +               package
> > +               output)))
>
> The Grand Plan¹ is to eventually get rid of labels entirely (or almost:
> there’d still be input alists on the build side).  As such, I thought we
> shouldn’t worry too much about what the actual label is.  But perhaps
> you stumbled upon situations where this is a problem?  Could you
> describe them?
>
> Thanks,
> Ludo’.
>
> ¹ https://guix.gnu.org/en/blog/2021/the-big-change/

My main concern is that currently modify-inputs, this-package-input
and this-package-native-input operate on input labels and there would
be duplicated labels if adding multiple outputs of a package.

For modify-inputs, I think there's no approach to solve this without
also specifying labels in inputs.

Although this-package-* can be replaced by search-input-*, I'd like to
avoid (dirname (dirname (search-input-file inputs "/lib/..."))) when
(this-package-input "...") is available.


For current this-package-* vs. search-input-*, I have other points:

1. In the context of build system arguments, like #:configure-flags,
inputs and native-inputs as variables aren't available, one may need
to use %build-inputs, %build-host-inputs and %build-target-inputs for
search-input-*, which is inconsistent with other parts.

2. It might be a bit confusing when, for example, adding
tzdata-for-test to native-inputs, and referencing it with proper
cross-compilation support:
--8<---------------cut here---------------start------------->8---
(setenv "TZDIR"
        (search-input-directory
         (if #$(%current-target-system) native-inputs inputs)
         "/share/zoneinfo"))
--8<---------------cut here---------------end--------------->8---

In such cases I may prefer this-package-*, but it would be unreliable
when there're duplicated labels.


There's also issue referencing a package when multiple versions of it
under a same name are added to the inputs, which may not fall under
this "Subject:".


Thanks
Josselin Poiret Aug. 25, 2023, 11:10 a.m. UTC | #3
Hi everyone,

Hilton Chain <hako@ultrarare.space> writes:

> 2. It might be a bit confusing when, for example, adding
> tzdata-for-test to native-inputs, and referencing it with proper
> cross-compilation support:
> --8<---------------cut here---------------start------------->8---
> (setenv "TZDIR"
>         (search-input-directory
>          (if #$(%current-target-system) native-inputs inputs)
>          "/share/zoneinfo"))
> --8<---------------cut here---------------end--------------->8---

FWIW, the idiomatic way in Guix is to use `(or native-inputs inputs)`
instead of that if.

HTH,
Ludovic Courtès Sept. 8, 2023, 10:03 p.m. UTC | #4
Hi,

Hilton Chain <hako@ultrarare.space> skribis:

>> Hilton Chain <hako@ultrarare.space> skribis:
>>
>> > * guix/packages.scm (add-input-label): Specify output when it's not "out".
>>
>> [...]
>>
>> > +         (list (string-append (package-name package) ":" output)
>> > +               package
>> > +               output)))
>>
>> The Grand Plan¹ is to eventually get rid of labels entirely (or almost:
>> there’d still be input alists on the build side).  As such, I thought we
>> shouldn’t worry too much about what the actual label is.  But perhaps
>> you stumbled upon situations where this is a problem?  Could you
>> describe them?

[...]

> My main concern is that currently modify-inputs, this-package-input
> and this-package-native-input operate on input labels and there would
> be duplicated labels if adding multiple outputs of a package.
>
> For modify-inputs, I think there's no approach to solve this without
> also specifying labels in inputs.

Yes, good point.

Another, more radical approach, would be to change semantics, whereby
(inputs (list p)) would mean that all the outputs of ‘p’, not just
“out”, are taken as inputs.  That’d simplify inputs at the expense of
precision, and (this-package-input NAME) would always be unambiguous.

But maybe that’s too radical and uncertain.

So all things considered, I guess you’re right and we should do what you
propose.

Minor issues:

> --- a/guix/packages.scm
> +++ b/guix/packages.scm
> @@ -626,7 +626,13 @@ (define (add-input-label input)
>      ((? package? package)
>       (list (package-name package) package))
>      (((? package? package) output)                ;XXX: ugly?
> -     (list (package-name package) package output))
> +     (if (string=? output "out")
> +         ;; (package "out") => ("package" package "out")
> +         (list (package-name package) package output)
> +         ;; (package "output") => ("package:output" package "output")
> +         (list (string-append (package-name package) ":" output)
> +               package
> +               output)))

Rather write it as two separate clauses, without comments:

  (((? package? package) "out")
   …)
  (((? package? package) output)
   …)

Could you also add a test case in ‘tests/packages.scm’ that would look
up inputs by those labels?

Thanks,
Ludo’.
Hilton Chain Oct. 3, 2023, 9:13 a.m. UTC | #5
Hi Ludo,

On Sat, 09 Sep 2023 06:03:53 +0800,
Ludovic Courtès wrote:
>
> Hi,
>
> Hilton Chain <hako@ultrarare.space> skribis:
>
> >> Hilton Chain <hako@ultrarare.space> skribis:
> >>
> >> > * guix/packages.scm (add-input-label): Specify output when it's not "out".
> >>
> >> [...]
> >>
> >> > +         (list (string-append (package-name package) ":" output)
> >> > +               package
> >> > +               output)))
> >>
> >> The Grand Plan¹ is to eventually get rid of labels entirely (or almost:
> >> there’d still be input alists on the build side).  As such, I thought we
> >> shouldn’t worry too much about what the actual label is.  But perhaps
> >> you stumbled upon situations where this is a problem?  Could you
> >> describe them?
>
> [...]
>
> > My main concern is that currently modify-inputs, this-package-input
> > and this-package-native-input operate on input labels and there would
> > be duplicated labels if adding multiple outputs of a package.
> >
> > For modify-inputs, I think there's no approach to solve this without
> > also specifying labels in inputs.
>
> Yes, good point.
>
> Another, more radical approach, would be to change semantics, whereby
> (inputs (list p)) would mean that all the outputs of ‘p’, not just
> “out”, are taken as inputs.  That’d simplify inputs at the expense of
> precision, and (this-package-input NAME) would always be unambiguous.
>
> But maybe that’s too radical and uncertain.
>
> So all things considered, I guess you’re right and we should do what you
> propose.


Thank you!


> Minor issues:
>
> > --- a/guix/packages.scm
> > +++ b/guix/packages.scm
> > @@ -626,7 +626,13 @@ (define (add-input-label input)
> >      ((? package? package)
> >       (list (package-name package) package))
> >      (((? package? package) output)                ;XXX: ugly?
> > -     (list (package-name package) package output))
> > +     (if (string=? output "out")
> > +         ;; (package "out") => ("package" package "out")
> > +         (list (package-name package) package output)
> > +         ;; (package "output") => ("package:output" package "output")
> > +         (list (string-append (package-name package) ":" output)
> > +               package
> > +               output)))
>
> Rather write it as two separate clauses, without comments:
>
>   (((? package? package) "out")
>    …)
>   (((? package? package) output)
>    …)
>
> Could you also add a test case in ‘tests/packages.scm’ that would look
> up inputs by those labels?


I have thought about this patch again recently.


First of all, I didn't describe my own trouble clearly:

I wanted to put `this-package-input' into #$gcc:lib, but didn't know how.  Now I
understand that (ungexp (this-package-input "gcc") "lib") can be used and input
labels are not quite related...


And then I realised that there's too much extra work in package definitions for
the label change.


So, how about looking up inputs by specification (name + version + output), and
falling back to input labels?  I think this can address the issue regarding
multiple outputs and versions, while keeping compatible with existing behavior.

I'll send v2 for the change, with a different subject.  Though I haven't written
new tests for it, the existing (tests packages) passes when applied to master
and no package definition needs changing at least for building guix.


Thanks
diff mbox series

Patch

diff --git a/guix/packages.scm b/guix/packages.scm
index ba98bb0fb4..d0e6e16cbb 100644
--- a/guix/packages.scm
+++ b/guix/packages.scm
@@ -626,7 +626,13 @@  (define (add-input-label input)
     ((? package? package)
      (list (package-name package) package))
     (((? package? package) output)                ;XXX: ugly?
-     (list (package-name package) package output))
+     (if (string=? output "out")
+         ;; (package "out") => ("package" package "out")
+         (list (package-name package) package output)
+         ;; (package "output") => ("package:output" package "output")
+         (list (string-append (package-name package) ":" output)
+               package
+               output)))
     ((? gexp-input?)       ;XXX: misplaced because 'native?' field is ignored?
      (let ((obj    (gexp-input-thing input))
            (output (gexp-input-output input)))