On Sat, 12 Dec 2015 12:44:40 -0800 Zac Medico <zmed...@gentoo.org> wrote:
> On 12/12/2015 06:50 AM, Michał Górny wrote:
> > On Sat, 12 Dec 2015 13:01:04 +0100
> > Alexander Berntsen <berna...@gentoo.org> 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/>
pgpnhL0eGfmQi.pgp
Description: OpenPGP digital signature
