Re: [gentoo-dev] [PATCH] git-r3.eclass: Make EGIT_LIVE_* associative arrays

[Michał Górny]

On Thu, 23 Jun 2016 14:28:39 -0500
Dan Douglas  wrote:

> On 06/23/2016 01:48 PM, Michał Górny wrote:
> > It would be best if we could kill the old way but I doubt it's a good idea 
> > right now.  
> 
> Yeah I was sad when I realized a nice naming scheme just barely
> wasn't going to work thanks to EGIT_LIVE_REPO_URI being shortened to
> EGIT_LIVE_REPO, but I didn't want to break it.
> 
> The other thing is the associative arrays not being exportable so if we
> want something that can work for interactive use / make.conf / package.env
> then we almost need both.
> 
> > One request though: don't use PN but CATEGORY/PN.  
> 
> True. I wasn't sure just how far to go with it. If you really wanted
> to go all-out I could split arbitrary atoms with versionator and figure out a
> custom lookup function to match up keys-values... but ugh. I'll just do
> CATEGORY/PN for now. :)

Well, just to be clear I don't like that magic either. However, I
wanted to preserve it for compatibility with old git-2 eclass, i.e. so
that when ebuilds are updated from git-2 to git-r3 people's overrides
won't suddenly stop working.

Originally I just wanted to expect people to override EGIT_REPO_URI via
package.env but that won't work since ebuilds would override it.

-- 
Best regards,
Michał Górny



pgpRfL6c10vZY.pgp
Description: OpenPGP digital signature

Re: [gentoo-dev] [PATCH] git-r3.eclass: Make EGIT_LIVE_* associative arrays

[Ulrich Mueller]

> On Thu, 23 Jun 2016, Dan Douglas wrote:

>> That's the wrong way around. You should keep the EAPI version check
>> but drop the check for the bash version.

> I don't think that will work. You don't want this to vary by EAPI.

Much less you want it to vary within a single EAPI, depending on
package versions installed by the user.

PMS defines the bash version for each EAPI for a reason. The spec even
says that the shell's compatibility level should be set to the exact
version specified if possible, so the intent is clear. Namely, ebuilds
and eclasses are expected to stick to the features available in that
bash version. Especially, they should not try to outsmart the spec by
sniffing for the version of the shell themselves.

> That aside, these variables are documented as being for
> configuration and not part of the API for inheriting code.

That sounds like the cleanest solution would be a git-r4 eclass
supporting only EAPI 6 (or later).

Ulrich


pgp0P5S9curUb.pgp
Description: PGP signature

Re: [gentoo-dev] [PATCH] git-r3.eclass: Make EGIT_LIVE_* associative arrays

[Dan Douglas]

On 06/23/2016 01:48 PM, Michał Górny wrote:
> It would be best if we could kill the old way but I doubt it's a good idea 
> right now.

Yeah I was sad when I realized a nice naming scheme just barely
wasn't going to work thanks to EGIT_LIVE_REPO_URI being shortened to
EGIT_LIVE_REPO, but I didn't want to break it.

The other thing is the associative arrays not being exportable so if we
want something that can work for interactive use / make.conf / package.env
then we almost need both.

> One request though: don't use PN but CATEGORY/PN.

True. I wasn't sure just how far to go with it. If you really wanted
to go all-out I could split arbitrary atoms with versionator and figure out a
custom lookup function to match up keys-values... but ugh. I'll just do
CATEGORY/PN for now. :)

(There's a branch for this here for reference):
https://github.com/ormaaj/gentoo/blob/git-r3-live-arrays/eclass/git-r3.eclass



signature.asc
Description: OpenPGP digital signature

Re: [gentoo-dev] [PATCH] git-r3.eclass: Make EGIT_LIVE_* associative arrays

[Dan Douglas]

On 06/23/2016 09:15 AM, Ulrich Mueller wrote:
>> On Thu, 23 Jun 2016, Dan Douglas wrote:
>
>> Well I just dropped the EAPI check since it was pointless anyway.
>
>> https://github.com/gentoo/gentoo/pull/1723/commits/3ebc1f57378a5ed4a62232ac87a0955ccdd33a4d
>
> That's the wrong way around. You should keep the EAPI version check
> but drop the check for the bash version.

I don't think that will work. You don't want this to vary by EAPI. For instance
I was using this for merging a set and wanted all of its packages to check out a
particular tag without having to care about which EAPI is used by each package.
All that matters is that it's consistent system-wide.

> PMS clearly defines the bash
> version for each EAPI (namely, bash-3.2 for EAPI 0 to 5 and bash-4.2
> for EAPI 6).

Ok. FWIW this is valid bash 3 and 4 code. I don't think it's so clear what that
means for eclass shared code.

The API exposed here is still invariant. It's typically the responsibility of
the calling code to know whether the current environment supports a particular
datatype before attempting to use it, and not a requirement for library code to
refuse handling that type (like through some ad-hoc polymorphism) when
requested unless doing so would somehow break backwards-compatibility.

That aside, these variables are documented as being for configuration and not
part of the API for inheriting code.

I won't fight too hard if you really hate this, but the previous ${PN}_var is
pretty ugly. The `s/[+-]/_/` name mangling rule wasn't even documented, which
was what got me looking at this code in the first place.



signature.asc
Description: OpenPGP digital signature

Re: [gentoo-dev] [PATCH] git-r3.eclass: Make EGIT_LIVE_* associative arrays

[Michał Górny]

On Wed, 22 Jun 2016 18:22:38 -0500
Dan Douglas  wrote:

> This creates new associative arrays for each "live" variable if bash4 /
> eapi6 is available. Mainly for user convenience in a bashrc as an
> alternative to magic variable names.
> ---
>  eclass/git-r3.eclass | 68 
> ++--
>  1 file changed, 39 insertions(+), 29 deletions(-)

My first thought was: no, thanks, no more complexity. But after some
thinking, I guess this may actually be better. It would be best if we
could kill the old way but I doubt it's a good idea right now.

Before giving you the final answer/review, I'm going to wait a while to
see what others think. In the meantime, you could also think if this
could be extended to support overrides on direct git-r3_fetch calls,
i.e. when ebuild fetches multiple repos.

My first thought would be to use original repo URI as the key instead
of PN. Or maybe support both, i.e. PN for global override and URI for
in-fetch override.

One request though: don't use PN but CATEGORY/PN.

-- 
Best regards,
Michał Górny



pgpGZ3dhKQT5g.pgp
Description: OpenPGP digital signature

Re: [gentoo-dev] [PATCH] git-r3.eclass: Make EGIT_LIVE_* associative arrays

[Ulrich Mueller]

> On Thu, 23 Jun 2016, Dan Douglas wrote:

> Well I just dropped the EAPI check since it was pointless anyway.

> https://github.com/gentoo/gentoo/pull/1723/commits/3ebc1f57378a5ed4a62232ac87a0955ccdd33a4d

That's the wrong way around. You should keep the EAPI version check
but drop the check for the bash version. PMS clearly defines the bash
version for each EAPI (namely, bash-3.2 for EAPI 0 to 5 and bash-4.2
for EAPI 6).

Ulrich


pgpAmqloOslx6.pgp
Description: PGP signature

Re: [gentoo-dev] [PATCH] git-r3.eclass: Make EGIT_LIVE_* associative arrays

[Dan Douglas]

On 06/22/2016 09:56 PM, Michael Orlitzky wrote:
> On 06/22/2016 08:34 PM, Dan Douglas wrote:
>> On 06/22/2016 07:12 PM, Ulrich Mueller wrote:
 On Wed, 22 Jun 2016, Dan Douglas wrote:
>>>
 +  [[
 +  ( BASH_VERSINFO[0] -ge 4 || EAPI -ge 6 ) &&
 +  $(declare -p "EGIT_${livevars[idx+1]}" 2>/dev/null) == 
 'declare -A'*
 +  ]] && ref=EGIT_${livevars[idx+1]}[\$PN]
>>>
>>> EAPI is not a number but a string, so don't use arithmetic comparison
>>> to test for it
>>
>> You mean in the future it may have non-digit values that pass the
>> `case $EAPI in ...)` at the top?
> 
> Yeah, it would be perfectly legal to name the next EAPI "seven" and add
> it to the case statement at the top. Then [[ "seven" -ge 6 ]] is false.
> 

Oh fun, thanks.

Well I just dropped the EAPI check since it was pointless anyway.

https://github.com/gentoo/gentoo/pull/1723/commits/3ebc1f57378a5ed4a62232ac87a0955ccdd33a4d



signature.asc
Description: OpenPGP digital signature

Re: [gentoo-dev] [PATCH] git-r3.eclass: Make EGIT_LIVE_* associative arrays

[Michael Orlitzky]

On 06/22/2016 08:34 PM, Dan Douglas wrote:
> On 06/22/2016 07:12 PM, Ulrich Mueller wrote:
>>> On Wed, 22 Jun 2016, Dan Douglas wrote:
>>
>>> +   [[
>>> +   ( BASH_VERSINFO[0] -ge 4 || EAPI -ge 6 ) &&
>>> +   $(declare -p "EGIT_${livevars[idx+1]}" 2>/dev/null) == 
>>> 'declare -A'*
>>> +   ]] && ref=EGIT_${livevars[idx+1]}[\$PN]
>>
>> EAPI is not a number but a string, so don't use arithmetic comparison
>> to test for it
> 
> You mean in the future it may have non-digit values that pass the
> `case $EAPI in ...)` at the top?

Yeah, it would be perfectly legal to name the next EAPI "seven" and add
it to the case statement at the top. Then [[ "seven" -ge 6 ]] is false.

Re: [gentoo-dev] [PATCH] git-r3.eclass: Make EGIT_LIVE_* associative arrays

[Dan Douglas]

On 06/22/2016 07:12 PM, Ulrich Mueller wrote:
>> On Wed, 22 Jun 2016, Dan Douglas wrote:
> 
>> +[[
>> +( BASH_VERSINFO[0] -ge 4 || EAPI -ge 6 ) &&
>> +$(declare -p "EGIT_${livevars[idx+1]}" 2>/dev/null) == 
>> 'declare -A'*
>> +]] && ref=EGIT_${livevars[idx+1]}[\$PN]
> 
> EAPI is not a number but a string, so don't use arithmetic comparison
> to test for it

You mean in the future it may have non-digit values that pass the
`case $EAPI in ...)` at the top? I debated whether to include that at
all since I thought the intention was for these variables to never be
used in an ebuild/eclass, so we're just using the array
opportunistically - basically.



signature.asc
Description: OpenPGP digital signature

Re: [gentoo-dev] [PATCH] git-r3.eclass: Make EGIT_LIVE_* associative arrays

[Ulrich Mueller]

> On Wed, 22 Jun 2016, Dan Douglas wrote:

> + [[
> + ( BASH_VERSINFO[0] -ge 4 || EAPI -ge 6 ) &&
> + $(declare -p "EGIT_${livevars[idx+1]}" 2>/dev/null) == 
> 'declare -A'*
> + ]] && ref=EGIT_${livevars[idx+1]}[\$PN]

EAPI is not a number but a string, so don't use arithmetic comparison
to test for it

Also, you cannot use bash 4 features in EAPI 5 or earlier.

Ulrich


pgps8eY2GSaRe.pgp
Description: PGP signature