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

[Zac Medico]

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?

> Those three already reverted.
>
>
> 8036223 _emerge.depgraph.depgraph._select_files(): Update message.
>
> Can probably be left, it's updating messages for repos.conf which is still valid.

Looks good.

> 9716f8a emirrordist: Delete support for deprecated --portdir and --portdir-overlay options.
> 2d40eb4 egencache: Delete support for deprecated --portdir and --portdir-overlay options.
>
> Not sure. Maybe something relies on this.

I don't care either way. Note that these options have triggered
deprecation warnings since before portage-2.2.0:

https://gitweb.gentoo.org/proj/portage.git/commit/?id=92fa6a1e1fcc92f2ad8ce8896c4ce3ec39477485
https://gitweb.gentoo.org/proj/portage.git/commit/?id=ab7d606eb6f66004a63c9406cf9cf87b352f6d01

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

Looks good.

> 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.

> 3917544 bin/socks5-server.py: Fix DeprecationWarning with Python >=3.4.4.
>
> This one looks good, and even the commit message is sane.

Looks good.

> 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.

> 1064765 portage.repository.config.RepoConfig: Delete user_location attribute and consistently use location attribute everywhere.
>
> I think we can keep this if it doesn't break anything. I didn't hear
> about this attribute before, so I suggest Zac reviews this.

repo.user_location is the original location specified in repos.conf, and
repo.location == os.path.realpath(repo.user_location)

So, repo.user_location can differ if the path contains any symlinks.

Is there any value in hiding the symlink indirection in repository paths
shown in output like emerge --info? If not, then the change is fine as
long as there are no external consumers of this API. I think there is
very low probability of there being external consumers.

> 48c2af4 Respect PYTHONDONTWRITEBYTECODE in test suite.
>
> I don't mind either way. But can't we make this simpler?

Looks good.

We can avoid code duplication if we add an environment setup function
for all the tests to share, but that can come in a later patch.

> 528dc7c portage.package.ebuild.fetch.fetch(): Clean setting of variables.
>
> Purely stylistic change.

I don't mind either way.

> 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

>
> 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


> 4f25fa6 ebuild: Move imports to the top.
> 5ea789a portage.package.ebuild._config.LicenseManager.LicenseManager._getPkgAcceptLicense(): Fix typo in call to hasattr().
>
> Makes sense.

Looks good.

> 3ffdbbe ebuild: Do not catch unexpected KeyErrors from aux_get().
>
> Maybe. Need others to confirm.

Looks good. This will prevent an unexpected KeyError from being
silently/confusingly swallowed.

> b8da04b portageq envvar: Return 1 for any nonexistent variable.
>
> Makes sense.

This might break compatibility for some scripts calling portageq envvar,
and I think the utility of this change is questionable, since callers
will typically have to check if the result is non-empty anyway. Perhaps
it should return 1 if _none_ of the specified variables are defined?

> 53a0b63 _emerge.actions.getgccversion(): Work with chost=None and with no explicit chost.
>
> Probably makes sense, needs extra review.

Looks good.

> 628aa10 portage.sync.modules.cvs.CheckCVSConfig.check_cvs_repo(): Fix "KeyError: 'sync-cvs-repo'".
>
> Makes sense.

Looks good.

> f05056c portage.repository.config.RepoConfigLoader._parse(): Delete unused portdir parametre.
> 041fc70 portage.repository.config.RepoConfigLoader._add_repositories(): Delete unused ignored_location_map parametre.
> c1a2714 portage.repository.config.RepoConfigLoader._parse(): Delete unused ignored_map and ignored_location_map parametres.
>
> Those all make sense.

Looks good.

-- 
Thanks,
Zac