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

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

[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 ;).

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.

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

-- 
1.8.5.2.8.g0f6c0d1

[gentoo-portage-dev] [PATCH 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 | 58 +++++++++++++++++++++----------------
 1 file changed, 33 insertions(+), 25 deletions(-)

diff --git a/pym/portage/package/ebuild/fetch.py b/pym/portage/package/ebuild/fetch.py
index 5316f03..4337247 100644
--- a/pym/portage/package/ebuild/fetch.py
+++ b/pym/portage/package/ebuild/fetch.py
@@ -240,6 +240,37 @@ _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 +294,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

[gentoo-portage-dev] [PATCH 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 4337247..bd572fa 100644
--- a/pym/portage/package/ebuild/fetch.py
+++ b/pym/portage/package/ebuild/fetch.py
@@ -271,6 +271,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):
@@ -296,29 +322,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 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 | 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
+	"""
+	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
+
+	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)
+
+
+def _get_uris(uris, settings, custom_mirrors=(), locations=()):
+	third_party_mirrors = settings.thirdpartymirrors()
+	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(
+				uri=uri, custom_mirrors=custom_mirrors,
+				third_party_mirrors=third_party_mirrors)
+			filedict[filename].extend(uri for group, uri in uris)
+			third_party_mirror_uris.setdefault(filename, []).extend(
+				uri for group, uri in uris
+				if group == 'third-party')
+			if not filedict[filename]:
+				writemsg(
+					_("No known mirror by the name: %s\n")
+					% (mirror,))
+		else:
+			if restrict_fetch or force_mirror:
+				# Only fetch from specific mirrors is allowed.
+				continue
+			primaryuris = primaryuri_dict.get(filename)
+			if primaryuris is None:
+				primaryuris = []
+				primaryuri_dict[filename] = primaryuris
+			primaryuris.append(uri)
+
+	# Order primaryuri_dict values to match that in SRC_URI.
+	for uris in primaryuri_dict.values():
+		uris.reverse()
+
+	# Prefer third_party_mirrors over normal mirrors in cases when
+	# the file does not yet exist on the normal mirrors.
+	for filename, uris in third_party_mirror_uris.items():
+		primaryuri_dict.setdefault(filename, []).extend(uris)
+
+	# Now merge primaryuri values into filedict (includes mirrors
+	# explicitly referenced in SRC_URI).
+	if "primaryuri" in restrict:
+		for filename, uris in filedict.items():
+			filedict[filename] = primaryuri_dict.get(filename, []) + uris
+	else:
+		for filename in filedict:
+			filedict[filename] += primaryuri_dict.get(filename, [])
+
+	return filedict, primaryuri_dict
+
+
 def fetch(myuris, mysettings, listonly=0, fetchonly=0,
 	locks_in_subdir=".locks", use_locks=1, try_mirrors=1, digests=None,
 	allow_missing_digests=True):
@@ -328,7 +440,6 @@ def fetch(myuris, mysettings, listonly=0, fetchonly=0,
 	# couple of checksum failures, to increase the probablility
 	# of success before checksum_failure_max_tries is reached.
 	checksum_failure_primaryuri = 2
-	thirdpartymirrors = mysettings.thirdpartymirrors()
 
 	# In the background parallel-fetch process, it's safe to skip checksum
 	# verification of pre-existing files in $DISTDIR that have the correct
@@ -412,83 +523,9 @@ def fetch(myuris, mysettings, listonly=0, fetchonly=0,
 	else:
 		locations = mymirrors
 
-	file_uri_tuples = []
-	# Check for 'items' attribute since OrderedDict is not a dict.
-	if hasattr(myuris, 'items'):
-		for myfile, uri_set in myuris.items():
-			for myuri in uri_set:
-				file_uri_tuples.append((myfile, myuri))
-			if not uri_set:
-				file_uri_tuples.append((myfile, None))
-	else:
-		for myuri in myuris:
-			if urlparse(myuri).scheme:
-				file_uri_tuples.append((os.path.basename(myuri), myuri))
-			else:
-				file_uri_tuples.append((os.path.basename(myuri), None))
-
-	filedict = OrderedDict()
-	primaryuri_dict = {}
-	thirdpartymirror_uris = {}
-	for myfile, myuri in file_uri_tuples:
-		if myfile not in filedict:
-			filedict[myfile]=[]
-			for y in range(0,len(locations)):
-				filedict[myfile].append(locations[y]+"/distfiles/"+myfile)
-		if myuri is None:
-			continue
-		if myuri[:9]=="mirror://":
-			eidx = myuri.find("/", 9)
-			if eidx != -1:
-				mirrorname = myuri[9:eidx]
-				path = myuri[eidx+1:]
-
-				# Try user-defined mirrors first
-				if mirrorname in custommirrors:
-					for cmirr in custommirrors[mirrorname]:
-						filedict[myfile].append(
-							cmirr.rstrip("/") + "/" + path)
-
-				# now try the official mirrors
-				if mirrorname in thirdpartymirrors:
-					uris = [locmirr.rstrip("/") + "/" + path \
-						for locmirr in thirdpartymirrors[mirrorname]]
-					random.shuffle(uris)
-					filedict[myfile].extend(uris)
-					thirdpartymirror_uris.setdefault(myfile, []).extend(uris)
-
-				if not filedict[myfile]:
-					writemsg(_("No known mirror by the name: %s\n") % (mirrorname))
-			else:
-				writemsg(_("Invalid mirror definition in SRC_URI:\n"), noiselevel=-1)
-				writemsg("  %s\n" % (myuri), noiselevel=-1)
-		else:
-			if restrict_fetch or force_mirror:
-				# Only fetch from specific mirrors is allowed.
-				continue
-			primaryuris = primaryuri_dict.get(myfile)
-			if primaryuris is None:
-				primaryuris = []
-				primaryuri_dict[myfile] = primaryuris
-			primaryuris.append(myuri)
-
-	# Order primaryuri_dict values to match that in SRC_URI.
-	for uris in primaryuri_dict.values():
-		uris.reverse()
-
-	# Prefer thirdpartymirrors over normal mirrors in cases when
-	# the file does not yet exist on the normal mirrors.
-	for myfile, uris in thirdpartymirror_uris.items():
-		primaryuri_dict.setdefault(myfile, []).extend(uris)
-
-	# Now merge primaryuri values into filedict (includes mirrors
-	# explicitly referenced in SRC_URI).
-	if "primaryuri" in restrict:
-		for myfile, uris in filedict.items():
-			filedict[myfile] = primaryuri_dict.get(myfile, []) + uris
-	else:
-		for myfile in filedict:
-			filedict[myfile] += primaryuri_dict.get(myfile, [])
+	filedict, primaryuri_dict = _get_uris(
+		uris=myuris, settings=mysettings,
+		custom_mirrors=custom_mirrors, locations=locations)
 
 	can_fetch=True
 
-- 
1.8.5.2.8.g0f6c0d1

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]

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

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

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

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 3/3] pym/portage/package/ebuild/fetch.py: Factor out _get_uris

[W. Trevor King]

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

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

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

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

    """<summary>

  to:

    """
    <summary>

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

[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 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)
+
+	third_party_mirrors = settings.thirdpartymirrors()
+	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(
+				uri=uri, custom_mirrors=custom_mirrors,
+				third_party_mirrors=third_party_mirrors)
+			filedict[filename].extend(uri for group, uri in uris)
+			third_party_mirror_uris.setdefault(filename, []).extend(
+				uri for group, uri in uris
+				if group == 'third-party')
+		else:
+			if restrict_fetch or force_mirror:
+				# Only fetch from specific mirrors is allowed.
+				continue
+			primaryuris = primaryuri_dict.get(filename)
+			if primaryuris is None:
+				primaryuris = []
+				primaryuri_dict[filename] = primaryuris
+			primaryuris.append(uri)
+
+	# Order primaryuri_dict values to match that in SRC_URI.
+	for uris in primaryuri_dict.values():
+		uris.reverse()
+
+	# Prefer third_party_mirrors over normal mirrors in cases when
+	# the file does not yet exist on the normal mirrors.
+	for filename, uris in third_party_mirror_uris.items():
+		primaryuri_dict.setdefault(filename, []).extend(uris)
+
+	# Now merge primaryuri values into filedict (includes mirrors
+	# explicitly referenced in SRC_URI).
+	if "primaryuri" in restrict:
+		for filename, uris in filedict.items():
+			filedict[filename] = primaryuri_dict.get(filename, []) + uris
+	else:
+		for filename in filedict:
+			filedict[filename] += primaryuri_dict.get(filename, [])
+
+	return filedict, primaryuri_dict
+
+
 def fetch(myuris, mysettings, listonly=0, fetchonly=0,
 	locks_in_subdir=".locks", use_locks=1, try_mirrors=1, digests=None,
 	allow_missing_digests=True):
@@ -329,7 +452,6 @@ def fetch(myuris, mysettings, listonly=0, fetchonly=0,
 	# couple of checksum failures, to increase the probablility
 	# of success before checksum_failure_max_tries is reached.
 	checksum_failure_primaryuri = 2
-	thirdpartymirrors = mysettings.thirdpartymirrors()
 
 	# In the background parallel-fetch process, it's safe to skip checksum
 	# verification of pre-existing files in $DISTDIR that have the correct
@@ -402,7 +524,6 @@ def fetch(myuris, mysettings, listonly=0, fetchonly=0,
 			del mymirrors[x]
 
 	restrict_fetch = "fetch" in restrict
-	force_mirror = "force-mirror" in features and not restrict_mirror
 	custom_local_mirrors = custommirrors.get("local", [])
 	if restrict_fetch:
 		# With fetch restriction, a normal uri may only be fetched from
@@ -413,83 +534,9 @@ def fetch(myuris, mysettings, listonly=0, fetchonly=0,
 	else:
 		locations = mymirrors
 
-	file_uri_tuples = []
-	# Check for 'items' attribute since OrderedDict is not a dict.
-	if hasattr(myuris, 'items'):
-		for myfile, uri_set in myuris.items():
-			for myuri in uri_set:
-				file_uri_tuples.append((myfile, myuri))
-			if not uri_set:
-				file_uri_tuples.append((myfile, None))
-	else:
-		for myuri in myuris:
-			if urlparse(myuri).scheme:
-				file_uri_tuples.append((os.path.basename(myuri), myuri))
-			else:
-				file_uri_tuples.append((os.path.basename(myuri), None))
-
-	filedict = OrderedDict()
-	primaryuri_dict = {}
-	thirdpartymirror_uris = {}
-	for myfile, myuri in file_uri_tuples:
-		if myfile not in filedict:
-			filedict[myfile]=[]
-			for y in range(0,len(locations)):
-				filedict[myfile].append(locations[y]+"/distfiles/"+myfile)
-		if myuri is None:
-			continue
-		if myuri[:9]=="mirror://":
-			eidx = myuri.find("/", 9)
-			if eidx != -1:
-				mirrorname = myuri[9:eidx]
-				path = myuri[eidx+1:]
-
-				# Try user-defined mirrors first
-				if mirrorname in custommirrors:
-					for cmirr in custommirrors[mirrorname]:
-						filedict[myfile].append(
-							cmirr.rstrip("/") + "/" + path)
-
-				# now try the official mirrors
-				if mirrorname in thirdpartymirrors:
-					uris = [locmirr.rstrip("/") + "/" + path \
-						for locmirr in thirdpartymirrors[mirrorname]]
-					random.shuffle(uris)
-					filedict[myfile].extend(uris)
-					thirdpartymirror_uris.setdefault(myfile, []).extend(uris)
-
-				if not filedict[myfile]:
-					writemsg(_("No known mirror by the name: %s\n") % (mirrorname))
-			else:
-				writemsg(_("Invalid mirror definition in SRC_URI:\n"), noiselevel=-1)
-				writemsg("  %s\n" % (myuri), noiselevel=-1)
-		else:
-			if restrict_fetch or force_mirror:
-				# Only fetch from specific mirrors is allowed.
-				continue
-			primaryuris = primaryuri_dict.get(myfile)
-			if primaryuris is None:
-				primaryuris = []
-				primaryuri_dict[myfile] = primaryuris
-			primaryuris.append(myuri)
-
-	# Order primaryuri_dict values to match that in SRC_URI.
-	for uris in primaryuri_dict.values():
-		uris.reverse()
-
-	# Prefer thirdpartymirrors over normal mirrors in cases when
-	# the file does not yet exist on the normal mirrors.
-	for myfile, uris in thirdpartymirror_uris.items():
-		primaryuri_dict.setdefault(myfile, []).extend(uris)
-
-	# Now merge primaryuri values into filedict (includes mirrors
-	# explicitly referenced in SRC_URI).
-	if "primaryuri" in restrict:
-		for myfile, uris in filedict.items():
-			filedict[myfile] = primaryuri_dict.get(myfile, []) + uris
-	else:
-		for myfile in filedict:
-			filedict[myfile] += primaryuri_dict.get(myfile, [])
+	filedict, primaryuri_dict = _get_uris(
+		uris=myuris, settings=mysettings,
+		custom_mirrors=custommirrors, locations=locations)
 
 	can_fetch=True
 
-- 
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]

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

On Sat, Jan 18, 2014 at 7:07 PM, W. Trevor King <wking@tremily.us> 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 constructing
mutable objects in parameter defaults:

>>> def func(a=[]):
...   a.append(5)
...
>>> del func
>>> def func(a=[]):
...   a.append(5)
...   print a
...
>>> func()
[5]
>>> func()
[5, 5]
>>> func()
[5, 5, 5]
>>> func()
[5, 5, 5, 5]
>>> func()
[5, 5, 5, 5, 5]

That doesn't function as most would expect, because it turns out that 'a'
is constructed at function definition time (not call time) and is re-used
between function calls.

Now of course, your code is constructing tuples, which are immutable. That
is handy.

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 = ()

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

def_get_uris(uris, settings, custom_mirrors, locations):
...

and so they will be forced to create the empty iterables at the call site.

A brief discussion with Sebastian on irc seemed to indicate that he thought
creating immutable objects in this way was permissible (we certainly do it
often for strings) so I won't raise a fuss. I merely want to point out that
you really need to be aware of whether the objects are mutable or immutable.


> +       third_party_mirrors = settings.thirdpartymirrors()
>

Why pass in all of settings? If you only need settings.thirdpartymirrors,
then just ask for 'a thirdparty mirrors iterable' or whatever it is. You do
not want the entire settings object, it is full of disgusting globals, and
it will only screw you over later.

Think about testing this function. Do you really want to try to construct
some sort of 'testing' settings object, or simply construct a list?


> +       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'?

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?


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


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



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

Are these globals or am I missing somethng?


> +                               # Only fetch from specific mirrors is
> allowed.
> +                               continue
> +                       primaryuris = primaryuri_dict.get(filename)
> +                       if primaryuris is None:
> +                               primaryuris = []
> +                               primaryuri_dict[filename] = primaryuris
> +                       primaryuris.append(uri)
> +
> +       # 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()? 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?


> +
> +       # Prefer third_party_mirrors over normal mirrors in cases when
> +       # the file does not yet exist on the normal mirrors.
> +       for filename, uris in third_party_mirror_uris.items():
> +               primaryuri_dict.setdefault(filename, []).extend(uris)
> +
> +       # Now merge primaryuri values into filedict (includes mirrors
> +       # explicitly referenced in SRC_URI).
> +       if "primaryuri" in restrict:
>

same questoin here about where 'restrict' comes from.


> +               for filename, uris in filedict.items():
> +                       filedict[filename] = primaryuri_dict.get(filename,
> []) + uris
> +       else:
> +               for filename in filedict:
> +                       filedict[filename] +=
> primaryuri_dict.get(filename, [])
> +
> +       return filedict, primaryuri_dict
> +
> +
>  def fetch(myuris, mysettings, listonly=0, fetchonly=0,
>         locks_in_subdir=".locks", use_locks=1, try_mirrors=1, digests=None,
>         allow_missing_digests=True):
> @@ -328,7 +440,6 @@ def fetch(myuris, mysettings, listonly=0, fetchonly=0,
>         # couple of checksum failures, to increase the probablility
>         # of success before checksum_failure_max_tries is reached.
>         checksum_failure_primaryuri = 2
> -       thirdpartymirrors = mysettings.thirdpartymirrors()
>
>         # In the background parallel-fetch process, it's safe to skip
> checksum
>         # verification of pre-existing files in $DISTDIR that have the
> correct
> @@ -412,83 +523,9 @@ def fetch(myuris, mysettings, listonly=0, fetchonly=0,
>         else:
>                 locations = mymirrors
>
> -       file_uri_tuples = []
> -       # Check for 'items' attribute since OrderedDict is not a dict.
> -       if hasattr(myuris, 'items'):
> -               for myfile, uri_set in myuris.items():
> -                       for myuri in uri_set:
> -                               file_uri_tuples.append((myfile, myuri))
> -                       if not uri_set:
> -                               file_uri_tuples.append((myfile, None))
> -       else:
> -               for myuri in myuris:
> -                       if urlparse(myuri).scheme:
> -
> file_uri_tuples.append((os.path.basename(myuri), myuri))
> -                       else:
> -
> file_uri_tuples.append((os.path.basename(myuri), None))
> -
> -       filedict = OrderedDict()
> -       primaryuri_dict = {}
> -       thirdpartymirror_uris = {}
> -       for myfile, myuri in file_uri_tuples:
> -               if myfile not in filedict:
> -                       filedict[myfile]=[]
> -                       for y in range(0,len(locations)):
> -
> filedict[myfile].append(locations[y]+"/distfiles/"+myfile)
> -               if myuri is None:
> -                       continue
> -               if myuri[:9]=="mirror://":
> -                       eidx = myuri.find("/", 9)
> -                       if eidx != -1:
> -                               mirrorname = myuri[9:eidx]
> -                               path = myuri[eidx+1:]
> -
> -                               # Try user-defined mirrors first
> -                               if mirrorname in custommirrors:
> -                                       for cmirr in
> custommirrors[mirrorname]:
> -                                               filedict[myfile].append(
> -                                                       cmirr.rstrip("/")
> + "/" + path)
> -
> -                               # now try the official mirrors
> -                               if mirrorname in thirdpartymirrors:
> -                                       uris = [locmirr.rstrip("/") + "/"
> + path \
> -                                               for locmirr in
> thirdpartymirrors[mirrorname]]
> -                                       random.shuffle(uris)
> -                                       filedict[myfile].extend(uris)
> -
> thirdpartymirror_uris.setdefault(myfile, []).extend(uris)
> -
> -                               if not filedict[myfile]:
> -                                       writemsg(_("No known mirror by the
> name: %s\n") % (mirrorname))
> -                       else:
> -                               writemsg(_("Invalid mirror definition in
> SRC_URI:\n"), noiselevel=-1)
> -                               writemsg("  %s\n" % (myuri), noiselevel=-1)
> -               else:
> -                       if restrict_fetch or force_mirror:
> -                               # Only fetch from specific mirrors is
> allowed.
> -                               continue
> -                       primaryuris = primaryuri_dict.get(myfile)
> -                       if primaryuris is None:
> -                               primaryuris = []
> -                               primaryuri_dict[myfile] = primaryuris
> -                       primaryuris.append(myuri)
> -
> -       # Order primaryuri_dict values to match that in SRC_URI.
> -       for uris in primaryuri_dict.values():
> -               uris.reverse()
> -
> -       # Prefer thirdpartymirrors over normal mirrors in cases when
> -       # the file does not yet exist on the normal mirrors.
> -       for myfile, uris in thirdpartymirror_uris.items():
> -               primaryuri_dict.setdefault(myfile, []).extend(uris)
> -
> -       # Now merge primaryuri values into filedict (includes mirrors
> -       # explicitly referenced in SRC_URI).
> -       if "primaryuri" in restrict:
> -               for myfile, uris in filedict.items():
> -                       filedict[myfile] = primaryuri_dict.get(myfile, [])
> + uris
> -       else:
> -               for myfile in filedict:
> -                       filedict[myfile] += primaryuri_dict.get(myfile, [])
> +       filedict, primaryuri_dict = _get_uris(
> +               uris=myuris, settings=mysettings,
> +               custom_mirrors=custom_mirrors, locations=locations)
>
>         can_fetch=True
>
> --
> 1.8.5.2.8.g0f6c0d1
>
>
>

[-- Attachment #2: Type: text/html, Size: 22563 bytes --]

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

[Alec Warner]

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

On Sun, Jan 19, 2014 at 2:14 PM, W. Trevor King <wking@tremily.us> 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(
> +               settings=mysettings)
>
>         fetch_resume_size_default = "350K"
>         fetch_resume_size = mysettings.get("PORTAGE_FETCH_RESUME_MIN_SIZE")
> --
> 1.8.5.2.8.g0f6c0d1
>
>
>

[-- Attachment #2: Type: text/html, Size: 7069 bytes --]

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

[Alec Warner]

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

On Sun, Jan 19, 2014 at 2:44 PM, Alec Warner <antarus@gentoo.org> wrote:

> On Sun, Jan 19, 2014 at 2:14 PM, W. Trevor King <wking@tremily.us> 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 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
>>
>>
>>
>

[-- Attachment #2: Type: text/html, Size: 7918 bytes --]

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]  <http://www.python.org/dev/peps/pep-0257/>
- -- 
Alexander
alexander@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]

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

On Sun, Jan 19, 2014 at 2:45 PM, Alexander Berntsen <alexander@plaimi.net>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]  <http://www.python.org/dev/peps/pep-0257/>
> - --
> Alexander
> alexander@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-----
>
>

[-- Attachment #2: Type: text/html, Size: 2041 bytes --]

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

[W. Trevor King]

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

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

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

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
alexander@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]

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

On Sun, Jan 19, 2014 at 02:45:24PM -0800, Alec Warner wrote:
> On Sun, Jan 19, 2014 at 2:44 PM, Alec Warner <antarus@gentoo.org> 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

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

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

[Alec Warner]

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

On Sun, Jan 19, 2014 at 2:51 PM, W. Trevor King <wking@tremily.us> 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 <antarus@gentoo.org> 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
>

[-- Attachment #2: Type: text/html, Size: 2174 bytes --]

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

[Alec Warner]

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

On Sun, Jan 19, 2014 at 2:50 PM, Alexander Berntsen <alexander@plaimi.net>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
> alexander@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-----
>
>

[-- Attachment #2: Type: text/html, Size: 1712 bytes --]

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

[W. Trevor King]

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

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 <wking@tremily.us> 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'))
     <type 'str'>

   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")
> > +                                       % (mirror,))
> > +               else:
> > +                       if restrict_fetch or force_mirror:
> >
> 
> Are these globals or am I missing somethng?

Fixed in v2.


> > +                               # Only fetch from specific mirrors is
> > allowed.
> > +                               continue
> > +                       primaryuris = primaryuri_dict.get(filename)
> > +                       if primaryuris is None:
> > +                               primaryuris = []
> > +                               primaryuri_dict[filename] = primaryuris
> > +                       primaryuris.append(uri)
> > +
> > +       # 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.

> > +
> > +       # Prefer third_party_mirrors over normal mirrors in cases when
> > +       # the file does not yet exist on the normal mirrors.
> > +       for filename, uris in third_party_mirror_uris.items():
> > +               primaryuri_dict.setdefault(filename, []).extend(uris)
> > +
> > +       # Now merge primaryuri values into filedict (includes mirrors
> > +       # explicitly referenced in SRC_URI).
> > +       if "primaryuri" in restrict:
> >
> 
> same questoin here about where 'restrict' comes from.

Fixed in v2.

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

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

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

[W. Trevor King]

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

On Sun, Jan 19, 2014 at 02:46:36PM -0800, Alec Warner wrote:
> On Sun, Jan 19, 2014 at 2:45 PM, Alexander Berntsen <alexander@plaimi.net>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

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

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

[W. Trevor King]

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

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 <wking@tremily.us> 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

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

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
alexander@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]

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

On Sun, Jan 19, 2014 at 3:51 PM, Alexander Berntsen <alexander@plaimi.net>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
> alexander@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-----
>
>

[-- Attachment #2: Type: text/html, Size: 2756 bytes --]

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

[Tom Wijsman]

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

On Sat, 18 Jan 2014 19:07:45 -0800
"W. Trevor King" <wking@tremily.us> 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  : TomWij@gentoo.org
GPG Public Key  : 6D34E57D
GPG Fingerprint : C165 AF18 AB4C 400B C3D2  ABF0 95B2 1FCD 6D34 E57D

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

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

[Tom Wijsman]

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

On Sat, 18 Jan 2014 19:07:46 -0800
"W. Trevor King" <wking@tremily.us> 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  : TomWij@gentoo.org
GPG Public Key  : 6D34E57D
GPG Fingerprint : C165 AF18 AB4C 400B C3D2  ABF0 95B2 1FCD 6D34 E57D

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

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

[W. Trevor King]

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

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" <wking@tremily.us> 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

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

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

[W. Trevor King]

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

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

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

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

[Tom Wijsman]

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

On Sun, 19 Jan 2014 18:01:23 -0800
"W. Trevor King" <wking@tremily.us> 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  : TomWij@gentoo.org
GPG Public Key  : 6D34E57D
GPG Fingerprint : C165 AF18 AB4C 400B C3D2  ABF0 95B2 1FCD 6D34 E57D

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

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

[W. Trevor King]

Changes since v2:

* Use """<summary>""" 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 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 0000000..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 implementation details, and their
+	presence and interface may change at any time.  Do not use
+	them directly (outside of these tests).
+	"""
+
+	def test_get_checksum_failure_max_tries(self):
+		self.assertEqual(
+			_get_checksum_failure_max_tries(settings={}),
+			5)
+		self.assertEqual(
+			_get_checksum_failure_max_tries(settings={
+				'PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS': ''}),
+			5)
+		self.assertEqual(
+			_get_checksum_failure_max_tries(settings={
+				'PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS': '3'}),
+			3)
+		self.assertEqual(
+			_get_checksum_failure_max_tries(settings={
+				'PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS': '-1'}),
+			5)
+		self.assertEqual(
+			_get_checksum_failure_max_tries(settings={
+				'PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS': 'oops'}),
+			5)
+		self.assertEqual(
+			_get_checksum_failure_max_tries(
+				settings={}, default=3),
+			3)
-- 
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)
+		self.assertEqual(
+			_get_fetch_resume_size(settings={
+				'PORTAGE_FETCH_RESUME_MIN_SIZE': '3'}),
+			3)
+		self.assertEqual(
+			_get_fetch_resume_size(settings={
+				'PORTAGE_FETCH_RESUME_MIN_SIZE': '3K'}),
+			3072)
+		self.assertEqual(
+			_get_fetch_resume_size(settings={
+				'PORTAGE_FETCH_RESUME_MIN_SIZE': '3 K'}),
+			3072)
+		self.assertEqual(
+			_get_fetch_resume_size(settings={
+				'PORTAGE_FETCH_RESUME_MIN_SIZE': '3M'}),
+			3145728)
+		self.assertEqual(
+			_get_fetch_resume_size(settings={
+				'PORTAGE_FETCH_RESUME_MIN_SIZE': '3G'}),
+			3221225472)
+		self.assertEqual(
+			_get_fetch_resume_size(settings={
+				'PORTAGE_FETCH_RESUME_MIN_SIZE': 'oops'}),
+			358400)
+		self.assertEqual(
+			_get_fetch_resume_size(settings={}, default='3K'),
+			3072)
-- 
1.8.5.2.8.g0f6c0d1

[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_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)
+
+	third_party_mirrors = settings.thirdpartymirrors()
+	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://'):
+			expanded_uris = _expand_mirror(
+				uri=uri, custom_mirrors=custom_mirrors,
+				third_party_mirrors=third_party_mirrors)
+			filedict[filename].extend(
+				uri for _, uri in expanded_uris)
+			third_party_mirror_uris.setdefault(filename, []).extend(
+				uri for group, uri in expanded_uris
+				if group == 'third-party')
+		else:
+			if restrict_fetch or force_mirror:
+				# Only fetch from specific mirrors is allowed.
+				continue
+			primaryuris = primaryuri_dict.get(filename)
+			if primaryuris is None:
+				primaryuris = []
+				primaryuri_dict[filename] = primaryuris
+			primaryuris.append(uri)
+
+	# Order primaryuri_dict values to match that in SRC_URI.
+	for uris in primaryuri_dict.values():
+		uris.reverse()
+
+	# Prefer third_party_mirrors over normal mirrors in cases when
+	# the file does not yet exist on the normal mirrors.
+	for filename, uris in third_party_mirror_uris.items():
+		primaryuri_dict.setdefault(filename, []).extend(uris)
+
+	# Now merge primaryuri values into filedict (includes mirrors
+	# explicitly referenced in SRC_URI).
+	if "primaryuri" in restrict:
+		for filename, uris in filedict.items():
+			filedict[filename] = primaryuri_dict.get(filename, []) + uris
+	else:
+		for filename in filedict:
+			filedict[filename] += primaryuri_dict.get(filename, [])
+
+	return filedict, primaryuri_dict
+
+
 def fetch(myuris, mysettings, listonly=0, fetchonly=0,
 	locks_in_subdir=".locks", use_locks=1, try_mirrors=1, digests=None,
 	allow_missing_digests=True):
@@ -332,7 +454,6 @@ def fetch(myuris, mysettings, listonly=0, fetchonly=0,
 	# couple of checksum failures, to increase the probablility
 	# of success before checksum_failure_max_tries is reached.
 	checksum_failure_primaryuri = 2
-	thirdpartymirrors = mysettings.thirdpartymirrors()
 
 	# In the background parallel-fetch process, it's safe to skip checksum
 	# verification of pre-existing files in $DISTDIR that have the correct
@@ -405,7 +526,6 @@ def fetch(myuris, mysettings, listonly=0, fetchonly=0,
 			del mymirrors[x]
 
 	restrict_fetch = "fetch" in restrict
-	force_mirror = "force-mirror" in features and not restrict_mirror
 	custom_local_mirrors = custommirrors.get("local", [])
 	if restrict_fetch:
 		# With fetch restriction, a normal uri may only be fetched from
@@ -416,83 +536,9 @@ def fetch(myuris, mysettings, listonly=0, fetchonly=0,
 	else:
 		locations = mymirrors
 
-	file_uri_tuples = []
-	# Check for 'items' attribute since OrderedDict is not a dict.
-	if hasattr(myuris, 'items'):
-		for myfile, uri_set in myuris.items():
-			for myuri in uri_set:
-				file_uri_tuples.append((myfile, myuri))
-			if not uri_set:
-				file_uri_tuples.append((myfile, None))
-	else:
-		for myuri in myuris:
-			if urlparse(myuri).scheme:
-				file_uri_tuples.append((os.path.basename(myuri), myuri))
-			else:
-				file_uri_tuples.append((os.path.basename(myuri), None))
-
-	filedict = OrderedDict()
-	primaryuri_dict = {}
-	thirdpartymirror_uris = {}
-	for myfile, myuri in file_uri_tuples:
-		if myfile not in filedict:
-			filedict[myfile]=[]
-			for y in range(0,len(locations)):
-				filedict[myfile].append(locations[y]+"/distfiles/"+myfile)
-		if myuri is None:
-			continue
-		if myuri[:9]=="mirror://":
-			eidx = myuri.find("/", 9)
-			if eidx != -1:
-				mirrorname = myuri[9:eidx]
-				path = myuri[eidx+1:]
-
-				# Try user-defined mirrors first
-				if mirrorname in custommirrors:
-					for cmirr in custommirrors[mirrorname]:
-						filedict[myfile].append(
-							cmirr.rstrip("/") + "/" + path)
-
-				# now try the official mirrors
-				if mirrorname in thirdpartymirrors:
-					uris = [locmirr.rstrip("/") + "/" + path \
-						for locmirr in thirdpartymirrors[mirrorname]]
-					random.shuffle(uris)
-					filedict[myfile].extend(uris)
-					thirdpartymirror_uris.setdefault(myfile, []).extend(uris)
-
-				if not filedict[myfile]:
-					writemsg(_("No known mirror by the name: %s\n") % (mirrorname))
-			else:
-				writemsg(_("Invalid mirror definition in SRC_URI:\n"), noiselevel=-1)
-				writemsg("  %s\n" % (myuri), noiselevel=-1)
-		else:
-			if restrict_fetch or force_mirror:
-				# Only fetch from specific mirrors is allowed.
-				continue
-			primaryuris = primaryuri_dict.get(myfile)
-			if primaryuris is None:
-				primaryuris = []
-				primaryuri_dict[myfile] = primaryuris
-			primaryuris.append(myuri)
-
-	# Order primaryuri_dict values to match that in SRC_URI.
-	for uris in primaryuri_dict.values():
-		uris.reverse()
-
-	# Prefer thirdpartymirrors over normal mirrors in cases when
-	# the file does not yet exist on the normal mirrors.
-	for myfile, uris in thirdpartymirror_uris.items():
-		primaryuri_dict.setdefault(myfile, []).extend(uris)
-
-	# Now merge primaryuri values into filedict (includes mirrors
-	# explicitly referenced in SRC_URI).
-	if "primaryuri" in restrict:
-		for myfile, uris in filedict.items():
-			filedict[myfile] = primaryuri_dict.get(myfile, []) + uris
-	else:
-		for myfile in filedict:
-			filedict[myfile] += primaryuri_dict.get(myfile, [])
+	filedict, primaryuri_dict = _get_uris(
+		uris=myuris, settings=mysettings,
+		custom_mirrors=custommirrors, locations=locations)
 
 	can_fetch=True
 
diff --git a/pym/portage/tests/ebuild/test_fetch.py b/pym/portage/tests/ebuild/test_fetch.py
index 2b06190..9e1635a 100644
--- a/pym/portage/tests/ebuild/test_fetch.py
+++ b/pym/portage/tests/ebuild/test_fetch.py
@@ -1,13 +1,27 @@
 # Copyright 1998-2013 Gentoo Foundation
 # Distributed under the terms of the GNU General Public License v2
 
+from portage import OrderedDict
 from portage.package.ebuild.fetch import (
 	_get_checksum_failure_max_tries,
 	_get_fetch_resume_size,
+	_get_file_uri_tuples,
+	_expand_mirror,
+	_get_uris,
 	)
 from portage.tests import TestCase
 
 
+class _Settings(dict):
+	"""Mock settings instance for testing."""
+	@property
+	def features(self):
+		return self['features']
+
+	def thirdpartymirrors(self):
+		return self['third-party mirrors']
+
+
 class FetchTestCase(TestCase):
 	"""
 	Test fetch and it's helper functions.
@@ -20,6 +34,26 @@ class FetchTestCase(TestCase):
 	them directly (outside of these tests).
 	"""
 
+	_third_party_mirrors = {
+		'gentoo': [
+			'http://distfiles.gentoo.org/',
+			'ftp://ftp.gentoo.org/',
+			],
+		}
+
+	_custom_mirrors = {
+		'gentoo': [
+			'http://192.168.0.1/',
+			'ftp://192.168.0.2/',
+			],
+		}
+
+	_settings = _Settings({
+			'PORTAGE_RESTRICT': '',
+			'features': [],
+			'third-party mirrors': _third_party_mirrors,
+		})
+
 	def test_get_checksum_failure_max_tries(self):
 		self.assertEqual(
 			_get_checksum_failure_max_tries(settings={}),
@@ -80,3 +114,90 @@ class FetchTestCase(TestCase):
 		self.assertEqual(
 			_get_fetch_resume_size(settings={}, default='3K'),
 			3072)
+
+	def test_get_file_uri_tuples(self):
+		self.assertEqual(
+			_get_file_uri_tuples(uris=[
+				'http://host/path',
+				'/filesystem/other-path',
+				]),
+			[
+				('path', 'http://host/path'),
+				('other-path', None),
+			])
+		uris = OrderedDict()
+		uris['path'] = {'http://host/path', 'http://other-host/path'}
+		uris['other-path'] = {}
+		self.assertEqual(
+			_get_file_uri_tuples(uris=uris),
+			[
+				('path', 'http://host/path'),
+				('path', 'http://other-host/path'),
+				('other-path', None),
+			])
+
+	def test_expand_mirror(self):
+		uris = list(_expand_mirror(
+			uri='mirror://gentoo/some/file',
+			custom_mirrors=self._custom_mirrors,
+			third_party_mirrors=self._third_party_mirrors,
+			))
+		self.assertEqual(
+			uris[:2],
+			[
+				('custom', 'http://192.168.0.1/some/file'),
+				('custom', 'ftp://192.168.0.2/some/file'),
+			])
+		self.assertEqual(
+			sorted(uris[2:]),  # de-randomize
+			[
+				('third-party',
+					'ftp://ftp.gentoo.org/some/file'),
+				('third-party',
+					'http://distfiles.gentoo.org/some/file'),
+			])
+
+
+	def test_get_uris(self):
+		files, primary_uris = _get_uris(
+			uris=[
+				'mirror://gentoo/some/file',
+				'http://example.net/my/other-file',
+				],
+			settings=self._settings,
+			custom_mirrors=self._custom_mirrors,
+			locations=[
+				'http://location.net/',
+				'ftp://location.com/path',
+				],
+			)
+		self.assertEqual(list(files.keys()), ['file', 'other-file'])
+		self.assertEqual(
+			files['file'][:4],
+			[
+				'http://location.net/distfiles/file',
+				'ftp://location.com/path/distfiles/file',
+				'http://192.168.0.1/some/file',
+				'ftp://192.168.0.2/some/file',
+			])
+		self.assertEqual(
+			sorted(files['file'][4:]),  # de-randomize
+			[
+				'ftp://ftp.gentoo.org/some/file',
+				'http://distfiles.gentoo.org/some/file',
+			])
+		self.assertEqual(
+			files['other-file'],
+			[
+				'http://location.net/distfiles/other-file',
+				'ftp://location.com/path/distfiles/other-file',
+				'http://example.net/my/other-file',
+			])
+		self.assertEqual(
+			primary_uris,
+			{
+				'file': [],
+				'other-file': [
+					'http://example.net/my/other-file',
+					],
+			})
-- 
1.8.5.2.8.g0f6c0d1

[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 <TomWij@gentoo.org>
---
 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

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

[W. Trevor King]

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

On Sun, Jan 19, 2014 at 07:26:09PM -0800, W. Trevor King wrote:
> 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 ;).

Queued for v4: “we're relying” → “we're not relying”.

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

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

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

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 1075 bytes --]

On Sunday 19 January 2014 17:46:36 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.

the good news is that i wrote a pylintrc module for Chromium OS to enforce 
docstring style.  the bad news is that it doesn't work with pyflakes.
https://chromium.googlesource.com/chromiumos/chromite/+/master/cros/commands/lint.py

what we did there was just merge it and then have people fix things up as they 
went rather than try to clean it all up first.
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 494 bytes --]

On Sunday 19 January 2014 22:26:06 W. Trevor King wrote:
> 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

no need to use the full file path.  a simpler prefix is fine like:
	ebuild: fetch: ...
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[W. Trevor King]

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

On Wed, Jan 22, 2014 at 12:35:13AM -0500, Mike Frysinger wrote:
> On Sunday 19 January 2014 22:26:06 W. Trevor King wrote:
> > 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
> 
> no need to use the full file path.  a simpler prefix is fine like:
> 	ebuild: fetch: ...

How about “ebuild.fetch: …”?  Queued for v4, but easy to change if you
prefer “ebuild: fetch: …”.

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

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

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

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 889 bytes --]

On Wednesday 22 January 2014 11:10:16 W. Trevor King wrote:
> On Wed, Jan 22, 2014 at 12:35:13AM -0500, Mike Frysinger wrote:
> > On Sunday 19 January 2014 22:26:06 W. Trevor King wrote:
> > > 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
> > 
> > no need to use the full file path.  a simpler prefix is fine like:
> > 	ebuild: fetch: ...
> 
> How about “ebuild.fetch: …”?  Queued for v4, but easy to change if you
> prefer “ebuild: fetch: …”.

i prefer the latter, but i can live with either.
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[W. Trevor King]

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

I've queued the following changes since v3:

* Fix “we're relying” → “we're not relying” in the “Factor out
  _get_uris” patch [1].
* Use an “ebuild: fetch:” prefix for all patches [2].

in my fetch-refactor branch, but it's been fairly quiet since I pushed
v3 last Monday.  Should I push v4 with just these commit message
changes, or should I let this cook a bit longer in case there are
further issues with v3?

Cheers,
Trevor

[1]: http://article.gmane.org/gmane.linux.gentoo.portage.devel/4079
[2]: http://article.gmane.org/gmane.linux.gentoo.portage.devel/4102

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

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