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

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

[Alexander Berntsen]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

Resending this from an address that will actually reach the list...

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.

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.

I will not sanction a public history rewrite, but in general I'm open
to other suggestions.
- -- 
Alexander
alexander@plaimi.net
https://secure.plaimi.net/~alexander
- -- 
Alexander
bernalex@gentoo.org
https://secure.plaimi.net/~alexander
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBCgAGBQJWbAyAAAoJENQqWdRUGk8B4IEP/icvsKd6crSZEkR1mHOuD19q
o6O57u23/ldPduKMh+GFTMdQC3ZDeqO9BV5+BFHGSHmnfWStFK2uPijzUxEcdpoU
+LXZw/aDPylDTnn7fS2Bt7Yc3TJfvfSf6vDyQ7D0kmKbgc3F9WV8ETOD2hAQykZr
4PYVvqAgK/NPvyRHkwFlcDzwfB+iI2+KYatlGrR6gdPH9cP0CMqHnKMQ+CQPvafb
GabNEpoingxWmCVzFjb6XKGkKCS1gNJ9Kkqrk66mr2cuAD0H5OxdPAdUhbASgny4
ZH+sll2ysDEAL4XpPh705iFw6Npc6AynWuWYC18p8XfgOOOyKoAJe/xiIJvFKbwR
a9z/pEAf+AMEnfLZbYs84cArTAVLk2fMt3HGln9gW+rb3rS0Cv3EibvmFC+xS2bs
NFSW9DEqLF9JLNUoSyX8wfrfay3i4xlAWzVE7tCXXddFWHWlqipl37dfKbXRu7VW
/g8x1XqTnXqF17uiMurgaWATQNmx+lg/iMex9Ayhcwo8DbLt0D1zt4Dya75ame7G
kkbUnPOBAvBv45k73Bv5mWn7HRipeJQoSh8Y5v+yknydw3UA+kWfR7D1NWM7fqXP
Q3rnlJP3Zd4CqB6M2u8csT6OCKd0YUxShkC2YU/jITV+Qz1h0xZ+DbfoUQggVKru
eMhC8UmtUUxaBwZgMA06
=M0Pq
-----END PGP SIGNATURE-----

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

[Michał Górny]

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

On Sat, 12 Dec 2015 13:01:04 +0100
Alexander Berntsen <bernalex@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.
> 
> 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):


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.
e7d95cb portage.repository.config.RepoConfig: Support location with trailing whitespace by using quoting.

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.


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.


fb4d1f4 ebuild: Rename some variables.

I'd keep it, doesn't hurt anything and the new names are more readable than 'myrepo' ;-).


9e104c4 ebuild: Set PORTAGE_REPOSITORIES instead of deprecated PORTDIR_OVERLAY.

Kill it as relying on weirdo PORTAGE_REPOSITORIES invention.


3917544 bin/socks5-server.py: Fix DeprecationWarning with Python >=3.4.4.

This one looks good, and even the commit message is sane.


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.


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.


48c2af4 Respect PYTHONDONTWRITEBYTECODE in test suite.

I don't mind either way. But can't we make this simpler?


528dc7c portage.package.ebuild.fetch.fetch(): Clean setting of variables.

Purely stylistic change.


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.


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.


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

Makes sense.


3ffdbbe ebuild: Do not catch unexpected KeyErrors from aux_get().

Maybe. Need others to confirm.


b8da04b portageq envvar: Return 1 for any nonexistent variable.

Makes sense.


53a0b63 _emerge.actions.getgccversion(): Work with chost=None and with no explicit chost.

Probably makes sense, needs extra review.


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

Makes sense.


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.

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 949 bytes --]

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

> 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

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

[Michał Górny]

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

On Sat, 12 Dec 2015 12:44:40 -0800
Zac Medico <zmedico@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 <bernalex@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/>

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 949 bytes --]

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

[Zac Medico]

On 12/13/2015 04:58 AM, Michał Górny wrote:
>>> 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?

Fixed:

https://gitweb.gentoo.org/proj/portage.git/commit/?id=6ba56ad7be84b18dcbf15e8c6b283f5a9a559123
-- 
Thanks,
Zac

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

[Zac Medico]

On 12/13/2015 08:58 AM, Zac Medico wrote:
> On 12/13/2015 04:58 AM, Michał Górny wrote:
>>>> 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?
> 
> Fixed:
> 
> https://gitweb.gentoo.org/proj/portage.git/commit/?id=6ba56ad7be84b18dcbf15e8c6b283f5a9a559123
> 

I've also fixed a blanket except clause from the same commit, so it will
raise BaseException (like SystemExit and KeyboardInterrupt):

https://gitweb.gentoo.org/proj/portage.git/commit/?id=ee7978914f27c6a48cd1d6ee2667470aed25687f

-- 
Thanks,
Zac

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

[Alexander Berntsen]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

Thanks for cleaning up guys. It won't happen again.
- -- 
Alexander
bernalex@gentoo.org
https://secure.plaimi.net/~alexander
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBCgAGBQJWbr83AAoJENQqWdRUGk8BksEP/3PIkgDX0EM14qwhXeSnanLt
M4HCitEVLM7qCiyhj8n1/Akbbs7DUumu4jpwvA0P/7MOzryZi2Qkta3XiUosuze7
7B5dTNrWlNNRMHHvlS3NpWRrjT/S5bbf225AGVF/ESZv7kPU4NIk1qraeIXGUjdy
DhYJ+156u+pNm6pGIYE/oe/rCwWHBR3j1jZY1n4WK2J/nQBlvP8v1BCY59WoOX2q
qHw1VCwFIFn4ZGQHShCYf/zWbgxMYOdGEOofA1njG86IgLO9JPsC0vfCLZLM4NgL
b89rTsQrc0QuyLNbG4y8dME91soB5JNGfOoehwKNP8l7ND2SWFA78n7gibWhumFz
mkX9sfpzEq1/e3HA3FqMy9yGCquFJuXDuoj9hmhGfyaXWfhSIErmHZo9MgD+0u81
DYXO9unGWfFH3g2WC369z67AaJPpCfd69DsaKYKD+fadryFVQfC+dq1PLsYxk7sq
77XZrAs5ra+zHITcaL/27w3F7C5bWbIxXs4CtdebZHE7c2owXERV7PEXtP7t5Kii
guiIYwTf11H25Nrhc41bzy1a3aEnljKRyCVmTmT0BBFpLFLD5n55Ql+AgnMiDfDd
ZCHF/G45XN1hq24ZiY5elOlSRM/fz4CaE95bCbFaVW9zCzWpO1egLix3a3sS8N0x
kHydSyS/GwHK/5nYRmbT
=Qqlo
-----END PGP SIGNATURE-----