[gentoo-portage-dev] Re: [PATCH v3 0/4] Initial fetch() refactoring
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 signature.asc Description: OpenPGP digital signature
Re: [gentoo-portage-dev] [PATCH v3 0/4] Initial fetch() refactoring
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 signature.asc Description: OpenPGP digital signature
Re: [gentoo-portage-dev] Signing off patches
On Tue, Jan 21, 2014 at 08:22:31PM +0100, Tom Wijsman wrote: > On Tue, 21 Jan 2014 11:18:39 -0800 W. Trevor King wrote: > > I'm all for recording suggested conventions in DEVELOPING, but I > > don't think it's worth the trouble to over-specify the conditions > > under which each tag should be used, or to lay out consequences > > for cases where they're forgotten. The faith part is trusting > > devs to understand and apply the written suggestions, not in > > determining what the suggestions are. > > Which brings me back to my very first words of this sub thread: > > Either someone cares about the background of a patch or he/she > doesn't. That's certainly true. My initial impression was that you were against suggesting these conventions in DEVELOPING. Perhaps I missunderstood. Cheers, Trevor -- This email may be signed or encrypted with GnuPG (http://www.gnupg.org). For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy signature.asc Description: OpenPGP digital signature
Re: [gentoo-portage-dev] Signing off patches
On Tue, Jan 21, 2014 at 07:41:21PM +0100, Tom Wijsman wrote: > On Tue, 21 Jan 2014 09:20:04 -0800 W. Trevor King wrote: > > On Tue, Jan 21, 2014 at 05:59:54PM +0100, Tom Wijsman wrote: > > > On Tue, 21 Jan 2014 08:51:14 -0800 W. Trevor King wrote: > > > > On Tue, Jan 21, 2014 at 04:40:19PM +0100, Tom Wijsman wrote: > > > > > On Sun, 19 Jan 2014 18:32:35 -0800 W. Trevor King wrote: > > > > > > No policy/suggestion/goal is going to be followed 100% of > > > > > > the time. > > > > > > > > > > This way, it seems preferable to use the mailing list when > > > > > blaming. > > > > > > > > Unless some of the discussion happened on IRC. There are > > > > several possible channels for patch discussion, but only one > > > > commit message per patch. > > > > > > Exactly, without knowledge codification all that will continue > > > to be a "feel like", "probably not", "shouldn't", "unless some". > > > > I don't see that as a problem. I guess I just have more faith > > that current devs will put in a reasonable best-effort without > > codification beyond “here are some conventions you may want to > > use”, and that future devs will be competent enough to still be > > productive in the face of unhelpful commit messages. > > If they are just mentioned at random all the time, perhaps half of > them get remembered or so; the half of what is remembered gets given > through to the next generation of future devs, this up to the point > that it would have been a better idea to write this down than to > have faith. I'm all for recording suggested conventions in DEVELOPING, but I don't think it's worth the trouble to over-specify the conditions under which each tag should be used, or to lay out consequences for cases where they're forgotten. The faith part is trusting devs to understand and apply the written suggestions, not in determining what the suggestions are. Cheers, Trevor -- This email may be signed or encrypted with GnuPG (http://www.gnupg.org). For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy signature.asc Description: OpenPGP digital signature
Re: [gentoo-portage-dev] Signing off patches
On Tue, Jan 21, 2014 at 05:59:54PM +0100, Tom Wijsman wrote: > On Tue, 21 Jan 2014 08:51:14 -0800 W. Trevor King wrote: > > On Tue, Jan 21, 2014 at 04:40:19PM +0100, Tom Wijsman wrote: > > > On Sun, 19 Jan 2014 18:32:35 -0800 W. Trevor King wrote: > > > > No policy/suggestion/goal is going to be followed 100% of the > > > > time. > > > > > > This way, it seems preferable to use the mailing list when > > > blaming. > > > > Unless some of the discussion happened on IRC. There are several > > possible channels for patch discussion, but only one commit > > message per patch. > > Exactly, without knowledge codification all that will continue to be > a "feel like", "probably not", "shouldn't", "unless some". I don't see that as a problem. I guess I just have more faith that current devs will put in a reasonable best-effort without codification beyond “here are some conventions you may want to use”, and that future devs will be competent enough to still be productive in the face of unhelpful commit messages. Cheers, Trevor -- This email may be signed or encrypted with GnuPG (http://www.gnupg.org). For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy signature.asc Description: OpenPGP digital signature
Re: [gentoo-portage-dev] Signing off patches
On Tue, Jan 21, 2014 at 04:40:19PM +0100, Tom Wijsman wrote: > On Sun, 19 Jan 2014 18:32:35 -0800 W. Trevor King wrote: > > If the initial submission does not express the consensus, you can > > either ask for a resubmission that does, or say “Alice, is it ok > > if I change your commit message to read ‘…’? when I commit it?”. > > This assumes the committer would ask that, which barely happens. If neither the committer nor the author feel like checking for this, it's probably not going to happen ;). > Also, history rewriting is not acceptable as far as I know... It's not rewriting history if the patch hasn't been pushed to a stable branch yet. When you commit the patch, adjusting it in ways that the author has agreed to shouldn't count as “rewriting history”. It's only rewriting after you've pushed the original patch into a stable branch. > > No policy/suggestion/goal is going to be followed 100% of the time. > > This way, it seems preferable to use the mailing list when blaming. Unless some of the discussion happened on IRC. There are several possible channels for patch discussion, but only one commit message per patch. Cheers, Trevor -- This email may be signed or encrypted with GnuPG (http://www.gnupg.org). For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy signature.asc Description: OpenPGP digital signature
[gentoo-portage-dev] Re: [PATCH v3 3/4] pym/portage/package/ebuild/fetch.py: Factor out _get_uris
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 signature.asc Description: OpenPGP digital signature
[gentoo-portage-dev] Re: layout.conf: What's our opinion?
On Mon, Jan 20, 2014 at 12:05:30PM +0100, Sebastian Luther wrote: > Currently layout.conf is not under PMS control. This basically means > that every PM (or version thereof) may support different keys and > assign different meanings to them. Standardizing in the PMS sounds like a good idea, for situations where a consensus can be reached. > Portage's behavior for unknown keys in layout.conf is to ignore them > without a warning. That makes sense, and is a good practice for forwards-compatibility [1]. Repositories providing custom keys (or package managers supporting custom keys) do so with the understanding that there may be incompatibilities. Namespaced custom keys (portage-eclass-masters?) would help avoid accidental collision. Versioning the layout.conf key spec would also be useful, so a PM in strict-mode could warn “unknown keys x, y, and z in a v1.2 layout.conf. I only understand v1.0, and it's possible that these keys have been added in subsequent versions”. > The bad thing about this is that some layout.conf keys portage > currently supports, may render the repository unusable for a PM if > it doesn't support them. Can you give an example of the breakage? Maybe I just haven't been listening in the right places. If the problem is that the repositories *need* a custom key that is not universally supported, then that seems like something the repository authors should expect. > After discussing this one IRC I came to the conclusion that we just > disagree on how we should handle additions to layout.conf. > > Basically it's either > 1) "We add things as we see fit." or > 2) "We should only add things if absolutely necessary.". Locking everything down completely seems overly harsh, and makes it harder to experiment with new features. Why not: 1.5) We can add custom keys as we see fit under the ‘portage-*’ namespace. * Portable repositories should not rely on them until they land in the PMS under a new layout.conf version, at which time the ‘portage-’ prefix will be removed. * Portage will recognize any newly standardized keys under their old ‘portage-*’ name to allow repositories to gracefully transition to the standardized names. Cheers, Trevor [1]: http://www.w3.org/2001/tag/doc/versioning-20070326.html#iddiv470454016 -- This email may be signed or encrypted with GnuPG (http://www.gnupg.org). For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy signature.asc Description: OpenPGP digital signature
[gentoo-portage-dev] [PATCH v3 4/4] pym/portage/package/ebuild/fetch.py: Flatten conditionals in _get_fetch_resume_size
Make this easier to read by avoiding nested conditionals [1].
[1]: http://article.gmane.org/gmane.linux.gentoo.portage.devel/4058
Reported-by: Tom Wijsman
---
pym/portage/package/ebuild/fetch.py | 30 ++
1 file changed, 14 insertions(+), 16 deletions(-)
diff --git a/pym/portage/package/ebuild/fetch.py
b/pym/portage/package/ebuild/fetch.py
index 0093a6e..2bf88d8 100644
--- a/pym/portage/package/ebuild/fetch.py
+++ b/pym/portage/package/ebuild/fetch.py
@@ -276,24 +276,22 @@ def _get_checksum_failure_max_tries(settings, default=5):
def _get_fetch_resume_size(settings, default='350K'):
key = 'PORTAGE_FETCH_RESUME_MIN_SIZE'
- v = settings.get(key)
+ v = settings.get(key, default)
if v is not None:
v = "".join(v.split())
- if not v:
- # If it's empty, silently use the default.
- v = default
- match = _fetch_resume_size_re.match(v)
- if (match is None or
- match.group(2).upper() not in _size_suffix_map):
- writemsg(_("!!! Variable %s contains an "
- "unrecognized format: '%s'\n")
- % (key, settings[key]),
- noiselevel=-1)
- writemsg(_("!!! Using %s default value: %s\n")
- % (key, default),
- noiselevel=-1)
- v = None
- if v is None:
+ if not v:
+ # If it's empty, silently use the default.
+ v = default
+ match = _fetch_resume_size_re.match(v)
+ if (match is None or
+ match.group(2).upper() not in _size_suffix_map):
+ writemsg(_("!!! Variable %s contains "
+ "an unrecognized format: '%s'\n")
+ % (key, settings[key]),
+ noiselevel=-1)
+ writemsg(_("!!! Using %s default value: %s\n")
+ % (key, default),
+ noiselevel=-1)
v = default
match = _fetch_resume_size_re.match(v)
v = int(match.group(1)) * \
--
1.8.5.2.8.g0f6c0d1
[gentoo-portage-dev] [PATCH v3 3/4] pym/portage/package/ebuild/fetch.py: Factor out _get_uris
The current fetch() function is quite long, which makes it hard to
know what I can change without adverse side effects. By pulling this
logic out of the main function, we get clearer logic in fetch() and
more explicit input for the config extraction.
This block was especially complicated, so I also created the helper
functions _get_file_uri_tuples and _expand_mirror. I'd prefer if
_expand_mirror iterated through URIs instead of (group, URI) tuples,
but we need a distinct marker for third-party URIs to build
third_party_mirror_uris which is used to build primaryuri_dict which
is used way down in fetch():
if checksum_failure_count == \
checksum_failure_primaryuri:
# Switch to "primaryuri" mode in order
# to increase the probablility of
# of success.
primaryuris = \
primaryuri_dict.get(myfile)
if primaryuris:
uri_list.extend(
reversed(primaryuris))
I don't know if this is useful enough to motivate the uglier
_expandmirror return values, but I'll kick that can down the road for
now.
There was some discussion on the list [1] about the defaults in:
def _get_uris(uris, settings, custom_mirrors=(), locations=()):
but I prefer this to Alec's floated:
def _get_uris(uris, settings, custom_mirrors=None, locations=None):
if not custom_mirrors:
custom_mirrors = ()
if not locations:
locations = ()
Which accomplishes the same thing with less clarity ;). My default
values are tuples (and not mutable lists) to make it extra-obvious
that we're relying on anything crazy like mutating our default values
;).
There was also discussion about whether the ugly settings object
should be passed to _get_uris, or whether the appropriate settings
should be extracted first and then passed through [2]. I've passed
settings through, because I'd prefer to have the uglyness handled in
helper functions. The fetch() function is what most folks will care
about, so we want to keep that as clean as possible. If the helper
functions have to suffer a bit for a cleaner fetch(), so be it.
[1]: http://thread.gmane.org/gmane.linux.gentoo.portage.devel/4002/focus=4041
[2]: http://thread.gmane.org/gmane.linux.gentoo.portage.devel/4002/focus=4042
---
pym/portage/package/ebuild/fetch.py| 208 -
pym/portage/tests/ebuild/test_fetch.py | 121 +++
2 files changed, 248 insertions(+), 81 deletions(-)
diff --git a/pym/portage/package/ebuild/fetch.py
b/pym/portage/package/ebuild/fetch.py
index 4ecefc9..0093a6e 100644
--- a/pym/portage/package/ebuild/fetch.py
+++ b/pym/portage/package/ebuild/fetch.py
@@ -15,9 +15,9 @@ import sys
import tempfile
try:
- from urllib.parse import urlparse
+ from urllib.parse import urlparse, urlunparse
except ImportError:
- from urlparse import urlparse
+ from urlparse import urlparse, urlunparse
import portage
portage.proxy.lazyimport.lazyimport(globals(),
@@ -301,6 +301,128 @@ def _get_fetch_resume_size(settings, default='350K'):
return v
+def _get_file_uri_tuples(uris):
+ """Return a list of (filename, URI) tuples."""
+ file_uri_tuples = []
+ # Check for 'items' attribute since OrderedDict is not a dict.
+ if hasattr(uris, 'items'):
+ for filename, uri_set in uris.items():
+ for uri in uri_set:
+ file_uri_tuples.append((filename, uri))
+ if not uri_set:
+ file_uri_tuples.append((filename, None))
+ else:
+ for uri in uris:
+ if urlparse(uri).scheme:
+ file_uri_tuples.append(
+ (os.path.basename(uri), uri))
+ else:
+ file_uri_tuples.append(
+ (os.path.basename(uri), None))
+ return file_uri_tuples
+
+
+def _expand_mirror(uri, custom_mirrors=(), third_party_mirrors=()):
+ """
+ Replace the 'mirror://' scheme and netloc in the URI.
+
+ Returns an iterable listing expanded (group, URI) tuples,
+ where the group is either 'custom' or 'third-party'.
+ """
+ parsed = urlparse(uri)
+ mirror = parsed.netloc
+ path = parsed.path
+ if path:
+ # Try user-defined mirrors first
+ if mirror in custom_mirrors:
+ for cmirr in custom_mirrors[mirror]:
+ m_uri = urlparse(cmirr)
+ yield ('custom', urlunparse((
+ m_uri.scheme, m_uri.netloc, path) +
+ parsed[3:]))
+
+ # now try the official mirrors
+ if mirror in third_party_mirrors:
+ uris = []
+ for locmirr in third_party_mir
[gentoo-portage-dev] [PATCH v3 2/4] pym/portage/package/ebuild/fetch.py: Factor out _get_fetch_resume_size
The current fetch() function is quite long, which makes it hard to
know what I can change without adverse side effects. By pulling this
logic out of the main function, we get clearer logic in fetch() and
more explicit input for the config extraction.
Following a suggestion by Tom Wijsman, I put the setting name in a new
'key' variable to cut down on the PORTAGE_FETCH_RESUME_MIN_SIZE noise.
---
pym/portage/package/ebuild/fetch.py| 51 +++---
pym/portage/tests/ebuild/test_fetch.py | 37
2 files changed, 65 insertions(+), 23 deletions(-)
diff --git a/pym/portage/package/ebuild/fetch.py
b/pym/portage/package/ebuild/fetch.py
index 911500a..4ecefc9 100644
--- a/pym/portage/package/ebuild/fetch.py
+++ b/pym/portage/package/ebuild/fetch.py
@@ -274,6 +274,33 @@ def _get_checksum_failure_max_tries(settings, default=5):
return v
+def _get_fetch_resume_size(settings, default='350K'):
+ key = 'PORTAGE_FETCH_RESUME_MIN_SIZE'
+ v = settings.get(key)
+ if v is not None:
+ v = "".join(v.split())
+ if not v:
+ # If it's empty, silently use the default.
+ v = default
+ match = _fetch_resume_size_re.match(v)
+ if (match is None or
+ match.group(2).upper() not in _size_suffix_map):
+ writemsg(_("!!! Variable %s contains an "
+ "unrecognized format: '%s'\n")
+ % (key, settings[key]),
+ noiselevel=-1)
+ writemsg(_("!!! Using %s default value: %s\n")
+ % (key, default),
+ noiselevel=-1)
+ v = None
+ if v is None:
+ v = default
+ match = _fetch_resume_size_re.match(v)
+ v = int(match.group(1)) * \
+ 2 ** _size_suffix_map[match.group(2).upper()]
+ return v
+
+
def fetch(myuris, mysettings, listonly=0, fetchonly=0,
locks_in_subdir=".locks", use_locks=1, try_mirrors=1, digests=None,
allow_missing_digests=True):
@@ -299,29 +326,7 @@ def fetch(myuris, mysettings, listonly=0, fetchonly=0,
checksum_failure_max_tries = _get_checksum_failure_max_tries(
settings=mysettings)
-
- fetch_resume_size_default = "350K"
- fetch_resume_size = mysettings.get("PORTAGE_FETCH_RESUME_MIN_SIZE")
- if fetch_resume_size is not None:
- fetch_resume_size = "".join(fetch_resume_size.split())
- if not fetch_resume_size:
- # If it's undefined or empty, silently use the default.
- fetch_resume_size = fetch_resume_size_default
- match = _fetch_resume_size_re.match(fetch_resume_size)
- if match is None or \
- (match.group(2).upper() not in _size_suffix_map):
- writemsg(_("!!! Variable PORTAGE_FETCH_RESUME_MIN_SIZE"
- " contains an unrecognized format: '%s'\n") % \
- mysettings["PORTAGE_FETCH_RESUME_MIN_SIZE"],
noiselevel=-1)
- writemsg(_("!!! Using PORTAGE_FETCH_RESUME_MIN_SIZE "
- "default value: %s\n") %
fetch_resume_size_default,
- noiselevel=-1)
- fetch_resume_size = None
- if fetch_resume_size is None:
- fetch_resume_size = fetch_resume_size_default
- match = _fetch_resume_size_re.match(fetch_resume_size)
- fetch_resume_size = int(match.group(1)) * \
- 2 ** _size_suffix_map[match.group(2).upper()]
+ fetch_resume_size = _get_fetch_resume_size(settings=mysettings)
# Behave like the package has RESTRICT="primaryuri" after a
# couple of checksum failures, to increase the probablility
diff --git a/pym/portage/tests/ebuild/test_fetch.py
b/pym/portage/tests/ebuild/test_fetch.py
index 26e0349..2b06190 100644
--- a/pym/portage/tests/ebuild/test_fetch.py
+++ b/pym/portage/tests/ebuild/test_fetch.py
@@ -3,6 +3,7 @@
from portage.package.ebuild.fetch import (
_get_checksum_failure_max_tries,
+ _get_fetch_resume_size,
)
from portage.tests import TestCase
@@ -43,3 +44,39 @@ class FetchTestCase(TestCase):
_get_checksum_failure_max_tries(
settings={}, default=3),
3)
+
+ def test_get_fetch_resume_size(self):
+ self.assertEqual(
+ _get_fetch_resume_size(settings={}),
+ 358400)
+ self.assertEqual(
+ _get_fetch_resume_size(settings={
+ 'PORTAGE_FETCH_RESUME_MIN_SIZE': ''}),
+ 358400)
+
[gentoo-portage-dev] [PATCH v3 1/4] pym/portage/package/ebuild/fetch.py: Factor out _get_checksum_failure_max_tries
The current fetch() function is quite long, which makes it hard to
know what I can change without adverse side effects. By pulling this
logic out of the main function, we get clearer logic in fetch() and
more explicit input for the config extraction.
Following a suggestion by Tom Wijsman, I put the setting name in a new
'key' variable to cut down on the PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS
noise.
---
pym/portage/package/ebuild/fetch.py| 61 --
pym/portage/tests/ebuild/test_fetch.py | 45 +
2 files changed, 81 insertions(+), 25 deletions(-)
create mode 100644 pym/portage/tests/ebuild/test_fetch.py
diff --git a/pym/portage/package/ebuild/fetch.py
b/pym/portage/package/ebuild/fetch.py
index 5316f03..911500a 100644
--- a/pym/portage/package/ebuild/fetch.py
+++ b/pym/portage/package/ebuild/fetch.py
@@ -240,6 +240,40 @@ _size_suffix_map = {
'Y' : 80,
}
+
+def _get_checksum_failure_max_tries(settings, default=5):
+ """
+ Get the maximum number of failed download attempts.
+
+ Generally, downloading the same file repeatedly from
+ every single available mirror is a waste of bandwidth
+ and time, so there needs to be a cap.
+ """
+ key = 'PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS'
+ v = default
+ try:
+ v = int(settings.get(key, default))
+ except (ValueError, OverflowError):
+ writemsg(_("!!! Variable %s contains "
+ "non-integer value: '%s'\n")
+ % (key, settings[key]),
+ noiselevel=-1)
+ writemsg(_("!!! Using %s default value: %s\n")
+ % (key, default),
+ noiselevel=-1)
+ v = default
+ if v < 1:
+ writemsg(_("!!! Variable %s contains "
+ "value less than 1: '%s'\n")
+ % (key, v),
+ noiselevel=-1)
+ writemsg(_("!!! Using %s default value: %s\n")
+ % (key, default),
+ noiselevel=-1)
+ v = default
+ return v
+
+
def fetch(myuris, mysettings, listonly=0, fetchonly=0,
locks_in_subdir=".locks", use_locks=1, try_mirrors=1, digests=None,
allow_missing_digests=True):
@@ -263,31 +297,8 @@ def fetch(myuris, mysettings, listonly=0, fetchonly=0,
print(_(">>> \"mirror\" mode desired and \"mirror\"
restriction found; skipping fetch."))
return 1
- # Generally, downloading the same file repeatedly from
- # every single available mirror is a waste of bandwidth
- # and time, so there needs to be a cap.
- checksum_failure_max_tries = 5
- v = checksum_failure_max_tries
- try:
- v = int(mysettings.get("PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS",
- checksum_failure_max_tries))
- except (ValueError, OverflowError):
- writemsg(_("!!! Variable PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS"
- " contains non-integer value: '%s'\n") % \
- mysettings["PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS"],
noiselevel=-1)
- writemsg(_("!!! Using PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS "
- "default value: %s\n") % checksum_failure_max_tries,
- noiselevel=-1)
- v = checksum_failure_max_tries
- if v < 1:
- writemsg(_("!!! Variable PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS"
- " contains value less than 1: '%s'\n") % v,
noiselevel=-1)
- writemsg(_("!!! Using PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS "
- "default value: %s\n") % checksum_failure_max_tries,
- noiselevel=-1)
- v = checksum_failure_max_tries
- checksum_failure_max_tries = v
- del v
+ checksum_failure_max_tries = _get_checksum_failure_max_tries(
+ settings=mysettings)
fetch_resume_size_default = "350K"
fetch_resume_size = mysettings.get("PORTAGE_FETCH_RESUME_MIN_SIZE")
diff --git a/pym/portage/tests/ebuild/test_fetch.py
b/pym/portage/tests/ebuild/test_fetch.py
new file mode 100644
index 000..26e0349
--- /dev/null
+++ b/pym/portage/tests/ebuild/test_fetch.py
@@ -0,0 +1,45 @@
+# Copyright 1998-2013 Gentoo Foundation
+# Distributed under the terms of the GNU General Public License v2
+
+from portage.package.ebuild.fetch import (
+ _get_checksum_failure_max_tries,
+ )
+from portage.tests import TestCase
+
+
+class FetchTestCase(TestCase):
+ """
+ Test fetch and it's helper functions.
+
+ The fetch function, as it stands, is too complicated to test
+ on its own. However, the new helper functions are much more
+ limited and easier to test. Despite these tests, the helper
+ functions are internal implementatio
[gentoo-portage-dev] [PATCH v3 0/4] Initial fetch() refactoring
Changes since v2: * Use """""" for one-line docstrings. * Terminate docstring summaries with periods. * Mention 'netloc' in _expand_mirror docstring. * Uppercase URI in docstrings. * Add a 'key' variable to cut down on setting-name noise. * Remove parens and line extensions (\) from % formatting. * Use 'expanded_uris' instead of 'uris' for _expand_mirror return value. * Additional line wrapping to stay under 80 chars. * New test_fetch.py test suite for the helper functions. * New patch #4 that refactors _get_fetch_resume_size. Not changed since v2: * Default arguments for _get_uris [1], but I've added an explicit defense in my commit message. * Use of settings in _get_uris [2], but I've added an explicit defense in my commit message. * Streamlining settings extraction [2], which I'm kicking down the road. I think Portage should just use ConfigParser for settings, but that is clearly outside the scope of this series. Cheers, Trevor [1]: http://thread.gmane.org/gmane.linux.gentoo.portage.devel/4002/focus=4041 [2]: http://thread.gmane.org/gmane.linux.gentoo.portage.devel/4002/focus=4042 W. Trevor King (4): pym/portage/package/ebuild/fetch.py: Factor out _get_checksum_failure_max_tries pym/portage/package/ebuild/fetch.py: Factor out _get_fetch_resume_size pym/portage/package/ebuild/fetch.py: Factor out _get_uris pym/portage/package/ebuild/fetch.py: Flatten conditionals in _get_fetch_resume_size pym/portage/package/ebuild/fetch.py| 318 - pym/portage/tests/ebuild/test_fetch.py | 203 + 2 files changed, 392 insertions(+), 129 deletions(-) create mode 100644 pym/portage/tests/ebuild/test_fetch.py -- 1.8.5.2.8.g0f6c0d1
Re: [gentoo-portage-dev] Signing off patches
On Mon, Jan 20, 2014 at 03:09:14AM +0100, Tom Wijsman wrote: > On Sat, 18 Jan 2014 20:15:57 -0800 W. Trevor King wrote: > > On Sun, Jan 19, 2014 at 02:33:06AM +0100, Tom Wijsman wrote: > > > On Sat, 18 Jan 2014 15:24:59 -0800 W. Trevor King wrote: > > > > If it doesn't need to get updated, then it probably already > > > > started out explaining the consensus ;). > > > > > > That is a guess, you can look this up in past patches. > > > > Sure. Will you? If I want to touch some code, and it looks > > confusing, I'll use blame to see who wrote it and whay they were > > thinking about. If the commit message is not informative, I usually > > give up. I have a hard time imaging folks tracking down the thread > > that spawned that patch, assuming such a thread even exists, for each > > troublesome line they'd like to touch. It's much easier to summarize > > any issues the list resolved (because they're likely the same > > questions the new dev is asking) in the commit message, where future > > developers can find them. > > How does this make the consensus written after the patch part of it? > > The person whom commits can be different than the person whom wrote the > patch; and hence, that person commits without writing down consensus. > If that person were to write it down, it would change the authorship. If the initial submission does not express the consensus, you can either ask for a resubmission that does, or say “Alice, is it ok if I change your commit message to read ‘…’? when I commit it?”. > Hence you made a guess, because I see pushed commits without > consensus. No policy/suggestion/goal is going to be followed 100% of the time. If most commits, especially those that were contentious enough to go through a few rounds of revision, have a commit message with a good summary, then the future developer using blame has good odds of finding something useful. I think that's a good goal to shoot for, even if you don't hit it all the time. Even if you only consider the present, improving your commit message to address tricky implementation details or unclear motivation before submitting the next revision on a patch series will help reviewers understand your patch better in the first place. > Here I like vapier's approach from the other reply in this sub > thread, that is to add it whenever people make the effort of > providing the tag; which is an approach the Linux kernel upstream > takes as well, if you want to be seen as a contributor you need to > provide the tags. Sounds fair to me. Cheers, Trevor -- This email may be signed or encrypted with GnuPG (http://www.gnupg.org). For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy signature.asc Description: OpenPGP digital signature
Re: [gentoo-portage-dev] [PATCH 2/3] pym/portage/package/ebuild/fetch.py: Factor out _get_fetch_resume_size
On Mon, Jan 20, 2014 at 02:41:41AM +0100, Tom Wijsman wrote: > There is some duplicate code here, I think the conditions can be > rewritten in such way that the duplicate code doesn't take place. Do you want a rewrite squashed into this commit, or as a follow-on commit after this one (which gets a test suite in v3)? Cheers, Trevor -- This email may be signed or encrypted with GnuPG (http://www.gnupg.org). For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy signature.asc Description: OpenPGP digital signature
Re: [gentoo-portage-dev] [PATCH 1/3] pym/portage/package/ebuild/fetch.py: Factor out _get_checksum_failure_max_tries
On Mon, Jan 20, 2014 at 02:26:59AM +0100, Tom Wijsman wrote:
> On Sat, 18 Jan 2014 19:07:45 -0800
> "W. Trevor King" wrote:
>
> > [...SNIP...]
> > + v =
> > int(settings.get("PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS",
> > + default))
> > …
>
> The code screams PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS to me which
> makes it hard to read, …
That's all copy-pasted out of fetch() under the assumption that the
fewer changes I made (besides creating _get_checksum_failure_max_tries
at all), the easier it would be to review those changes. Perhaps I
was mistaken ;).
> I would suggest assigning it to a variable in advance as to not have
> to repeat it this often. Some code snippets for ideas:
>
> key = "PORTAGE_FETCH_CHECKSUM_TRY_MIRRORS"
Will queue to v3.
Cheers,
Trevor
--
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy
signature.asc
Description: OpenPGP digital signature
Re: [gentoo-portage-dev] [PATCH 3/3] pym/portage/package/ebuild/fetch.py: Factor out _get_uris
On Sun, Jan 19, 2014 at 03:06:29PM -0800, W. Trevor King wrote: > On Sun, Jan 19, 2014 at 02:36:43PM -0800, Alec Warner wrote: > > On Sat, Jan 18, 2014 at 7:07 PM, W. Trevor King wrote: > > > + # Order primaryuri_dict values to match that in SRC_URI. > > > + for uris in primaryuri_dict.values(): > > > + uris.reverse() > > > > Is there any guaranteed ordering for dict.values()? > > No. > > > I thought dict order was random (and they seriously make it random > > in modern python versions, to detect bugs.) How does this > > uris.reverse() match the SRC_URI ordering? > > No idea (copy-pasted code). I'd be happy to cut this out in v3. Ah, we're not reversing the ordering of values(), we're reversing the ordering of each individual value. Keeping it in. Cheers, Trevor -- This email may be signed or encrypted with GnuPG (http://www.gnupg.org). For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy signature.asc Description: OpenPGP digital signature
Re: [gentoo-portage-dev] [PATCH 0/3] Initial fetch() refactoring
On Sun, Jan 19, 2014 at 02:46:36PM -0800, Alec Warner wrote: > On Sun, Jan 19, 2014 at 2:45 PM, Alexander Berntsen > wrote: > > On 19/01/14 22:22, Sebastian Luther wrote: > > > The usual doc string style used in portage is: > > > > > > """ text """ > > > > > > Please use that for new functions. Also make sure you don't use > > > spaces to indent the last """. > > > > As mentioned by Mike in another thread, we should use PEP 257[0]. I > > will convert old code to conform to this... sometime... soon... (I > > promise!) > > > > So if new patches could just do that right away, that would be neat. > > Does pylint or pyflakes point out if you mess it up? > > Automation for the win. As of Emacs 24.3.1, fill-paragraph (M-q) in python-mode will automatically format your docstring like this (which is how the space-indented-""" snuck into my v1 ;). Not as nice as an independent checker, but still useful automation. Cheers, Trevor -- This email may be signed or encrypted with GnuPG (http://www.gnupg.org). For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy signature.asc Description: OpenPGP digital signature
Re: [gentoo-portage-dev] [PATCH 3/3] pym/portage/package/ebuild/fetch.py: Factor out _get_uris
On Sun, Jan 19, 2014 at 02:36:43PM -0800, Alec Warner wrote:
> On Sat, Jan 18, 2014 at 7:07 PM, W. Trevor King wrote:
> > +def _get_file_uri_tuples(uris):
> > + """Return a list of (filename, uri) tuples
> > + """
> >
>
> As mike noted on another thread:
>
> ""Return a list of (filename, uri) tuples."""
I'd replaced this with:
"""
Return a list of (filename, uri) tuples
"""
in v2, but I'll queue the one-line form in v3.
> > +def _expand_mirror(uri, custom_mirrors=(), third_party_mirrors=()):
> > + """Replace the 'mirror://' scheme in the uri
> >
>
> Sentence should end in a period.
Ok. I'll do that for the other summaries as well, and uppercase URI,
in v3.
> > + writemsg(_("Invalid mirror definition in SRC_URI:\n"),
> > +noiselevel=-1)
> > + writemsg(" %s\n" % (uri), noiselevel=-1)
>
>
> Is this a py3k thing? You should be able to write:
>
> writemsg(" %s\n" % uri, noiselevel=-1)
>
> The tuple is only needed for multiple arguments in py2k.
1. I think I just copied and pasted it ;).
2. (uri) is not actually a tuple:
>>> type(('hello'))
To get a tuple, you need (uri,).
I'm fine removing the parens in v3.
> > +def _get_uris(uris, settings, custom_mirrors=(), locations=()):
> >
>
> I want you to be careful here. This is bad style when you are
> constructing mutable objects in parameter defaults:
I used tuples instead of lists because they are not mutable (as you
point out later on).
> I'm not sure if I appreciate that more or less than the other form
>
> def _get_uris(uris, settings, custom_mirrors=None, locations=None):
> if not custom_mirrors:
> custom_mirrors = ()
> if not locations:
> locations = ()
If you want me to do it that way, I'll queue it for v3. I think the
default tuples are more readable. They are certainly shorter.
> Another advisable way to write it is to simply not have default
> arguments, and to force the caller to sync in an empty iterable:
Meh. I don't have strong feelings about this, but custom_mirrors
struck me as something that would not always be required ;).
> > + third_party_mirrors = settings.thirdpartymirrors()
> >
>
> Why pass in all of settings?
We need other stuff too, see v2. The less settings parsing I need to
do in fetch() itself, the happier I am. If that makes the helper
functions a bit uglier, that's fine. Fewer people will have to read
those.
> Think about testing this function.
Working on it for v3.
> Do you really want to try to construct some sort of 'testing'
> settings object, or simply construct a list?
I was going to use a dict for testing, and stub out a
thirdpartymirrors mock for _get_uris.
> > + third_party_mirror_uris = {}
> > + filedict = OrderedDict()
> > + primaryuri_dict = {}
> > + for filename, uri in _get_file_uri_tuples(uris=uris):
> > + if filename not in filedict:
> > + filedict[filename] = [
> > + os.path.join(location, 'distfiles',
> > filename)
> > + for location in locations]
> > + if uri is None:
> > + continue
> > + if uri.startswith('mirror://'):
> > + uris = _expand_mirror(
> >
>
> too many uri / uris variables...
>
> can we called this 'expanded_uris'?
Queued for v3.
> I'm really having trouble tracking what is what. Remember that
> 'uris' is already a parameter to this function. Are you shadowing it
> here, or trying to overwrite it?
Clobbering it. The original value is only used for the
_get_file_uri_tuples call.
> > + uri=uri, custom_mirrors=custom_mirrors,
> > + third_party_mirrors=third_party_mirrors)
> > + filedict[filename].extend(uri for group, uri in
> > uris)
> >
>
> group appears unused in your implicit iterable here.
>
> perhaps:
>
> filedict[filename].extend(uri for _, uri in uris)
Queued for v3.
> > + third_party_mirror_uris.setdefault(filename,
> > []).extend(
> > + uri for group, uri in uris
> > + if group == 'third-party')
> >
>
> I'm curious if this matters. We are iter
Re: [gentoo-portage-dev] [PATCH v2 1/3] pym/portage/package/ebuild/fetch.py: Factor out _get_checksum_failure_max_tries
On Sun, Jan 19, 2014 at 02:45:24PM -0800, Alec Warner wrote: > On Sun, Jan 19, 2014 at 2:44 PM, Alec Warner wrote: > > This function and the next function you wrote are identical. How > > about making a single function? > > > > … > > > > def getIntValueFromSettings(settings, key, default): > > … > > Note, don't actually use these function names, they are terrible. A good name would be getint [1]. If we used ConfigParser, we wouldn't have to write any Portage-specific code at all ;). I don't think there's a shortage of things that could be streamlined here, and was trying to minimize rewriting until fetch() had been reduced to something I can comprehend ;). Cheers, Trevor [1]: http://docs.python.org/3/library/configparser.html#configparser.ConfigParser.getint -- This email may be signed or encrypted with GnuPG (http://www.gnupg.org). For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy signature.asc Description: OpenPGP digital signature
Re: [gentoo-portage-dev] [PATCH 0/3] Initial fetch() refactoring
On Sun, Jan 19, 2014 at 10:39:03PM +0100, Sebastian Luther wrote: > Am 19.01.2014 04:07, schrieb W. Trevor King: > > The patches aren't particularly well tested yet. I ran the test suite > > and got some errors, but they seemed to be related to my non-root > > invocation, and not due to these changes themselves. I thought I'd > > post my work so far, before digging deeper into the test suite. > > Since I doubt there's currently any test or any way to write one, I'm working on a pym/portage/tests/ebuild/test_fetch.py to exercise the helpers. Will post in v3. > I'd suggest to try and run emerge and analyze coverage with > dev-python/coverage or a similar tool. Also a good idea. I'll have to dust off my coverage notes ;). Thanks, Trevor -- This email may be signed or encrypted with GnuPG (http://www.gnupg.org). For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy signature.asc Description: OpenPGP digital signature
[gentoo-portage-dev] [PATCH v2 3/3] pym/portage/package/ebuild/fetch.py: Factor out _get_uris
The current fetch() function is quite long, which makes it hard to
know what I can change without adverse side effects. By pulling this
logic out of the main function, we get clearer logic in fetch() and
more explicit input for the config extraction.
This block was especially complicated, so I also created the helper
functions _get_file_uri_tuples and _expand_mirror. I'd prefer if
_expand_mirror iterated through URIs instead of (group, URI) tuples,
but we need a distinct marker for third-party URIs to build
third_party_mirror_uris which is used to build primaryuri_dict which
is used way down in fetch():
if checksum_failure_count == \
checksum_failure_primaryuri:
# Switch to "primaryuri" mode in order
# to increase the probablility of
# of success.
primaryuris = \
primaryuri_dict.get(myfile)
if primaryuris:
uri_list.extend(
reversed(primaryuris))
I don't know if this is useful enough to motivate the uglier
_expandmirror return values, but I'll kick that can down the road for
now.
---
pym/portage/package/ebuild/fetch.py | 209 ++--
1 file changed, 128 insertions(+), 81 deletions(-)
diff --git a/pym/portage/package/ebuild/fetch.py
b/pym/portage/package/ebuild/fetch.py
index de5bf00..a1940f4 100644
--- a/pym/portage/package/ebuild/fetch.py
+++ b/pym/portage/package/ebuild/fetch.py
@@ -15,9 +15,9 @@ import sys
import tempfile
try:
- from urllib.parse import urlparse
+ from urllib.parse import urlparse, urlunparse
except ImportError:
- from urlparse import urlparse
+ from urlparse import urlparse, urlunparse
import portage
portage.proxy.lazyimport.lazyimport(globals(),
@@ -298,6 +298,129 @@ def _get_fetch_resume_size(settings, default='350K'):
return v
+def _get_file_uri_tuples(uris):
+ """
+ Return a list of (filename, uri) tuples
+ """
+ file_uri_tuples = []
+ # Check for 'items' attribute since OrderedDict is not a dict.
+ if hasattr(uris, 'items'):
+ for filename, uri_set in uris.items():
+ for uri in uri_set:
+ file_uri_tuples.append((filename, uri))
+ if not uri_set:
+ file_uri_tuples.append((filename, None))
+ else:
+ for uri in uris:
+ if urlparse(uri).scheme:
+ file_uri_tuples.append(
+ (os.path.basename(uri), uri))
+ else:
+ file_uri_tuples.append(
+ (os.path.basename(uri), None))
+ return file_uri_tuples
+
+
+def _expand_mirror(uri, custom_mirrors=(), third_party_mirrors=()):
+ """
+ Replace the 'mirror://' scheme in the uri
+
+ Returns an iterable listing expanded (group, URI) tuples,
+ where the group is either 'custom' or 'third-party'.
+ """
+ parsed = urlparse(uri)
+ mirror = parsed.netloc
+ path = parsed.path
+ if path:
+ # Try user-defined mirrors first
+ if mirror in custom_mirrors:
+ for cmirr in custom_mirrors[mirror]:
+ m_uri = urlparse(cmirr)
+ yield ('custom', urlunparse((
+ m_uri.scheme, m_uri.netloc, path) +
+ parsed[3:]))
+
+ # now try the official mirrors
+ if mirror in third_party_mirrors:
+ uris = []
+ for locmirr in third_party_mirrors[mirror]:
+ m_uri = urlparse(locmirr)
+ uris.append(urlunparse((
+ m_uri.scheme, m_uri.netloc, path) +
+ parsed[3:]))
+ random.shuffle(uris)
+ for uri in uris:
+ yield ('third-party', uri)
+
+ if (not custom_mirrors.get(mirror, []) and
+ not third_party_mirrors.get(mirror, [])):
+ writemsg(
+ _("No known mirror by the name: %s\n")
+ % (mirror,))
+ else:
+ writemsg(_("Invalid mirror definition in SRC_URI:\n"),
+noiselevel=-1)
+ writemsg(" %s\n" % (uri), noiselevel=-1)
+
+
+def _get_uris(uris, settings, custom_mirrors=(), locations=()):
+ restrict = settings.get("PORTAGE_RESTRICT", "").split()
+ restrict_fetch = "fetch" in restrict
+ restrict_mirror = "mirror" in restrict or "nomirror" in restrict
+ force_mirror = (
+ "force-mirror" in settings.features and
+ not restrict_mirror)
+
+
[gentoo-portage-dev] [PATCH v2 1/3] pym/portage/package/ebuild/fetch.py: Factor out _get_checksum_failure_max_tries
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 0/3] Initial fetch() refactoring
Following Sebastian's pyflakes and testing suggestions turned up a few bugs in the _get_uris patch. Changes since v1: Patch #1: * Fixed: """ to: """ and converted some from tabs to spaces where I'd missed them in v1. Patch #3: * Fixed docstrings as for patch #1. * Fix 'myuri' → 'uri' in _get_file_uri_tuples. * Fix 'cmirr' → 'm_uri' in _expand_mirror's urlunparse calls. * Fix 'cmirr' → 'locmirr' in _expand_mirror's third_party_mirrors branch. * Move “No known mirror” error from _get_uris to _expand_mirror. * Calculate 'restrict', 'restrict_fetch', 'restrict_mirror', and 'force_mirror' in _get_uris. * Remove 'force_mirror' from fetch, because it's only used in _get_uris. * Fix 'custom_mirrors' → 'custommirrors' in _get_uris invocation. After these changes, runtests.sh passes with only TODO messages on Python 2.7, 3.2, and 3.3, although I'm not sure how thoroughly the test suite covers fetch. There are also a few unrelated: /usr/lib64/python3.3/xml/etree/ElementTree.py:1726: DeprecationWarning: This method of XMLParser is deprecated. Define doctype() method on the TreeBuilder target. parser.feed(data) warnings in the 3.3 results. For folks who prefer Git fetches to the emailed patches, my current series is at: git://tremily.us/portage.git fetch-refactor For those who prefer Git diffs to the above “changes since v1”, the v1 version of this series is tagged in my repository as fetch-refactor-v1. Sebastian, I'd normally cc you directly on v2, but I'm not sure what the Portage team's conventions are here. Let me know if there is a convention, or if you have a personal preference on direct mail vs the list. I think projects that encourage cc-ing tend to have reasonable numbers of NNTP readers, and I don't know where the gentoo-portage-dev@ population falls on that issue. W. Trevor King (3): pym/portage/package/ebuild/fetch.py: Factor out _get_checksum_failure_max_tries pym/portage/package/ebuild/fetch.py: Factor out _get_fetch_resume_size pym/portage/package/ebuild/fetch.py: Factor out _get_uris pym/portage/package/ebuild/fetch.py | 318 +--- 1 file changed, 189 insertions(+), 129 deletions(-) -- 1.8.5.2.8.g0f6c0d1
[gentoo-portage-dev] [PATCH v2 2/3] pym/portage/package/ebuild/fetch.py: Factor out _get_fetch_resume_size
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
Re: [gentoo-portage-dev] [PATCH 3/3] pym/portage/package/ebuild/fetch.py: Factor out _get_uris
On Sun, Jan 19, 2014 at 10:36:37PM +0100, Sebastian Luther wrote: > Am 19.01.2014 04:07, schrieb W. Trevor King: > > +def _get_uris(uris, settings, custom_mirrors=(), locations=()): > > missing doc string I'm still not entirely clear on what this function does ;). Splitting fetch() into chunks is giving me a clearer idea of how it works, but I don't understand the distinction between filedict and primaryuri_dict. I thought I had enough of an understanding to name the function, but not yet enough to document it. Cheers, Trevor -- This email may be signed or encrypted with GnuPG (http://www.gnupg.org). For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy signature.asc Description: OpenPGP digital signature
Re: [gentoo-portage-dev] [PATCH 0/3] Initial fetch() refactoring
On Sun, Jan 19, 2014 at 09:05:41PM +0100, Sebastian Luther wrote: > Am 19.01.2014 04:07, schrieb W. Trevor King: > > The patches aren't particularly well tested yet. I ran the test > > suite and got some errors, but they seemed to be related to my > > non-root invocation, and not due to these changes themselves. I > > thought I'd post my work so far, before digging deeper into the > > test suite. > > The tests can be run as non-root user. Make sure all the files in > the git repo are +rw for that user. Ok. I'll go back and check the results on master so I can compare. > You should check the result using pyflakes. Thanks. This picked up a few errors in the third patch. I'll post v2 shortly. > Do you have these patches in the publicly accessible git repo? I do now: git://tremily.us/portage.git fetch-refactor Cheers, Trevor -- This email may be signed or encrypted with GnuPG (http://www.gnupg.org). For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy signature.asc Description: OpenPGP digital signature
Re: [gentoo-portage-dev] Signing off patches
On Sun, Jan 19, 2014 at 02:33:06AM +0100, Tom Wijsman wrote: > On Sat, 18 Jan 2014 15:24:59 -0800 > "W. Trevor King" wrote: > > If it doesn't need to get updated, then it probably already > > started out explaining the consensus ;). > > That is a guess, you can look this up in past patches. Sure. Will you? If I want to touch some code, and it looks confusing, I'll use blame to see who wrote it and whay they were thinking about. If the commit message is not informative, I usually give up. I have a hard time imaging folks tracking down the thread that spawned that patch, assuming such a thread even exists, for each troublesome line they'd like to touch. It's much easier to summarize any issues the list resolved (because they're likely the same questions the new dev is asking) in the commit message, where future developers can find them. > > You spend time if you want to spend time and add whoever you feel > > moved to add. > > We discuss whether to make it policy to add involved people. But “involved” can be hard to pin down, especially by someone who may be applying v5 of a patch that hasn't been carefully following the whole discussion in earlier versions. The Linux kernel docs say [1]: If this patch fixes a problem reported by somebody else, consider adding a Reported-by: tag to credit the reporter for their contribution. Note the “consider” wiggle word. They are a bit more formal about Reviewed-by, but only because it's signing off on their Reviewer's statement of oversight. In general, if you're not signing some statement with the tag, formalizing “involved” is hard. > > If you are submitting v2 of a patch, and feel a desire > > to credit reviewers / testers with this syntax, I think that's > > considerate of you. If you are committing someone else's patch to > > master, and want to record the folks who acked it on the list to > > distribute responsibility, that's fine too. If you want to use > > another syntax, or not do any of this at all, it's still fine by me > > ;). However, if a consistent syntax already exists, I see no reason > > not to use it when it suits your purpose. > > We discuss here whether to make it policy to use the same syntax. I don't understand the distinction between “here are some guidelines, apply as and if you see fit” and “make it a policy to …”. Say you have a situation like this: 1. Alice submits a bug-report to bugs.g.o. 2. Bob codes up a Portage patch and sends it to the list. 3. Charlie responds to Bob's patch on the list with "Reviewed-by". 4. Dan responds to Bob's patch on the list with "Reviewed-by", and asks for any opposition. … time passes, and nobody speaks up … 5. Dan applies Bob's patch to the master branch, but neglects: Submitted-by: Alice Reviewed-by: Charlie Reviewed-by: Dan 6. ? As I understand it, 6 should be: 6a. Everyone gets on with their lives. I could see a situation where: 6b. Charlie reminds Dan that he could have used the tags. Everyone gets on with their lives. Is there another alternative step 6 implied by the “policy” keyword? Or is the policy workflow even more different somehow? Cheers, Trevor [1]: https://www.kernel.org/doc/Documentation/SubmittingPatches -- This email may be signed or encrypted with GnuPG (http://www.gnupg.org). For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy signature.asc Description: OpenPGP digital signature
[gentoo-portage-dev] [PATCH 3/3] pym/portage/package/ebuild/fetch.py: Factor out _get_uris
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(
[gentoo-portage-dev] [PATCH 0/3] Initial fetch() refactoring
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
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
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
Re: [gentoo-portage-dev] Signing off patches
On Sat, Jan 18, 2014 at 11:57:38PM +0100, Tom Wijsman wrote: > On Sat, 18 Jan 2014 08:43:12 -0800 Trevor King wrote: > > The body of the commit message should summarize the consensus > > reached on the mailing list, > > That message is written as part of the patch that is reviewed; it > rarely gets updated with the consensus, unless we suggest / require > people to do that. However, similar to vapier's response, I'd think > introducing such processes feel like unnecessary efforts. If it doesn't need to get updated, then it probably already started out explaining the consensus ;). > > and these tags are basically standardized thank-you notes crediting > > non-authors who were involved in that process. They don't have to go > > on every patch, but if you want to mention somebody: > > > > Reviewed-by: Random J Developer > > Reviewed-by: Other R Developer > > > > at the end of the commit message is easier to write and read than: > > > > This patch was reviewed Random J Developer > >and Other R Developer > > . > > Exactly: Do we want to spend time on this or not? Do we add everyone > involved? Or do we just add people whom are not on the Portage team? You spend time if you want to spend time and add whoever you feel moved to add. I think the spirit of Alexander's original proposal [1] was “here is a common syntax for crediting collaborators, we might want to use it” not “ye non-conformers will be hounded unto the ends of the Earth”. If you are submitting v2 of a patch, and feel a desire to credit reviewers / testers with this syntax, I think that's considerate of you. If you are committing someone else's patch to master, and want to record the folks who acked it on the list to distribute responsibility, that's fine too. If you want to use another syntax, or not do any of this at all, it's still fine by me ;). However, if a consistent syntax already exists, I see no reason not to use it when it suits your purpose. > Unless we intend to introduce this for statistics, although I think > that prior annotation history missing as well as people that will > casually forgot to add these annotations will make the statistics a > misrepresentation. I agree that statistics based on these tags are not meaningful. Cheers, Trevor [1]: http://thread.gmane.org/gmane.linux.gentoo.portage.devel/3948 -- This email may be signed or encrypted with GnuPG (http://www.gnupg.org). For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy signature.asc Description: OpenPGP digital signature
Re: [gentoo-portage-dev] Signing off patches
On Sat, Jan 18, 2014 at 04:02:02PM +0100, Tom Wijsman wrote: > - Reviewed-by: Reviewed the patch thoroughly > >What extra information does this tell us? This can be seen on ML. I think the idea is that you shouldn't need to refer to an external resource like the mailing list to understand the idea behind the patch, or the amount of review it received. The body of the commit message should summarize the consensus reached on the mailing list, and these tags are basically standardized thank-you notes crediting non-authors who were involved in that process. They don't have to go on every patch, but if you want to mention somebody: Reviewed-by: Random J Developer Reviewed-by: Other R Developer at the end of the commit message is easier to write and read than: This patch was reviewed Random J Developer and Other R Developer . Cheers, Trevor -- This email may be signed or encrypted with GnuPG (http://www.gnupg.org). For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy signature.asc Description: OpenPGP digital signature
Re: [gentoo-portage-dev] Re: Signing off patches
On Thu, Jan 16, 2014 at 07:54:57PM +, Duncan wrote: > And one final note: A signed-off-by is a useful indicator of a patch that > an author considers ready to go, pending review, etc. Lack of that (from > a seasoned submitter who is familiar with the process) can be an > indication that the author believes the patch is or may be preliminary, > and they're not yet ready for integration-tree inclusion or final review, > tho they usually say as much in the patch description as well. You can also use: $ git format-patch --subject-prefix RFC … to mark preliminary patches with [RFC] tags in the subject. It also works with versions: $ git format-patch --subject-prefix RFC -v2 … will generate [RFC v2] tags. Once you think it's mergable, drop the --subject-prefix to get [PATCH] tags. Cheers, Trevor -- This email may be signed or encrypted with GnuPG (http://www.gnupg.org). For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy signature.asc Description: OpenPGP digital signature
Re: [gentoo-portage-dev] Signing off patches
On Thu, Jan 16, 2014 at 06:05:50PM +0100, Alexander Berntsen wrote: > On 16/01/14 17:45, W. Trevor King wrote: > > I love Signed-off-by, but in all projects where I've seen it used > > it means the signer is agreeing to some form of a Developer's > > Certificate of Origin [1]. Without such a DCO, I think the usual > > commit author is sufficient. > > I agree. However, it might be prudent to introduce a DCO. After all, > copyright is assigned to the Gentoo Foundation. If you add a DCO (and I'd certainly think that would be prudent if you require copyright assignment), then you probably don't need a separate Assisted-by. Anyone with enough co-authorship to matter will be using a Signed-off-by. Cheers, Trevor -- This email may be signed or encrypted with GnuPG (http://www.gnupg.org). For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy signature.asc Description: OpenPGP digital signature
Re: [gentoo-portage-dev] Signing off patches
On Thu, Jan 16, 2014 at 02:20:27PM +0100, Alexander Berntsen wrote: > Signed-off-by: Wrote (a substantial portion of) the patch > … > These suggestions all stem from the Linux project. I love Signed-off-by, but in all projects where I've seen it used it means the signer is agreeing to some form of a Developer's Certificate of Origin [1]. Without such a DCO, I think the usual commit author is sufficient. Co-authors can be listed with: Assisted-by: or something. Two cents from an outsider, Trevor [1]: https://www.kernel.org/doc/Documentation/SubmittingPatches -- This email may be signed or encrypted with GnuPG (http://www.gnupg.org). For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy signature.asc Description: OpenPGP digital signature
[gentoo-portage-dev] Re: Commit Policy
On Mon, Jan 13, 2014 at 07:43:27AM +0100, Sebastian Luther wrote: > Put this in ~/.gitconfig: > [color] > ui = auto This is the default for Git >=1.8.4, so you don't need to bother setting it by hand if you're running an ~arch ;). Cheers, Trevor -- This email may be signed or encrypted with GnuPG (http://www.gnupg.org). For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy signature.asc Description: OpenPGP digital signature
