Re: [gentoo-portage-dev] [PATCH 1/3] emerge: Deprecate --autounmask

[Pacho Ramos]

El dom, 19-01-2014 a las 01:21 +0100, Alexander Berntsen escribió:
> Remove the --autounmask option from emerge. Please note that removing
> the option does not mean that the variable used for keeping track of
> autounmasking is not removed from depgraph.py.

If I understand the change correctly (I don't know much about python
but, but per the diff, looks like you are dropping the option and making
the code behave like it's always 'True'), seems that you are forcing
autounmask to be on always.

Even if I use this in all my systems (passing it in by default emerge
opts), I think we still need a way to disable it sometimes. For example,
I need that when portage shows me really strange error messages. I
remember this was an old bug related with backtracking, but can't find
it just now (it should contains quite a few duplicates) :S

I am referring to that kind of errors that reports the wrong package as
being the culprit of some conflict 

Re: [gentoo-portage-dev] Signing off patches

[Mike Frysinger]

On Saturday 18 January 2014 17:57:38 Tom Wijsman wrote:
> On Sat, 18 Jan 2014 08:43:12 -0800 "W. Trevor King" wrote:
> > On Sat, Jan 18, 2014 at 04:02:02PM +0100, Tom Wijsman wrote:
> > I think the idea is that you shouldn't need to refer to an external
> > resource like the mailing list to understand the idea behind the
> > patch,
> 
> Either someone cares about the background of a patch or he/she doesn't.

full details to understand the change must be in the commit message.  saying 
"go find it in the mailing list" is not a workable solution.

this doesn't mean you have to copy & paste the entire discussion, but it does 
mean you have to distill things down.  for topics that go on for a while, 
adding a tag linking to the mailing list archive is certainly OK.

when it comes to tags, i only copy in what other people have bothered posting.  
so if someone posts their Reviewed-by or Acked-by, i'll use them.
-mike


signature.asc
Description: This is a digitally signed message part.

Re: [gentoo-portage-dev] Bugzilla workflow

[Mike Frysinger]

On Tuesday 14 January 2014 21:58:38 Tom Wijsman wrote:
> On Sun, 05 Jan 2014 15:42:48 -0800 Brian Dolbec wrote:
> > 2) start working on a solution,
> >a) if you have significant progress, but need more time, mark it
> >   accordingly, assign it to yourself, leave a comment, etc.
> 
> Assigning it to oneself is a quite good idea as it allows to easily keep
> track of the bugs you are working on, otherwise you have to rely on
> techniques that aren't optimal; which are unfortunate.
> 
> In the lists of all bugs, that can be obtained by checking out the
> product and/or categories; this gives a quite clear overview of who is
> working on what, as well as which bugs are still free. As this is still
> able to be done, there seems no need to assign the bug to Portage team.

i disagree.  dev-portage@ get's cc-ed on bugs when they're being kept abreast 
of developments (like PMS), or someone just wants an opinion/feedback on an 
issue.  so there's no way to differentiate between bugs that are assigned to 
the portage team and bugs where the portage team's opinion is being requested.  
i want a query for the former and i just rely on generated bugzilla e-mails 
for the latter.

what's wrong with using the whiteboard ?  it's a free text field and you can 
easily construct a query that produces exactly what you want.  just stick in 
your username in there.

> > 3) commit the fix.  Mark it as InVCS, if not already, set status to
> >IN_PROGRESS
> 
> InVCS becomes redundant; other than that, good.

i don't see how it's redundant.  there is no other flag that indicates things 
have been fixed in the git tree and the only reason the bug remains open is 
that a release has not yet been cut.
-mike


signature.asc
Description: This is a digitally signed message part.

Re: [gentoo-portage-dev] [PATCH v3] Test for read-only filesystems, bail out during preinst if there are any which will be written to and display a useful error message. Fixes bug 378869.

[Mike Frysinger]

On Saturday 18 January 2014 21:00:48 Chris Reffett wrote:
> --- /dev/null
> +++ b/pym/portage/util/rochecker.py
> @@ -0,0 +1,81 @@
> +#-*- coding:utf-8 -*-
> +# Copyright 2014 Gentoo Foundation
> +# Distributed under the terms of the GNU General Public License v2
> +"""
> +Methods to check whether Portage is going to write to read-only
> filesystems. +Since the methods are not portable across different OSes,
> each OS needs its +own method. To expand RO checking for different OSes,
> add a method which +accepts a list of directories and returns a list of
> mounts which need to be +remounted RW, then add "elif ostype == (the
> ostype value for your OS)" to

prefer OSes -> OS's

> +get_ro_checker()
> +"""

period at the end

> +def get_ro_checker():
> + """
> + Uses the system type to find an appropriate method for testing whether
> Portage
> + is going to write to any read-only filesystems
> +
> + @return:
> + 1. A method for testing for RO filesystems appropriate to the current
> system
> + """

not a new issue, but we really need to get away from this style and use 
PEP257.  someone feel like fixing that in the code base ? :)

> + if ostype == "Linux":
> + return linux_ro_checker
> + else:
> + return empty_ro_checker

isn't this a glorified dict ?

_CHECKERS = {
'Linux': linux_ro_checker,
}

def get_ro_checker()
return _CHECKERS.get(ostype, empty_ro_checker)
-mike


signature.asc
Description: This is a digitally signed message part.

Re: [gentoo-portage-dev] [PATCH 2/3] emerge: Rename --autounmask-write to --autounmask

[Mike Frysinger]

On Saturday 18 January 2014 19:21:11 Alexander Berntsen wrote:
> Rename --autounmask-write to --autounmask.

typically when we delete/rename an option, we give users a heads up.  that 
means a cycle where the old option exists but emits a warning before switching 
to the new one.  then we can delete it.
-mike


signature.asc
Description: This is a digitally signed message part.

Re: [gentoo-portage-dev] [PATCH v4] Add repoman check to warn if src_prepare/src_configure are used in EAPI 0/1 and if pkg_pretend is used in EAPI < 4. Fixes bug 379491.

[Mike Frysinger]

Acked-by: Mike Frysinger 

btw you should keep the patch summary short.  if it goes long, better to move 
it to the main body.
-mike


signature.asc
Description: This is a digitally signed message part.

Re: [gentoo-portage-dev] [PATCH 3/3] Have repoman deprecate G2CONF for the GNOME team. (bug #482084).

[Mike Frysinger]

On Wednesday 15 January 2014 19:07:20 Tom Wijsman wrote:
> +class DeprecateG2CONF(LineCheck):
> + repoman_check_name = 'G2CONF.deprecated'
> + re = re.compile(r'.*G2CONF.*')
> +
> + def check(self, num, line):
> + """Run the check on line and return error if there is one"""
> + m = self.re.match(line)

use re.search instead of re.match so you can drop the redundant ".*".  
further, i think you want to use word boundaries.  so:
re = re.compile(r'\bG2CONF\b')
-mike


signature.asc
Description: This is a digitally signed message part.

Re: [gentoo-portage-dev] [PATCH 3/3] Have repoman deprecate G2CONF for the GNOME team. (bug #482084).

[Mike Frysinger]

btw, suggestion for commit summary/message:

repoman: deprecate G2CONF

Deprecate the G2CONF variable as requested by the GNOME team.

URL: http://bugs.gentoo.org/482084
-mike


signature.asc
Description: This is a digitally signed message part.

Re: [gentoo-portage-dev] [PATCH 1/3 v2] Have repoman check if the packages to unpack rare archive formats from SRC_URI are present in DEPEND (bug #205909).

[Mike Frysinger]

On Friday 17 January 2014 18:03:57 Tom Wijsman wrote:
> ---

please shorten your commit summary and move more content to the body

> +getNonSystemArchiveDepends.archivers = {

it is super weird to attach to the object like this.  some might even say 
wrong.  move it into the class definition.

def getNonSystemArchiveDepends(fetchlist, eapi):
...

ARCHIVERS = {
...
}

> + re.compile('.*\.7[zZ]$'):"app-arch/p7zip",

regexes should always use raw strings.  there should also be a space after the 
colon in dicts.  so you want:

re.compile(r'.*\.7[zZ]$'): 'app-arch/p7zip',

> + re.compile('.*\.lzma$'):"app-arch/lzma-utils",

xz-utils, not lzma-utils

> + re.compile('.*\.(bz2?|tbz2)$'):"app-arch/bzip2",
> + re.compile('.*\.(tar(\.(bz2?|gz|Z))?|tbz2|t[bg]z)?$'):"app-arch/tar",
> + re.compile('.*\.(gz|tar\.Z|t[bg]z|[zZ])$'):"app-arch/gzip",
> + re.compile('.*\.tar.xz$'):"app-arch/tar",

requiring people list gzip, tar, and bzip2 is a significant policy change 
(which i'm inclined to say is wrong).  it needs discussion on the dev mailing 
list first.

> +def checkArchiveDepends(atoms, catpkg, relative_path, \
> + system_set_atoms, needed_unpack_depends, stats, fails):

you don't need the \ there because you have paren already to gather things.

> + for entry in needed_unpack_depends[catpkg]:
> + if entry not in [atom.cp for atom in atoms if atom != "||"]:
> + stats['unpack.' + mytype + '.missing'] += 1
> + fails['unpack.' + mytype + '.missing'].append( \
> + relative_path + ": %s is missing in %s" % \
> + (entry, mytype))

i know existing style doesn't follow it, but imo we should avoid string 
concatenation.  it makes the code harder to read imo.

key = 'unpack.%s.missing' % mytype
stats[key] += 1
fails[key].append(...)

> +def eapi_has_xz_utils(eapi):
> + return eapi not in ("0", "1", "2")

i would use "eapi_supports_xz_archives" unless there's a pre-existing style
-mike


signature.asc
Description: This is a digitally signed message part.

Re: [gentoo-portage-dev] [PATCH] Add repoman check to warn if src_prepare/src_configure are used in EAPI 0/1

[Mike Frysinger]

On Tuesday 14 January 2014 16:15:48 Tom Wijsman wrote:
> On Mon, 13 Jan 2014 23:59:11 -0500 Mike Frysinger wrote:
> > we probably should just use dev branches in the main repo, at least
> > for people who have write access to the repo
> > 
> > dev/$USERNAME/
> 
> To be more clear, which one? g.o.g.o, GitHub or is one of both fine?

g.o.g.o

> > > r'\s*src_(configure|prepare)\s*\(\)'
> > > 
> > > You can then proceed further and move the re outside:
> > the idea was to walk a balance between simplicity and
> > maintainability.  imo, the fixed version above is the best.
> 
> What about the latter improvements about the parentheses?

seems fine

> > as long as portage supports an EAPI, i see no reason to omit useful
> > checks like this.
> 
> Repeating my original question in different words: Why is it useful?

people run repoman outside of the main tree.  we don't really know their 
desire for EAPI compatibility.  legacy/old portage/who knows.  Chromium OS for 
a long time was restricted to EAPI 4 for two reasons -- it had an old portage 
version (and upgrading to a newer one regressed performance significantly, so 
we held off until we could figure out why), and it was using a really old 
stage3 
to build the SDK (which means we needed to support upgrading an old system 
too).
-mike


signature.asc
Description: This is a digitally signed message part.

Re: [gentoo-portage-dev] [PATCH 2/3] emerge: Rename --autounmask-write to --autounmask

[Alexander Berntsen]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA256

On 19/01/14 03:14, Mike Gilbert wrote:
> Ok, so how do I tell portage to suggest autounmask entries without 
> writing them to /etc/portage? Is that feature gone after your
> changes?
"emerge --ask foo" would suggest them, and prompt you to write them
for you.
- -- 
Alexander
alexan...@plaimi.net
http://plaimi.net/~alexander
-BEGIN PGP SIGNATURE-
Version: GnuPG v2.0.22 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iF4EAREIAAYFAlLbpm0ACgkQRtClrXBQc7XeGQD+Ng7PQm9+YweOAdRRdmKR692a
4EwqoeOZUK8OyhDIzEwA/2tJ4MTHmJRxlfYvf4IFVhmeHU/Nq46UE0eNp1sl7YBD
=S7Rd
-END PGP SIGNATURE-

Re: [gentoo-portage-dev] [PATCH 1/3] emerge: Deprecate --autounmask

[Alexander Berntsen]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA256

On 19/01/14 09:01, Pacho Ramos wrote:
> If I understand the change correctly (I don't know much about
> python but, but per the diff, looks like you are dropping the
> option and making the code behave like it's always 'True'), seems
> that you are forcing autounmask to be on always.
You do not understand it correctly. It makes --ask imply
- --autounmask-write.

> Even if I use this in all my systems (passing it in by default 
> emerge opts), I think we still need a way to disable it sometimes.
There is. Regardless of whether you mean (current) --autounmask or
(current) --autounmask-write behaviour.

emerge --ask foo # This won't -write
emerge --autounmask --pretend foo # Same as the above
emerge --ask --pretend foo # This won't even offer the suggestions

Please see [0] for more information.

[0]  
- -- 
Alexander
alexan...@plaimi.net
http://plaimi.net/~alexander
-BEGIN PGP SIGNATURE-
Version: GnuPG v2.0.22 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iF4EAREIAAYFAlLbp50ACgkQRtClrXBQc7UOqgEAkTdpMbn4b0ndvME8QT1+rdVa
VkYYLucfV8C3MG6DIJwBALW3zZlQeG/a/dHNdo+KuEnp/xnsUDacI+q0xP1GF3Ak
=6VxI
-END PGP SIGNATURE-

Re: [gentoo-portage-dev] [PATCH 1/3] emerge: Deprecate --autounmask

[Alexander Berntsen]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA256

On 19/01/14 11:23, Alexander Berntsen wrote:
> emerge --ask foo # This won't -write emerge --autounmask --pretend
> foo # Same as the above
Sorry, the comments here are imprecise. The first-mentioned will
*prompt* the user for writing the changes. The second-mentioned will
merely print out the suggestions.

- -- 
Alexander
alexan...@plaimi.net
http://plaimi.net/~alexander
-BEGIN PGP SIGNATURE-
Version: GnuPG v2.0.22 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iF4EAREIAAYFAlLbqD4ACgkQRtClrXBQc7Xq2gEAnZm6ZFdSykzpv6sOO/Wg6KFh
cbbijfaQXGmmrXM2gJQA/AklPbjJGgJ/TxfSydymu3gTo7UutkLwawpea+dCcAEf
=7Q4V
-END PGP SIGNATURE-

Re: [gentoo-portage-dev] [PATCH 1/3] emerge: Deprecate --autounmask

[Pacho Ramos]

El dom, 19-01-2014 a las 11:23 +0100, Alexander Berntsen escribió:
> On 19/01/14 09:01, Pacho Ramos wrote:
> > If I understand the change correctly (I don't know much about
> > python but, but per the diff, looks like you are dropping the
> > option and making the code behave like it's always 'True'), seems
> > that you are forcing autounmask to be on always.
> You do not understand it correctly. It makes --ask imply
> --autounmask-write.

Ah, nice :)

> 
> > Even if I use this in all my systems (passing it in by default 
> > emerge opts), I think we still need a way to disable it sometimes.
> There is. Regardless of whether you mean (current) --autounmask or
> (current) --autounmask-write behaviour.
> 
> emerge --ask foo # This won't -write
> emerge --autounmask --pretend foo # Same as the above
> emerge --ask --pretend foo # This won't even offer the suggestions
> 
> Please see [0] for more information.
> 
> [0]  

Then, I guess "-ap" would be the equivalent of --autounmask=n and should
behave in the same way, right? In that case, no problem (even if I think
we should document this since using --ask --pretend at the same time
doesn't look so intuitive to me :( )

Re: [gentoo-portage-dev] [PATCH v3] Test for read-only filesystems, bail out during preinst if there are any which will be written to and display a useful error message. Fixes bug 378869.

[Alexander Berntsen]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA256

On 19/01/14 10:17, Mike Frysinger wrote:
> prefer OSes -> OS's
That's just not proper English. It makes no sense. Please don't prefer
that. If for nothing else, then to prevent me from sighing whenever I
have to read it.

> not a new issue, but we really need to get away from this style and
> use PEP257.  someone feel like fixing that in the code base ? :)
I agree. If you give me at least a week (won't have time until next
weekend), I can look into it.
- -- 
Alexander
alexan...@plaimi.net
http://plaimi.net/~alexander
-BEGIN PGP SIGNATURE-
Version: GnuPG v2.0.22 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iF4EAREIAAYFAlLbq10ACgkQRtClrXBQc7U7MAD/Yl2pd0ood3UKmvNMg9gdWMsw
hQqnReHPRwLpDNRccOcA/jUODDanycVmNLpD/rjkYnKyllnbUSIxduYuMcJFqXme
=vaDs
-END PGP SIGNATURE-

Re: [gentoo-portage-dev] [PATCH 1/3] emerge: Deprecate --autounmask

[Alexander Berntsen]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA256

On 19/01/14 11:32, Pacho Ramos wrote:
> Then, I guess "-ap" would be the equivalent of --autounmask=n and 
> should behave in the same way, right? In that case, no problem
> (even if I think we should document this since using --ask
> --pretend at the same time doesn't look so intuitive to me :( )
Since --ask implies --autounmask, the following are all the same for
autounmask-purposes:

emerge --pretend --ask foo
emerge --pretend --autounmask foo

As for "emerge --autounmask=n foo", This will actually spit out the
suggestions, just not ask to write them.

While playing with this, I discovered a possible misbehaviour though.
With "emerge --ask --autounmask=n", --ask takes precedence to
- --autounmask=n. Maybe it shouldn't. But this can always be changed.
- -- 
Alexander
alexan...@plaimi.net
http://plaimi.net/~alexander
-BEGIN PGP SIGNATURE-
Version: GnuPG v2.0.22 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iF4EAREIAAYFAlLbrUYACgkQRtClrXBQc7WpKAD/fK7gLyN6P9CZ4XE9UGBBdG7y
ugfFlnTXMYo//zsXu8cA/isRW4G+sk7L1g1xOp60y9XT7DZ8rEn8bw7LNoFKzIrH
=VOy3
-END PGP SIGNATURE-

Re: [gentoo-portage-dev] [PATCH 2/3] emerge: Rename --autounmask-write to --autounmask

[Alexander Berntsen]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA256

On 19/01/14 10:20, Mike Frysinger wrote:
> typically when we delete/rename an option, we give users a heads
> up. that means a cycle where the old option exists but emits a
> warning before switching to the new one.  then we can delete it.
We should have a news item.

[0]  
- -- 
Alexander
alexan...@plaimi.net
http://plaimi.net/~alexander
-BEGIN PGP SIGNATURE-
Version: GnuPG v2.0.22 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iF4EAREIAAYFAlLbrYEACgkQRtClrXBQc7WUcAEAosYXsLWlWJj1r7vAoVjOD2CG
qncNxjMh2qzfAOPjbPIA/3E6DZ7WztAdNweVLpRdxeeg/LKA8TlUBqjniH8Nvt6B
=5oJv
-END PGP SIGNATURE-

Re: [gentoo-portage-dev] Bugzilla workflow

[Tom Wijsman]

On Sun, 19 Jan 2014 04:10:20 -0500
Mike Frysinger  wrote:

> i disagree.  dev-portage@ get's cc-ed on bugs when they're being kept
> abreast of developments (like PMS), or someone just wants an
> opinion/feedback on an issue.  so there's no way to differentiate
> between bugs that are assigned to the portage team and bugs where the
> portage team's opinion is being requested. i want a query for the
> former and i just rely on generated bugzilla e-mails for the latter.

We have a whole product that tracks the bugs as well as individual
components; as a query can be made for that, this forms no problem.

Whether Portage is CC-ed is out of this context.

> what's wrong with using the whiteboard ?  it's a free text field and
> you can easily construct a query that produces exactly what you
> want.  just stick in your username in there.

This is a very good idea; I think that I can also search the URL, we
could make it commonly known that if the URL or whiteboard respcetively
contains a patch or a name that we know the person works on it.

> On Tuesday 14 January 2014 21:58:38 Tom Wijsman wrote:
>
> > InVCS becomes redundant; other than that, good.  
>
> i don't see how it's redundant.  there is no other flag that
> indicates things have been fixed in the git tree and the only reason
> the bug remains open is that a release has not yet been cut.

It is redundant because IN_PROGRESS would mean that it is in VCS; which
is needed for better overview on the tracker, hence setting InVCS would
add no extra meaning but just reflect the IN_PROGRESS status.

-- 
With kind regards,

Tom Wijsman (TomWij)
Gentoo Developer

E-mail address  : tom...@gentoo.org
GPG Public Key  : 6D34E57D
GPG Fingerprint : C165 AF18 AB4C 400B C3D2  ABF0 95B2 1FCD 6D34 E57D


signature.asc
Description: PGP signature

Re: [gentoo-portage-dev] [PATCH 1/3] emerge: Deprecate --autounmask

[Mike Gilbert]

On Sun, Jan 19, 2014 at 5:47 AM, Alexander Berntsen
 wrote:
> On 19/01/14 11:32, Pacho Ramos wrote:
>> Then, I guess "-ap" would be the equivalent of --autounmask=n and
>> should behave in the same way, right? In that case, no problem
>> (even if I think we should document this since using --ask
>> --pretend at the same time doesn't look so intuitive to me :( )
> Since --ask implies --autounmask, the following are all the same for
> autounmask-purposes:
>
> emerge --pretend --ask foo
> emerge --pretend --autounmask foo
>
> As for "emerge --autounmask=n foo", This will actually spit out the
> suggestions, just not ask to write them.
>
> While playing with this, I discovered a possible misbehaviour though.
> With "emerge --ask --autounmask=n", --ask takes precedence to
> - --autounmask=n. Maybe it shouldn't. But this can always be changed.

Please give me a way to shut off autounmask entirely no mater what
other options I pass to emerge. Tying it to --ask with no way to
disable it is not acceptable.

Generating the unmask entries can cause quite a performance hit, and
often causes portage to come up with nonsensical solutions when an
error message would be much more helpful.

Re: [gentoo-portage-dev] [PATCH] Add repoman check to warn if src_prepare/src_configure are used in EAPI 0/1

[Mike Gilbert]

On Sun, Jan 19, 2014 at 4:44 AM, Mike Frysinger  wrote:
> Chromium OS for a long time was restricted to EAPI 4 for two reasons -- it 
> had an old portage
> version (and upgrading to a newer one regressed performance significantly, so
> we held off until we could figure out why)

I am curious to know more about the performance regression if you can
share. Is that something that got fixed, or did you disable some
features (like the slot-operator stuff)?

Re: [gentoo-portage-dev] [PATCH 1/3] emerge: Deprecate --autounmask

[Alexander Berntsen]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA256

On 19/01/14 17:49, Mike Gilbert wrote:
> Please give me a way to shut off autounmask entirely no mater what
>  other options I pass to emerge. Tying it to --ask with no way to 
> disable it is not acceptable.
- --autounmask=n should do this. I messed up something. Sorry. I wrote
this months ago, and don't have it all in my head right now. I will
fix this next weekend.

- -- 
Alexander
alexan...@plaimi.net
http://plaimi.net/~alexander
-BEGIN PGP SIGNATURE-
Version: GnuPG v2.0.22 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iF4EAREIAAYFAlLcGSUACgkQRtClrXBQc7VehAEAhPIv0r7WZMOQSzzctvsDjkEF
CwmLlLNb8tzb52E/Ng0A/AxQueneyo2uK1HktnU9YPOAYmi6ziAImKS4SNYS5bF6
=ysNQ
-END PGP SIGNATURE-

[gentoo-portage-dev] [PATCH] Implement FEATURES="mirror" for emerge (bug 498498)

[Sebastian Luther]

This was only implemented for the ebuild command before.
---
 pym/_emerge/Scheduler.py | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/pym/_emerge/Scheduler.py b/pym/_emerge/Scheduler.py
index d663e97..2fd4d7e 100644
--- a/pym/_emerge/Scheduler.py
+++ b/pym/_emerge/Scheduler.py
@@ -165,6 +165,8 @@ class Scheduler(PollScheduler):
self._build_opts.buildpkg_exclude = InternalPackageSet( \
initial_atoms=" ".join(myopts.get("--buildpkg-exclude", 
[])).split(), \
allow_wildcard=True, allow_repo=True)
+   if "mirror" in self.settings.features:
+   self._build_opts.fetch_all_uri = True
 
self._binpkg_opts = self._binpkg_opts_class()
for k in self._binpkg_opts.__slots__:
@@ -752,11 +754,11 @@ class Scheduler(PollScheduler):
pass
 
elif pkg.type_name == "ebuild":
-
prefetcher = EbuildFetcher(background=True,
config_pool=self._ConfigPool(pkg.root,
self._allocate_config, self._deallocate_config),
-   fetchonly=1, logfile=self._fetch_log,
+   fetchonly=1, 
fetchall=self._build_opts.fetch_all_uri,
+   logfile=self._fetch_log,
pkg=pkg, prefetch=True, 
scheduler=self._sched_iface)
 
elif pkg.type_name == "binary" and \
-- 
1.8.3.2

Re: [gentoo-portage-dev] [PATCH] Implement FEATURES="mirror" for emerge (bug 498498)

[Sebastian Luther]

The removed white space was unintentional. Please remove that change
before committing that patch.

Re: [gentoo-portage-dev] [PATCH 0/3] Initial fetch() refactoring

[Sebastian Luther]

Am 19.01.2014 04:07, schrieb W. Trevor King:
> I felt like I should actually make a useful contribution to balance
> the less useful commit-message discussion ;).  Browsing through the
> open Portage bugs, #175612 looked interesting.  After I looked at
> pym/portage/package/ebuild/fetch.py, I decided to take a step back and
> just try and refactor fetch(), which was pushing 1k lines.  Here are
> three paches pulling fairly self-contained blocks out of fetch().  I
> thought I'd float them to the list to see if this was a productive
> avenue, or if this is going to be too much work to review.  I tried to
> avoid making too many changes other than the function-extraction, but
> in some places I couldn't help myself ;).

Good idea.

> 
> The patches aren't particularly well tested yet.  I ran the test suite
> and got some errors, but they seemed to be related to my non-root
> invocation, and not due to these changes themselves.  I thought I'd
> post my work so far, before digging deeper into the test suite.

The tests can be run as non-root user. Make sure all the files in the
git repo are +rw for that user.

You should check the result using pyflakes.

Do you have these patches in the publicly accessible git repo?

> 
> Cheers,
> Trevor
> 
> [1]: https://bugs.gentoo.org/show_bug.cgi?id=175612
> 
> W. Trevor King (3):
>   pym/portage/package/ebuild/fetch.py: Factor out
> _get_checksum_failure_max_tries
>   pym/portage/package/ebuild/fetch.py: Factor out _get_fetch_resume_size
>   pym/portage/package/ebuild/fetch.py: Factor out _get_uris
> 
>  pym/portage/package/ebuild/fetch.py | 305 
> +---
>  1 file changed, 177 insertions(+), 128 deletions(-)
> 

Re: [gentoo-portage-dev] [PATCH 0/3] Initial fetch() refactoring

[W. Trevor King]

On Sun, Jan 19, 2014 at 09:05:41PM +0100, Sebastian Luther wrote:
> Am 19.01.2014 04:07, schrieb W. Trevor King:
> > The patches aren't particularly well tested yet.  I ran the test
> > suite and got some errors, but they seemed to be related to my
> > non-root invocation, and not due to these changes themselves.  I
> > thought I'd post my work so far, before digging deeper into the
> > test suite.
> 
> The tests can be run as non-root user. Make sure all the files in
> the git repo are +rw for that user.

Ok.  I'll go back and check the results on master so I can compare.

> You should check the result using pyflakes.

Thanks.  This picked up a few errors in the third patch.  I'll post v2
shortly.

> Do you have these patches in the publicly accessible git repo?

I do now:

  git://tremily.us/portage.git fetch-refactor

Cheers,
Trevor

-- 
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy


signature.asc
Description: OpenPGP digital signature

Re: [gentoo-portage-dev] [PATCH 0/3] Initial fetch() refactoring

[Sebastian Luther]

The usual doc string style used in portage is:

"""
text
"""

Please use that for new functions. Also make sure you don't use spaces
to indent the last """.

Re: [gentoo-portage-dev] [PATCH 3/3] pym/portage/package/ebuild/fetch.py: Factor out _get_uris

[Sebastian Luther]

Am 19.01.2014 04:07, schrieb W. Trevor King:
> +def _get_uris(uris, settings, custom_mirrors=(), locations=()):

missing doc string

Re: [gentoo-portage-dev] [PATCH 0/3] Initial fetch() refactoring

[Sebastian Luther]

Am 19.01.2014 04:07, schrieb W. Trevor King:
> 
> The patches aren't particularly well tested yet.  I ran the test suite
> and got some errors, but they seemed to be related to my non-root
> invocation, and not due to these changes themselves.  I thought I'd
> post my work so far, before digging deeper into the test suite.

Since I doubt there's currently any test or any way to write one, I'd
suggest to try and run emerge and analyze coverage with
dev-python/coverage or a similar tool.

Re: [gentoo-portage-dev] [PATCH v3] Test for read-only filesystems, bail out during preinst if there are any which will be written to and display a useful error message. Fixes bug 378869.

[Alec Warner]

On Sun, Jan 19, 2014 at 2:39 AM, Alexander Berntsen wrote:

> -BEGIN PGP SIGNED MESSAGE-
> Hash: SHA256
>
> On 19/01/14 10:17, Mike Frysinger wrote:
> > prefer OSes -> OS's
> That's just not proper English. It makes no sense. Please don't prefer
> that. If for nothing else, then to prevent me from sighing whenever I
> have to read it.
>

The bikeshed should be red! Be happy in the fact that this bit has no
significant bearing on the code and just pick one.


> > not a new issue, but we really need to get away from this style and
> > use PEP257.  someone feel like fixing that in the code base ? :)
> I agree. If you give me at least a week (won't have time until next
> weekend), I can look into it.
>

Sorry, my fault. I added docstrings in 2006 (looks like the first one was
vercmp in
http://git.overlays.gentoo.org/gitweb/?p=proj/portage.git;a=blobdiff;f=pym/portage_versions.py;h=8e58c2f47766c0ed6c007c0cba1fccebca7d4eb4;hp=ec01773ad199a87d7906eb3862a14b9c8102fc29;hb=cfc497009e6054a5403461be676bb94deebbbf50;hpb=53f14db70339391fb13ca0adab353645f84be3f2)
I'm happy for these to change. ;p

-A



> - --

Alexander
> alexan...@plaimi.net
> http://plaimi.net/~alexander
> -BEGIN PGP SIGNATURE-
> Version: GnuPG v2.0.22 (GNU/Linux)
> Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
>
> iF4EAREIAAYFAlLbq10ACgkQRtClrXBQc7U7MAD/Yl2pd0ood3UKmvNMg9gdWMsw
> hQqnReHPRwLpDNRccOcA/jUODDanycVmNLpD/rjkYnKyllnbUSIxduYuMcJFqXme
> =vaDs
> -END PGP SIGNATURE-
>
>

Re: [gentoo-portage-dev] [PATCH 3/3] pym/portage/package/ebuild/fetch.py: Factor out _get_uris

[W. Trevor King]

On Sun, Jan 19, 2014 at 10:36:37PM +0100, Sebastian Luther wrote:
> Am 19.01.2014 04:07, schrieb W. Trevor King:
> > +def _get_uris(uris, settings, custom_mirrors=(), locations=()):
> 
> missing doc string

I'm still not entirely clear on what this function does ;).  Splitting
fetch() into chunks is giving me a clearer idea of how it works, but I
don't understand the distinction between filedict and primaryuri_dict.
I thought I had enough of an understanding to name the function, but
not yet enough to document it.

Cheers,
Trevor

-- 
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy


signature.asc
Description: OpenPGP digital signature

[gentoo-portage-dev] [PATCH v2 2/3] pym/portage/package/ebuild/fetch.py: Factor out _get_fetch_resume_size

[W. Trevor King]

The current fetch() function is quite long, which makes it hard to
know what I can change without adverse side effects.  By pulling this
logic out of the main function, we get clearer logic in fetch() and
more explicit input for the config extraction.
---
 pym/portage/package/ebuild/fetch.py | 50 -
 1 file changed, 27 insertions(+), 23 deletions(-)

diff --git a/pym/portage/package/ebuild/fetch.py 
b/pym/portage/package/ebuild/fetch.py
index 6f46572..de5bf00 100644
--- a/pym/portage/package/ebuild/fetch.py
+++ b/pym/portage/package/ebuild/fetch.py
@@ -272,6 +272,32 @@ def _get_checksum_failure_max_tries(settings, default=5):
return v
 
 
+def _get_fetch_resume_size(settings, default='350K'):
+   v = settings.get("PORTAGE_FETCH_RESUME_MIN_SIZE")
+   if v is not None:
+   v = "".join(v.split())
+   if not v:
+   # If it's empty, silently use the default.
+   v = default
+   match = _fetch_resume_size_re.match(v)
+   if (match is None or
+   match.group(2).upper() not in _size_suffix_map):
+   writemsg(_("!!! Variable PORTAGE_FETCH_RESUME_MIN_SIZE"
+   " contains an unrecognized format: '%s'\n") % \
+   settings["PORTAGE_FETCH_RESUME_MIN_SIZE"],
+   noiselevel=-1)
+   writemsg(_("!!! Using PORTAGE_FETCH_RESUME_MIN_SIZE "
+   "default value: %s\n") % default,
+   noiselevel=-1)
+   v = None
+   if v is None:
+   v = default
+   match = _fetch_resume_size_re.match(v)
+   v = int(match.group(1)) * \
+   2 ** _size_suffix_map[match.group(2).upper()]
+   return v
+
+
 def fetch(myuris, mysettings, listonly=0, fetchonly=0,
locks_in_subdir=".locks", use_locks=1, try_mirrors=1, digests=None,
allow_missing_digests=True):
@@ -297,29 +323,7 @@ def fetch(myuris, mysettings, listonly=0, fetchonly=0,
 
checksum_failure_max_tries = _get_checksum_failure_max_tries(
settings=mysettings)
-
-   fetch_resume_size_default = "350K"
-   fetch_resume_size = mysettings.get("PORTAGE_FETCH_RESUME_MIN_SIZE")
-   if fetch_resume_size is not None:
-   fetch_resume_size = "".join(fetch_resume_size.split())
-   if not fetch_resume_size:
-   # If it's undefined or empty, silently use the default.
-   fetch_resume_size = fetch_resume_size_default
-   match = _fetch_resume_size_re.match(fetch_resume_size)
-   if match is None or \
-   (match.group(2).upper() not in _size_suffix_map):
-   writemsg(_("!!! Variable PORTAGE_FETCH_RESUME_MIN_SIZE"
-   " contains an unrecognized format: '%s'\n") % \
-   mysettings["PORTAGE_FETCH_RESUME_MIN_SIZE"], 
noiselevel=-1)
-   writemsg(_("!!! Using PORTAGE_FETCH_RESUME_MIN_SIZE "
-   "default value: %s\n") % 
fetch_resume_size_default,
-   noiselevel=-1)
-   fetch_resume_size = None
-   if fetch_resume_size is None:
-   fetch_resume_size = fetch_resume_size_default
-   match = _fetch_resume_size_re.match(fetch_resume_size)
-   fetch_resume_size = int(match.group(1)) * \
-   2 ** _size_suffix_map[match.group(2).upper()]
+   fetch_resume_size = _get_fetch_resume_size(settings=mysettings)
 
# Behave like the package has RESTRICT="primaryuri" after a
# couple of checksum failures, to increase the probablility
-- 
1.8.5.2.8.g0f6c0d1

[gentoo-portage-dev] [PATCH v2 0/3] Initial fetch() refactoring

[W. Trevor King]

Following Sebastian's pyflakes and testing suggestions turned up a few
bugs in the _get_uris patch.  Changes since v1:

Patch #1:

* Fixed:

"""

  to:

"""


  and converted some from tabs to spaces where I'd missed them in v1.

Patch #3:

* Fixed docstrings as for patch #1.
* Fix 'myuri' → 'uri' in _get_file_uri_tuples.
* Fix 'cmirr' → 'm_uri' in _expand_mirror's urlunparse calls.
* Fix 'cmirr' → 'locmirr' in _expand_mirror's third_party_mirrors branch.
* Move “No known mirror” error from _get_uris to _expand_mirror.
* Calculate 'restrict', 'restrict_fetch', 'restrict_mirror', and
  'force_mirror' in _get_uris.
* Remove 'force_mirror' from fetch, because it's only used in _get_uris.
* Fix 'custom_mirrors' → 'custommirrors' in _get_uris invocation. 

After these changes, runtests.sh passes with only TODO messages on
Python 2.7, 3.2, and 3.3, although I'm not sure how thoroughly the
test suite covers fetch.  There are also a few unrelated:

  /usr/lib64/python3.3/xml/etree/ElementTree.py:1726:
  DeprecationWarning: This method of XMLParser is deprecated.
  Define doctype() method on the TreeBuilder target.
parser.feed(data)

warnings in the 3.3 results.

For folks who prefer Git fetches to the emailed patches, my current
series is at:

  git://tremily.us/portage.git fetch-refactor

For those who prefer Git diffs to the above “changes since v1”, the v1
version of this series is tagged in my repository as
fetch-refactor-v1.

Sebastian, I'd normally cc you directly on v2, but I'm not sure what
the Portage team's conventions are here.  Let me know if there is a
convention, or if you have a personal preference on direct mail vs the
list.  I think projects that encourage cc-ing tend to have reasonable
numbers of NNTP readers, and I don't know where the
gentoo-portage-dev@ population falls on that issue.

W. Trevor King (3):
  pym/portage/package/ebuild/fetch.py: Factor out
_get_checksum_failure_max_tries
  pym/portage/package/ebuild/fetch.py: Factor out _get_fetch_resume_size
  pym/portage/package/ebuild/fetch.py: Factor out _get_uris

 pym/portage/package/ebuild/fetch.py | 318 +---
 1 file changed, 189 insertions(+), 129 deletions(-)

-- 
1.8.5.2.8.g0f6c0d1

[gentoo-portage-dev] [PATCH v2 3/3] pym/portage/package/ebuild/fetch.py: Factor out _get_uris

[W. Trevor King]

The current fetch() function is quite long, which makes it hard to
know what I can change without adverse side effects.  By pulling this
logic out of the main function, we get clearer logic in fetch() and
more explicit input for the config extraction.

This block was especially complicated, so I also created the helper
functions _get_file_uri_tuples and _expand_mirror.  I'd prefer if
_expand_mirror iterated through URIs instead of (group, URI) tuples,
but we need a distinct marker for third-party URIs to build
third_party_mirror_uris which is used to build primaryuri_dict which
is used way down in fetch():

  if checksum_failure_count == \
  checksum_failure_primaryuri:
  # Switch to "primaryuri" mode in order
  # to increase the probablility of
  # of success.
  primaryuris = \
  primaryuri_dict.get(myfile)
  if primaryuris:
  uri_list.extend(
  reversed(primaryuris))

I don't know if this is useful enough to motivate the uglier
_expandmirror return values, but I'll kick that can down the road for
now.
---
 pym/portage/package/ebuild/fetch.py | 209 ++--
 1 file changed, 128 insertions(+), 81 deletions(-)

diff --git a/pym/portage/package/ebuild/fetch.py 
b/pym/portage/package/ebuild/fetch.py
index de5bf00..a1940f4 100644
--- a/pym/portage/package/ebuild/fetch.py
+++ b/pym/portage/package/ebuild/fetch.py
@@ -15,9 +15,9 @@ import sys
 import tempfile
 
 try:
-   from urllib.parse import urlparse
+   from urllib.parse import urlparse, urlunparse
 except ImportError:
-   from urlparse import urlparse
+   from urlparse import urlparse, urlunparse
 
 import portage
 portage.proxy.lazyimport.lazyimport(globals(),
@@ -298,6 +298,129 @@ def _get_fetch_resume_size(settings, default='350K'):
return v
 
 
+def _get_file_uri_tuples(uris):
+   """
+   Return a list of (filename, uri) tuples
+   """
+   file_uri_tuples = []
+   # Check for 'items' attribute since OrderedDict is not a dict.
+   if hasattr(uris, 'items'):
+   for filename, uri_set in uris.items():
+   for uri in uri_set:
+   file_uri_tuples.append((filename, uri))
+   if not uri_set:
+   file_uri_tuples.append((filename, None))
+   else:
+   for uri in uris:
+   if urlparse(uri).scheme:
+   file_uri_tuples.append(
+   (os.path.basename(uri), uri))
+   else:
+   file_uri_tuples.append(
+   (os.path.basename(uri), None))
+   return file_uri_tuples
+
+
+def _expand_mirror(uri, custom_mirrors=(), third_party_mirrors=()):
+   """
+   Replace the 'mirror://' scheme in the uri
+
+   Returns an iterable listing expanded (group, URI) tuples,
+   where the group is either 'custom' or 'third-party'.
+   """
+   parsed = urlparse(uri)
+   mirror = parsed.netloc
+   path = parsed.path
+   if path:
+   # Try user-defined mirrors first
+   if mirror in custom_mirrors:
+   for cmirr in custom_mirrors[mirror]:
+   m_uri = urlparse(cmirr)
+   yield ('custom', urlunparse((
+   m_uri.scheme, m_uri.netloc, path) +
+   parsed[3:]))
+
+   # now try the official mirrors
+   if mirror in third_party_mirrors:
+   uris = []
+   for locmirr in third_party_mirrors[mirror]:
+   m_uri = urlparse(locmirr)
+   uris.append(urlunparse((
+   m_uri.scheme, m_uri.netloc, path) +
+   parsed[3:]))
+   random.shuffle(uris)
+   for uri in uris:
+   yield ('third-party', uri)
+
+   if (not custom_mirrors.get(mirror, []) and
+   not third_party_mirrors.get(mirror, [])):
+   writemsg(
+   _("No known mirror by the name: %s\n")
+   % (mirror,))
+   else:
+   writemsg(_("Invalid mirror definition in SRC_URI:\n"),
+noiselevel=-1)
+   writemsg("  %s\n" % (uri), noiselevel=-1)
+
+
+def _get_uris(uris, settings, custom_mirrors=(), locations=()):
+   restrict = settings.get("PORTAGE_RESTRICT", "").split()
+   restrict_fetch = "fetch" in restrict
+   restrict_mirror = "mirror" in restrict or "nomirror" in restrict
+   force_mirror = (
+   "force-mirror" in settings.features and
+   not restrict_mirror)
+
+   

[gentoo-portage-dev] [PATCH v2 1/3] pym/portage/package/ebuild/fetch.py: Factor out _get_checksum_failure_max_tries

[W. Trevor King]

The current fetch() function is quite long, which makes it hard to
know what I can change without adverse side effects.  By pulling this
logic out of the main function, we get clearer logic in fetch() and
more explicit input for the config extraction.
---
 pym/portage/package/ebuild/fetch.py | 59 +
 1 file changed, 34 insertions(+), 25 deletions(-)

diff --git a/pym/portage/package/ebuild/fetch.py 
b/pym/portage/package/ebuild/fetch.py
index 5316f03..6f46572 100644
--- a/pym/portage/package/ebuild/fetch.py
+++ b/pym/portage/package/ebuild/fetch.py
@@ -240,6 +240,38 @@ _size_suffix_map = {
'Y' : 80,
 }
 
+
+def _get_checksum_failure_max_tries(settings, default=5):
+   """
+   Get the maximum number of failed download attempts
+
+   Generally, downloading the same file repeatedly from
+   every single available mirror is a waste of bandwidth
+   and time, so there needs to be a cap.
+   """
+   v = default
+   try:
+   v = int(settings.get("PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS",
+   default))
+   except (ValueError, OverflowError):
+   writemsg(_("!!! Variable PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS"
+   " contains non-integer value: '%s'\n") % \
+   settings["PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS"],
+   noiselevel=-1)
+   writemsg(_("!!! Using PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS "
+   "default value: %s\n") % default,
+   noiselevel=-1)
+   v = default
+   if v < 1:
+   writemsg(_("!!! Variable PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS"
+   " contains value less than 1: '%s'\n") % v, 
noiselevel=-1)
+   writemsg(_("!!! Using PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS "
+   "default value: %s\n") % default,
+   noiselevel=-1)
+   v = default
+   return v
+
+
 def fetch(myuris, mysettings, listonly=0, fetchonly=0,
locks_in_subdir=".locks", use_locks=1, try_mirrors=1, digests=None,
allow_missing_digests=True):
@@ -263,31 +295,8 @@ def fetch(myuris, mysettings, listonly=0, fetchonly=0,
print(_(">>> \"mirror\" mode desired and \"mirror\" 
restriction found; skipping fetch."))
return 1
 
-   # Generally, downloading the same file repeatedly from
-   # every single available mirror is a waste of bandwidth
-   # and time, so there needs to be a cap.
-   checksum_failure_max_tries = 5
-   v = checksum_failure_max_tries
-   try:
-   v = int(mysettings.get("PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS",
-   checksum_failure_max_tries))
-   except (ValueError, OverflowError):
-   writemsg(_("!!! Variable PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS"
-   " contains non-integer value: '%s'\n") % \
-   mysettings["PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS"], 
noiselevel=-1)
-   writemsg(_("!!! Using PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS "
-   "default value: %s\n") % checksum_failure_max_tries,
-   noiselevel=-1)
-   v = checksum_failure_max_tries
-   if v < 1:
-   writemsg(_("!!! Variable PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS"
-   " contains value less than 1: '%s'\n") % v, 
noiselevel=-1)
-   writemsg(_("!!! Using PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS "
-   "default value: %s\n") % checksum_failure_max_tries,
-   noiselevel=-1)
-   v = checksum_failure_max_tries
-   checksum_failure_max_tries = v
-   del v
+   checksum_failure_max_tries = _get_checksum_failure_max_tries(
+   settings=mysettings)
 
fetch_resume_size_default = "350K"
fetch_resume_size = mysettings.get("PORTAGE_FETCH_RESUME_MIN_SIZE")
-- 
1.8.5.2.8.g0f6c0d1

Re: [gentoo-portage-dev] [PATCH 3/3] pym/portage/package/ebuild/fetch.py: Factor out _get_uris

[Alec Warner]

On Sat, Jan 18, 2014 at 7:07 PM, W. Trevor King  wrote:

> The current fetch() function is quite long, which makes it hard to
> know what I can change without adverse side effects.  By pulling this
> logic out of the main function, we get clearer logic in fetch() and
> more explicit input for the config extraction.
>
> This block was especially complicated, so I also created the helper
> functions _get_file_uri_tuples and _expand_mirror.  I'd prefer if
> _expand_mirror iterated through URIs instead of (group, URI) tuples,
> but we need a distinct marker for third-party URIs to build
> third_party_mirror_uris which is used to build primaryuri_dict which
> is used way down in fetch():
>
>   if checksum_failure_count == \
>   checksum_failure_primaryuri:
>   # Switch to "primaryuri" mode in order
>   # to increase the probablility of
>   # of success.
>   primaryuris = \
>   primaryuri_dict.get(myfile)
>   if primaryuris:
>   uri_list.extend(
>   reversed(primaryuris))
>
> I don't know if this is useful enough to motivate the uglier
> _expandmirror return values, but I'll kick that can down the road for
> now.
> ---
>  pym/portage/package/ebuild/fetch.py | 197
> +---
>  1 file changed, 117 insertions(+), 80 deletions(-)
>
> diff --git a/pym/portage/package/ebuild/fetch.py
> b/pym/portage/package/ebuild/fetch.py
> index bd572fa..a617945 100644
> --- a/pym/portage/package/ebuild/fetch.py
> +++ b/pym/portage/package/ebuild/fetch.py
> @@ -15,9 +15,9 @@ import sys
>  import tempfile
>
>  try:
> -   from urllib.parse import urlparse
> +   from urllib.parse import urlparse, urlunparse
>  except ImportError:
> -   from urlparse import urlparse
> +   from urlparse import urlparse, urlunparse
>
>  import portage
>  portage.proxy.lazyimport.lazyimport(globals(),
> @@ -297,6 +297,118 @@ def _get_fetch_resume_size(settings, default='350K'):
> return v
>
>
> +def _get_file_uri_tuples(uris):
> +   """Return a list of (filename, uri) tuples
> +   """
>

As mike noted on another thread:

""Return a list of (filename, uri) tuples."""

+   file_uri_tuples = []
> +   # Check for 'items' attribute since OrderedDict is not a dict.
> +   if hasattr(uris, 'items'):
> +   for filename, uri_set in uris.items():
> +   for uri in uri_set:
> +   file_uri_tuples.append((filename, uri))
> +   if not uri_set:
> +   file_uri_tuples.append((filename, None))
> +   else:
> +   for uri in uris:
> +   if urlparse(uri).scheme:
> +   file_uri_tuples.append(
> +   (os.path.basename(myuri), myuri))
> +   else:
> +   file_uri_tuples.append(
> +   (os.path.basename(myuri), None))
> +   return file_uri_tuples
> +
> +
> +def _expand_mirror(uri, custom_mirrors=(), third_party_mirrors=()):
> +   """Replace the 'mirror://' scheme in the uri
>

Sentence should end in a period.


> +
> +   Returns an iterable listing expanded (group, URI) tuples,
> +   where the group is either 'custom' or 'third-party'.
> +"""
> +   parsed = urlparse(uri)
> +   mirror = parsed.netloc
> +   path = parsed.path
> +   if path:
> +   # Try user-defined mirrors first
> +   if mirror in custom_mirrors:
> +   for cmirr in custom_mirrors[mirror]:
> +   m_uri = urlparse(cmirr)
> +   yield ('custom', urlunparse((
> +   cmirr.scheme, cmirr.netloc, path) +
> +   parsed[3:]))
> +
> +   # now try the official mirrors
> +   if mirror in third_party_mirrors:
> +   uris = []
> +   for locmirr in third_party_mirrors[mirror]:
> +   m_uri = urlparse(cmirr)
> +   uris.append(urlunparse((
> +   cmirr.scheme, cmirr.netloc, path) +
> +   parsed[3:]))
> +   random.shuffle(uris)
> +   for uri in uris:
> +   yield ('third-party', uri)
> +   else:
> +   writemsg(_("Invalid mirror definition in SRC_URI:\n"),
> +noiselevel=-1)
> +   writemsg("  %s\n" % (uri), noiselevel=-1)


Is this a py3k thing? You should be able to write:

writemsg("  %s\n" % uri, noiselevel=-1)

The tuple is only needed for multiple arguments in py2k.



+
> +
> +def _get_uris(uris, settings, custom_mirrors=(), locations=()):
>

I want you to be careful here. This is bad style when you are constr

Re: [gentoo-portage-dev] [PATCH v2 1/3] pym/portage/package/ebuild/fetch.py: Factor out _get_checksum_failure_max_tries

[Alec Warner]

On Sun, Jan 19, 2014 at 2:44 PM, Alec Warner  wrote:

> On Sun, Jan 19, 2014 at 2:14 PM, W. Trevor King  wrote:
>
>> The current fetch() function is quite long, which makes it hard to
>> know what I can change without adverse side effects.  By pulling this
>> logic out of the main function, we get clearer logic in fetch() and
>> more explicit input for the config extraction.
>> ---
>>  pym/portage/package/ebuild/fetch.py | 59
>> +
>>  1 file changed, 34 insertions(+), 25 deletions(-)
>>
>> diff --git a/pym/portage/package/ebuild/fetch.py
>> b/pym/portage/package/ebuild/fetch.py
>> index 5316f03..6f46572 100644
>> --- a/pym/portage/package/ebuild/fetch.py
>> +++ b/pym/portage/package/ebuild/fetch.py
>> @@ -240,6 +240,38 @@ _size_suffix_map = {
>> 'Y' : 80,
>>  }
>>
>> +
>> +def _get_checksum_failure_max_tries(settings, default=5):
>> +   """
>> +   Get the maximum number of failed download attempts
>> +
>> +   Generally, downloading the same file repeatedly from
>> +   every single available mirror is a waste of bandwidth
>> +   and time, so there needs to be a cap.
>> +   """
>> +   v = default
>> +   try:
>> +   v = int(settings.get("PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS",
>> +   default))
>> +   except (ValueError, OverflowError):
>> +   writemsg(_("!!! Variable
>> PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS"
>> +   " contains non-integer value: '%s'\n") % \
>> +   settings["PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS"],
>> +   noiselevel=-1)
>> +   writemsg(_("!!! Using PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS "
>> +   "default value: %s\n") % default,
>> +   noiselevel=-1)
>> +   v = default
>> +   if v < 1:
>> +   writemsg(_("!!! Variable
>> PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS"
>> +   " contains value less than 1: '%s'\n") % v,
>> noiselevel=-1)
>> +   writemsg(_("!!! Using PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS "
>> +   "default value: %s\n") % default,
>> +   noiselevel=-1)
>> +   v = default
>> +   return v
>> +
>> +
>>
>
> This function and the next function you wrote are identical. How about
> making a single function?
>
> def getValueFromSettings(settings, key, default, validator):
>   try:
> return validator(settings.get(key, default))
>   except ValidationError as e:
> # writemsg the exception, it will contain the juicy bits.)
>
> def getIntValueFromSettings(settings, key, default):
>   def validator(v):
> try:
>   v = int(v)
>except (ValueError, OverflowError) as e:
>   raise ValidationError(" bla bla bla not an int, we picked the
> default.")
> if v < 1:
>   v = default
>   raise ValidationError("bla bla bla less than one we used the
> defualt.")
>
>   return getValueFromSettings(settings, key, default, validator)
>
>
Note, don't actually use these function names, they are terrible.

-A



> Then we are not necessarily writing a bunch of the same functions over and
> over. The defaults we can stick in a config file, or in python, or at the
> call site.
>
> -A
>
>
>
>
>
>
>
>
>>  def fetch(myuris, mysettings, listonly=0, fetchonly=0,
>> locks_in_subdir=".locks", use_locks=1, try_mirrors=1,
>> digests=None,
>> allow_missing_digests=True):
>> @@ -263,31 +295,8 @@ def fetch(myuris, mysettings, listonly=0,
>> fetchonly=0,
>> print(_(">>> \"mirror\" mode desired and
>> \"mirror\" restriction found; skipping fetch."))
>> return 1
>>
>> -   # Generally, downloading the same file repeatedly from
>> -   # every single available mirror is a waste of bandwidth
>> -   # and time, so there needs to be a cap.
>> -   checksum_failure_max_tries = 5
>> -   v = checksum_failure_max_tries
>> -   try:
>> -   v =
>> int(mysettings.get("PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS",
>> -   checksum_failure_max_tries))
>> -   except (ValueError, OverflowError):
>> -   writemsg(_("!!! Variable
>> PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS"
>> -   " contains non-integer value: '%s'\n") % \
>> -   mysettings["PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS"],
>> noiselevel=-1)
>> -   writemsg(_("!!! Using PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS "
>> -   "default value: %s\n") %
>> checksum_failure_max_tries,
>> -   noiselevel=-1)
>> -   v = checksum_failure_max_tries
>> -   if v < 1:
>> -   writemsg(_("!!! Variable
>> PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS"
>> -   " contains value less than 1: '%s'\n") % v,
>> noiselevel=-1)
>> -   writemsg(_("!!! Using PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS "
>> -   "default 

Re: [gentoo-portage-dev] [PATCH v2 1/3] pym/portage/package/ebuild/fetch.py: Factor out _get_checksum_failure_max_tries

[Alec Warner]

On Sun, Jan 19, 2014 at 2:14 PM, W. Trevor King  wrote:

> The current fetch() function is quite long, which makes it hard to
> know what I can change without adverse side effects.  By pulling this
> logic out of the main function, we get clearer logic in fetch() and
> more explicit input for the config extraction.
> ---
>  pym/portage/package/ebuild/fetch.py | 59
> +
>  1 file changed, 34 insertions(+), 25 deletions(-)
>
> diff --git a/pym/portage/package/ebuild/fetch.py
> b/pym/portage/package/ebuild/fetch.py
> index 5316f03..6f46572 100644
> --- a/pym/portage/package/ebuild/fetch.py
> +++ b/pym/portage/package/ebuild/fetch.py
> @@ -240,6 +240,38 @@ _size_suffix_map = {
> 'Y' : 80,
>  }
>
> +
> +def _get_checksum_failure_max_tries(settings, default=5):
> +   """
> +   Get the maximum number of failed download attempts
> +
> +   Generally, downloading the same file repeatedly from
> +   every single available mirror is a waste of bandwidth
> +   and time, so there needs to be a cap.
> +   """
> +   v = default
> +   try:
> +   v = int(settings.get("PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS",
> +   default))
> +   except (ValueError, OverflowError):
> +   writemsg(_("!!! Variable
> PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS"
> +   " contains non-integer value: '%s'\n") % \
> +   settings["PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS"],
> +   noiselevel=-1)
> +   writemsg(_("!!! Using PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS "
> +   "default value: %s\n") % default,
> +   noiselevel=-1)
> +   v = default
> +   if v < 1:
> +   writemsg(_("!!! Variable
> PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS"
> +   " contains value less than 1: '%s'\n") % v,
> noiselevel=-1)
> +   writemsg(_("!!! Using PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS "
> +   "default value: %s\n") % default,
> +   noiselevel=-1)
> +   v = default
> +   return v
> +
> +
>

This function and the next function you wrote are identical. How about
making a single function?

def getValueFromSettings(settings, key, default, validator):
  try:
return validator(settings.get(key, default))
  except ValidationError as e:
# writemsg the exception, it will contain the juicy bits.)

def getIntValueFromSettings(settings, key, default):
  def validator(v):
try:
  v = int(v)
   except (ValueError, OverflowError) as e:
  raise ValidationError(" bla bla bla not an int, we picked the
default.")
if v < 1:
  v = default
  raise ValidationError("bla bla bla less than one we used the
defualt.")

  return getValueFromSettings(settings, key, default, validator)

Then we are not necessarily writing a bunch of the same functions over and
over. The defaults we can stick in a config file, or in python, or at the
call site.

-A








>  def fetch(myuris, mysettings, listonly=0, fetchonly=0,
> locks_in_subdir=".locks", use_locks=1, try_mirrors=1, digests=None,
> allow_missing_digests=True):
> @@ -263,31 +295,8 @@ def fetch(myuris, mysettings, listonly=0, fetchonly=0,
> print(_(">>> \"mirror\" mode desired and
> \"mirror\" restriction found; skipping fetch."))
> return 1
>
> -   # Generally, downloading the same file repeatedly from
> -   # every single available mirror is a waste of bandwidth
> -   # and time, so there needs to be a cap.
> -   checksum_failure_max_tries = 5
> -   v = checksum_failure_max_tries
> -   try:
> -   v =
> int(mysettings.get("PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS",
> -   checksum_failure_max_tries))
> -   except (ValueError, OverflowError):
> -   writemsg(_("!!! Variable
> PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS"
> -   " contains non-integer value: '%s'\n") % \
> -   mysettings["PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS"],
> noiselevel=-1)
> -   writemsg(_("!!! Using PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS "
> -   "default value: %s\n") %
> checksum_failure_max_tries,
> -   noiselevel=-1)
> -   v = checksum_failure_max_tries
> -   if v < 1:
> -   writemsg(_("!!! Variable
> PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS"
> -   " contains value less than 1: '%s'\n") % v,
> noiselevel=-1)
> -   writemsg(_("!!! Using PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS "
> -   "default value: %s\n") %
> checksum_failure_max_tries,
> -   noiselevel=-1)
> -   v = checksum_failure_max_tries
> -   checksum_failure_max_tries = v
> -   del v
> +   checksum_failure_max_tries = _get_checksum_failure_max_tries(
> +   sett

Re: [gentoo-portage-dev] [PATCH 0/3] Initial fetch() refactoring

[Alexander Berntsen]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA256

On 19/01/14 22:22, Sebastian Luther wrote:
> The usual doc string style used in portage is:
> 
> """ text """
> 
> Please use that for new functions. Also make sure you don't use
> spaces to indent the last """.
> 
As mentioned by Mike in another thread, we should use PEP 257[0]. I
will convert old code to conform to this... sometime... soon... (I
promise!)

So if new patches could just do that right away, that would be neat.

[0]  
- -- 
Alexander
alexan...@plaimi.net
http://plaimi.net/~alexander
-BEGIN PGP SIGNATURE-
Version: GnuPG v2.0.22 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iF4EAREIAAYFAlLcVaAACgkQRtClrXBQc7XjTwD/UsCQo+SJdiCX/QsMC8jFDPic
k0jEAN6DA5xML6/nJYQA/36FszMfVMZ/vzg5VF9FS6BDWwGm/Qy2whyqLiF3FOrX
=+5Yy
-END PGP SIGNATURE-

Re: [gentoo-portage-dev] [PATCH 0/3] Initial fetch() refactoring

[Alec Warner]

On Sun, Jan 19, 2014 at 2:45 PM, Alexander Berntsen wrote:

> -BEGIN PGP SIGNED MESSAGE-
> Hash: SHA256
>
> On 19/01/14 22:22, Sebastian Luther wrote:
> > The usual doc string style used in portage is:
> >
> > """ text """
> >
> > Please use that for new functions. Also make sure you don't use
> > spaces to indent the last """.
> >
> As mentioned by Mike in another thread, we should use PEP 257[0]. I
> will convert old code to conform to this... sometime... soon... (I
> promise!)
>
> So if new patches could just do that right away, that would be neat.
>

Does pylint or pyflakes point out if you mess it up?

Automation for the win.

-A


>
> [0]  
> - --
> Alexander
> alexan...@plaimi.net
> http://plaimi.net/~alexander
> -BEGIN PGP SIGNATURE-
> Version: GnuPG v2.0.22 (GNU/Linux)
> Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
>
> iF4EAREIAAYFAlLcVaAACgkQRtClrXBQc7XjTwD/UsCQo+SJdiCX/QsMC8jFDPic
> k0jEAN6DA5xML6/nJYQA/36FszMfVMZ/vzg5VF9FS6BDWwGm/Qy2whyqLiF3FOrX
> =+5Yy
> -END PGP SIGNATURE-
>
>

Re: [gentoo-portage-dev] [PATCH 0/3] Initial fetch() refactoring

[W. Trevor King]

On Sun, Jan 19, 2014 at 10:39:03PM +0100, Sebastian Luther wrote:
> Am 19.01.2014 04:07, schrieb W. Trevor King:
> > The patches aren't particularly well tested yet.  I ran the test suite
> > and got some errors, but they seemed to be related to my non-root
> > invocation, and not due to these changes themselves.  I thought I'd
> > post my work so far, before digging deeper into the test suite.
> 
> Since I doubt there's currently any test or any way to write one,

I'm working on a pym/portage/tests/ebuild/test_fetch.py to exercise
the helpers.  Will post in v3.

> I'd suggest to try and run emerge and analyze coverage with
> dev-python/coverage or a similar tool.

Also a good idea.  I'll have to dust off my coverage notes ;).

Thanks,
Trevor

-- 
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy


signature.asc
Description: OpenPGP digital signature

Re: [gentoo-portage-dev] [PATCH 0/3] Initial fetch() refactoring

[Alexander Berntsen]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA256

On 19/01/14 23:46, Alec Warner wrote:
> Does pylint or pyflakes point out if you mess it up?
Not very successfully in my experience.
- -- 
Alexander
alexan...@plaimi.net
http://plaimi.net/~alexander
-BEGIN PGP SIGNATURE-
Version: GnuPG v2.0.22 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iF4EAREIAAYFAlLcVrsACgkQRtClrXBQc7XZcQD9GD8lV8HpZbQjpqu8ggNh4vOo
/fvFsCI2PaWiEsC4ISUA/iYRiXAYD7SEF4a3R/t6vOdmEQSHyvZcxH9NDwI4/biQ
=RMJ0
-END PGP SIGNATURE-

Re: [gentoo-portage-dev] [PATCH v2 1/3] pym/portage/package/ebuild/fetch.py: Factor out _get_checksum_failure_max_tries

[W. Trevor King]

On Sun, Jan 19, 2014 at 02:45:24PM -0800, Alec Warner wrote:
> On Sun, Jan 19, 2014 at 2:44 PM, Alec Warner  wrote:
> > This function and the next function you wrote are identical. How
> > about making a single function?
> >
> > …
> >
> > def getIntValueFromSettings(settings, key, default):
> > …
>
> Note, don't actually use these function names, they are terrible.

A good name would be getint [1].  If we used ConfigParser, we wouldn't
have to write any Portage-specific code at all ;).  I don't think
there's a shortage of things that could be streamlined here, and was
trying to minimize rewriting until fetch() had been reduced to
something I can comprehend ;).

Cheers,
Trevor

[1]: 
http://docs.python.org/3/library/configparser.html#configparser.ConfigParser.getint

-- 
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy


signature.asc
Description: OpenPGP digital signature

Re: [gentoo-portage-dev] [PATCH v2 1/3] pym/portage/package/ebuild/fetch.py: Factor out _get_checksum_failure_max_tries

[Alec Warner]

On Sun, Jan 19, 2014 at 2:51 PM, W. Trevor King  wrote:

> On Sun, Jan 19, 2014 at 02:45:24PM -0800, Alec Warner wrote:
> > On Sun, Jan 19, 2014 at 2:44 PM, Alec Warner  wrote:
> > > This function and the next function you wrote are identical. How
> > > about making a single function?
> > >
> > > …
> > >
> > > def getIntValueFromSettings(settings, key, default):
> > > …
> >
> > Note, don't actually use these function names, they are terrible.
>
> A good name would be getint [1].  If we used ConfigParser, we wouldn't
> have to write any Portage-specific code at all ;).  I don't think
> there's a shortage of things that could be streamlined here, and was
> trying to minimize rewriting until fetch() had been reduced to
> something I can comprehend ;).
>
>
Settings isn't a config, although we could emulate that API I guess ;)



> Cheers,
> Trevor
>
> [1]:
> http://docs.python.org/3/library/configparser.html#configparser.ConfigParser.getint
>
> --
> This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
> For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy
>

Re: [gentoo-portage-dev] [PATCH 0/3] Initial fetch() refactoring

[Alec Warner]

On Sun, Jan 19, 2014 at 2:50 PM, Alexander Berntsen wrote:

> -BEGIN PGP SIGNED MESSAGE-
> Hash: SHA256
>
> On 19/01/14 23:46, Alec Warner wrote:
> > Does pylint or pyflakes point out if you mess it up?
> Not very successfully in my experience.
>

I'm very against add a bunch of extra rules that have to be enforced by
hand. I want to make it easy to contribute, not more difficult. If bob can
run a tool that tells him all the things that are wrong with his patch,
that avoids us having like 1/3rd of the conversations on list ;)

-A


> - --
> Alexander
> alexan...@plaimi.net
> http://plaimi.net/~alexander
> -BEGIN PGP SIGNATURE-
> Version: GnuPG v2.0.22 (GNU/Linux)
> Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
>
> iF4EAREIAAYFAlLcVrsACgkQRtClrXBQc7XZcQD9GD8lV8HpZbQjpqu8ggNh4vOo
> /fvFsCI2PaWiEsC4ISUA/iYRiXAYD7SEF4a3R/t6vOdmEQSHyvZcxH9NDwI4/biQ
> =RMJ0
> -END PGP SIGNATURE-
>
>

Re: [gentoo-portage-dev] [PATCH 3/3] pym/portage/package/ebuild/fetch.py: Factor out _get_uris

[W. Trevor King]

On Sun, Jan 19, 2014 at 02:36:43PM -0800, Alec Warner wrote:
> On Sat, Jan 18, 2014 at 7:07 PM, W. Trevor King  wrote:
> > +def _get_file_uri_tuples(uris):
> > +   """Return a list of (filename, uri) tuples
> > +   """
> >
> 
> As mike noted on another thread:
> 
> ""Return a list of (filename, uri) tuples."""

I'd replaced this with:

  """
  Return a list of (filename, uri) tuples
  """

in v2, but I'll queue the one-line form in v3.

> > +def _expand_mirror(uri, custom_mirrors=(), third_party_mirrors=()):
> > +   """Replace the 'mirror://' scheme in the uri
> >
> 
> Sentence should end in a period.

Ok.  I'll do that for the other summaries as well, and uppercase URI,
in v3.

> > +   writemsg(_("Invalid mirror definition in SRC_URI:\n"),
> > +noiselevel=-1)
> > +   writemsg("  %s\n" % (uri), noiselevel=-1)
> 
> 
> Is this a py3k thing? You should be able to write:
> 
> writemsg("  %s\n" % uri, noiselevel=-1)
> 
> The tuple is only needed for multiple arguments in py2k.

1. I think I just copied and pasted it ;).
2. (uri) is not actually a tuple:

 >>> type(('hello'))
 

   To get a tuple, you need (uri,).

I'm fine removing the parens in v3.

> > +def _get_uris(uris, settings, custom_mirrors=(), locations=()):
> >
> 
> I want you to be careful here. This is bad style when you are
> constructing mutable objects in parameter defaults:

I used tuples instead of lists because they are not mutable (as you
point out later on).

> I'm not sure if I appreciate that more or less than the other form
> 
> def _get_uris(uris, settings, custom_mirrors=None, locations=None):
>   if not custom_mirrors:
> custom_mirrors = ()
>   if not locations:
> locations = ()

If you want me to do it that way, I'll queue it for v3.  I think the
default tuples are more readable.  They are certainly shorter.

> Another advisable way to write it is to simply not have default
> arguments, and to force the caller to sync in an empty iterable:

Meh.  I don't have strong feelings about this, but custom_mirrors
struck me as something that would not always be required ;).

> > +   third_party_mirrors = settings.thirdpartymirrors()
> >
> 
> Why pass in all of settings?

We need other stuff too, see v2.  The less settings parsing I need to
do in fetch() itself, the happier I am.  If that makes the helper
functions a bit uglier, that's fine.  Fewer people will have to read
those.

> Think about testing this function.

Working on it for v3.

> Do you really want to try to construct some sort of 'testing'
> settings object, or simply construct a list?

I was going to use a dict for testing, and stub out a
thirdpartymirrors mock for _get_uris.
> > +   third_party_mirror_uris = {}
> > +   filedict = OrderedDict()
> > +   primaryuri_dict = {}
> > +   for filename, uri in _get_file_uri_tuples(uris=uris):
> > +   if filename not in filedict:
> > +   filedict[filename] = [
> > +   os.path.join(location, 'distfiles',
> > filename)
> > +   for location in locations]
> > +   if uri is None:
> > +   continue
> > +   if uri.startswith('mirror://'):
> > +   uris = _expand_mirror(
> >
> 
> too many uri / uris variables...
> 
> can we called this 'expanded_uris'?

Queued for v3.

> I'm really having trouble tracking what is what. Remember that
> 'uris' is already a parameter to this function. Are you shadowing it
> here, or trying to overwrite it?

Clobbering it.  The original value is only used for the
_get_file_uri_tuples call.

> > +   uri=uri, custom_mirrors=custom_mirrors,
> > +   third_party_mirrors=third_party_mirrors)
> > +   filedict[filename].extend(uri for group, uri in
> > uris)
> >
> 
> group appears unused in your implicit iterable here.
> 
> perhaps:
> 
> filedict[filename].extend(uri for _, uri in uris)

Queued for v3.

> > +   third_party_mirror_uris.setdefault(filename,
> > []).extend(
> > +   uri for group, uri in uris
> > +   if group == 'third-party')
> >
> 
> I'm curious if this matters. We are iterator over 'uris' twice. Is it
> cheaper to do it once and build the iterables once?
> 
> third_party_uris = []
> for group, uri in uris:
>   if group == 'third_party':
> third_party_uris.append(uri)
>   filedict[filename].append(uri)
> 
> I'm guessing the iterable is so small it doesn't matter.

My guess is that the speed gain from list comprehension outweighs the
loss of double iterations, but I agree that it is probably doesn't
matter ;).

> > +   if not filedict[filename]:
> > +   writemsg(
> > +   _("No known mirror by the name:
> > %s\n")
> > +

Re: [gentoo-portage-dev] [PATCH 0/3] Initial fetch() refactoring

[W. Trevor King]

On Sun, Jan 19, 2014 at 02:46:36PM -0800, Alec Warner wrote:
> On Sun, Jan 19, 2014 at 2:45 PM, Alexander Berntsen 
> wrote:
> > On 19/01/14 22:22, Sebastian Luther wrote:
> > > The usual doc string style used in portage is:
> > >
> > > """ text """
> > >
> > > Please use that for new functions. Also make sure you don't use
> > > spaces to indent the last """.
> >
> > As mentioned by Mike in another thread, we should use PEP 257[0]. I
> > will convert old code to conform to this... sometime... soon... (I
> > promise!)
> >
> > So if new patches could just do that right away, that would be neat.
>
> Does pylint or pyflakes point out if you mess it up?
> 
> Automation for the win.

As of Emacs 24.3.1, fill-paragraph (M-q) in python-mode will
automatically format your docstring like this (which is how the
space-indented-""" snuck into my v1 ;).  Not as nice as an independent
checker, but still useful automation.

Cheers,
Trevor

-- 
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy


signature.asc
Description: OpenPGP digital signature

Re: [gentoo-portage-dev] [PATCH 3/3] pym/portage/package/ebuild/fetch.py: Factor out _get_uris

[W. Trevor King]

On Sun, Jan 19, 2014 at 03:06:29PM -0800, W. Trevor King wrote:
> On Sun, Jan 19, 2014 at 02:36:43PM -0800, Alec Warner wrote:
> > On Sat, Jan 18, 2014 at 7:07 PM, W. Trevor King  wrote:
> > > +   # Order primaryuri_dict values to match that in SRC_URI.
> > > +   for uris in primaryuri_dict.values():
> > > +   uris.reverse()
> > 
> > Is there any guaranteed ordering for dict.values()?
> 
> No.
> 
> > I thought dict order was random (and they seriously make it random
> > in modern python versions, to detect bugs.) How does this
> > uris.reverse() match the SRC_URI ordering?
> 
> No idea (copy-pasted code).  I'd be happy to cut this out in v3.

Ah, we're not reversing the ordering of values(), we're reversing the
ordering of each individual value.  Keeping it in.

Cheers,
Trevor

-- 
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy


signature.asc
Description: OpenPGP digital signature

Re: [gentoo-portage-dev] [PATCH 0/3] Initial fetch() refactoring

[Alexander Berntsen]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA256

On 19/01/14 23:54, Alec Warner wrote:
> I'm very against add a bunch of extra rules that have to be 
> enforced by hand. I want to make it easy to contribute, not more 
> difficult. If bob can run a tool that tells him all the things
> that are wrong with his patch, that avoids us having like 1/3rd of
> the conversations on list ;)
Feel free to write a tool for this, or to contribute to any of the
numerous linters and/or editor plug-ins. It would be much appreciated.

As for the difficulty of PEP 257... I have higher hopes for Portage
contributors than getting stuck at that. If I write a patch that makes
most of the docstrings follow it, then they can infer 99% of the
"extra rules" by just looking at the other functions and methods. If
they fail to comply, we can just mention it. If the docstring
formatting is the biggest issue with their patch, I doubt they'll have
a hard time fixing it.
- -- 
Alexander
alexan...@plaimi.net
http://plaimi.net/~alexander
-BEGIN PGP SIGNATURE-
Version: GnuPG v2.0.22 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iF4EAREIAAYFAlLcZPQACgkQRtClrXBQc7XizAD/Y/Gxc7N6VkgNFWRgP5lmQ84r
UwSne2xaqJYphY9x1TcBAIRpjBHB580edLz/8zpT14lqhW3oOmeMz0pNMB8ssW5d
=+zp5
-END PGP SIGNATURE-

Re: [gentoo-portage-dev] [PATCH 0/3] Initial fetch() refactoring

[Alec Warner]

On Sun, Jan 19, 2014 at 3:51 PM, Alexander Berntsen wrote:

> -BEGIN PGP SIGNED MESSAGE-
> Hash: SHA256
>
> On 19/01/14 23:54, Alec Warner wrote:
> > I'm very against add a bunch of extra rules that have to be
> > enforced by hand. I want to make it easy to contribute, not more
> > difficult. If bob can run a tool that tells him all the things
> > that are wrong with his patch, that avoids us having like 1/3rd of
> > the conversations on list ;)
> Feel free to write a tool for this, or to contribute to any of the
> numerous linters and/or editor plug-ins. It would be much appreciated.
>
>
I already prefer pylint, and I think it does cover most of what I want. I
am working a pylintrc patch.


> As for the difficulty of PEP 257... I have higher hopes for Portage
> contributors than getting stuck at that. If I write a patch that makes
> most of the docstrings follow it, then they can infer 99% of the
> "extra rules" by just looking at the other functions and methods. If
> they fail to comply, we can just mention it. If the docstring
> formatting is the biggest issue with their patch, I doubt they'll have
> a hard time fixing it.
>

I'm not saying its hard, I'm saying it is a giant waste of time for the
list to tell people 'hey your docstrings are wrong' when they can just run
a tool to do it ;)

-A




> - --
> Alexander
> alexan...@plaimi.net
> http://plaimi.net/~alexander
> -BEGIN PGP SIGNATURE-
> Version: GnuPG v2.0.22 (GNU/Linux)
> Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
>
> iF4EAREIAAYFAlLcZPQACgkQRtClrXBQc7XizAD/Y/Gxc7N6VkgNFWRgP5lmQ84r
> UwSne2xaqJYphY9x1TcBAIRpjBHB580edLz/8zpT14lqhW3oOmeMz0pNMB8ssW5d
> =+zp5
> -END PGP SIGNATURE-
>
>

[gentoo-portage-dev] Patchworks test: ignore [eom]

[Alec Warner]

Re: [gentoo-portage-dev] [PATCH 1/3] pym/portage/package/ebuild/fetch.py: Factor out _get_checksum_failure_max_tries

[Tom Wijsman]

On Sat, 18 Jan 2014 19:07:45 -0800
"W. Trevor King"  wrote:

> [...SNIP...]
> + v =
> int(settings.get("PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS",
> + default))
> + except (ValueError, OverflowError):
> + writemsg(_("!!! Variable
> PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS"
> + " contains non-integer value: '%s'\n") % \
> +
> settings["PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS"],
> + noiselevel=-1)
> + writemsg(_("!!! Using
> PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS "
> + "default value: %s\n") % default,
> + noiselevel=-1)
> + v = default
> + if v < 1:
> + writemsg(_("!!! Variable
> PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS"
> + " contains value less than 1: '%s'\n") % v,
> noiselevel=-1)
> + writemsg(_("!!! Using
> PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS "
> [...SNIP...]

The code screams PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS to me which makes
it hard to read, I would suggest assigning it to a variable in advance
as to not have to repeat it this often. Some code snippets for ideas:

key = "PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS"

v = int(settings.get(key), default)

"!!! Variable %s contains non-integer value: '%s" % (key, ...)

If needed, add a word to key to make the variable name slightly more
meaningful; but avoid the full length if possible. eg. try_mirrors_key

-- 
With kind regards,

Tom Wijsman (TomWij)
Gentoo Developer

E-mail address  : tom...@gentoo.org
GPG Public Key  : 6D34E57D
GPG Fingerprint : C165 AF18 AB4C 400B C3D2  ABF0 95B2 1FCD 6D34 E57D


signature.asc
Description: PGP signature

Re: [gentoo-portage-dev] [PATCH 2/3] pym/portage/package/ebuild/fetch.py: Factor out _get_fetch_resume_size

[Tom Wijsman]

On Sat, 18 Jan 2014 19:07:46 -0800
"W. Trevor King"  wrote:

> +def _get_fetch_resume_size(settings, default='350K'):
> + v = settings.get("PORTAGE_FETCH_RESUME_MIN_SIZE")
> + if v is not None:
> + v = "".join(v.split())
> + if not v:
> + # If it's empty, silently use the default.
> + v = default
> + match = _fetch_resume_size_re.match(v)
> + if (match is None or
> + match.group(2).upper() not in
> _size_suffix_map):
> + writemsg(_("!!! Variable
> PORTAGE_FETCH_RESUME_MIN_SIZE"
> + " contains an unrecognized format:
> '%s'\n") % \
> +
> settings["PORTAGE_FETCH_RESUME_MIN_SIZE"],
> + noiselevel=-1)
> + writemsg(_("!!! Using
> PORTAGE_FETCH_RESUME_MIN_SIZE "
> + "default value: %s\n") % default,
> + noiselevel=-1)
> + v = None
> + if v is None:
> + v = default
> + match = _fetch_resume_size_re.match(v)
> + v = int(match.group(1)) * \
> + 2 ** _size_suffix_map[match.group(2).upper()]
> + return v

There is some duplicate code here, I think the conditions can be
rewritten in such way that the duplicate code doesn't take place.

Move everything from the first 'if' except for the first line out
of that 'if', that way get a bare:

if v is not None:
v = "".join(v.split())

Outside that, you now have 'if not v:' whereas you later have
'if v is None:'; you can optimize this to 'if not v or v is None:'
(maybe that can be optimized further, dunno) and just have one
condition set the default here. This gives you:

if not v or v is None:
# If it's empty, silently use the default.
v = default

Afterwards, you have 'match = _fetch_resume_size_re.match(v)' in both
places, you can again just have this once.

Can the two remaining code blocks just be placed after that?

-- 
With kind regards,

Tom Wijsman (TomWij)
Gentoo Developer

E-mail address  : tom...@gentoo.org
GPG Public Key  : 6D34E57D
GPG Fingerprint : C165 AF18 AB4C 400B C3D2  ABF0 95B2 1FCD 6D34 E57D


signature.asc
Description: PGP signature

Re: [gentoo-portage-dev] [PATCH 1/3] pym/portage/package/ebuild/fetch.py: Factor out _get_checksum_failure_max_tries

[W. Trevor King]

On Mon, Jan 20, 2014 at 02:26:59AM +0100, Tom Wijsman wrote:
> On Sat, 18 Jan 2014 19:07:45 -0800
> "W. Trevor King"  wrote:
> 
> > [...SNIP...]
> > +   v =
> > int(settings.get("PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS",
> > +   default))
> > …
>
> The code screams PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS to me which
> makes it hard to read, …

That's all copy-pasted out of fetch() under the assumption that the
fewer changes I made (besides creating _get_checksum_failure_max_tries
at all), the easier it would be to review those changes.  Perhaps I
was mistaken ;).

> I would suggest assigning it to a variable in advance as to not have
> to repeat it this often. Some code snippets for ideas:
> 
> key = "PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS"

Will queue to v3.

Cheers,
Trevor

-- 
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy


signature.asc
Description: OpenPGP digital signature

Re: [gentoo-portage-dev] [PATCH 2/3] pym/portage/package/ebuild/fetch.py: Factor out _get_fetch_resume_size

[W. Trevor King]

On Mon, Jan 20, 2014 at 02:41:41AM +0100, Tom Wijsman wrote:
> There is some duplicate code here, I think the conditions can be
> rewritten in such way that the duplicate code doesn't take place.

Do you want a rewrite squashed into this commit, or as a follow-on
commit after this one (which gets a test suite in v3)?

Cheers,
Trevor

-- 
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy


signature.asc
Description: OpenPGP digital signature

Re: [gentoo-portage-dev] Signing off patches

[Tom Wijsman]

On Sat, 18 Jan 2014 20:15:57 -0800
"W. Trevor King"  wrote:

> On Sun, Jan 19, 2014 at 02:33:06AM +0100, Tom Wijsman wrote:
> > On Sat, 18 Jan 2014 15:24:59 -0800
> > "W. Trevor King"  wrote:
> > > If it doesn't need to get updated, then it probably already
> > > started out explaining the consensus ;).
> > 
> > That is a guess, you can look this up in past patches.
> 
> Sure.  Will you?  If I want to touch some code, and it looks
> confusing, I'll use blame to see who wrote it and whay they were
> thinking about.  If the commit message is not informative, I usually
> give up.  I have a hard time imaging folks tracking down the thread
> that spawned that patch, assuming such a thread even exists, for each
> troublesome line they'd like to touch.  It's much easier to summarize
> any issues the list resolved (because they're likely the same
> questions the new dev is asking) in the commit message, where future
> developers can find them.

How does this make the consensus written after the patch part of it?

The person whom commits can be different than the person whom wrote the
patch; and hence, that person commits without writing down consensus.
If that person were to write it down, it would change the authorship.

Hence you made a guess, because I see pushed commits without consensus.

> > > You spend time if you want to spend time and add whoever you feel
> > > moved to add.
> > 
> > We discuss whether to make it policy to add involved people.
> 
> But “involved” can be hard to pin down, especially by someone who may
> be applying v5 of a patch that hasn't been carefully following the
> whole discussion in earlier versions.

If this would be formalized, then v5 would include all tags until then.

> The Linux kernel docs say [1]:
> 
>   If this patch fixes a problem reported by somebody else, consider
>   adding a Reported-by: tag to credit the reporter for their
>   contribution.
> 
> Note the “consider” wiggle word.  They are a bit more formal about
> Reviewed-by, but only because it's signing off on their Reviewer's
> statement of oversight.  In general, if you're not signing some
> statement with the tag, formalizing “involved” is hard.

Here I like vapier's approach from the other reply in this sub thread,
that is to add it whenever people make the effort of providing the tag;
which is an approach the Linux kernel upstream takes as well, if you
want to be seen as a contributor you need to provide the tags.

> > > If you are submitting v2 of a patch, and feel a desire
> > > to credit reviewers / testers with this syntax, I think that's
> > > considerate of you. If you are committing someone else's patch to
> > > master, and want to record the folks who acked it on the list to
> > > distribute responsibility, that's fine too. If you want to use
> > > another syntax, or not do any of this at all, it's still fine by
> > > me ;). However, if a consistent syntax already exists, I see no
> > > reason not to use it when it suits your purpose.
> >
> > We discuss here whether to make it policy to use the same syntax.
> 
> I don't understand the distinction between “here are some guidelines,
> apply as and if you see fit” and “make it a policy to …”.  Say you
> have a situation like this:

When the thread starts with words like "ensure", "exercised",
"followed", "should", "proposals" then I rather get the impression that
this is rather working towards making it part of policy than some
random guidelines; even if these words are not mentioned, it is for us
to discuss whether we should make this more than just guidelines.
 
> As I understand it, 6 should be:
> 
>   6a. Everyone gets on with their lives.
> 
> I could see a situation where:
> 
>   6b. Charlie reminds Dan that he could have used the tags.  Everyone
>   gets on with their lives.
> 
> Is there another alternative step 6 implied by the “policy” keyword?
> Or is the policy workflow even more different somehow?

This discussion is based on that second situation 6b, as I think
everyone is fine with 6a; but, some will want this to not become
policy, others might want to see this to become policy. Hence this
leads to a discussion. 

At the moment people seem fine with them being used optional, thus I
agree with what was said here regarding it is a respectful suggestion to
consider; at least nobody has strongly opposed to using them at all.

-- 
With kind regards,

Tom Wijsman (TomWij)
Gentoo Developer

E-mail address  : tom...@gentoo.org
GPG Public Key  : 6D34E57D
GPG Fingerprint : C165 AF18 AB4C 400B C3D2  ABF0 95B2 1FCD 6D34 E57D


signature.asc
Description: PGP signature

Re: [gentoo-portage-dev] [PATCH 1/3 v2] Have repoman check if the packages to unpack rare archive formats from SRC_URI are present in DEPEND (bug #205909).

[Tom Wijsman]

On Sun, 19 Jan 2014 04:38:31 -0500
Mike Frysinger  wrote:

> On Friday 17 January 2014 18:03:57 Tom Wijsman wrote:
> > ---
> 
> please shorten your commit summary and move more content to the body

Right, I'm used to writing this on the command line; I'll try to look
into setting up an editor or a GUI (gitg, ...) to do my commit messages.
 
> > +getNonSystemArchiveDepends.archivers = {
> 
> it is super weird to attach to the object like this.  some might even
> say wrong.  

It a function, this makes it a static function variable.

What is weird here? It works properly, what would be wrong?

> move it into the class definition.
> def getNonSystemArchiveDepends(fetchlist, eapi):
>   ...
> 
>   ARCHIVERS = {
>   ...
>   }

That makes it a non-static function variable? This is a regression.

> > +   re.compile('.*\.7[zZ]$'):"app-arch/p7zip",
> 
> regexes should always use raw strings.  there should also be a space
> after the colon in dicts.  so you want:
> 
>   re.compile(r'.*\.7[zZ]$'): 'app-arch/p7zip',

Raw strings are a solution when \ forms a problem, which is not the
case here. The colon has no space after it in the rest of repoman near
it, hence I left it out; is there a coding standard we're trying to
respect here? Can you refer it to me?

> > +   re.compile('.*\.lzma$'):"app-arch/lzma-utils",
> xz-utils, not lzma-utils

http://dev.gentoo.org/~ulm/pms/5/pms.html

Please see "unpack" in PMS "11.3.3.13 Misc Commands", quote:

"lzma-compressed files (*.lzma). Ebuilds must ensure that LZMA Utils is
installed."

The TODO comment that had a PMS reference was near it before, but with
the refactor it has since moved; I'll put back the reference to it.

> > +   re.compile('.*\.(bz2?|tbz2)$'):"app-arch/bzip2",
> > +
> > re.compile('.*\.(tar(\.(bz2?|gz|Z))?|tbz2|t[bg]z)?$'):"app-arch/tar",
> > +   re.compile('.*\.(gz|tar\.Z|t[bg]z|[zZ])$'):"app-arch/gzip",
> > +   re.compile('.*\.tar.xz$'):"app-arch/tar",
> 
> requiring people list gzip, tar, and bzip2 is a significant policy
> change (which i'm inclined to say is wrong).  it needs discussion on
> the dev mailing list first.

Please see "unpack" in PMS "11.3.3.13 Misc Commands", example quote:

"gzip-compressed tar files (*.tar.gz, *.tgz, *.tar.Z, *.tbz). Ebuilds
must ensure that GNU gzip and GNU tar are installed."

To spare on mail length, I don't quote the rest of that enumeration.

> > +def checkArchiveDepends(atoms, catpkg, relative_path, \
> > +   system_set_atoms, needed_unpack_depends, stats, fails):
> 
> you don't need the \ there because you have paren already to gather
> things.
> 
> > +   for entry in needed_unpack_depends[catpkg]:
> > +   if entry not in [atom.cp for atom in atoms if
> > atom != "||"]:
> > +   stats['unpack.' + mytype + '.missing'] += 1
> > +   fails['unpack.' + mytype +
> > '.missing'].append( \
> > +   relative_path + ": %s is missing
> > in %s" % \
> > +   (entry, mytype))
> 
> i know existing style doesn't follow it, but imo we should avoid
> string concatenation.  it makes the code harder to read imo.
> 
>   key = 'unpack.%s.missing' % mytype
>   stats[key] += 1
>   fails[key].append(...)
> 
> > +def eapi_has_xz_utils(eapi):
> > +   return eapi not in ("0", "1", "2")
> 
> i would use "eapi_supports_xz_archives" unless there's a pre-existing
> style -mike

Good idea, I'll take these last ones into account in the next version.

Thank you everyone for the reviews so far.

-- 
With kind regards,

Tom Wijsman (TomWij)
Gentoo Developer

E-mail address  : tom...@gentoo.org
GPG Public Key  : 6D34E57D
GPG Fingerprint : C165 AF18 AB4C 400B C3D2  ABF0 95B2 1FCD 6D34 E57D


signature.asc
Description: PGP signature

Re: [gentoo-portage-dev] [PATCH 2/3] pym/portage/package/ebuild/fetch.py: Factor out _get_fetch_resume_size

[Tom Wijsman]

On Sun, 19 Jan 2014 18:01:23 -0800
"W. Trevor King"  wrote:

> On Mon, Jan 20, 2014 at 02:41:41AM +0100, Tom Wijsman wrote:
> > There is some duplicate code here, I think the conditions can be
> > rewritten in such way that the duplicate code doesn't take place.
> 
> Do you want a rewrite squashed into this commit, or as a follow-on
> commit after this one (which gets a test suite in v3)?

Sound more sane to do in a follow-up commit.

While writing this review I didn't note that you were just moving most
code, now you have ideas for further refactoring I guess. :)

-- 
With kind regards,

Tom Wijsman (TomWij)
Gentoo Developer

E-mail address  : tom...@gentoo.org
GPG Public Key  : 6D34E57D
GPG Fingerprint : C165 AF18 AB4C 400B C3D2  ABF0 95B2 1FCD 6D34 E57D


signature.asc
Description: PGP signature

Re: [gentoo-portage-dev] Signing off patches

[W. Trevor King]

On Mon, Jan 20, 2014 at 03:09:14AM +0100, Tom Wijsman wrote:
> On Sat, 18 Jan 2014 20:15:57 -0800 W. Trevor King wrote:
> > On Sun, Jan 19, 2014 at 02:33:06AM +0100, Tom Wijsman wrote:
> > > On Sat, 18 Jan 2014 15:24:59 -0800 W. Trevor King wrote:
> > > > If it doesn't need to get updated, then it probably already
> > > > started out explaining the consensus ;).
> > > 
> > > That is a guess, you can look this up in past patches.
> > 
> > Sure.  Will you?  If I want to touch some code, and it looks
> > confusing, I'll use blame to see who wrote it and whay they were
> > thinking about.  If the commit message is not informative, I usually
> > give up.  I have a hard time imaging folks tracking down the thread
> > that spawned that patch, assuming such a thread even exists, for each
> > troublesome line they'd like to touch.  It's much easier to summarize
> > any issues the list resolved (because they're likely the same
> > questions the new dev is asking) in the commit message, where future
> > developers can find them.
> 
> How does this make the consensus written after the patch part of it?
> 
> The person whom commits can be different than the person whom wrote the
> patch; and hence, that person commits without writing down consensus.
> If that person were to write it down, it would change the authorship.

If the initial submission does not express the consensus, you can
either ask for a resubmission that does, or say “Alice, is it ok if I
change your commit message to read ‘…’? when I commit it?”.

> Hence you made a guess, because I see pushed commits without
> consensus.

No policy/suggestion/goal is going to be followed 100% of the time.
If most commits, especially those that were contentious enough to go
through a few rounds of revision, have a commit message with a good
summary, then the future developer using blame has good odds of
finding something useful.  I think that's a good goal to shoot for,
even if you don't hit it all the time.

Even if you only consider the present, improving your commit message
to address tricky implementation details or unclear motivation before
submitting the next revision on a patch series will help reviewers
understand your patch better in the first place.

> Here I like vapier's approach from the other reply in this sub
> thread, that is to add it whenever people make the effort of
> providing the tag; which is an approach the Linux kernel upstream
> takes as well, if you want to be seen as a contributor you need to
> provide the tags.

Sounds fair to me.

Cheers,
Trevor

-- 
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy


signature.asc
Description: OpenPGP digital signature

Re: [gentoo-portage-dev] [PATCH 1/3 v2] Have repoman check if the packages to unpack rare archive formats from SRC_URI are present in DEPEND (bug #205909).

[Alec Warner]

On Sun, Jan 19, 2014 at 6:23 PM, Tom Wijsman  wrote:

> On Sun, 19 Jan 2014 04:38:31 -0500
> Mike Frysinger  wrote:
>
> > On Friday 17 January 2014 18:03:57 Tom Wijsman wrote:
> > > ---
> >
> > please shorten your commit summary and move more content to the body
>
> Right, I'm used to writing this on the command line; I'll try to look
> into setting up an editor or a GUI (gitg, ...) to do my commit messages.
>
> > > +getNonSystemArchiveDepends.archivers = {
> >
> > it is super weird to attach to the object like this.  some might even
> > say wrong.
>
> It a function, this makes it a static function variable.
>
> What is weird here? It works properly, what would be wrong?
>

It is certainly weird (as we discussed on IRC.) I've never seen anyone do
it in any codebase I liked.

One of the problems is that it isn't immutable, so that earlier callers can
mess with later callers. That is not possible in vapier's proposal, as the
attributes are hidden in the function code and are not visible to callers.

> move it into the class definition.
> > def getNonSystemArchiveDepends(fetchlist, eapi):
> >   ...
> >
> >   ARCHIVERS = {
> >   ...
> >   }
>
> That makes it a non-static function variable? This is a regression.
>

I guess I am not seeing why it must be a static function variable. Can you
explain?


>
> > > +   re.compile('.*\.7[zZ]$'):"app-arch/p7zip",
> >
> > regexes should always use raw strings.  there should also be a space
> > after the colon in dicts.  so you want:
> >
> >   re.compile(r'.*\.7[zZ]$'): 'app-arch/p7zip',
>
> Raw strings are a solution when \ forms a problem, which is not the
> case here. The colon has no space after it in the rest of repoman near
> it, hence I left it out; is there a coding standard we're trying to
> respect here? Can you refer it to me?
>

For the colon's in dicts thing:

http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Whitespace


>
> > > +   re.compile('.*\.lzma$'):"app-arch/lzma-utils",
> > xz-utils, not lzma-utils
>
> http://dev.gentoo.org/~ulm/pms/5/pms.html
>
> Please see "unpack" in PMS "11.3.3.13 Misc Commands", quote:
>
> "lzma-compressed files (*.lzma). Ebuilds must ensure that LZMA Utils is
> installed."
>
> The TODO comment that had a PMS reference was near it before, but with
> the refactor it has since moved; I'll put back the reference to it.
>
> > > +   re.compile('.*\.(bz2?|tbz2)$'):"app-arch/bzip2",
> > > +
> > > re.compile('.*\.(tar(\.(bz2?|gz|Z))?|tbz2|t[bg]z)?$'):"app-arch/tar",
> > > +   re.compile('.*\.(gz|tar\.Z|t[bg]z|[zZ])$'):"app-arch/gzip",
> > > +   re.compile('.*\.tar.xz$'):"app-arch/tar",
> >
> > requiring people list gzip, tar, and bzip2 is a significant policy
> > change (which i'm inclined to say is wrong).  it needs discussion on
> > the dev mailing list first.
>
> Please see "unpack" in PMS "11.3.3.13 Misc Commands", example quote:
>
> "gzip-compressed tar files (*.tar.gz, *.tgz, *.tar.Z, *.tbz). Ebuilds
> must ensure that GNU gzip and GNU tar are installed."
>
> To spare on mail length, I don't quote the rest of that enumeration.
>

The @system set in gentoo will ensure these are installed. I understand the
wording of PMS (as the dependencies should be expressed somewhere) but in
general we prefer to do that in @system. For the same reason all packages
do not depend on glibc, or the compiler, or anything else.


>
> > > +def checkArchiveDepends(atoms, catpkg, relative_path, \
> > > +   system_set_atoms, needed_unpack_depends, stats, fails):
> >
> > you don't need the \ there because you have paren already to gather
> > things.
> >
> > > +   for entry in needed_unpack_depends[catpkg]:
> > > +   if entry not in [atom.cp for atom in atoms if
> > > atom != "||"]:
> > > +   stats['unpack.' + mytype + '.missing'] += 1
> > > +   fails['unpack.' + mytype +
> > > '.missing'].append( \
> > > +   relative_path + ": %s is missing
> > > in %s" % \
> > > +   (entry, mytype))
> >
> > i know existing style doesn't follow it, but imo we should avoid
> > string concatenation.  it makes the code harder to read imo.
> >
> >   key = 'unpack.%s.missing' % mytype
> >   stats[key] += 1
> >   fails[key].append(...)
> >
> > > +def eapi_has_xz_utils(eapi):
> > > +   return eapi not in ("0", "1", "2")
> >
> > i would use "eapi_supports_xz_archives" unless there's a pre-existing
> > style -mike
>
> Good idea, I'll take these last ones into account in the next version.
>
> Thank you everyone for the reviews so far.
>
> --
> With kind regards,
>
> Tom Wijsman (TomWij)
> Gentoo Developer
>
> E-mail address  : tom...@gentoo.org
> GPG Public Key  : 6D34E57D
> GPG Fingerprint : C165 AF18 AB4C 400B C3D2  ABF0 95B2 1FCD 6D34 E57D
>

[gentoo-portage-dev] [PATCH v3 1/4] pym/portage/package/ebuild/fetch.py: Factor out _get_checksum_failure_max_tries

[W. Trevor King]

The current fetch() function is quite long, which makes it hard to
know what I can change without adverse side effects.  By pulling this
logic out of the main function, we get clearer logic in fetch() and
more explicit input for the config extraction.

Following a suggestion by Tom Wijsman, I put the setting name in a new
'key' variable to cut down on the PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS
noise.
---
 pym/portage/package/ebuild/fetch.py| 61 --
 pym/portage/tests/ebuild/test_fetch.py | 45 +
 2 files changed, 81 insertions(+), 25 deletions(-)
 create mode 100644 pym/portage/tests/ebuild/test_fetch.py

diff --git a/pym/portage/package/ebuild/fetch.py 
b/pym/portage/package/ebuild/fetch.py
index 5316f03..911500a 100644
--- a/pym/portage/package/ebuild/fetch.py
+++ b/pym/portage/package/ebuild/fetch.py
@@ -240,6 +240,40 @@ _size_suffix_map = {
'Y' : 80,
 }
 
+
+def _get_checksum_failure_max_tries(settings, default=5):
+   """
+   Get the maximum number of failed download attempts.
+
+   Generally, downloading the same file repeatedly from
+   every single available mirror is a waste of bandwidth
+   and time, so there needs to be a cap.
+   """
+   key = 'PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS'
+   v = default
+   try:
+   v = int(settings.get(key, default))
+   except (ValueError, OverflowError):
+   writemsg(_("!!! Variable %s contains "
+   "non-integer value: '%s'\n")
+   % (key, settings[key]),
+   noiselevel=-1)
+   writemsg(_("!!! Using %s default value: %s\n")
+   % (key, default),
+   noiselevel=-1)
+   v = default
+   if v < 1:
+   writemsg(_("!!! Variable %s contains "
+   "value less than 1: '%s'\n")
+   % (key, v),
+   noiselevel=-1)
+   writemsg(_("!!! Using %s default value: %s\n")
+   % (key, default),
+   noiselevel=-1)
+   v = default
+   return v
+
+
 def fetch(myuris, mysettings, listonly=0, fetchonly=0,
locks_in_subdir=".locks", use_locks=1, try_mirrors=1, digests=None,
allow_missing_digests=True):
@@ -263,31 +297,8 @@ def fetch(myuris, mysettings, listonly=0, fetchonly=0,
print(_(">>> \"mirror\" mode desired and \"mirror\" 
restriction found; skipping fetch."))
return 1
 
-   # Generally, downloading the same file repeatedly from
-   # every single available mirror is a waste of bandwidth
-   # and time, so there needs to be a cap.
-   checksum_failure_max_tries = 5
-   v = checksum_failure_max_tries
-   try:
-   v = int(mysettings.get("PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS",
-   checksum_failure_max_tries))
-   except (ValueError, OverflowError):
-   writemsg(_("!!! Variable PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS"
-   " contains non-integer value: '%s'\n") % \
-   mysettings["PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS"], 
noiselevel=-1)
-   writemsg(_("!!! Using PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS "
-   "default value: %s\n") % checksum_failure_max_tries,
-   noiselevel=-1)
-   v = checksum_failure_max_tries
-   if v < 1:
-   writemsg(_("!!! Variable PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS"
-   " contains value less than 1: '%s'\n") % v, 
noiselevel=-1)
-   writemsg(_("!!! Using PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS "
-   "default value: %s\n") % checksum_failure_max_tries,
-   noiselevel=-1)
-   v = checksum_failure_max_tries
-   checksum_failure_max_tries = v
-   del v
+   checksum_failure_max_tries = _get_checksum_failure_max_tries(
+   settings=mysettings)
 
fetch_resume_size_default = "350K"
fetch_resume_size = mysettings.get("PORTAGE_FETCH_RESUME_MIN_SIZE")
diff --git a/pym/portage/tests/ebuild/test_fetch.py 
b/pym/portage/tests/ebuild/test_fetch.py
new file mode 100644
index 000..26e0349
--- /dev/null
+++ b/pym/portage/tests/ebuild/test_fetch.py
@@ -0,0 +1,45 @@
+# Copyright 1998-2013 Gentoo Foundation
+# Distributed under the terms of the GNU General Public License v2
+
+from portage.package.ebuild.fetch import (
+   _get_checksum_failure_max_tries,
+   )
+from portage.tests import TestCase
+
+
+class FetchTestCase(TestCase):
+   """
+   Test fetch and it's helper functions.
+
+   The fetch function, as it stands, is too complicated to test
+   on its own.  However, the new helper functions are much more
+   limited and easier to test.  Despite these tests, the helper
+   functions are internal implementatio

[gentoo-portage-dev] [PATCH v3 0/4] Initial fetch() refactoring

[W. Trevor King]

Changes since v2:

* Use "" for one-line docstrings.
* Terminate docstring summaries with periods.
* Mention 'netloc' in _expand_mirror docstring.
* Uppercase URI in docstrings.
* Add a 'key' variable to cut down on setting-name noise.
* Remove parens and line extensions (\) from % formatting.
* Use 'expanded_uris' instead of 'uris' for _expand_mirror return
  value.
* Additional line wrapping to stay under 80 chars.
* New test_fetch.py test suite for the helper functions.
* New patch #4 that refactors _get_fetch_resume_size.

Not changed since v2:

* Default arguments for _get_uris [1], but I've added an explicit
  defense in my commit message.
* Use of settings in _get_uris [2], but I've added an explicit defense
  in my commit message.
* Streamlining settings extraction [2], which I'm kicking down the
  road.  I think Portage should just use ConfigParser for settings,
  but that is clearly outside the scope of this series.

Cheers,
Trevor

[1]: http://thread.gmane.org/gmane.linux.gentoo.portage.devel/4002/focus=4041
[2]: http://thread.gmane.org/gmane.linux.gentoo.portage.devel/4002/focus=4042

W. Trevor King (4):
  pym/portage/package/ebuild/fetch.py: Factor out
_get_checksum_failure_max_tries
  pym/portage/package/ebuild/fetch.py: Factor out _get_fetch_resume_size
  pym/portage/package/ebuild/fetch.py: Factor out _get_uris
  pym/portage/package/ebuild/fetch.py: Flatten conditionals in
_get_fetch_resume_size

 pym/portage/package/ebuild/fetch.py| 318 -
 pym/portage/tests/ebuild/test_fetch.py | 203 +
 2 files changed, 392 insertions(+), 129 deletions(-)
 create mode 100644 pym/portage/tests/ebuild/test_fetch.py

-- 
1.8.5.2.8.g0f6c0d1

[gentoo-portage-dev] [PATCH v3 2/4] pym/portage/package/ebuild/fetch.py: Factor out _get_fetch_resume_size

[W. Trevor King]

The current fetch() function is quite long, which makes it hard to
know what I can change without adverse side effects.  By pulling this
logic out of the main function, we get clearer logic in fetch() and
more explicit input for the config extraction.

Following a suggestion by Tom Wijsman, I put the setting name in a new
'key' variable to cut down on the PORTAGE_FETCH_RESUME_MIN_SIZE noise.
---
 pym/portage/package/ebuild/fetch.py| 51 +++---
 pym/portage/tests/ebuild/test_fetch.py | 37 
 2 files changed, 65 insertions(+), 23 deletions(-)

diff --git a/pym/portage/package/ebuild/fetch.py 
b/pym/portage/package/ebuild/fetch.py
index 911500a..4ecefc9 100644
--- a/pym/portage/package/ebuild/fetch.py
+++ b/pym/portage/package/ebuild/fetch.py
@@ -274,6 +274,33 @@ def _get_checksum_failure_max_tries(settings, default=5):
return v
 
 
+def _get_fetch_resume_size(settings, default='350K'):
+   key = 'PORTAGE_FETCH_RESUME_MIN_SIZE'
+   v = settings.get(key)
+   if v is not None:
+   v = "".join(v.split())
+   if not v:
+   # If it's empty, silently use the default.
+   v = default
+   match = _fetch_resume_size_re.match(v)
+   if (match is None or
+   match.group(2).upper() not in _size_suffix_map):
+   writemsg(_("!!! Variable %s contains an "
+   "unrecognized format: '%s'\n")
+   % (key, settings[key]),
+   noiselevel=-1)
+   writemsg(_("!!! Using %s default value: %s\n")
+   % (key, default),
+   noiselevel=-1)
+   v = None
+   if v is None:
+   v = default
+   match = _fetch_resume_size_re.match(v)
+   v = int(match.group(1)) * \
+   2 ** _size_suffix_map[match.group(2).upper()]
+   return v
+
+
 def fetch(myuris, mysettings, listonly=0, fetchonly=0,
locks_in_subdir=".locks", use_locks=1, try_mirrors=1, digests=None,
allow_missing_digests=True):
@@ -299,29 +326,7 @@ def fetch(myuris, mysettings, listonly=0, fetchonly=0,
 
checksum_failure_max_tries = _get_checksum_failure_max_tries(
settings=mysettings)
-
-   fetch_resume_size_default = "350K"
-   fetch_resume_size = mysettings.get("PORTAGE_FETCH_RESUME_MIN_SIZE")
-   if fetch_resume_size is not None:
-   fetch_resume_size = "".join(fetch_resume_size.split())
-   if not fetch_resume_size:
-   # If it's undefined or empty, silently use the default.
-   fetch_resume_size = fetch_resume_size_default
-   match = _fetch_resume_size_re.match(fetch_resume_size)
-   if match is None or \
-   (match.group(2).upper() not in _size_suffix_map):
-   writemsg(_("!!! Variable PORTAGE_FETCH_RESUME_MIN_SIZE"
-   " contains an unrecognized format: '%s'\n") % \
-   mysettings["PORTAGE_FETCH_RESUME_MIN_SIZE"], 
noiselevel=-1)
-   writemsg(_("!!! Using PORTAGE_FETCH_RESUME_MIN_SIZE "
-   "default value: %s\n") % 
fetch_resume_size_default,
-   noiselevel=-1)
-   fetch_resume_size = None
-   if fetch_resume_size is None:
-   fetch_resume_size = fetch_resume_size_default
-   match = _fetch_resume_size_re.match(fetch_resume_size)
-   fetch_resume_size = int(match.group(1)) * \
-   2 ** _size_suffix_map[match.group(2).upper()]
+   fetch_resume_size = _get_fetch_resume_size(settings=mysettings)
 
# Behave like the package has RESTRICT="primaryuri" after a
# couple of checksum failures, to increase the probablility
diff --git a/pym/portage/tests/ebuild/test_fetch.py 
b/pym/portage/tests/ebuild/test_fetch.py
index 26e0349..2b06190 100644
--- a/pym/portage/tests/ebuild/test_fetch.py
+++ b/pym/portage/tests/ebuild/test_fetch.py
@@ -3,6 +3,7 @@
 
 from portage.package.ebuild.fetch import (
_get_checksum_failure_max_tries,
+   _get_fetch_resume_size,
)
 from portage.tests import TestCase
 
@@ -43,3 +44,39 @@ class FetchTestCase(TestCase):
_get_checksum_failure_max_tries(
settings={}, default=3),
3)
+
+   def test_get_fetch_resume_size(self):
+   self.assertEqual(
+   _get_fetch_resume_size(settings={}),
+   358400)
+   self.assertEqual(
+   _get_fetch_resume_size(settings={
+   'PORTAGE_FETCH_RESUME_MIN_SIZE': ''}),
+   358400)
+   

[gentoo-portage-dev] [PATCH v3 3/4] pym/portage/package/ebuild/fetch.py: Factor out _get_uris

[W. Trevor King]

The current fetch() function is quite long, which makes it hard to
know what I can change without adverse side effects.  By pulling this
logic out of the main function, we get clearer logic in fetch() and
more explicit input for the config extraction.

This block was especially complicated, so I also created the helper
functions _get_file_uri_tuples and _expand_mirror.  I'd prefer if
_expand_mirror iterated through URIs instead of (group, URI) tuples,
but we need a distinct marker for third-party URIs to build
third_party_mirror_uris which is used to build primaryuri_dict which
is used way down in fetch():

  if checksum_failure_count == \
  checksum_failure_primaryuri:
  # Switch to "primaryuri" mode in order
  # to increase the probablility of
  # of success.
  primaryuris = \
  primaryuri_dict.get(myfile)
  if primaryuris:
  uri_list.extend(
  reversed(primaryuris))

I don't know if this is useful enough to motivate the uglier
_expandmirror return values, but I'll kick that can down the road for
now.

There was some discussion on the list [1] about the defaults in:

  def _get_uris(uris, settings, custom_mirrors=(), locations=()):

but I prefer this to Alec's floated:

  def _get_uris(uris, settings, custom_mirrors=None, locations=None):
  if not custom_mirrors:
  custom_mirrors = ()
  if not locations:
  locations = ()

Which accomplishes the same thing with less clarity ;).  My default
values are tuples (and not mutable lists) to make it extra-obvious
that we're relying on anything crazy like mutating our default values
;).

There was also discussion about whether the ugly settings object
should be passed to _get_uris, or whether the appropriate settings
should be extracted first and then passed through [2].  I've passed
settings through, because I'd prefer to have the uglyness handled in
helper functions.  The fetch() function is what most folks will care
about, so we want to keep that as clean as possible.  If the helper
functions have to suffer a bit for a cleaner fetch(), so be it.

[1]: http://thread.gmane.org/gmane.linux.gentoo.portage.devel/4002/focus=4041
[2]: http://thread.gmane.org/gmane.linux.gentoo.portage.devel/4002/focus=4042
---
 pym/portage/package/ebuild/fetch.py| 208 -
 pym/portage/tests/ebuild/test_fetch.py | 121 +++
 2 files changed, 248 insertions(+), 81 deletions(-)

diff --git a/pym/portage/package/ebuild/fetch.py 
b/pym/portage/package/ebuild/fetch.py
index 4ecefc9..0093a6e 100644
--- a/pym/portage/package/ebuild/fetch.py
+++ b/pym/portage/package/ebuild/fetch.py
@@ -15,9 +15,9 @@ import sys
 import tempfile
 
 try:
-   from urllib.parse import urlparse
+   from urllib.parse import urlparse, urlunparse
 except ImportError:
-   from urlparse import urlparse
+   from urlparse import urlparse, urlunparse
 
 import portage
 portage.proxy.lazyimport.lazyimport(globals(),
@@ -301,6 +301,128 @@ def _get_fetch_resume_size(settings, default='350K'):
return v
 
 
+def _get_file_uri_tuples(uris):
+   """Return a list of (filename, URI) tuples."""
+   file_uri_tuples = []
+   # Check for 'items' attribute since OrderedDict is not a dict.
+   if hasattr(uris, 'items'):
+   for filename, uri_set in uris.items():
+   for uri in uri_set:
+   file_uri_tuples.append((filename, uri))
+   if not uri_set:
+   file_uri_tuples.append((filename, None))
+   else:
+   for uri in uris:
+   if urlparse(uri).scheme:
+   file_uri_tuples.append(
+   (os.path.basename(uri), uri))
+   else:
+   file_uri_tuples.append(
+   (os.path.basename(uri), None))
+   return file_uri_tuples
+
+
+def _expand_mirror(uri, custom_mirrors=(), third_party_mirrors=()):
+   """
+   Replace the 'mirror://' scheme and netloc in the URI.
+
+   Returns an iterable listing expanded (group, URI) tuples,
+   where the group is either 'custom' or 'third-party'.
+   """
+   parsed = urlparse(uri)
+   mirror = parsed.netloc
+   path = parsed.path
+   if path:
+   # Try user-defined mirrors first
+   if mirror in custom_mirrors:
+   for cmirr in custom_mirrors[mirror]:
+   m_uri = urlparse(cmirr)
+   yield ('custom', urlunparse((
+   m_uri.scheme, m_uri.netloc, path) +
+   parsed[3:]))
+
+   # now try the official mirrors
+   if mirror in third_party_mirrors:
+   uris = []
+   for locmirr in third_party_mir

[gentoo-portage-dev] [PATCH v3 4/4] pym/portage/package/ebuild/fetch.py: Flatten conditionals in _get_fetch_resume_size

[W. Trevor King]

Make this easier to read by avoiding nested conditionals [1].

[1]: http://article.gmane.org/gmane.linux.gentoo.portage.devel/4058

Reported-by: Tom Wijsman 
---
 pym/portage/package/ebuild/fetch.py | 30 ++
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/pym/portage/package/ebuild/fetch.py 
b/pym/portage/package/ebuild/fetch.py
index 0093a6e..2bf88d8 100644
--- a/pym/portage/package/ebuild/fetch.py
+++ b/pym/portage/package/ebuild/fetch.py
@@ -276,24 +276,22 @@ def _get_checksum_failure_max_tries(settings, default=5):
 
 def _get_fetch_resume_size(settings, default='350K'):
key = 'PORTAGE_FETCH_RESUME_MIN_SIZE'
-   v = settings.get(key)
+   v = settings.get(key, default)
if v is not None:
v = "".join(v.split())
-   if not v:
-   # If it's empty, silently use the default.
-   v = default
-   match = _fetch_resume_size_re.match(v)
-   if (match is None or
-   match.group(2).upper() not in _size_suffix_map):
-   writemsg(_("!!! Variable %s contains an "
-   "unrecognized format: '%s'\n")
-   % (key, settings[key]),
-   noiselevel=-1)
-   writemsg(_("!!! Using %s default value: %s\n")
-   % (key, default),
-   noiselevel=-1)
-   v = None
-   if v is None:
+   if not v:
+   # If it's empty, silently use the default.
+   v = default
+   match = _fetch_resume_size_re.match(v)
+   if (match is None or
+   match.group(2).upper() not in _size_suffix_map):
+   writemsg(_("!!! Variable %s contains "
+   "an unrecognized format: '%s'\n")
+   % (key, settings[key]),
+   noiselevel=-1)
+   writemsg(_("!!! Using %s default value: %s\n")
+   % (key, default),
+   noiselevel=-1)
v = default
match = _fetch_resume_size_re.match(v)
v = int(match.group(1)) * \
-- 
1.8.5.2.8.g0f6c0d1

Re: [gentoo-portage-dev] [PATCH 2/3] emerge: Rename --autounmask-write to --autounmask

[Mike Frysinger]

On Sunday 19 January 2014 05:48:33 Alexander Berntsen wrote:
> On 19/01/14 10:20, Mike Frysinger wrote:
> > typically when we delete/rename an option, we give users a heads
> > up. that means a cycle where the old option exists but emits a
> > warning before switching to the new one.  then we can delete it.
> 
> We should have a news item.

i don't think so.  i doubt this flag is in common use enough to warrant 
spamming every user of Gentoo.  especially when the people who do use it can 
easily be migrated per my recommendation.  similarly, people who do use this 
flag but miss the news would be broken.

generally speaking, i think it makes a lot more sense to put the notice at the 
point where it matters.
-mike


signature.asc
Description: This is a digitally signed message part.

Re: [gentoo-portage-dev] [PATCH] Add repoman check to warn if src_prepare/src_configure are used in EAPI 0/1

[Mike Frysinger]

On Sunday 19 January 2014 11:59:36 Mike Gilbert wrote:
> On Sun, Jan 19, 2014 at 4:44 AM, Mike Frysinger  wrote:
> > Chromium OS for a long time was restricted to EAPI 4 for two reasons --
> > it had an old portage version (and upgrading to a newer one regressed
> > performance significantly, so we held off until we could figure out why)
> 
> I am curious to know more about the performance regression if you can
> share. Is that something that got fixed, or did you disable some
> features (like the slot-operator stuff)?

we finally tracked it down (was due to new the new FEATURES=merge-sync option.  
when you're installing 200 to 500 binary packages (with like 32 in parallel), 
that can easily choke your throughput.  some systems saw really excessive 
latencies (which i would guess was due to their drive taking longer to process 
dirty blocks).  but it took us some time to figure that out as we were making a 
large version jump and we didn't have too much time to dedicate to tracking it 
down.  lots o bugs to fix.
-mike


signature.asc
Description: This is a digitally signed message part.

Re: [gentoo-portage-dev] [PATCH] Add repoman check to warn if src_prepare/src_configure are used in EAPI 0/1

[Alec Warner]

On Sun, Jan 19, 2014 at 9:47 PM, Mike Frysinger  wrote:

> On Sunday 19 January 2014 11:59:36 Mike Gilbert wrote:
> > On Sun, Jan 19, 2014 at 4:44 AM, Mike Frysinger 
> wrote:
> > > Chromium OS for a long time was restricted to EAPI 4 for two reasons --
> > > it had an old portage version (and upgrading to a newer one regressed
> > > performance significantly, so we held off until we could figure out
> why)
> >
> > I am curious to know more about the performance regression if you can
> > share. Is that something that got fixed, or did you disable some
> > features (like the slot-operator stuff)?
>
> we finally tracked it down (was due to new the new FEATURES=merge-sync
> option.
> when you're installing 200 to 500 binary packages (with like 32 in
> parallel),
> that can easily choke your throughput.  some systems saw really excessive
> latencies (which i would guess was due to their drive taking longer to
> process
> dirty blocks).  but it took us some time to figure that out as we were
> making a
> large version jump and we didn't have too much time to dedicate to
> tracking it
> down.  lots o bugs to fix.
> -mike
>

Are you still doing crazy crap like disabling all of the portage locking? ;p

-A

Re: [gentoo-portage-dev] [PATCH] Add repoman check to warn if src_prepare/src_configure are used in EAPI 0/1

[Mike Frysinger]

On Monday 20 January 2014 01:02:31 Alec Warner wrote:
> Are you still doing crazy crap like disabling all of the portage locking?
> ;p

yes & no.  we make sure we hold the right locks and don't do operations that 
otherwise cause problems.  then we disable the core lock grabbing :P.

the current locking logic is pessimistic and causes way too much of a slow 
down for us to do anything else.
-mike


signature.asc
Description: This is a digitally signed message part.