Re: [gentoo-portage-dev] How do we want to deal with Arfrever's bunch of patches?

[Michał Górny]

On Sat, 12 Dec 2015 12:44:40 -0800
Zac Medico <zmedico@g.o> wrote:

> On 12/12/2015 06:50 AM, Michał Górny wrote:
> > On Sat, 12 Dec 2015 13:01:04 +0100
> > Alexander Berntsen <bernalex@g.o> wrote:
> >
> >> Friends,
> >>
> >> As I'm sure you've noticed, Arfrever pushed a bunch of unreviewed
> >> stuff. Michał reverted some of them as they were breaking changes in
> >> addition to being unreviewed (oh, and of course they don't follow the
> >> commit message format -- why would they). There's a bunch left.
> >>
> >> Do we want to cherry-pick Zac's patches (i.e. revert Arfrever's
> >> patches), and have him put them up for review as usual? It's a bit of
> >> a mess because of Zac's patches. If we go down this route it would
> >> likely be easiest for Zac to revert them, since he'll be the more
> >> confident regarding what to keep in the ensuing merge conflicts.
>
> Yeah, I'd be happy to do that.
>
> >> Arfrever asked that someone review the patches and keep them if they
> >> are OK, and revert only the ones that are problematic. I think this
> >> would be OK for now, but obviously not something we want to encourage
> >> or allow in the future.
> >
> > Here's my quick review of the remaining commits (newest first):
>
> Thanks!
>
> > 31923f4 portage.package.ebuild.config.config.__init__(): Skip some warnings for Portage Python scripts called in ebuild environment.
> > 7853950 Delete support for PORTDIR and PORTDIR_OVERLAY from make.conf and environment.
>
> Before dropping PORTDIR and PORTDIR_OVERLAY, we need deprecation
> warnings for a couple of years in advance.
>
> > e7d95cb portage.repository.config.RepoConfig: Support location with trailing whitespace by using quoting.
>
> This one might be okay. Did it really break anything?

I don't know. I reverted the whole bunch of unreviewed stuff then,
better safe than sorry. Furthermore, it looks suspicious, so I'd rather
have that reviewed properly rather than post-factum.

> > fb4d1f4 ebuild: Rename some variables.
> >
> > I'd keep it, doesn't hurt anything and the new names are more readable than 'myrepo' ;-).
>
> Looks good.

I had to revert it because it relies on the previous commit. I'd rather
have it reintroduced cleanly rather than having fixes mixed with
reverts, or partially broken states.

> > 9e104c4 ebuild: Set PORTAGE_REPOSITORIES instead of deprecated PORTDIR_OVERLAY.
> >
> > Kill it as relying on weirdo PORTAGE_REPOSITORIES invention.
>
> I think this one is mostly okay, since PORTAGE_REPOSITORIES has been
> supported for some time, and it would be nice to eliminate internal
> usage for PORTDIR_OVERLAY.
>
> However, this patch relies on the _read_repo_name method added in
> 2cde1f6. When we revert 2cde1f6, we can also change this code to use the
> RepoConfig._read_valid_repo_name staticmethod that was removed in 2cde1f6.

I have reverted it for this reason. As above, we can reintroduce it
when it's fixed to work with the resulting code.

> > 2cde1f6 portage.repository.config: Clean reading of repository names and drop support for repositories with missing or invalid names.
> >
> > I'd revert it, it's a breaking change.
>
> I think this is too much at once, so we should revert it.
>
> As a first step in the direction that this patch is going, we could
> remove the special case for /usr/local/portage from the repo_name_check
> functions, so that people will start getting warnings for a missing
> repo_name there.

Reverted. We can proceed from here.

> > 10cccf7 Support environmental variables overriding parts of configuration of repositories.
> >
> > Kill this ugly thing with a lot of fire. This is so wrong you can't get
> > much worse.
>
> I don't like this mainly because variables of the form
> PORTAGE_REPOSITORY:${repository_name}:${attribute} cannot be exported in
> bash:
>
> $ export foo:bar=baz
> bash: export: `foo:bar=baz': not a valid identifier

Reverted.

> > 39d81c5 portage.package.ebuild.config.config.__getitem__(): Partially drop backward compatibility for nonexistent keys.
> >
> > Maybe keep it but needs someone smarter than me to review.
>
> Looks good, except the last hunk seems redundant because "for x in self"
> should only yield valid keys:
>
> @@ -2697,7 +2714,9 @@ class config(object):
>  		for x in self:
>  			if x in environ_filter:
>  				continue
> -			myvalue = self[x]
> +			myvalue = self.get(x)
> +			if myvalue is None:
> +				continue

Could you fix it then, please?

I've left all the other unmentioned commits.

-- 
Best regards,
Michał Górny
<http://dev.gentoo.org/~mgorny/>