The package/inherit trap (was: gnu: stellarium: Enable ShowMySky.)

The package/inherit trap (was: gnu: stellarium: Enable ShowMySky.)

[Tobias Geerinckx-Rice]

[-- Attachment #1: Type: text/plain, Size: 1194 bytes --]

Hi!

Maxim Cournoyer 写道：
> Simon Tournier <zimon.toutoune@gmail.com> writes:
>> It is not clear for me why you choose one over the other.  From 
>> my
>> current understanding, I would be tempted to always use
>> 'package/inherit' and never plain 'inherit'.
>
> I also got confused by that in the past;

Same.  I think it's a rite of passage.  A questionable one.

> The way I process it internally now is this:
>
> If the inheritance is for *same-source/same-version* variants of 
> a
> package, they should use package/inherit, as any security issues 
> found
> in the parent package should also be applied to that package 
> (since they
> use the same source).  Otherwise, plain 'inherit' should be used
> (e.g. for newer version variants).

That about jives with my intuition.

Judging by the (IMO) universal confusion this causes, it is (IMO) 
spectacularly poorly-named.  A docstring doesn't fix that.

Could we rename it to something like 
‘package+replacements/inherit’?  To me, that captures the spirit, 
without being overly longer.

But I'll gladly judge other bikesheds if they lead to a less 
misleading name.

Kind regards,

T G-R

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 247 bytes --]

Re: The package/inherit trap

[Maxim Cournoyer]

Hi,

Tobias Geerinckx-Rice <me@tobias.gr> writes:

> Hi!
>
> Maxim Cournoyer 写道：
>> Simon Tournier <zimon.toutoune@gmail.com> writes:
>>> It is not clear for me why you choose one over the other.  From my
>>> current understanding, I would be tempted to always use
>>> 'package/inherit' and never plain 'inherit'.
>>
>> I also got confused by that in the past;
>
> Same.  I think it's a rite of passage.  A questionable one.
>
>> The way I process it internally now is this:
>>
>> If the inheritance is for *same-source/same-version* variants of a
>> package, they should use package/inherit, as any security issues
>> found
>> in the parent package should also be applied to that package (since
>> they
>> use the same source).  Otherwise, plain 'inherit' should be used
>> (e.g. for newer version variants).
>
> That about jives with my intuition.
>
> Judging by the (IMO) universal confusion this causes, it is (IMO)
> spectacularly poorly-named.  A docstring doesn't fix that.
>
> Could we rename it to something like ‘package+replacements/inherit’?
> To me, that captures the spirit, without being overly longer.

That'd be better, I think.  The more verbose name at least wouldn't let
one think, 'oh, some questionable syntax sugar for the lazy', like I did
in the past :-).

-- 
Thanks,
Maxim

Re: The package/inherit trap (was: gnu: stellarium: Enable ShowMySky.)

[Simon Tournier]

Hi,

On Fri, 03 Mar 2023 at 20:21, Tobias Geerinckx-Rice <me@tobias.gr> wrote:

> Could we rename it to something like 
> ‘package+replacements/inherit’?  To me, that captures the spirit, 
> without being overly longer.

Well, I gave a look at the code and have seen the replacement.  But I
had not thought about the package transformation and the like.

From my point of view, the best would to add a paragraph with index
entries under “Defining-Package-Variants” section [1].

However, in the light of Maxim’s explanations, the example from the
manual appears to me inconsistent:

--8<---------------cut here---------------start------------->8---
You can just as well define variants with a different set of
dependencies than the original package.  For example, the default
@code{gdb} package depends on @code{guile}, but since that is an
optional dependency, you can define a variant that removes that
dependency like so:

@lisp
(use-modules (gnu packages gdb))   ;for 'gdb'

(define gdb-sans-guile
  (package
    (inherit gdb)
    (inputs (modify-inputs (package-inputs gdb)
              (delete "guile")))))
@end lisp
--8<---------------cut here---------------end--------------->8---

Well, since the trap is not completely clear for me yet, I am not able
to propose a paragraph.  IMHO, a paragraph here would help in mitigating
the trap.  Whatever the name of ’package/inherit’. :-)

1: <https://guix.gnu.org/manual/devel/en/guix.html#Defining-Package-Variants>


Cheers,
simon

Re: The package/inherit trap

[Maxim Cournoyer]

Hi Simon,

Simon Tournier <zimon.toutoune@gmail.com> writes:

> Hi,
>
> On Fri, 03 Mar 2023 at 20:21, Tobias Geerinckx-Rice <me@tobias.gr> wrote:
>
>> Could we rename it to something like 
>> ‘package+replacements/inherit’?  To me, that captures the spirit, 
>> without being overly longer.
>
> Well, I gave a look at the code and have seen the replacement.  But I
> had not thought about the package transformation and the like.
>
> From my point of view, the best would to add a paragraph with index
> entries under “Defining-Package-Variants” section [1].
>
> However, in the light of Maxim’s explanations, the example from the
> manual appears to me inconsistent:
>
> You can just as well define variants with a different set of
> dependencies than the original package.  For example, the default
> @code{gdb} package depends on @code{guile}, but since that is an
> optional dependency, you can define a variant that removes that
> dependency like so:
>
> @lisp
> (use-modules (gnu packages gdb))   ;for 'gdb'
>
> (define gdb-sans-guile
>   (package
>     (inherit gdb)
>     (inputs (modify-inputs (package-inputs gdb)
>               (delete "guile")))))
> @end lisp

Do you mean inconsistent because based on what I wrote it should have
used "package/inherit gdb ..." instead of (package (inherit gdb) ...) ?
If so, I agree.  It could be modified to use the former and an extra
explanation offered about why package/inherit is used here when it's to
be preferred to plain inheritance.

-- 
Thanks,
Maxim

Re: The package/inherit trap

[Simon Tournier]

Hi Maxim,

On Tue, 07 Mar 2023 at 11:43, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote:

>> @lisp
>> (use-modules (gnu packages gdb))   ;for 'gdb'
>>
>> (define gdb-sans-guile
>>   (package
>>     (inherit gdb)
>>     (inputs (modify-inputs (package-inputs gdb)
>>               (delete "guile")))))
>> @end lisp
>
> Do you mean inconsistent because based on what I wrote it should have
> used "package/inherit gdb ..." instead of (package (inherit gdb) ...) ?

Based on my understanding about what you wrote.

> If so, I agree.  It could be modified to use the former and an extra
> explanation offered about why package/inherit is used here when it's to
> be preferred to plain inheritance.

Well, from my point of view, we have a trap because the documentation is
not clear. :-)

Well, I think it is not only by replacing in the example.  I think the
manual should provide 2 examples and makes a clear line when one needs
to pick ’inherit’ or when one needs to pick ’package/inherit’.

Somehow, we have a similar issue as we had before with “Snippet vs
Phases“.  It would help to have plain words for ’package/inherit’ use
cases; assuming all the other use cases are covered by ’inherit’. ;-)

Cheers,
simon

Re: The package/inherit trap

[Tobias Geerinckx-Rice]

Hi,

On 7 March 2023 17:46:50 UTC, Simon Tournier <zimon.toutoune@gmail.com> wrote:
>Well, from my point of view, we have a trap because the documentation is
>not clear. :-)

Both.

However, merely documenting something is not enough when we have the chance to fix misleading naming, as we do here.  It would be nice to have, but orthogonal.



Kind regards,

T G-R

Sent on the go.  Excuse or enjoy my brevity.

Re: The package/inherit trap

[Simon Tournier]

Hi Tobias,

On Tue, 07 Mar 2023 at 22:11, Tobias Geerinckx-Rice <me@tobias.gr> wrote:

> However, merely documenting something is not enough when we have the
> chance to fix misleading naming, as we do here.  It would be nice to
> have, but orthogonal. 

I would not say it is orthogonal because renaming would not have been
enough, at least for me, in order to get the difference with ’inherit’.

For sure, it is a real trap. :-)  For instance,

    $ git log --grep='package/inherit' --oneline | grep use
    5f83dd03a2 gnu: kodi/wayland: Do not use package/inherit.
    6ecf88a6a1 gnu: poppler-next: Don't use 'package/inherit'.
    5a5b729d66 gnu: abseil-cpp: Don't use 'package/inherit'.
    dbcf2b61b1 gnu: Fix erroneous uses of 'package/inherit'.
    2f97a666a5 gnu: python-urllib3: Don't use 'package/inherit' on replacement package.
    4163b6d855 gnu: avahi: Don't use package/inherit.

and in the light of this discussion, 5f83dd03a2 seems incorrect, no?

Well, I agree that naming is important and probably the most difficult
task. :-)

Do we go for renaming + deprecated?  Then do we apply the rename to all
occurrences in Guix?

Cheers,
simon

Re: The package/inherit trap

[John Kehayias]

Hi all,

On Wed, Mar 08, 2023 at 10:07 AM, Simon Tournier wrote:

> Hi Tobias,
>
> On Tue, 07 Mar 2023 at 22:11, Tobias Geerinckx-Rice <me@tobias.gr> wrote:
>
>> However, merely documenting something is not enough when we have the
>> chance to fix misleading naming, as we do here.  It would be nice to
>> have, but orthogonal.
>
> I would not say it is orthogonal because renaming would not have been
> enough, at least for me, in order to get the difference with ’inherit’.
>
> For sure, it is a real trap. :-)  For instance,
>
>     $ git log --grep='package/inherit' --oneline | grep use
>     5f83dd03a2 gnu: kodi/wayland: Do not use package/inherit.
>     6ecf88a6a1 gnu: poppler-next: Don't use 'package/inherit'.
>     5a5b729d66 gnu: abseil-cpp: Don't use 'package/inherit'.
>     dbcf2b61b1 gnu: Fix erroneous uses of 'package/inherit'.
>     2f97a666a5 gnu: python-urllib3: Don't use 'package/inherit' on replacement package.
>     4163b6d855 gnu: avahi: Don't use package/inherit.
>
> and in the light of this discussion, 5f83dd03a2 seems incorrect, no?
>
> Well, I agree that naming is important and probably the most difficult
> task. :-)
>
> Do we go for renaming + deprecated?  Then do we apply the rename to all
> occurrences in Guix?
>
> Cheers,
> simon

I know, reviving an old thread but I assume this wasn't resolved?
Throw my vote for the more explicit renaming with better doc/examples,
as one who never remembers the difference either.

John

Re: The package/inherit trap

[Mark H Weaver]

Simon Tournier <zimon.toutoune@gmail.com> writes:

> Hi Tobias,
>
> On Tue, 07 Mar 2023 at 22:11, Tobias Geerinckx-Rice <me@tobias.gr> wrote:
>
>> However, merely documenting something is not enough when we have the
>> chance to fix misleading naming, as we do here.  It would be nice to
>> have, but orthogonal. 
>
> I would not say it is orthogonal because renaming would not have been
> enough, at least for me, in order to get the difference with ’inherit’.
>
> For sure, it is a real trap. :-)  For instance,
>
>     $ git log --grep='package/inherit' --oneline | grep use
>     5f83dd03a2 gnu: kodi/wayland: Do not use package/inherit.
>     6ecf88a6a1 gnu: poppler-next: Don't use 'package/inherit'.
>     5a5b729d66 gnu: abseil-cpp: Don't use 'package/inherit'.
>     dbcf2b61b1 gnu: Fix erroneous uses of 'package/inherit'.
>     2f97a666a5 gnu: python-urllib3: Don't use 'package/inherit' on replacement package.
>     4163b6d855 gnu: avahi: Don't use package/inherit.
>
> and in the light of this discussion, 5f83dd03a2 seems incorrect, no?

I agree that commit 5f83dd03a2 is incorrect.  'kodi/wayland' is quite
clearly a case where 'package/inherit' should be used.

       Mark

Re: The package/inherit trap

[Maxim Cournoyer]

Hello,

Mark H Weaver <mhw@netris.org> writes:

> Simon Tournier <zimon.toutoune@gmail.com> writes:
>
>> Hi Tobias,
>>
>> On Tue, 07 Mar 2023 at 22:11, Tobias Geerinckx-Rice <me@tobias.gr> wrote:
>>
>>> However, merely documenting something is not enough when we have the
>>> chance to fix misleading naming, as we do here.  It would be nice to
>>> have, but orthogonal. 
>>
>> I would not say it is orthogonal because renaming would not have been
>> enough, at least for me, in order to get the difference with ’inherit’.
>>
>> For sure, it is a real trap. :-)  For instance,
>>
>>     $ git log --grep='package/inherit' --oneline | grep use
>>     5f83dd03a2 gnu: kodi/wayland: Do not use package/inherit.
>>     6ecf88a6a1 gnu: poppler-next: Don't use 'package/inherit'.
>>     5a5b729d66 gnu: abseil-cpp: Don't use 'package/inherit'.
>>     dbcf2b61b1 gnu: Fix erroneous uses of 'package/inherit'.
>>     2f97a666a5 gnu: python-urllib3: Don't use 'package/inherit' on replacement package.
>>     4163b6d855 gnu: avahi: Don't use package/inherit.
>>
>> and in the light of this discussion, 5f83dd03a2 seems incorrect, no?
>
> I agree that commit 5f83dd03a2 is incorrect.  'kodi/wayland' is quite
> clearly a case where 'package/inherit' should be used.

Would you be able to write a bit of doc explaining when both should be
used?  It's a common pitfalls among contributors, and I suspect few of
us have an understanding good enough to write it down in a manner clear
enough to be understood by new contributors.

-- 
Thanks,
Maxim

Re: The package/inherit trap

[Mark H Weaver]

Hi,

Maxim Cournoyer <maxim.cournoyer@gmail.com> writes:

> Mark H Weaver <mhw@netris.org> writes:
>
>> Simon Tournier <zimon.toutoune@gmail.com> writes:
>>> and in the light of this discussion, 5f83dd03a2 seems incorrect, no?
>>
>> I agree that commit 5f83dd03a2 is incorrect.  'kodi/wayland' is quite
>> clearly a case where 'package/inherit' should be used.
>
> Would you be able to write a bit of doc explaining when both should be
> used?  It's a common pitfalls among contributors, and I suspect few of
> us have an understanding good enough to write it down in a manner clear
> enough to be understood by new contributors.

I don't have time to write the doc, but I can offer a few words here.

The fundamental question that needs to be asked, to decide whether to
write (define DERIVED-PACKAGE (package/inherit BASE OVERRIDES ...)) or
(define DERIVED-PACKAGE (package (inherit BASE) OVERRIDES ...)) is this:
if BASE had a replacement, would it make sense to apply the same
OVERRIDES to BASE's replacement to automatically create a replacement
for DERIVED-PACKAGE?

Consider 'kodi/wayland' as an example:

__ (define-public kodi/wayland
____ (package
______ (inherit kodi)
______ (name "kodi-wayland")
______ (arguments
_______ (substitute-keyword-arguments (package-arguments kodi)
_________ ((#:configure-flags flags)
__________ `(cons "-DCORE_PLATFORM_NAME=wayland"
_________________ (delete "-DCORE_PLATFORM_NAME=x11" ,flags)))))
______ (inputs
_______ (modify-inputs (package-inputs kodi)
_________ (prepend libinput
__________________ libxkbcommon
__________________ waylandpp
__________________ wayland-protocols)))
______ (synopsis "Kodi with Wayland rendering backend")))

Suppose, at some future time, 'kodi' were given a replacement named
'kodi-with-fixes' to apply a security update.

The question to ask yourself is this: would it make sense for
'kodi/wayland' to automatically be given a replacement field defined
exactly as above except with (inherit kodi) changed to (inherit
kodi-with-fixes)?

If the answer is "yes", then use (package/inherit BASE OVERRIDES ...),
and make sure that BASE is simply a variable name.

If the answer is "no", use (package (inherit BASE) OVERRIDES ...), in
which case any replacement of BASE will simply be dropped, and the new
package will not have a replacement unless one is explicitly given in
OVERRIDES.

The rationale for 'package/inherit' is to ensure that, e.g., security
updates applied to 'kodi' will automatically be propagated to
'kodi/wayland', and similarly for other packages.

Unless I'm mistaken, the criterion given above is fully general, and
thus sufficient on its own.  However, for the sake of reducing the
cognitive load on contributors, one could proceed to enumerate simpler
heuristics that can be used in common cases.

For example, if OVERRIDES includes a new 'source' field, the next
question to ask is: does the new 'source' field make an incremental
change to (package-source BASE) e.g. by simply adding another patch?  If
so, then 'package/inherit' might do the right thing.  However, if the
new 'source' field doesn't even look at (package-source BASE), then it's
certainly not going to work sensibly when applied to BASE's replacement.

Another common case is when the package you're defining *is* BASE's
replacement.  In that case, you certainly don't want to use
'package/inherit'.

If you want to rename 'package/inherit', I'd suggest something like
'package/inherit-with-replacements'.

Hope this helps.  Feel free to do as you wish without consulting me
further.  I'm using a private branch that hasn't been merged with
upstream Guix since April 2021, so your decisions won't affect me
in any case.

      Mark

Re: The package/inherit trap

[Mark H Weaver]

Hello again,

I made a mistake in my previous message.

Earlier, I wrote:
> Consider 'kodi/wayland' as an example:
>
> __ (define-public kodi/wayland
> ____ (package
> ______ (inherit kodi)
> ______ (name "kodi-wayland")
> ______ (arguments
> _______ (substitute-keyword-arguments (package-arguments kodi)
> _________ ((#:configure-flags flags)
> __________ `(cons "-DCORE_PLATFORM_NAME=wayland"
> _________________ (delete "-DCORE_PLATFORM_NAME=x11" ,flags)))))
> ______ (inputs
> _______ (modify-inputs (package-inputs kodi)
> _________ (prepend libinput
> __________________ libxkbcommon
> __________________ waylandpp
> __________________ wayland-protocols)))
> ______ (synopsis "Kodi with Wayland rendering backend")))
>
> Suppose, at some future time, 'kodi' were given a replacement named
> 'kodi-with-fixes' to apply a security update.
>
> The question to ask yourself is this: would it make sense for
> 'kodi/wayland' to automatically be given a replacement field defined
> exactly as above except with (inherit kodi) changed to (inherit
> kodi-with-fixes)?

The above paragraph isn't quite right.  It should be:

> The question to ask yourself is this: would it make sense for
> 'kodi/wayland' to automatically be given a replacement field defined
> exactly as above except with all free references to 'kodi' within the
> 'package' (or 'package/inherit') form changed to 'kodi-with-fixes'?

Alternatively, for the sake of those who don't know what it means for a
variable reference to be "free" within a given expression, here's
another way to write it:

> The question to ask yourself is this: would it make sense for
> 'kodi/wayland' to automatically be given a replacement field defined
> exactly as above except with the 'package' (or 'package/inherit') form
> wrapped within (let ((kodi kodi-with-fixes)) ...).

This point is crucial.  Observe that in the 'kodi/wayland' definition,
the OVERRIDES for the 'arguments' and 'inputs' fields are defined as
incremental changes to (package-arguments kodi) and (package-inputs
kodi), respectively.

In order for (package/inherit BASE OVERRIDES ...) to work as intended,
the incremental changes made in OVERRIDES should be based upon the
variable BASE, because that is the only variable that will be rebound to
(package-replacement BASE) when automatically producing the replacement
for the derived package.

      Regards,
        Mark

Re: The package/inherit trap

[Maxim Cournoyer]

Hi Mark,

Mark H Weaver <mhw@netris.org> writes:

[...]

> I don't have time to write the doc, but I can offer a few words here.
>
> The fundamental question that needs to be asked, to decide whether to
> write (define DERIVED-PACKAGE (package/inherit BASE OVERRIDES ...)) or
> (define DERIVED-PACKAGE (package (inherit BASE) OVERRIDES ...)) is this:
> if BASE had a replacement, would it make sense to apply the same
> OVERRIDES to BASE's replacement to automatically create a replacement
> for DERIVED-PACKAGE?
>
> Consider 'kodi/wayland' as an example:
>
> __ (define-public kodi/wayland
> ____ (package
> ______ (inherit kodi)
> ______ (name "kodi-wayland")
> ______ (arguments
> _______ (substitute-keyword-arguments (package-arguments kodi)
> _________ ((#:configure-flags flags)
> __________ `(cons "-DCORE_PLATFORM_NAME=wayland"
> _________________ (delete "-DCORE_PLATFORM_NAME=x11" ,flags)))))
> ______ (inputs
> _______ (modify-inputs (package-inputs kodi)
> _________ (prepend libinput
> __________________ libxkbcommon
> __________________ waylandpp
> __________________ wayland-protocols)))
> ______ (synopsis "Kodi with Wayland rendering backend")))
>
> Suppose, at some future time, 'kodi' were given a replacement named
> 'kodi-with-fixes' to apply a security update.
>
> The question to ask yourself is this: would it make sense for
> 'kodi/wayland' to automatically be given a replacement field defined
> exactly as above except with (inherit kodi) changed to (inherit
> kodi-with-fixes)?
>
> If the answer is "yes", then use (package/inherit BASE OVERRIDES ...),
> and make sure that BASE is simply a variable name.
>
> If the answer is "no", use (package (inherit BASE) OVERRIDES ...), in
> which case any replacement of BASE will simply be dropped, and the new
> package will not have a replacement unless one is explicitly given in
> OVERRIDES.
>
> The rationale for 'package/inherit' is to ensure that, e.g., security
> updates applied to 'kodi' will automatically be propagated to
> 'kodi/wayland', and similarly for other packages.
>
> Unless I'm mistaken, the criterion given above is fully general, and
> thus sufficient on its own.  However, for the sake of reducing the
> cognitive load on contributors, one could proceed to enumerate simpler
> heuristics that can be used in common cases.
>
> For example, if OVERRIDES includes a new 'source' field, the next
> question to ask is: does the new 'source' field make an incremental
> change to (package-source BASE) e.g. by simply adding another patch?  If
> so, then 'package/inherit' might do the right thing.  However, if the
> new 'source' field doesn't even look at (package-source BASE), then it's
> certainly not going to work sensibly when applied to BASE's replacement.
>
> Another common case is when the package you're defining *is* BASE's
> replacement.  In that case, you certainly don't want to use
> 'package/inherit'.
>
> If you want to rename 'package/inherit', I'd suggest something like
> 'package/inherit-with-replacements'.
>
> Hope this helps.  Feel free to do as you wish without consulting me
> further.  I'm using a private branch that hasn't been merged with
> upstream Guix since April 2021, so your decisions won't affect me
> in any case.

Thank you, that is abundantly detailed and clear; I'll try adapting it
in our manual.

Cheers!

-- 
Maxim

Re: The package/inherit trap (was: gnu: stellarium: Enable ShowMySky.)

[(]

Tobias Geerinckx-Rice <me@tobias.gr> writes:
> But I'll gladly judge other bikesheds if they lead to a less misleading name.

How about 'package/variant' or 'package/variant-of'?

  -- (

Re: The package/inherit trap (was: gnu: stellarium: Enable ShowMySky.)

[Attila Lendvai]

> How about 'package/variant' or 'package/variant-of'?

+1 for trying to capture the intention in the name, instead of the means of implementation.

-- 
• attila lendvai
• PGP: 963F 5D5F 45C7 DFCD 0A39
--
“Although teachers do care and do work very, very hard, the institution is psychopathic-it has no conscience. It rings a bell and the young man in the middle of writing a poem must close his notebook and move to a different cell where he must memorize that humans and monkeys derive from a common ancestor.”
	— John Taylor Gatto (1935–2018), 'Dumbing Us Down: The Hidden Curriculum of Compulsory Education' (1992)

Re: The package/inherit trap

[Maxim Cournoyer]

Hi,

Attila Lendvai <attila@lendvai.name> writes:

>> How about 'package/variant' or 'package/variant-of'?
>
> +1 for trying to capture the intention in the name, instead of the means of implementation.

I like 'package/variant'.  It should be possible to automatically update
our code base to use it, and deprecate the legacy name.

-- 
Thanks,
Maxim