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

> On 12/12/2015 06:50 AM, Michał Górny wrote:
> > On Sat, 12 Dec 2015 13:01:04 +0100
> > Alexander Berntsen  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



pgpnhL0eGfmQi.pgp
Description: OpenPGP digital signature

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

[gentoo-portage-dev] Re: [PATCH] _dep_check_composite_db._visible: verify that highest_visible matches (bug 567686)

[Zac Medico]

On 12/12/2015 10:48 PM, Zac Medico wrote:
> If the highest visible match for a package slot does not match the
> required atom, then do not mask other packages in the same slot.
> Bug 567686 was triggered when the highest visible match for the
> package slot did not match the subslot specified by the required atom.
> 
> X-Gentoo-Bug: 567686
> X-Gentoo-Bug-url: https://bugs.gentoo.org/show_bug.cgi?id=567686
> ---
>  pym/_emerge/depgraph.py | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/pym/_emerge/depgraph.py b/pym/_emerge/depgraph.py
> index 2169b00..fd2c771 100644
> --- a/pym/_emerge/depgraph.py
> +++ b/pym/_emerge/depgraph.py
> @@ -9064,7 +9064,9 @@ class _dep_check_composite_db(dbapi):
>   # Note: highest_visible is not necessarily the real 
> highest
>   # visible, especially when --update is not enabled, so 
> use
>   # < operator instead of !=.
> - if highest_visible is not None and pkg < 
> highest_visible:
> + if (highest_visible is not None and pkg < 
> highest_visible
> + and atom_set.findAtomForPackage(highest_visible,
> + 
> modified_use=self._depgraph._pkg_use_enabled(highest_visible))):
>   return False
>   elif in_graph != pkg:
>   # Mask choices for packages that would trigger a slot
> 

This is pretty trivial, so I've pushed it.
-- 
Thanks,
Zac

[gentoo-portage-dev] [PATCH] Add .git dir to excluded dirs in default PORTAGE_RSYNC_OPTS

[NP-Hardass]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA256

Based off of git master at Mon Dec 14 02:36:31 UTC 2015, commit
7cbd04cd62c8f13ed41e07ff8bc9b7e5d5ac700b


- - From b49fba5c16a931d3ab041446dd8aeba4d2403260 Mon Sep 17 00:00:00 2001
From: NP-Hardass 
Date: Sun, 13 Dec 2015 21:20:39 -0500
Subject: [PATCH] Add .git dir to excluded dirs in default PORTAGE_RSYNC_OPTS

Adding the .git dir to the default exclude dirs should have no ill side
effects as rsync is not allowed when .git dirs are present and should,
on the user's side prevent future potential sync issues like those that
we recently experienced.
- - ---
 cnf/make.globals | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cnf/make.globals b/cnf/make.globals
index 82d8cc1..836bb5c 100644
- - --- a/cnf/make.globals
+++ b/cnf/make.globals
@@ -92,7 +92,7 @@ PORTAGE_RSYNC_RETRIES="-1"
 # Number of seconds rsync will wait before timing out.
 #RSYNC_TIMEOUT="180"
 
- - -PORTAGE_RSYNC_OPTS="--recursive --links --safe-links --perms --times 
--omit-dir-times --compress --force --whole-file --delete --stats 
--human-readable --timeout=180 --exclude=/distfiles --exclude=/local 
--exclude=/packages"
+PORTAGE_RSYNC_OPTS="--recursive --links --safe-links --perms --times 
--omit-dir-times --compress --force --whole-file --delete --stats 
--human-readable --timeout=180 --exclude=/distfiles --exclude=/local 
--exclude=/packages --exclude=/.git"
 
 # The number of days after the last `emerge --sync` that a warning
 # message should be produced.
- - -- 
2.6.4


- - -- 
NP-Hardass
-BEGIN PGP SIGNATURE-
Version: GnuPG v2

iQIcBAEBCAAGBQJWbjLHAAoJEBzZQR2yrxj7/v0P/3NAxycwWB+EyLmTRTJI3whL
VLlN+oW3Pvw+PInnv23o/cMlfH3i8wNgi6syaWj+z6guasN0vC3pGJ9XCTo7SJW5
BKUEL8tzbQFa8W5k0nK9IkCOrEvqartBep9KLu8vj2SUQ4xRcEWk/uBksczJ+13g
CzP3kDdJmKGWZHER8+viwQ6tVNKxJW41SwQG5pz4emfADyceQnu/iy9SlRk8PsOD
LfHbuoZTv6YejuA8sDj09vRiNGfEwjuDTqBvjUziHdDg8J9is+vtmj04dqf05ZBR
PrEpoPo5FulGMzQjfF7dgyTpzM5pHBkxvPwM2U3HupnV1H5xixpXkNMEYQ6V3zK+
Max5LbOpzMzHdAx7+gYSRHC4pUa6FTy9lvEqgBfgDG7niq82HADltEruoFT0PRzX
ggRqxGJtRStrwHJDBGRNGdmOelmzI5FCC1497GdlOIvRdiRxvmIdlConVvWD5cii
FbaZsTxrV36NRUXCvR8G9hHLjJUPAjXMU3Ri+huCn0KjDN705envnBhF0LbJ3bvj
jRA/5KDLJcwPMOneEMp5Vb4SQJtC7n3OG1oxaqdQoZ4pu61i0LVdoEe5JpuHtKkM
g0PL7S+J8AA1eBTIlcYnsuwDt1QeAPq5eyHnQfBwF2ptKmRjNz3dPf4VNzM+ZYcW
xZDbhRKNifiWTq6Ee0Ee
=1vMD
-END PGP SIGNATURE-
From b49fba5c16a931d3ab041446dd8aeba4d2403260 Mon Sep 17 00:00:00 2001
From: NP-Hardass 
Date: Sun, 13 Dec 2015 21:20:39 -0500
Subject: [PATCH] Add .git dir to excluded dirs in default PORTAGE_RSYNC_OPTS

Adding the .git dir to the default exclude dirs should have no ill side
effects as rsync is not allowed when .git dirs are present and should,
on the user's side prevent future potential sync issues like those that
we recently experienced.
---
 cnf/make.globals | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cnf/make.globals b/cnf/make.globals
index 82d8cc1..836bb5c 100644
--- a/cnf/make.globals
+++ b/cnf/make.globals
@@ -92,7 +92,7 @@ PORTAGE_RSYNC_RETRIES="-1"
 # Number of seconds rsync will wait before timing out.
 #RSYNC_TIMEOUT="180"
 
-PORTAGE_RSYNC_OPTS="--recursive --links --safe-links --perms --times --omit-dir-times --compress --force --whole-file --delete --stats --human-readable --timeout=180 --exclude=/distfiles --exclude=/local --exclude=/packages"
+PORTAGE_RSYNC_OPTS="--recursive --links --safe-links --perms --times --omit-dir-times --compress --force --whole-file --delete --stats --human-readable --timeout=180 --exclude=/distfiles --exclude=/local --exclude=/packages --exclude=/.git"
 
 # The number of days after the last `emerge --sync` that a warning
 # message should be produced.
-- 
2.6.4