[gentoo-portage-dev] Repoman patches for bugs #205909, #245305 and #482084.

[gentoo-portage-dev] Repoman patches for bugs #205909, #245305 and #482084.

[Tom Wijsman]

In reply, you will find three repoman patches; PATCH 1 is a bit more complex
which I will detail here, the other two patches should be fairly trivial.

In the first patch I need to use the @system set, as I only want to check
DEPEND for packages not in the @system set; thus here is kept in mind that
the @system set could possible change, in which case the check continues to
work. After checking up with Arfrever, a first version that I came up with is

> +from portage._sets.profiles import PackagesSystemSet
> +system_set_atoms = PackagesSystemSet(portage.settings.profiles).getAtoms()

but I am not sure if this is appropriate when used in other repositories.
Arfrever warned me about this but I think I do not fully understand that.

Both archive_formats* variables are based on the PMS specifications.

The rest of the code has comments and should be trivial to understand.

For the check name we came up with "unpack.DEPEND.missing"; most of the
checks are two words, so, I don't know if three words is permitted. At least
repoman runs without complaining or bailing out because of this.

There's still a TODO left in the code that leaves me in doubt on how to
properly ask the keywords to Portage; seems that I still need to learn to
find my way through the documentation, but I guess after getting pointed to
it a few times it will become easier.

These are my first patches to the Repoman code, all three patches introduce a 
new warning / error, therefore it might be possible that I missed something.
Grepping on an existing warning I saw the man page needs to be updated; since
I never did that before, feel free to check if the syntax of that is right.

Thank you for taking your time to review these.

--
 bin/repoman           | 63 ++++++++++++++++++++++++++++++++++++++++++++++++
                            +++++++++++++++
 man/repoman.1         | 10 ++++++++++
 pym/repoman/checks.py | 10 ++++++++++
 3 files changed, 83 insertions(+)

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

 [PATCH 2/3] Have repoman check that a package directory contains at least
             one ebuild (bug #245305).

 [PATCH 3/3] Have repoman deprecate G2CONF for the GNOME team. (bug #482084).

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

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

[Tom Wijsman]

---
 bin/repoman   | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 man/repoman.1 |  4 ++++
 2 files changed, 57 insertions(+)

diff --git a/bin/repoman b/bin/repoman
index d1542e9..9b703dc 100755
--- a/bin/repoman
+++ b/bin/repoman
@@ -36,6 +36,9 @@ pym_path = osp.join(osp.dirname(osp.dirname(osp.realpath(__file__))), "pym")
 sys.path.insert(0, pym_path)
 import portage
 portage._internal_caller = True
+
+from portage._sets.profiles import PackagesSystemSet
+system_set_atoms = PackagesSystemSet(portage.settings.profiles).getAtoms()
 portage._disable_legacy_globals()
 
 try:
@@ -300,6 +303,7 @@ qahelp = {
 	"inherit.missing": "Ebuild uses functions from an eclass but does not inherit it",
 	"inherit.unused": "Ebuild inherits an eclass but does not use it",
 	"java.eclassesnotused": "With virtual/jdk in DEPEND you must inherit a java eclass",
+	"unpack.DEPEND.missing": "A rare archive format was used in SRC_URI, but its package to unpack it is missing in DEPEND.",
 	"wxwidgets.eclassnotused": "Ebuild DEPENDs on x11-libs/wxGTK without inheriting wxwidgets.eclass",
 	"KEYWORDS.dropped": "Ebuilds that appear to have dropped KEYWORDS for some arch",
 	"KEYWORDS.missing": "Ebuilds that have a missing or empty KEYWORDS variable",
@@ -399,6 +403,7 @@ qawarnings = set((
 "metadata.warning",
 "portage.internal",
 "repo.eapi.deprecated",
+"unpack.DEPEND.missing",
 "usage.obsolete",
 "upstream.workaround",
 "LIVEVCS.stable",
@@ -479,6 +484,25 @@ ruby_deprecated = frozenset([
 	"ruby_targets_ree18",
 ])
 
+# TODO: Add functionality to support checking for deb2targz on platforms where
+#       GNU binutils is absent; see PMS 5, section 11.3.3.13.
+archive_formats = {
+	"\.7[zZ]":"app-arch/p7zip",
+	"\.(bz2?|tbz2)":"app-arch/bzip2",
+	"\.jar":"app-arch/unzip",
+	"\.(LH[aA]|lha|lzh)":"app-arch/lha",
+	"\.lzma":"app-arch/lzma-utils",
+	"\.(rar|RAR)":"app-arch/unrar",
+	"\.(tar(\.(bz2?|gz|Z))?|tbz2|t[bg]z)?":"app-arch/tar",
+	"\.(gz|tar\.Z|t[bg]z|[zZ])":"app-arch/gzip",
+	"\.(zip|ZIP)":"app-arch/unzip",
+}
+
+archive_formats_eapi_3_to_5 = {
+	"\.tar.xz":"app-arch/tar",
+	"\.xz":"app-arch/xz-utils",
+}
+
 metadata_xml_encoding = 'UTF-8'
 metadata_xml_declaration = '<?xml version="1.0" encoding="%s"?>' % \
 	(metadata_xml_encoding,)
@@ -1559,6 +1583,7 @@ for x in effective_scanlist:
 	fetchlist_dict = portage.FetchlistDict(checkdir, repoman_settings, portdb)
 	myfiles_all = []
 	src_uri_error = False
+	needed_unpack_depends = {}
 	for mykey in fetchlist_dict:
 		try:
 			myfiles_all.extend(fetchlist_dict[mykey])
@@ -1573,7 +1598,22 @@ for x in effective_scanlist:
 				stats["SRC_URI.syntax"] += 1
 				fails["SRC_URI.syntax"].append(
 					"%s.ebuild SRC_URI: %s" % (mykey, e))
+
+		# Compare each SRC_URI entry against archive_formats; if one of the
+		# extensions match, we remember which archive depends are needed to
+		# check them later on.
+		needed_unpack_depends[mykey] = []
+		for file_extension in archive_formats or \
+			((re.match('[345]$', eapi) is not None) \
+				and file_extension in archive_formats_eapi_3_to_5):
+			for entry in fetchlist_dict[mykey]:
+				if re.match('.*%s$' % file_extension, entry) is not None:
+					format = archive_formats[file_extension]
+
+					if format not in needed_unpack_depends[mykey]:
+						needed_unpack_depends[mykey].append(format)
 	del fetchlist_dict
+
 	if not src_uri_error:
 		# This test can produce false positives if SRC_URI could not
 		# be parsed for one or more ebuilds. There's no point in
@@ -2010,6 +2050,17 @@ for x in effective_scanlist:
 				atoms = None
 				badsyntax.append(str(e))
 
+			if atoms and mytype == 'DEPEND':
+				# We check whether the needed archive dependencies are present
+				# in DEPEND, which were determined from SRC_URI.
+				for entry in needed_unpack_depends[catdir + '/' + y]:
+					if entry not in system_set_atoms and entry \
+						not in [atom.cp for atom in atoms if atom != "||"]:
+						stats['unpack.' + mytype + '.missing'] += 1
+						fails['unpack.' + mytype + '.missing'].append( \
+							relative_path + ": %s is missing in %s" % \
+							(entry, mytype))
+
 			if atoms and mytype.endswith("DEPEND"):
 				if runtime and \
 					"test?" in mydepstr.split():
@@ -2384,6 +2435,8 @@ for x in effective_scanlist:
 				"%s/metadata.xml: unused local USE-description: '%s'" % \
 				(x, myflag))
 
+	del needed_unpack_depends
+
 if options.if_modified == "y" and len(effective_scanlist) < 1:
 	logging.warn("--if-modified is enabled, but no modified packages were found!")
 
diff --git a/man/repoman.1 b/man/repoman.1
index a78f94e..e739d56 100644
--- a/man/repoman.1
+++ b/man/repoman.1
@@ -334,6 +334,10 @@ Ebuild inherits a deprecated eclass
 With virtual/jdk in DEPEND you must inherit a java eclass. Refer to
 \fIhttp://www.gentoo.org/proj/en/java/java\-devel.xml\fR for more information.
 .TP
+.B unpack.DEPEND.missing
+A rare archive format was used in SRC_URI, but its package to unpack it is
+missing in DEPEND.
+TP
 .B manifest.bad
 Manifest has missing or incorrect digests
 .TP
-- 
1.8.5.2

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

[Alec Warner]

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

On Wed, Jan 15, 2014 at 4:07 PM, Tom Wijsman <tomwij@gentoo.org> wrote:

> ---
>  bin/repoman   | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  man/repoman.1 |  4 ++++
>  2 files changed, 57 insertions(+)
>
>
I urge you to not author new checks like this. /usr/bin/repoman is already
a mess.

Write these checks as functions, put them in a different file, and just
call them from the giant messy loop. At least that way the checks are self
contained (great for avoiding things like variable re-use or shadowing).

-A



> diff --git a/bin/repoman b/bin/repoman
> index d1542e9..9b703dc 100755
> --- a/bin/repoman
> +++ b/bin/repoman
> @@ -36,6 +36,9 @@ pym_path =
> osp.join(osp.dirname(osp.dirname(osp.realpath(__file__))), "pym")
>  sys.path.insert(0, pym_path)
>  import portage
>  portage._internal_caller = True
> +
> +from portage._sets.profiles import PackagesSystemSet
> +system_set_atoms = PackagesSystemSet(portage.settings.profiles).getAtoms()
>  portage._disable_legacy_globals()
>
>  try:
> @@ -300,6 +303,7 @@ qahelp = {
>         "inherit.missing": "Ebuild uses functions from an eclass but does
> not inherit it",
>         "inherit.unused": "Ebuild inherits an eclass but does not use it",
>         "java.eclassesnotused": "With virtual/jdk in DEPEND you must
> inherit a java eclass",
> +       "unpack.DEPEND.missing": "A rare archive format was used in
> SRC_URI, but its package to unpack it is missing in DEPEND.",
>         "wxwidgets.eclassnotused": "Ebuild DEPENDs on x11-libs/wxGTK
> without inheriting wxwidgets.eclass",
>         "KEYWORDS.dropped": "Ebuilds that appear to have dropped KEYWORDS
> for some arch",
>         "KEYWORDS.missing": "Ebuilds that have a missing or empty KEYWORDS
> variable",
> @@ -399,6 +403,7 @@ qawarnings = set((
>  "metadata.warning",
>  "portage.internal",
>  "repo.eapi.deprecated",
> +"unpack.DEPEND.missing",
>  "usage.obsolete",
>  "upstream.workaround",
>  "LIVEVCS.stable",
> @@ -479,6 +484,25 @@ ruby_deprecated = frozenset([
>         "ruby_targets_ree18",
>  ])
>
> +# TODO: Add functionality to support checking for deb2targz on platforms
> where
> +#       GNU binutils is absent; see PMS 5, section 11.3.3.13.
> +archive_formats = {
> +       "\.7[zZ]":"app-arch/p7zip",
> +       "\.(bz2?|tbz2)":"app-arch/bzip2",
> +       "\.jar":"app-arch/unzip",
> +       "\.(LH[aA]|lha|lzh)":"app-arch/lha",
> +       "\.lzma":"app-arch/lzma-utils",
> +       "\.(rar|RAR)":"app-arch/unrar",
> +       "\.(tar(\.(bz2?|gz|Z))?|tbz2|t[bg]z)?":"app-arch/tar",
> +       "\.(gz|tar\.Z|t[bg]z|[zZ])":"app-arch/gzip",
> +       "\.(zip|ZIP)":"app-arch/unzip",
> +}
> +
> +archive_formats_eapi_3_to_5 = {
> +       "\.tar.xz":"app-arch/tar",
> +       "\.xz":"app-arch/xz-utils",
> +}
> +
>  metadata_xml_encoding = 'UTF-8'
>  metadata_xml_declaration = '<?xml version="1.0" encoding="%s"?>' % \
>         (metadata_xml_encoding,)
> @@ -1559,6 +1583,7 @@ for x in effective_scanlist:
>         fetchlist_dict = portage.FetchlistDict(checkdir, repoman_settings,
> portdb)
>         myfiles_all = []
>         src_uri_error = False
> +       needed_unpack_depends = {}
>         for mykey in fetchlist_dict:
>                 try:
>                         myfiles_all.extend(fetchlist_dict[mykey])
> @@ -1573,7 +1598,22 @@ for x in effective_scanlist:
>                                 stats["SRC_URI.syntax"] += 1
>                                 fails["SRC_URI.syntax"].append(
>                                         "%s.ebuild SRC_URI: %s" % (mykey,
> e))
> +
> +               # Compare each SRC_URI entry against archive_formats; if
> one of the
> +               # extensions match, we remember which archive depends are
> needed to
> +               # check them later on.
> +               needed_unpack_depends[mykey] = []
> +               for file_extension in archive_formats or \
> +                       ((re.match('[345]$', eapi) is not None) \
> +                               and file_extension in
> archive_formats_eapi_3_to_5):
> +                       for entry in fetchlist_dict[mykey]:
> +                               if re.match('.*%s$' % file_extension,
> entry) is not None:
> +                                       format =
> archive_formats[file_extension]
> +
> +                                       if format not in
> needed_unpack_depends[mykey]:
> +
> needed_unpack_depends[mykey].append(format)
>         del fetchlist_dict
> +
>         if not src_uri_error:
>                 # This test can produce false positives if SRC_URI could
> not
>                 # be parsed for one or more ebuilds. There's no point in
> @@ -2010,6 +2050,17 @@ for x in effective_scanlist:
>                                 atoms = None
>                                 badsyntax.append(str(e))
>
> +                       if atoms and mytype == 'DEPEND':
> +                               # We check whether the needed archive
> dependencies are present
> +                               # in DEPEND, which were determined from
> SRC_URI.
> +                               for entry in needed_unpack_depends[catdir
> + '/' + y]:
> +                                       if entry not in system_set_atoms
> and entry \
> +                                               not in [atom.cp for atom
> in atoms if atom != "||"]:
> +                                               stats['unpack.' + mytype +
> '.missing'] += 1
> +                                               fails['unpack.' + mytype +
> '.missing'].append( \
> +                                                       relative_path + ":
> %s is missing in %s" % \
> +                                                       (entry, mytype))
> +
>                         if atoms and mytype.endswith("DEPEND"):
>                                 if runtime and \
>                                         "test?" in mydepstr.split():
> @@ -2384,6 +2435,8 @@ for x in effective_scanlist:
>                                 "%s/metadata.xml: unused local
> USE-description: '%s'" % \
>                                 (x, myflag))
>
> +       del needed_unpack_depends
> +
>  if options.if_modified == "y" and len(effective_scanlist) < 1:
>         logging.warn("--if-modified is enabled, but no modified packages
> were found!")
>
> diff --git a/man/repoman.1 b/man/repoman.1
> index a78f94e..e739d56 100644
> --- a/man/repoman.1
> +++ b/man/repoman.1
> @@ -334,6 +334,10 @@ Ebuild inherits a deprecated eclass
>  With virtual/jdk in DEPEND you must inherit a java eclass. Refer to
>  \fIhttp://www.gentoo.org/proj/en/java/java\-devel.xml\fR for more
> information.
>  .TP
> +.B unpack.DEPEND.missing
> +A rare archive format was used in SRC_URI, but its package to unpack it is
> +missing in DEPEND.
> +TP
>  .B manifest.bad
>  Manifest has missing or incorrect digests
>  .TP
> --
> 1.8.5.2
>
>
>

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

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

[Tom Wijsman]

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

On Wed, 15 Jan 2014 17:44:15 -0800
Alec Warner <antarus@gentoo.org> wrote:

> On Wed, Jan 15, 2014 at 4:07 PM, Tom Wijsman <tomwij@gentoo.org>
> wrote:
> 
> > ---
> >  bin/repoman   | 53
> > +++++++++++++++++++++++++++++++++++++++++++++++++++++ man/repoman.1
> > |  4 ++++ 2 files changed, 57 insertions(+)
> >
> >
> I urge you to not author new checks like this. /usr/bin/repoman is
> already a mess.
> 
> Write these checks as functions, put them in a different file, and
> just call them from the giant messy loop. At least that way the
> checks are self contained (great for avoiding things like variable
> re-use or shadowing).

My plan is to first work a bit on repoman to get to know it, then when
knowing better where everything is work on refactoring it. If I start
to refactor right away, or start adding random files without knowing
where everything is; I would create a more broken design than it is.

Don't worry, I plan on a fully refactored Portage as well as good
practices when adding new checks; which will definitely become
necessary as we go forward. There's ~80 bugs, imagine all the code that
that would bring; definitely don't want those files to grow much longer.

-- 
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] Have repoman check if the packages to unpack rare archive formats from SRC_URI are present in DEPEND (bug #205909).

[Alexander Berntsen]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On 16/01/14 22:18, Tom Wijsman wrote:
> My plan is to first work a bit on repoman to get to know it, then 
> when knowing better where everything is work on refactoring it.
That, along with "I'll use this ugly short cut, but only this one
time!", is what they all say. Plans change. Things happen.

> If I start to refactor right away, or start adding random files 
> without knowing where everything is; I would create a more broken 
> design than it is.
Yes. Please think before you start hammering your keyboard. The keyboard
is just a means to an end.

> Don't worry, I plan on a fully refactored Portage as well as good 
> practices when adding new checks; which will definitely become 
> necessary as we go forward.
We have to worry. Because, as mentioned, plans change; things happen.


Please do not push this patch. And please do not carry on with this
mentality of "yes, I know what I am doing is wrong, but I'll fix it
eventually, some day, I promise."
- -- 
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/

iF4EAREIAAYFAlLYTd4ACgkQRtClrXBQc7VNmQD+KhIxhOAC4/IPB4hn+ugL1NIH
zNtPtTObzBMIivybstQBAISvE55N7Gb7hh6os+pZcvVilVqhhQP/V3b1RIk9tC2j
=0lAa
-----END PGP SIGNATURE-----

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

[Tom Wijsman]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On Thu, 16 Jan 2014 22:23:42 +0100
Alexander Berntsen <alexander@plaimi.net> wrote:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA256
> 
> On 16/01/14 22:18, Tom Wijsman wrote:
> > My plan is to first work a bit on repoman to get to know it, then 
> > when knowing better where everything is work on refactoring it.
> That, along with "I'll use this ugly short cut, but only this one
> time!", is what they all say. Plans change. Things happen.

"I'll rewrite everything first, hopefully I finish some day!"

> > If I start to refactor right away, or start adding random files 
> > without knowing where everything is; I would create a more broken 
> > design than it is.
> Yes. Please think before you start hammering your keyboard. The
> keyboard is just a means to an end.

You are quoting my thoughts, which came up before hammering; suffices.

> > Don't worry, I plan on a fully refactored Portage as well as good 
> > practices when adding new checks; which will definitely become 
> > necessary as we go forward.
> We have to worry. Because, as mentioned, plans change; things happen.

Don't do that. Because, as mentioned, necessity stays; progress happens.

> Please do not push this patch. And please do not carry on with this
> mentality of "yes, I know what I am doing is wrong, but I'll fix it
> eventually, some day, I promise."

Please do not stop progress. And please do not carry on with this
mentality of "yes, I know what I am doing is right, but I won't break it
eventually, some day, I promise.".

- -- 
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
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iQEcBAEBAgAGBQJS2FSNAAoJEJWyH81tNOV9Ag8IALyxWWZULciNsD8u2fHfm7ct
g0UYPG0rIffgmKAUfKED5kaIEit5NAWoZgzhdvec85l8aSOHHmHgT9LyEyoqUs4Z
6/wSrHgwynwNiOd1083ourAeLUz+UfOVuZ2uvYiym1qRhWbWPa8k3s6ksgqE48uY
h/X8nV5lfCG9uD+5k1cFG7CQRVZZM91Wo7VzqwGw/ltZepQLqLzALHLVH/yED0bt
js4NbLI6c3DObqucG/cypPI9OU6duzaOILq56g14h4hj39x1LjhAlXeE3IpcrPr1
cwj+7sXBcmhRFpiod9zTkjfCxdwU0GLiCdI4GTK+9RzNJP8WgYR1WEvWCjfntfE=
=0BgF
-----END PGP SIGNATURE-----

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

[Alexander Berntsen]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

Your ill-placed attempts at being clever are missing the point.
Portage is a mess. We don't need it to become more messy to the point
of maintainability.

Yes, no one fixing bugs (because they are all designing a grand
redesign of Portage) would be bad. However, this is not likely to
happen. It hasn't happened before. And now we have a bunch of great
new volunteers.

Sebastian even *told* you specifically how to do this properly. So
please do.

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

iF4EAREIAAYFAlLYW7QACgkQRtClrXBQc7WgOwEAqOuBXIMZyWbXRyY9VQEbwDCg
tXmXa266YsSYgqRNlDMBAJTq9pyOvIhqhRPjLTrlA3EodPUSiDY5wRlMjSZ08xeE
=WljA
-----END PGP SIGNATURE-----

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

[Tom Wijsman]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On Thu, 16 Jan 2014 23:22:44 +0100
Alexander Berntsen <alexander@plaimi.net> wrote:

> Your ill-placed attempts at being clever are missing the point.

Why are they missing the point?

> Portage is a mess. We don't need it to become more messy to the point
> of maintainability.

How do we plan to fix that?

> Yes, no one fixing bugs (because they are all designing a grand
> redesign of Portage) would be bad.

Do you agree?

> However, this is not likely to happen.

Why do you think so?

> It hasn't happened before.

It could happen in the future.

> And now we have a bunch of great new volunteers.

Yes, we do.

> Sebastian even *told* you specifically how to do this properly.

Where did he?

> So please do.

Why?

- -- 
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
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iQEcBAEBAgAGBQJS2GT9AAoJEJWyH81tNOV95ukH+wZ0yB4KOgfOd6z90cpYC0Ec
4RLK8HbKVYIIytxnnhR5Ny/BR5/ANlOYQDIFUytkJyKNmVPx3nP6kY+wHD5qOYkF
3DlJpxRy6wx/E43+wpvVv+dSNDHxkvyy8OeZ5QuAcFi1oYaeYdctfOU4/URihGzO
MjA5h0ydQ8CcHoTMkJsFjS7wL3HdSy1m1SRh9kiOuOr9hz4HqOImjJQ1/6Yb38uS
MVewW2oMEnEB99UANB5CswZHvvHcxU6d4hSmKlL1DfEuEq8hodM552n+WqpTgRhW
YzLmhUFqe0fkS9p4wAa5tPDB8O34P+nEvSAwtVqDFwqJmX0+H+r2INp7LJmDARo=
=FaJT
-----END PGP SIGNATURE-----

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

[Alec Warner]

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

On Thu, Jan 16, 2014 at 3:02 PM, Tom Wijsman <TomWij@gentoo.org> wrote:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
>
I want to summarize our IRC conversation on the list.



> On Thu, 16 Jan 2014 23:22:44 +0100
> Alexander Berntsen <alexander@plaimi.net> wrote:
>
> > Your ill-placed attempts at being clever are missing the point.
>
> Why are they missing the point?
>

I think the point Alexander is trying to make here is also a point I tried
to make on IRC. Many times large refactoring projects fail. I've tried to
do them in Portage, and they have failed. I think folks are leery of this.
We are often more comfortable with incremental changes. That means one
change at a time; portage is a large ship and I don't think anyone is under
the impression that portage will 'turn on a dime.'

What I think we are trying to avoid, is changes that go in the opposite
direction.


>
> > Portage is a mess. We don't need it to become more messy to the point
> > of maintainability.
>
> How do we plan to fix that?


Summarizing from our IRC conversation:

Generally one of the first things you decide to do when you want to clean
something up is to stop making new messes. I think that is the logic we are
trying to convey here. Repoman has a giant unreadable loop that runs in
module scope. All variables are global shared variables. Trampling on
variables used by other functions is commonplace. So when we say 'add new
code in a function', we say that because adding functions protects you from
this mess.

You get your own locals(). Using globals() triggers warnings. Shadowing
variables triggers warnings. The function is callable (you could write a
test, for instance.) These are all positive things. I realize they are not
necessarily the 'refactoring' you want; however I think most people
consider these as positive attributes of functions, which is why we are
advocating for them.

I am in no way claiming that 'putting code in functions' is somehow the
best system, or the most maintainable system; merely that it is much better
than what we have today. It also costs almost nothing to implement. As we
discussed on IRC, even placing this code in a function in repoman itself
offers most of these benefits.


>
> > Yes, no one fixing bugs (because they are all designing a grand
> > redesign of Portage) would be bad.
>
> Do you agree?
>

Again, I think this is an appeal for incremental changes, and not huge
hard-to-grasp changes.


>
> > However, this is not likely to happen.
>
> Why do you think so?
>
> > It hasn't happened before.
>
> It could happen in the future.
>
> > And now we have a bunch of great new volunteers.
>
> Yes, we do.
>

I'm not really following this part of the conversation. Speaking has
someone who has tried the grand refactoring of portage; it has not worked
well for me in the past. Incremental changes are easier to get accepted,
they get released more often, they are tested faster, and so forth.


>
> > Sebastian even *told* you specifically how to do this properly.
>
> I think he was referring to the feedback that Sebestian gave on the thread
> later on. I recommend you consider incorporating

the feedback into your patch.
>

I think he was referring to the feedback that Sebastian gave you on the
thread later on.


> > So please do.
>
> Why?
>

Look, I realize Alexander (and probably myself) didn't have a great tone;
but I thought Sebastian's feedback was pretty useful. Is there some reason
you wouldn't want to incorporate it?

-A

>
> - --
> 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
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v2.0.22 (GNU/Linux)
>
> iQEcBAEBAgAGBQJS2GT9AAoJEJWyH81tNOV95ukH+wZ0yB4KOgfOd6z90cpYC0Ec
> 4RLK8HbKVYIIytxnnhR5Ny/BR5/ANlOYQDIFUytkJyKNmVPx3nP6kY+wHD5qOYkF
> 3DlJpxRy6wx/E43+wpvVv+dSNDHxkvyy8OeZ5QuAcFi1oYaeYdctfOU4/URihGzO
> MjA5h0ydQ8CcHoTMkJsFjS7wL3HdSy1m1SRh9kiOuOr9hz4HqOImjJQ1/6Yb38uS
> MVewW2oMEnEB99UANB5CswZHvvHcxU6d4hSmKlL1DfEuEq8hodM552n+WqpTgRhW
> YzLmhUFqe0fkS9p4wAa5tPDB8O34P+nEvSAwtVqDFwqJmX0+H+r2INp7LJmDARo=
> =FaJT
> -----END PGP SIGNATURE-----
>

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

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

[Tom Wijsman]

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

On Thu, 16 Jan 2014 22:14:10 -0800
Alec Warner <antarus@gentoo.org> wrote:

> I think the point Alexander is trying to make here is also a point I
> tried to make on IRC. Many times large refactoring projects fail.

Repoman's code base is of small enough size for it to succeed for sure.

> I've tried to do them in Portage, and they have failed.

Portage is indeed quite a bit bigger, that's why I might look at that
from a software re-engineering perspective with the necessary practices
that are done in that particular field to deal with legacy code.

But, in one of the bugs I indeed have seen such a smaller attempt; if we
want to see Portage itself refactored, it will require agreement and
cooperation. Otherwise the refactoring commits would never be accepted.

Definitely we will want to look into reproducible and incremental
operations; so, we shouldn't so much send-email a huge diff, but rather
send scripted commands for review that are easier to understand. Eg.

    move function1 to file1
    rename functin2 to function3
    ...

Of course not everything can be written down that way; but the big
stuff can, and that's exactly the stuff which in the diff form would
be much harder to accept.

> I think folks are leery of this.

Yes, the more manual work of refactoring can break things; good coverage
testing can help keep this minimal, I see Portage has quite some tests
which is a good thing but I'm not sure how well they cover Portage.

> We are often more comfortable with incremental changes. That means
> one change at a time; portage is large ship and I don't think anyone
> is under the impression that portage will 'turn on a dime.'

The only thing I like to see here is that these small incremental
changes work towards a good design; there's a difference between doing
it just because you can and doing it on purpose.

> What I think we are trying to avoid, is changes that go in the
> opposite direction.

"Yes, we will not sail there; but where do we go to instead?"

> Generally one of the first things you decide to do when you want to
> clean something up is to stop making new messes. I think that is the
> logic we are trying to convey here.

Your first comment was clear to me, I didn't object to putting it in a
function. But anything more than that needs a proper discussion, as just
creating a random file and putting it in there is a mess as well. :)

(My response was based on that I _want_ to refactor it, hence the care)

> I'm not really following this part of the conversation. Speaking has
> someone who has tried the grand refactoring of portage; it has not
> worked well for me in the past.

What caused trouble doing this?

> Incremental changes are easier to get accepted, they get released
> more often, they are tested faster, and so forth.

A refactor attempt can be planned to be done in incremental steps.

> > > Sebastian even *told* you specifically how to do this properly.
> >
> > I think he was referring to the feedback that Sebestian gave on the
> > thread later on. I recommend you consider incorporating
> 
> the feedback into your patch.
>
> I think he was referring to the feedback that Sebastian gave you on
> the thread later on.

The feedback has no mention of what is referred to as far as I can see.

> Look, I realize Alexander (and probably myself) didn't have a great
> tone; but I thought Sebastian's feedback was pretty useful. Is there
> some reason you wouldn't want to incorporate it?

It is incorporated in v2 of the patch.

-- 
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] Have repoman check if the packages to unpack rare archive formats from SRC_URI are present in DEPEND (bug #205909).

[Tom Wijsman]

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

On Wed, 15 Jan 2014 17:44:15 -0800
Alec Warner <antarus@gentoo.org> wrote:

> Write these checks as functions

Will do in v2, might also look into whether this part of the code can
be refactored already into its own file; having it similar to the
structure we have in Checks.py.

We could then name Checks.py as SyntaxChecks.py and name this new file
for the metadata checks of the loop as MetadataChecks.py; then later,
we can look into creating VCSDiffChecks.py.

Given that these files could grow, we will probably want to split the
files up and put the split up files into directories like
checks/syntax/, checks/metadata/ and so on.

I really would love to see repoman refactored, redesigned and/or
rewritten; but it is to soon for me to do this the most wise way, as I
need to understand the code. I plan to do this 2 - 4 weeks from now.

-- 
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] Have repoman check if the packages to unpack rare archive formats from SRC_URI are present in DEPEND (bug #205909).

[Sebastian Luther]

Am 16.01.2014 01:07, schrieb Tom Wijsman:
> ---
>  bin/repoman   | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  man/repoman.1 |  4 ++++
>  2 files changed, 57 insertions(+)
> 
> diff --git a/bin/repoman b/bin/repoman
> index d1542e9..9b703dc 100755
> --- a/bin/repoman
> +++ b/bin/repoman
> @@ -36,6 +36,9 @@ pym_path = osp.join(osp.dirname(osp.dirname(osp.realpath(__file__))), "pym")
>  sys.path.insert(0, pym_path)
>  import portage
>  portage._internal_caller = True
> +
> +from portage._sets.profiles import PackagesSystemSet
> +system_set_atoms = PackagesSystemSet(portage.settings.profiles).getAtoms()
>  portage._disable_legacy_globals()
>  

You should be using repoman_settings instead of portage.settings.

Considering the later use, you don't need PackagesSystemSet set here,
just use a set. And use atom.cp instead of the atoms.

>  try:
> @@ -300,6 +303,7 @@ qahelp = {
>  	"inherit.missing": "Ebuild uses functions from an eclass but does not inherit it",
>  	"inherit.unused": "Ebuild inherits an eclass but does not use it",
>  	"java.eclassesnotused": "With virtual/jdk in DEPEND you must inherit a java eclass",
> +	"unpack.DEPEND.missing": "A rare archive format was used in SRC_URI, but its package to unpack it is missing in DEPEND.",
>  	"wxwidgets.eclassnotused": "Ebuild DEPENDs on x11-libs/wxGTK without inheriting wxwidgets.eclass",
>  	"KEYWORDS.dropped": "Ebuilds that appear to have dropped KEYWORDS for some arch",
>  	"KEYWORDS.missing": "Ebuilds that have a missing or empty KEYWORDS variable",
> @@ -399,6 +403,7 @@ qawarnings = set((
>  "metadata.warning",
>  "portage.internal",
>  "repo.eapi.deprecated",
> +"unpack.DEPEND.missing",
>  "usage.obsolete",
>  "upstream.workaround",
>  "LIVEVCS.stable",
> @@ -479,6 +484,25 @@ ruby_deprecated = frozenset([
>  	"ruby_targets_ree18",
>  ])
>  
> +# TODO: Add functionality to support checking for deb2targz on platforms where
> +#       GNU binutils is absent; see PMS 5, section 11.3.3.13.
> +archive_formats = {
> +	"\.7[zZ]":"app-arch/p7zip",
> +	"\.(bz2?|tbz2)":"app-arch/bzip2",
> +	"\.jar":"app-arch/unzip",
> +	"\.(LH[aA]|lha|lzh)":"app-arch/lha",
> +	"\.lzma":"app-arch/lzma-utils",
> +	"\.(rar|RAR)":"app-arch/unrar",
> +	"\.(tar(\.(bz2?|gz|Z))?|tbz2|t[bg]z)?":"app-arch/tar",
> +	"\.(gz|tar\.Z|t[bg]z|[zZ])":"app-arch/gzip",
> +	"\.(zip|ZIP)":"app-arch/unzip",
> +}
> +
> +archive_formats_eapi_3_to_5 = {
> +	"\.tar.xz":"app-arch/tar",
> +	"\.xz":"app-arch/xz-utils",
> +}
> +
>  metadata_xml_encoding = 'UTF-8'
>  metadata_xml_declaration = '<?xml version="1.0" encoding="%s"?>' % \
>  	(metadata_xml_encoding,)
> @@ -1559,6 +1583,7 @@ for x in effective_scanlist:
>  	fetchlist_dict = portage.FetchlistDict(checkdir, repoman_settings, portdb)
>  	myfiles_all = []
>  	src_uri_error = False
> +	needed_unpack_depends = {}
>  	for mykey in fetchlist_dict:
>  		try:
>  			myfiles_all.extend(fetchlist_dict[mykey])
> @@ -1573,7 +1598,22 @@ for x in effective_scanlist:
>  				stats["SRC_URI.syntax"] += 1
>  				fails["SRC_URI.syntax"].append(
>  					"%s.ebuild SRC_URI: %s" % (mykey, e))
> +
> +		# Compare each SRC_URI entry against archive_formats; if one of the
> +		# extensions match, we remember which archive depends are needed to
> +		# check them later on.
> +		needed_unpack_depends[mykey] = []
> +		for file_extension in archive_formats or \
> +			((re.match('[345]$', eapi) is not None) \

Use portage.eapi for the line above. You may have to add a new function
to portage.eapi.

> +				and file_extension in archive_formats_eapi_3_to_5):
> +			for entry in fetchlist_dict[mykey]:
> +				if re.match('.*%s$' % file_extension, entry) is not None:
> +					format = archive_formats[file_extension]

As these regex are used frequently, they should be compiled using
re.compile.

> +
> +					if format not in needed_unpack_depends[mykey]:
> +						needed_unpack_depends[mykey].append(format)

I'd make needed_unpack_depends[mykey] a set. Then you can just add()
instead of checking and appending.

>  	del fetchlist_dict
> +
>  	if not src_uri_error:
>  		# This test can produce false positives if SRC_URI could not
>  		# be parsed for one or more ebuilds. There's no point in
> @@ -2010,6 +2050,17 @@ for x in effective_scanlist:
>  				atoms = None
>  				badsyntax.append(str(e))
>  
> +			if atoms and mytype == 'DEPEND':

Use "if atoms and buildtime:" here.

> +				# We check whether the needed archive dependencies are present
> +				# in DEPEND, which were determined from SRC_URI.
> +				for entry in needed_unpack_depends[catdir + '/' + y]:

Use the existing catpkg here.

> +					if entry not in system_set_atoms and entry \
> +						not in [atom.cp for atom in atoms if atom != "||"]:
> +						stats['unpack.' + mytype + '.missing'] += 1
> +						fails['unpack.' + mytype + '.missing'].append( \
> +							relative_path + ": %s is missing in %s" % \
> +							(entry, mytype))
> +
>  			if atoms and mytype.endswith("DEPEND"):
>  				if runtime and \
>  					"test?" in mydepstr.split():
> @@ -2384,6 +2435,8 @@ for x in effective_scanlist:
>  				"%s/metadata.xml: unused local USE-description: '%s'" % \
>  				(x, myflag))
>  
> +	del needed_unpack_depends
> +
>  if options.if_modified == "y" and len(effective_scanlist) < 1:
>  	logging.warn("--if-modified is enabled, but no modified packages were found!")
>  
> diff --git a/man/repoman.1 b/man/repoman.1
> index a78f94e..e739d56 100644
> --- a/man/repoman.1
> +++ b/man/repoman.1
> @@ -334,6 +334,10 @@ Ebuild inherits a deprecated eclass
>  With virtual/jdk in DEPEND you must inherit a java eclass. Refer to
>  \fIhttp://www.gentoo.org/proj/en/java/java\-devel.xml\fR for more information.
>  .TP
> +.B unpack.DEPEND.missing
> +A rare archive format was used in SRC_URI, but its package to unpack it is
                                                  ^^^
                                                  the(?)
> +missing in DEPEND.
           ^^
           from(?)
> +TP
>  .B manifest.bad
>  Manifest has missing or incorrect digests
>  .TP
> 


Maybe you could remove the entries from the archive_formats variable
once you know if they are in the system set.

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

[Tom Wijsman]

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

On Thu, 16 Jan 2014 08:03:03 +0100
Sebastian Luther <SebastianLuther@gmx.de> wrote:

> Am 16.01.2014 01:07, schrieb Tom Wijsman:
> > ---
> >  bin/repoman   | 53
> > +++++++++++++++++++++++++++++++++++++++++++++++++++++ man/repoman.1
> > |  4 ++++ 2 files changed, 57 insertions(+)
> > 
> > diff --git a/bin/repoman b/bin/repoman
> > index d1542e9..9b703dc 100755
> > --- a/bin/repoman
> > +++ b/bin/repoman
> > @@ -36,6 +36,9 @@ pym_path =
> > osp.join(osp.dirname(osp.dirname(osp.realpath(__file__))), "pym")
> > sys.path.insert(0, pym_path) import portage
> >  portage._internal_caller = True
> > +
> > +from portage._sets.profiles import PackagesSystemSet
> > +system_set_atoms =
> > PackagesSystemSet(portage.settings.profiles).getAtoms()
> > portage._disable_legacy_globals() 
> 
> You should be using repoman_settings instead of portage.settings.

If I understand correctly, that is this URL?

http://dev.gentoo.org/~zmedico/portage/doc/api/portage.repository.config-module.html

How do I get the @system set out of that?

> Considering the later use

Which use?

> you don't need PackagesSystemSet set here,
> just use a set.

Okay, thus I need to create some kind of set object here (I don't see
one in the list of http://dev.gentoo.org/~zmedico/portage/doc/api/
though) and then specify that it would be the @system set? Which class?

> And use atom.cp instead of the atoms.

So, if I understood correctly; using list comprehension, I directly
transform the getAtoms() to a list of atom.cp's... Okay, good idea.

> >  try:
> > @@ -300,6 +303,7 @@ qahelp = {
> >  	"inherit.missing": "Ebuild uses functions from an eclass
> > but does not inherit it", "inherit.unused": "Ebuild inherits an
> > eclass but does not use it", "java.eclassesnotused": "With
> > virtual/jdk in DEPEND you must inherit a java eclass",
> > +	"unpack.DEPEND.missing": "A rare archive format was used
> > in SRC_URI, but its package to unpack it is missing in DEPEND.",
> > "wxwidgets.eclassnotused": "Ebuild DEPENDs on x11-libs/wxGTK
> > without inheriting wxwidgets.eclass", "KEYWORDS.dropped": "Ebuilds
> > that appear to have dropped KEYWORDS for some arch",
> > "KEYWORDS.missing": "Ebuilds that have a missing or empty KEYWORDS
> > variable", @@ -399,6 +403,7 @@ qawarnings =
> > set(( "metadata.warning", "portage.internal",
> > "repo.eapi.deprecated", +"unpack.DEPEND.missing",
> >  "usage.obsolete",
> >  "upstream.workaround",
> >  "LIVEVCS.stable",
> > @@ -479,6 +484,25 @@ ruby_deprecated = frozenset([
> >  	"ruby_targets_ree18",
> >  ])
> >  
> > +# TODO: Add functionality to support checking for deb2targz on
> > platforms where +#       GNU binutils is absent; see PMS 5, section
> > 11.3.3.13. +archive_formats = {
> > +	"\.7[zZ]":"app-arch/p7zip",
> > +	"\.(bz2?|tbz2)":"app-arch/bzip2",
> > +	"\.jar":"app-arch/unzip",
> > +	"\.(LH[aA]|lha|lzh)":"app-arch/lha",
> > +	"\.lzma":"app-arch/lzma-utils",
> > +	"\.(rar|RAR)":"app-arch/unrar",
> > +	"\.(tar(\.(bz2?|gz|Z))?|tbz2|t[bg]z)?":"app-arch/tar",
> > +	"\.(gz|tar\.Z|t[bg]z|[zZ])":"app-arch/gzip",
> > +	"\.(zip|ZIP)":"app-arch/unzip",
> > +}
> > +
> > +archive_formats_eapi_3_to_5 = {
> > +	"\.tar.xz":"app-arch/tar",
> > +	"\.xz":"app-arch/xz-utils",
> > +}
> > +
> >  metadata_xml_encoding = 'UTF-8'
> >  metadata_xml_declaration = '<?xml version="1.0" encoding="%s"?>' %
> > \ (metadata_xml_encoding,)
> > @@ -1559,6 +1583,7 @@ for x in effective_scanlist:
> >  	fetchlist_dict = portage.FetchlistDict(checkdir,
> > repoman_settings, portdb) myfiles_all = []
> >  	src_uri_error = False
> > +	needed_unpack_depends = {}
> >  	for mykey in fetchlist_dict:
> >  		try:
> >  			myfiles_all.extend(fetchlist_dict[mykey])
> > @@ -1573,7 +1598,22 @@ for x in effective_scanlist:
> >  				stats["SRC_URI.syntax"] += 1
> >  				fails["SRC_URI.syntax"].append(
> >  					"%s.ebuild SRC_URI: %s" %
> > (mykey, e)) +
> > +		# Compare each SRC_URI entry against
> > archive_formats; if one of the
> > +		# extensions match, we remember which archive
> > depends are needed to
> > +		# check them later on.
> > +		needed_unpack_depends[mykey] = []
> > +		for file_extension in archive_formats or \
> > +			((re.match('[345]$', eapi) is not None) \
> 
> Use portage.eapi for the line above.

Why? 'eapi' is the EAPI of the ebuild, what is wrong with that?

> You may have to add a new function to portage.eapi.

What would the purpose of that function be?

> > +				and file_extension in
> > archive_formats_eapi_3_to_5):
> > +			for entry in fetchlist_dict[mykey]:
> > +				if re.match('.*%s$' %
> > file_extension, entry) is not None:
> > +					format =
> > archive_formats[file_extension]
> 
> As these regex are used frequently, they should be compiled using
> re.compile.

I know, but it contains %s; but, I'll look if I can make a list of
regex, one for each file extension. Or rather, I'll first try to instead
match the last characters of the string using a substring without
having to create a regex at all, which should be even faster.

> > +					if format not in
> > needed_unpack_depends[mykey]:
> > +
> > needed_unpack_depends[mykey].append(format)
> 
> I'd make needed_unpack_depends[mykey] a set. Then you can just add()
> instead of checking and appending.

Thanks for the suggestion, I'll look into this.

> >  	del fetchlist_dict
> > +
> >  	if not src_uri_error:
> >  		# This test can produce false positives if SRC_URI
> > could not # be parsed for one or more ebuilds. There's no point in
> > @@ -2010,6 +2050,17 @@ for x in effective_scanlist:
> >  				atoms = None
> >  				badsyntax.append(str(e))
> >  
> > +			if atoms and mytype == 'DEPEND':
> 
> Use "if atoms and buildtime:" here.

+1

> > +				# We check whether the needed
> > archive dependencies are present
> > +				# in DEPEND, which were determined
> > from SRC_URI.
> > +				for entry in
> > needed_unpack_depends[catdir + '/' + y]:
> 
> Use the existing catpkg here.

Missed that, thank you.

> > +					if entry not in
> > system_set_atoms and entry \
> > +						not in [atom.cp
> > for atom in atoms if atom != "||"]:
> > +						stats['unpack.' +
> > mytype + '.missing'] += 1
> > +						fails['unpack.' +
> > mytype + '.missing'].append( \
> > +
> > relative_path + ": %s is missing in %s" % \
> > +							(entry,
> > mytype)) +
> >  			if atoms and mytype.endswith("DEPEND"):
> >  				if runtime and \
> >  					"test?" in
> > mydepstr.split(): @@ -2384,6 +2435,8 @@ for x in effective_scanlist:
> >  				"%s/metadata.xml: unused local
> > USE-description: '%s'" % \ (x, myflag))
> >  
> > +	del needed_unpack_depends
> > +
> >  if options.if_modified == "y" and len(effective_scanlist) < 1:
> >  	logging.warn("--if-modified is enabled, but no modified
> > packages were found!") 
> > diff --git a/man/repoman.1 b/man/repoman.1
> > index a78f94e..e739d56 100644
> > --- a/man/repoman.1
> > +++ b/man/repoman.1
> > @@ -334,6 +334,10 @@ Ebuild inherits a deprecated eclass
> >  With virtual/jdk in DEPEND you must inherit a java eclass. Refer to
> >  \fIhttp://www.gentoo.org/proj/en/java/java\-devel.xml\fR for more
> > information. .TP
> > +.B unpack.DEPEND.missing
> > +A rare archive format was used in SRC_URI, but its package to
> > unpack it is
>                                                   ^^^
>                                                   the(?)

Unsure myself as well, but yes; the is the safe option here.

> > +missing in DEPEND.
>            ^^
>            from(?)

Yes, 'in action' or 'from something'; thus 'from'. Thanks.

> > +TP
> >  .B manifest.bad
> >  Manifest has missing or incorrect digests
> >  .TP
> > 
> 
> Maybe you could remove the entries from the archive_formats variable
> once you know if they are in the system set.

The purpose here is to allow to support changes in the system set; when
something is added or present in the system set, it doesn't necessarily
imply that it will stay. Keeping them listed foresees that a format
could become deprecated or less used in the future.

-- 
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] Have repoman check if the packages to unpack rare archive formats from SRC_URI are present in DEPEND (bug #205909).

[Sebastian Luther]

Am 16.01.2014 22:40, schrieb Tom Wijsman:
> On Thu, 16 Jan 2014 08:03:03 +0100 Sebastian Luther
> <SebastianLuther@gmx.de> wrote:
> 
>> Am 16.01.2014 01:07, schrieb Tom Wijsman:
>>> --- bin/repoman   | 53 
>>> +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>> man/repoman.1 |  4 ++++ 2 files changed, 57 insertions(+)
>>> 
>>> diff --git a/bin/repoman b/bin/repoman index d1542e9..9b703dc
>>> 100755 --- a/bin/repoman +++ b/bin/repoman @@ -36,6 +36,9 @@
>>> pym_path = 
>>> osp.join(osp.dirname(osp.dirname(osp.realpath(__file__))),
>>> "pym") sys.path.insert(0, pym_path) import portage 
>>> portage._internal_caller = True + +from portage._sets.profiles
>>> import PackagesSystemSet +system_set_atoms = 
>>> PackagesSystemSet(portage.settings.profiles).getAtoms() 
>>> portage._disable_legacy_globals()
>> 
>> You should be using repoman_settings instead of
>> portage.settings.
> 
> If I understand correctly, that is this URL?
> 
> http://dev.gentoo.org/~zmedico/portage/doc/api/portage.repository.config-module.html
>
>  How do I get the @system set out of that?

portage.settings and repoman_settings are instances of
portage.package.ebuild.config.config. I just looked at the code and
think now that you should keep using PackagesSystemSet to get the
@system atoms. Just turn them into a set afterwards with set(atom.cp
for atom in system_set_atoms).

The reason to use atom.cp is that >=cat/foo-1 could be part of that
set and then '"cat/foo" in system_set_atoms' would return False.

> 
>> Considering the later use
> 
> Which use?

The "if entry not in system_set_atoms" line. You're using __contains__
there (with the 'in'). You don't use the additional magic provided by
PackageSet (which is a super class of PackagesSystemSet).

> 
>> you don't need PackagesSystemSet set here, just use a set.
> 
> Okay, thus I need to create some kind of set object here (I don't
> see one in the list of
> http://dev.gentoo.org/~zmedico/portage/doc/api/ though) and then
> specify that it would be the @system set? Which class?
> 

Whenever I say 'set' I mean python's builtin set.

>> And use atom.cp instead of the atoms.
> 
> So, if I understood correctly; using list comprehension, I
> directly transform the getAtoms() to a list of atom.cp's... Okay,
> good idea.
> 
>>> try: @@ -300,6 +303,7 @@ qahelp = { "inherit.missing": "Ebuild
>>> uses functions from an eclass but does not inherit it",
>>> "inherit.unused": "Ebuild inherits an eclass but does not use
>>> it", "java.eclassesnotused": "With virtual/jdk in DEPEND you
>>> must inherit a java eclass", +	"unpack.DEPEND.missing": "A rare
>>> archive format was used in SRC_URI, but its package to unpack
>>> it is missing in DEPEND.", "wxwidgets.eclassnotused": "Ebuild
>>> DEPENDs on x11-libs/wxGTK without inheriting wxwidgets.eclass",
>>> "KEYWORDS.dropped": "Ebuilds that appear to have dropped
>>> KEYWORDS for some arch", "KEYWORDS.missing": "Ebuilds that have
>>> a missing or empty KEYWORDS variable", @@ -399,6 +403,7 @@
>>> qawarnings = set(( "metadata.warning", "portage.internal", 
>>> "repo.eapi.deprecated", +"unpack.DEPEND.missing", 
>>> "usage.obsolete", "upstream.workaround", "LIVEVCS.stable", @@
>>> -479,6 +484,25 @@ ruby_deprecated = frozenset([ 
>>> "ruby_targets_ree18", ])
>>> 
>>> +# TODO: Add functionality to support checking for deb2targz
>>> on platforms where +#       GNU binutils is absent; see PMS 5,
>>> section 11.3.3.13. +archive_formats = { +
>>> "\.7[zZ]":"app-arch/p7zip", +
>>> "\.(bz2?|tbz2)":"app-arch/bzip2", +	"\.jar":"app-arch/unzip", +
>>> "\.(LH[aA]|lha|lzh)":"app-arch/lha", +
>>> "\.lzma":"app-arch/lzma-utils", +
>>> "\.(rar|RAR)":"app-arch/unrar", +
>>> "\.(tar(\.(bz2?|gz|Z))?|tbz2|t[bg]z)?":"app-arch/tar", +
>>> "\.(gz|tar\.Z|t[bg]z|[zZ])":"app-arch/gzip", +
>>> "\.(zip|ZIP)":"app-arch/unzip", +} + 
>>> +archive_formats_eapi_3_to_5 = { +	"\.tar.xz":"app-arch/tar", +
>>> "\.xz":"app-arch/xz-utils", +} + metadata_xml_encoding =
>>> 'UTF-8' metadata_xml_declaration = '<?xml version="1.0"
>>> encoding="%s"?>' % \ (metadata_xml_encoding,) @@ -1559,6
>>> +1583,7 @@ for x in effective_scanlist: fetchlist_dict =
>>> portage.FetchlistDict(checkdir, repoman_settings, portdb)
>>> myfiles_all = [] src_uri_error = False +	needed_unpack_depends
>>> = {} for mykey in fetchlist_dict: try: 
>>> myfiles_all.extend(fetchlist_dict[mykey]) @@ -1573,7 +1598,22
>>> @@ for x in effective_scanlist: stats["SRC_URI.syntax"] += 1 
>>> fails["SRC_URI.syntax"].append( "%s.ebuild SRC_URI: %s" % 
>>> (mykey, e)) + +		# Compare each SRC_URI entry against 
>>> archive_formats; if one of the +		# extensions match, we
>>> remember which archive depends are needed to +		# check them
>>> later on. +		needed_unpack_depends[mykey] = [] +		for
>>> file_extension in archive_formats or \ +			((re.match('[345]$',
>>> eapi) is not None) \
>> 
>> Use portage.eapi for the line above.
> 
> Why? 'eapi' is the EAPI of the ebuild, what is wrong with that?

What I want you to do is to change "(re.match('[345]$', eapi)" into
something like: "eapi_has_xz_unpack(eapi)". the function
eapi_has_xz_unpack needs to be written. It should be part of portage.eapi.

> 
>> You may have to add a new function to portage.eapi.
> 
> What would the purpose of that function be?
> 
>>> +				and file_extension in archive_formats_eapi_3_to_5): +
>>> for entry in fetchlist_dict[mykey]: +				if re.match('.*%s$' % 
>>> file_extension, entry) is not None: +					format = 
>>> archive_formats[file_extension]
>> 
>> As these regex are used frequently, they should be compiled
>> using re.compile.
> 
> I know, but it contains %s; but, I'll look if I can make a list of 
> regex, one for each file extension. Or rather, I'll first try to
> instead match the last characters of the string using a substring
> without having to create a regex at all, which should be even
> faster.
> 
>>> +					if format not in needed_unpack_depends[mykey]: + 
>>> needed_unpack_depends[mykey].append(format)
>> 
>> I'd make needed_unpack_depends[mykey] a set. Then you can just
>> add() instead of checking and appending.
> 
> Thanks for the suggestion, I'll look into this.
> 
>>> del fetchlist_dict + if not src_uri_error: # This test can
>>> produce false positives if SRC_URI could not # be parsed for
>>> one or more ebuilds. There's no point in @@ -2010,6 +2050,17 @@
>>> for x in effective_scanlist: atoms = None 
>>> badsyntax.append(str(e))
>>> 
>>> +			if atoms and mytype == 'DEPEND':
>> 
>> Use "if atoms and buildtime:" here.
> 
> +1
> 
>>> +				# We check whether the needed archive dependencies are
>>> present +				# in DEPEND, which were determined from SRC_URI. +
>>> for entry in needed_unpack_depends[catdir + '/' + y]:
>> 
>> Use the existing catpkg here.
> 
> Missed that, thank you.
> 
>>> +					if entry not in system_set_atoms and entry \ +						not
>>> in [atom.cp for atom in atoms if atom != "||"]: +
>>> stats['unpack.' + mytype + '.missing'] += 1 +
>>> fails['unpack.' + mytype + '.missing'].append( \ + 
>>> relative_path + ": %s is missing in %s" % \ +							(entry, 
>>> mytype)) + if atoms and mytype.endswith("DEPEND"): if runtime
>>> and \ "test?" in mydepstr.split(): @@ -2384,6 +2435,8 @@ for x
>>> in effective_scanlist: "%s/metadata.xml: unused local 
>>> USE-description: '%s'" % \ (x, myflag))
>>> 
>>> +	del needed_unpack_depends + if options.if_modified == "y" and
>>> len(effective_scanlist) < 1: logging.warn("--if-modified is
>>> enabled, but no modified packages were found!") diff --git
>>> a/man/repoman.1 b/man/repoman.1 index a78f94e..e739d56 100644 
>>> --- a/man/repoman.1 +++ b/man/repoman.1 @@ -334,6 +334,10 @@
>>> Ebuild inherits a deprecated eclass With virtual/jdk in DEPEND
>>> you must inherit a java eclass. Refer to 
>>> \fIhttp://www.gentoo.org/proj/en/java/java\-devel.xml\fR for
>>> more information. .TP +.B unpack.DEPEND.missing +A rare archive
>>> format was used in SRC_URI, but its package to unpack it is
>> ^^^ the(?)
> 
> Unsure myself as well, but yes; the is the safe option here.
> 
>>> +missing in DEPEND.
>> ^^ from(?)
> 
> Yes, 'in action' or 'from something'; thus 'from'. Thanks.
> 
>>> +TP .B manifest.bad Manifest has missing or incorrect digests 
>>> .TP
>>> 
>> 
>> Maybe you could remove the entries from the archive_formats
>> variable once you know if they are in the system set.
> 
> The purpose here is to allow to support changes in the system set;
> when something is added or present in the system set, it doesn't
> necessarily imply that it will stay. Keeping them listed foresees
> that a format could become deprecated or less used in the future.
> 
I didn't mean to remove it from the hardcoded list, but to remove it
inside the code once you know the contents of @system.

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

[Tom Wijsman]

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

On Fri, 17 Jan 2014 09:35:58 +0100
Sebastian Luther <SebastianLuther@gmx.de> wrote:

> The "if entry not in system_set_atoms" line. You're using __contains__
> there (with the 'in'). You don't use the additional magic provided by
> PackageSet (which is a super class of PackagesSystemSet).

This is __contains__, which does what I want as far as I can see:
http://dev.gentoo.org/~zmedico/portage/doc/api/portage._sets.base-pysrc.html#PackageSet.__contains__
What is there wrong with this?

As for the additional magic, do you mean containsCPV? Looking at it:
http://dev.gentoo.org/~zmedico/portage/doc/api/portage._sets.base-pysrc.html#PackageSet.containsCPV
It seems more complex than is necessary, is there a benefit to this?

Sorry, I don't understand what is meant here; all the rest of your
feedback is clear and has been implemented and is in the v2 I plan to
send to the mailing list soon.

-- 
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] Have repoman check if the packages to unpack rare archive formats from SRC_URI are present in DEPEND (bug #205909).

[Sebastian Luther]

Am 18.01.2014 00:00, schrieb Tom Wijsman:
> On Fri, 17 Jan 2014 09:35:58 +0100
> Sebastian Luther <SebastianLuther@gmx.de> wrote:
> 
>> The "if entry not in system_set_atoms" line. You're using __contains__
>> there (with the 'in'). You don't use the additional magic provided by
>> PackageSet (which is a super class of PackagesSystemSet).
> 
> This is __contains__, which does what I want as far as I can see:
> http://dev.gentoo.org/~zmedico/portage/doc/api/portage._sets.base-pysrc.html#PackageSet.__contains__
> What is there wrong with this?

There's nothing wrong with that. It's just that the builtin set type
supports that too.

> 
> As for the additional magic, do you mean containsCPV? Looking at it:
> http://dev.gentoo.org/~zmedico/portage/doc/api/portage._sets.base-pysrc.html#PackageSet.containsCPV
> It seems more complex than is necessary, is there a benefit to this?

I thought more about iterAtomsForPackage, which is used in lots of
places in the resolver.

The comment was about the fact that you're using a more complicated
selfmade class instead of a builtin type, even if the builtin type would
suffice. The comment made more sense when I was still suggesting to not
use PackageSystemSet at all.

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

[Sebastian Luther]

Am 16.01.2014 01:07, schrieb Tom Wijsman:

> +
> +archive_formats_eapi_3_to_5 = {
> +	"\.tar.xz":"app-arch/tar",
> +	"\.xz":"app-arch/xz-utils",

Shouldn't ".tar.xz" require both tar and xz-utils? Other entries may
have the same problem.

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

[Tom Wijsman]

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

On Fri, 17 Jan 2014 09:28:10 +0100
Sebastian Luther <SebastianLuther@gmx.de> wrote:

> Am 16.01.2014 01:07, schrieb Tom Wijsman:
> 
> > +
> > +archive_formats_eapi_3_to_5 = {
> > +	"\.tar.xz":"app-arch/tar",
> > +	"\.xz":"app-arch/xz-utils",
> 
> Shouldn't ".tar.xz" require both tar and xz-utils? Other entries may
> have the same problem.

When you match a string that ends with .xz, you match a string that
ends with .tar.xz; hence the xz-utils check will be done for .tar.xz.

-- 
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 1/3 v2] Have repoman check if the packages to unpack rare archive formats from SRC_URI are present in DEPEND (bug #205909).

[Tom Wijsman]

---
 bin/repoman         | 88 ++++++++++++++++++++++++++++++++++++++++++++++++++---
 man/repoman.1       |  4 +++
 pym/portage/eapi.py |  3 ++
 3 files changed, 91 insertions(+), 4 deletions(-)

diff --git a/bin/repoman b/bin/repoman
index d1542e9..eed1024 100755
--- a/bin/repoman
+++ b/bin/repoman
@@ -36,6 +36,14 @@ pym_path = osp.join(osp.dirname(osp.dirname(osp.realpath(__file__))), "pym")
 sys.path.insert(0, pym_path)
 import portage
 portage._internal_caller = True
+
+from portage._sets.profiles import PackagesSystemSet
+from portage import os
+
+config_root = os.environ.get("PORTAGE_CONFIGROOT")
+repoman_settings = portage.config(config_root=config_root, local_config=False)
+system_set_atoms = [atom.cp \
+	for atom in PackagesSystemSet(repoman_settings.profiles).getAtoms()]
 portage._disable_legacy_globals()
 
 try:
@@ -53,7 +61,6 @@ except (ImportError, SystemError, RuntimeError, Exception):
 		out.eerror(line)
 	sys.exit(1)
 
-from portage import os
 from portage import _encodings
 from portage import _unicode_encode
 import repoman.checks
@@ -78,7 +85,8 @@ from portage.output import ConsoleStyleFile, StyleWriter
 from portage.util import writemsg_level
 from portage.util._argparse import ArgumentParser
 from portage.package.ebuild.digestgen import digestgen
-from portage.eapi import eapi_has_iuse_defaults, eapi_has_required_use
+from portage.eapi import (eapi_has_iuse_defaults, eapi_has_required_use,
+	eapi_has_xz_utils)
 
 if sys.hexversion >= 0x3000000:
 	basestring = str
@@ -98,8 +106,6 @@ os.umask(0o22)
 # behave incrementally.
 repoman_incrementals = tuple(x for x in \
 	portage.const.INCREMENTALS if x != 'ACCEPT_KEYWORDS')
-config_root = os.environ.get("PORTAGE_CONFIGROOT")
-repoman_settings = portage.config(config_root=config_root, local_config=False)
 
 if repoman_settings.get("NOCOLOR", "").lower() in ("yes", "true") or \
 	repoman_settings.get('TERM') == 'dumb' or \
@@ -300,6 +306,7 @@ qahelp = {
 	"inherit.missing": "Ebuild uses functions from an eclass but does not inherit it",
 	"inherit.unused": "Ebuild inherits an eclass but does not use it",
 	"java.eclassesnotused": "With virtual/jdk in DEPEND you must inherit a java eclass",
+	"unpack.DEPEND.missing": "A rare archive format was used in SRC_URI, but its package to unpack it is missing in DEPEND.",
 	"wxwidgets.eclassnotused": "Ebuild DEPENDs on x11-libs/wxGTK without inheriting wxwidgets.eclass",
 	"KEYWORDS.dropped": "Ebuilds that appear to have dropped KEYWORDS for some arch",
 	"KEYWORDS.missing": "Ebuilds that have a missing or empty KEYWORDS variable",
@@ -399,6 +406,7 @@ qawarnings = set((
 "metadata.warning",
 "portage.internal",
 "repo.eapi.deprecated",
+"unpack.DEPEND.missing",
 "usage.obsolete",
 "upstream.workaround",
 "LIVEVCS.stable",
@@ -479,6 +487,65 @@ ruby_deprecated = frozenset([
 	"ruby_targets_ree18",
 ])
 
+def getNonSystemArchiveDepends(fetchlist, eapi):
+	"""
+	Compare each SRC_URI entry against archivers; if one of the
+	extensions match, we remember which archive depends are needed.
+
+	We don't pass back dependencies that are in the system set.
+
+	TODO:	Add functionality to support checking for deb2targz on platforms
+			where GNU binutils is absent; see PMS 5, section 11.3.3.13.
+	"""
+
+	def addMatchingFormatsToResult(formats, result):
+		for file_ext in formats:
+			format = formats[file_ext]
+
+			if format not in system_set_atoms:
+				for entry in fetchlist:
+					if file_ext.match(entry) is not None:
+						result.add(format)
+
+	result = set()
+
+	addMatchingFormatsToResult(getNonSystemArchiveDepends.archivers, result)
+
+	if eapi_has_xz_utils(eapi):
+		addMatchingFormatsToResult(getNonSystemArchiveDepends.archivers_xz, result)
+
+	return result
+
+getNonSystemArchiveDepends.archivers = {
+	re.compile('.*\.7[zZ]$'):"app-arch/p7zip",
+	re.compile('.*\.(bz2?|tbz2)$'):"app-arch/bzip2",
+	re.compile('.*\.jar$'):"app-arch/unzip",
+	re.compile('.*\.(LH[aA]|lha|lzh)$'):"app-arch/lha",
+	re.compile('.*\.lzma$'):"app-arch/lzma-utils",
+	re.compile('.*\.(rar|RAR)$'):"app-arch/unrar",
+	re.compile('.*\.(tar(\.(bz2?|gz|Z))?|tbz2|t[bg]z)?$'):"app-arch/tar",
+	re.compile('.*\.(gz|tar\.Z|t[bg]z|[zZ])$'):"app-arch/gzip",
+	re.compile('.*\.(zip|ZIP)$'):"app-arch/unzip",
+}
+
+getNonSystemArchiveDepends.archivers_xz = {
+	re.compile('.*\.tar.xz$'):"app-arch/tar",
+	re.compile('.*\.xz$'):"app-arch/xz-utils",
+}
+
+def checkArchiveDepends(atoms, catpkg, relative_path, \
+	system_set_atoms, needed_unpack_depends, stats, fails):
+	"""
+	We check whether the needed archive dependencies are present in DEPEND,
+	which were determined from SRC_URI.
+	"""
+	for entry in needed_unpack_depends[catpkg]:
+		if entry not in [atom.cp for atom in atoms if atom != "||"]:
+			stats['unpack.' + mytype + '.missing'] += 1
+			fails['unpack.' + mytype + '.missing'].append( \
+				relative_path + ": %s is missing in %s" % \
+				(entry, mytype))
+
 metadata_xml_encoding = 'UTF-8'
 metadata_xml_declaration = '<?xml version="1.0" encoding="%s"?>' % \
 	(metadata_xml_encoding,)
@@ -1559,6 +1626,7 @@ for x in effective_scanlist:
 	fetchlist_dict = portage.FetchlistDict(checkdir, repoman_settings, portdb)
 	myfiles_all = []
 	src_uri_error = False
+	needed_unpack_depends = {}
 	for mykey in fetchlist_dict:
 		try:
 			myfiles_all.extend(fetchlist_dict[mykey])
@@ -1573,7 +1641,13 @@ for x in effective_scanlist:
 				stats["SRC_URI.syntax"] += 1
 				fails["SRC_URI.syntax"].append(
 					"%s.ebuild SRC_URI: %s" % (mykey, e))
+
+		needed_unpack_depends[mykey] = \
+			getNonSystemArchiveDepends(fetchlist_dict[mykey], \
+				pkgs[y[:-7]]._metadata["EAPI"])
+
 	del fetchlist_dict
+
 	if not src_uri_error:
 		# This test can produce false positives if SRC_URI could not
 		# be parsed for one or more ebuilds. There's no point in
@@ -2010,6 +2084,10 @@ for x in effective_scanlist:
 				atoms = None
 				badsyntax.append(str(e))
 
+			if atoms and buildtime:
+				checkArchiveDepends(atoms, catpkg, relative_path, \
+					system_set_atoms, needed_unpack_depends, stats, fails)
+
 			if atoms and mytype.endswith("DEPEND"):
 				if runtime and \
 					"test?" in mydepstr.split():
@@ -2384,6 +2462,8 @@ for x in effective_scanlist:
 				"%s/metadata.xml: unused local USE-description: '%s'" % \
 				(x, myflag))
 
+	del needed_unpack_depends
+
 if options.if_modified == "y" and len(effective_scanlist) < 1:
 	logging.warn("--if-modified is enabled, but no modified packages were found!")
 
diff --git a/man/repoman.1 b/man/repoman.1
index a78f94e..fb89610 100644
--- a/man/repoman.1
+++ b/man/repoman.1
@@ -334,6 +334,10 @@ Ebuild inherits a deprecated eclass
 With virtual/jdk in DEPEND you must inherit a java eclass. Refer to
 \fIhttp://www.gentoo.org/proj/en/java/java\-devel.xml\fR for more information.
 .TP
+.B unpack.DEPEND.missing
+A rare archive format was used in SRC_URI, but the package to unpack it is
+missing from DEPEND.
+TP
 .B manifest.bad
 Manifest has missing or incorrect digests
 .TP
diff --git a/pym/portage/eapi.py b/pym/portage/eapi.py
index 4f77910..5f8919b 100644
--- a/pym/portage/eapi.py
+++ b/pym/portage/eapi.py
@@ -95,6 +95,9 @@ def eapi_has_hdepend(eapi):
 def eapi_has_targetroot(eapi):
 	return eapi in ("5-hdepend",)
 
+def eapi_has_xz_utils(eapi):
+	return eapi not in ("0", "1", "2")
+
 _eapi_attrs = collections.namedtuple('_eapi_attrs',
 	'dots_in_PN dots_in_use_flags exports_EBUILD_PHASE_FUNC '
 	'feature_flag_test feature_flag_targetroot '
-- 
1.8.5.2

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

[Mike Frysinger]

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

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

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

> +getNonSystemArchiveDepends.archivers = {

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

def getNonSystemArchiveDepends(fetchlist, eapi):
	...

	ARCHIVERS = {
		...
	}

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

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

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

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

xz-utils, not lzma-utils

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

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

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

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

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

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

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

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

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

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

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

[Tom Wijsman]

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

On Sun, 19 Jan 2014 04:38:31 -0500
Mike Frysinger <vapier@gentoo.org> wrote:

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Thank you everyone for the reviews so far.

-- 
With kind regards,

Tom Wijsman (TomWij)
Gentoo Developer

E-mail address  : 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 v2] Have repoman check if the packages to unpack rare archive formats from SRC_URI are present in DEPEND (bug #205909).

[Alec Warner]

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

On Sun, Jan 19, 2014 at 6:23 PM, Tom Wijsman <TomWij@gentoo.org> wrote:

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

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

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

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

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


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

For the colon's in dicts thing:

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


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

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


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

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

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

[Tom Wijsman]

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

On Sun, 19 Jan 2014 18:43:46 -0800
Alec Warner <antarus@gentoo.org> wrote:

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

My backlog was limited so I didn't catch that discussion, feel free to
share the log; I've since increased it. There's a lot more talk than my
defaults on IRC (as well as here on the mailing list). :)

On a side note, "I liked" seems a too subjective way to review patches.

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

True, but do you have a better suggestion? (Not the one below)

From a quick lookup Python seems to not really provide a clean
immutable solution here; one option would be to use a frozenset, but
then one has to make classes to put into that (which are still
mutable). That is a misuse for what could just be a dictionary.

> > move it into the class definition.
> > > def getNonSystemArchiveDepends(fetchlist, eapi):
> > >       ...
> > >
> > >       ARCHIVERS = {
> > >               ...
> > >       }
> >
> > That makes it a non-static function variable? This is a regression.
> 
> I guess I am not seeing why it must be a static function variable.
> Can you explain?

Because you would call re.compile for each time that function is
called; while the most recent compiled versions of re.compile are
cached, I still do not see a reason for this variable not to be static.

> For the colon's in dicts thing:
> 
> http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Whitespace

Okay, I will follow those guidelines.

> The @system set in gentoo will ensure these are installed.

You can compare @system against the PMS and you will note that entries
are missing in @system; the @system set only covers the most popular
ones, the rest is left up to the maintainer to add to the ebuild. Thus
this enumerates all of them; as the @system set can change in the
future, we need to make the code future proof hence the @system check.

It is possible for such atom to get removed from @system later.

> understand the wording of PMS (as the dependencies should be
> expressed somewhere) but in general we prefer to do that in @system.
> For the same reason all packages do not depend on glibc, or the
> compiler, or anything else.

Things that are in @system are not complained about by this code:

    if format not in system_set_atoms:

-- 
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 2/3] Have repoman check that a package directory contains at least one ebuild (bug #245305).

[Tom Wijsman]

---
 bin/repoman   | 8 ++++++++
 man/repoman.1 | 3 +++
 2 files changed, 11 insertions(+)

diff --git a/bin/repoman b/bin/repoman
index 9b703dc..3263ceb 100755
--- a/bin/repoman
+++ b/bin/repoman
@@ -330,6 +330,7 @@ qahelp = {
 	"SRC_URI.mirror": "A uri listed in profiles/thirdpartymirrors is found in SRC_URI",
 	"ebuild.syntax": "Error generating cache entry for ebuild; typically caused by ebuild syntax error or digest verification failure",
 	"ebuild.output": "A simple sourcing of the ebuild produces output; this breaks ebuild policy.",
+	"ebuild.missing": "A package directory must at least contain one ebuild or be treecleaned.",
 	"ebuild.nesteddie": "Placing 'die' inside ( ) prints an error, but doesn't stop the ebuild.",
 	"variable.invalidchar": "A variable contains an invalid character that is not part of the ASCII character set",
 	"variable.readonly": "Assigning a readonly variable",
@@ -1466,6 +1467,13 @@ for x in effective_scanlist:
 		can_force = False
 		continue
 
+	if len(ebuildlist) == 0:
+		stats["ebuild.missing"] += 1
+		fails["ebuild.missing"].append("%s must at least contain one " % x + \
+			"ebuild or be treecleaned.")
+		can_force = False
+		continue
+
 	# Sort ebuilds in ascending order for the KEYWORDS.dropped check.
 	ebuildlist = sorted(pkgs.values())
 	ebuildlist = [pkg.pf for pkg in ebuildlist]
diff --git a/man/repoman.1 b/man/repoman.1
index e739d56..2bf3765 100644
--- a/man/repoman.1
+++ b/man/repoman.1
@@ -301,6 +301,9 @@ Ebuilds that exist but have not been added to cvs
 .B ebuild.output
 A simple sourcing of the ebuild produces output; this breaks ebuild policy.
 .TP
+.B ebuild.missing
+A package directory must at least contain one ebuild or be treecleaned.
+.TP
 .B ebuild.patches
 PATCHES variable should be a bash array to ensure white space safety
 .TP
-- 
1.8.5.2

Re: [gentoo-portage-dev] [PATCH 2/3] Have repoman check that a package directory contains at least one ebuild (bug #245305).

[Jesus Rivero (Neurogeek)]

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

On Jan 15, 2014 7:09 PM, "Tom Wijsman" <tomwij@gentoo.org> wrote:
>
> ---
>  bin/repoman   | 8 ++++++++
>  man/repoman.1 | 3 +++
>  2 files changed, 11 insertions(+)
>
> diff --git a/bin/repoman b/bin/repoman
> index 9b703dc..3263ceb 100755
> --- a/bin/repoman
> +++ b/bin/repoman
> @@ -330,6 +330,7 @@ qahelp = {
>         "SRC_URI.mirror": "A uri listed in profiles/thirdpartymirrors is
found in SRC_URI",
>         "ebuild.syntax": "Error generating cache entry for ebuild;
typically caused by ebuild syntax error or digest verification failure",
>         "ebuild.output": "A simple sourcing of the ebuild produces
output; this breaks ebuild policy.",
> +       "ebuild.missing": "A package directory must at least contain one
ebuild or be treecleaned.",
>         "ebuild.nesteddie": "Placing 'die' inside ( ) prints an error,
but doesn't stop the ebuild.",
>         "variable.invalidchar": "A variable contains an invalid character
that is not part of the ASCII character set",
>         "variable.readonly": "Assigning a readonly variable",
> @@ -1466,6 +1467,13 @@ for x in effective_scanlist:
>                 can_force = False
>                 continue
>
> +       if len(ebuildlist) == 0:
I'd rather see:
             if not ebuildlist:
> +               stats["ebuild.missing"] += 1
> +               fails["ebuild.missing"].append("%s must at least contain
one " % x + \
> +                       "ebuild or be treecleaned.")
> +               can_force = False
> +               continue
> +
>         # Sort ebuilds in ascending order for the KEYWORDS.dropped check.
>         ebuildlist = sorted(pkgs.values())
>         ebuildlist = [pkg.pf for pkg in ebuildlist]
> diff --git a/man/repoman.1 b/man/repoman.1
> index e739d56..2bf3765 100644
> --- a/man/repoman.1
> +++ b/man/repoman.1
> @@ -301,6 +301,9 @@ Ebuilds that exist but have not been added to cvs
>  .B ebuild.output
>  A simple sourcing of the ebuild produces output; this breaks ebuild
policy.
>  .TP
> +.B ebuild.missing
> +A package directory must at least contain one ebuild or be treecleaned.
> +.TP
>  .B ebuild.patches
>  PATCHES variable should be a bash array to ensure white space safety
>  .TP
> --
> 1.8.5.2
>
>

Everything else looks good to me.

Cheers,

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

Re: [gentoo-portage-dev] [PATCH 2/3] Have repoman check that a package directory contains at least one ebuild (bug #245305).

[Alec Warner]

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

On Wed, Jan 15, 2014 at 5:07 PM, Jesus Rivero (Neurogeek) <
neurogeek@gentoo.org> wrote:

>
> On Jan 15, 2014 7:09 PM, "Tom Wijsman" <tomwij@gentoo.org> wrote:
> >
> > ---
> >  bin/repoman   | 8 ++++++++
> >  man/repoman.1 | 3 +++
> >  2 files changed, 11 insertions(+)
> >
> > diff --git a/bin/repoman b/bin/repoman
> > index 9b703dc..3263ceb 100755
> > --- a/bin/repoman
> > +++ b/bin/repoman
> > @@ -330,6 +330,7 @@ qahelp = {
> >         "SRC_URI.mirror": "A uri listed in profiles/thirdpartymirrors is
> found in SRC_URI",
> >         "ebuild.syntax": "Error generating cache entry for ebuild;
> typically caused by ebuild syntax error or digest verification failure",
> >         "ebuild.output": "A simple sourcing of the ebuild produces
> output; this breaks ebuild policy.",
> > +       "ebuild.missing": "A package directory must at least contain one
> ebuild or be treecleaned.",
> >         "ebuild.nesteddie": "Placing 'die' inside ( ) prints an error,
> but doesn't stop the ebuild.",
> >         "variable.invalidchar": "A variable contains an invalid
> character that is not part of the ASCII character set",
> >         "variable.readonly": "Assigning a readonly variable",
> > @@ -1466,6 +1467,13 @@ for x in effective_scanlist:
> >                 can_force = False
> >                 continue
> >
> > +       if len(ebuildlist) == 0:
> I'd rather see:
>              if not ebuildlist:
>
> Agreed, "if not list" is how you check if a list is empty.

-A


> > +               stats["ebuild.missing"] += 1
> > +               fails["ebuild.missing"].append("%s must at least contain
> one " % x + \
> > +                       "ebuild or be treecleaned.")
> > +               can_force = False
> > +               continue
> > +
> >         # Sort ebuilds in ascending order for the KEYWORDS.dropped check.
> >         ebuildlist = sorted(pkgs.values())
> >         ebuildlist = [pkg.pf for pkg in ebuildlist]
> > diff --git a/man/repoman.1 b/man/repoman.1
> > index e739d56..2bf3765 100644
> > --- a/man/repoman.1
> > +++ b/man/repoman.1
> > @@ -301,6 +301,9 @@ Ebuilds that exist but have not been added to cvs
> >  .B ebuild.output
> >  A simple sourcing of the ebuild produces output; this breaks ebuild
> policy.
> >  .TP
> > +.B ebuild.missing
> > +A package directory must at least contain one ebuild or be treecleaned.
> > +.TP
> >  .B ebuild.patches
> >  PATCHES variable should be a bash array to ensure white space safety
> >  .TP
> > --
> > 1.8.5.2
> >
> >
>
> Everything else looks good to me.
>
> Cheers,
>

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

[gentoo-portage-dev] [PATCH 2/3 v2] Have repoman check that a package directory contains at least one ebuild (bug #245305).

[Tom Wijsman]

---
 bin/repoman   | 8 ++++++++
 man/repoman.1 | 3 +++
 2 files changed, 11 insertions(+)

diff --git a/bin/repoman b/bin/repoman
index d1542e9..44f3d3d 100755
--- a/bin/repoman
+++ b/bin/repoman
@@ -326,6 +326,7 @@ qahelp = {
 	"SRC_URI.mirror": "A uri listed in profiles/thirdpartymirrors is found in SRC_URI",
 	"ebuild.syntax": "Error generating cache entry for ebuild; typically caused by ebuild syntax error or digest verification failure",
 	"ebuild.output": "A simple sourcing of the ebuild produces output; this breaks ebuild policy.",
+	"ebuild.missing": "A package directory must at least contain one ebuild or be treecleaned.",
 	"ebuild.nesteddie": "Placing 'die' inside ( ) prints an error, but doesn't stop the ebuild.",
 	"variable.invalidchar": "A variable contains an invalid character that is not part of the ASCII character set",
 	"variable.readonly": "Assigning a readonly variable",
@@ -1442,6 +1443,13 @@ for x in effective_scanlist:
 		can_force = False
 		continue
 
+	if not ebuildlist:
+		stats["ebuild.missing"] += 1
+		fails["ebuild.missing"].append("%s must at least contain one " % x + \
+			"ebuild or be treecleaned.")
+		can_force = False
+		continue
+
 	# Sort ebuilds in ascending order for the KEYWORDS.dropped check.
 	ebuildlist = sorted(pkgs.values())
 	ebuildlist = [pkg.pf for pkg in ebuildlist]
diff --git a/man/repoman.1 b/man/repoman.1
index a78f94e..6315ea9 100644
--- a/man/repoman.1
+++ b/man/repoman.1
@@ -301,6 +301,9 @@ Ebuilds that exist but have not been added to cvs
 .B ebuild.output
 A simple sourcing of the ebuild produces output; this breaks ebuild policy.
 .TP
+.B ebuild.missing
+A package directory must at least contain one ebuild or be treecleaned.
+.TP
 .B ebuild.patches
 PATCHES variable should be a bash array to ensure white space safety
 .TP
-- 
1.8.5.2

Re: [gentoo-portage-dev] [PATCH 2/3 v2] Have repoman check that a package directory contains at least one ebuild (bug #245305).

[Jesus Rivero (Neurogeek)]

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

On Fri, Jan 17, 2014 at 4:36 PM, Tom Wijsman <tomwij@gentoo.org> wrote:

> ---
>  bin/repoman   | 8 ++++++++
>  man/repoman.1 | 3 +++
>  2 files changed, 11 insertions(+)
>
> diff --git a/bin/repoman b/bin/repoman
> index d1542e9..44f3d3d 100755
> --- a/bin/repoman
> +++ b/bin/repoman
> @@ -326,6 +326,7 @@ qahelp = {
>         "SRC_URI.mirror": "A uri listed in profiles/thirdpartymirrors is
> found in SRC_URI",
>         "ebuild.syntax": "Error generating cache entry for ebuild;
> typically caused by ebuild syntax error or digest verification failure",
>         "ebuild.output": "A simple sourcing of the ebuild produces output;
> this breaks ebuild policy.",
> +       "ebuild.missing": "A package directory must at least contain one
> ebuild or be treecleaned.",
>         "ebuild.nesteddie": "Placing 'die' inside ( ) prints an error, but
> doesn't stop the ebuild.",
>         "variable.invalidchar": "A variable contains an invalid character
> that is not part of the ASCII character set",
>         "variable.readonly": "Assigning a readonly variable",
> @@ -1442,6 +1443,13 @@ for x in effective_scanlist:
>                 can_force = False
>                 continue
>
> +       if not ebuildlist:
> +               stats["ebuild.missing"] += 1
> +               fails["ebuild.missing"].append("%s must at least contain
> one " % x + \
> +                       "ebuild or be treecleaned.")
> +               can_force = False
> +               continue
> +
>         # Sort ebuilds in ascending order for the KEYWORDS.dropped check.
>         ebuildlist = sorted(pkgs.values())
>         ebuildlist = [pkg.pf for pkg in ebuildlist]
> diff --git a/man/repoman.1 b/man/repoman.1
> index a78f94e..6315ea9 100644
> --- a/man/repoman.1
> +++ b/man/repoman.1
> @@ -301,6 +301,9 @@ Ebuilds that exist but have not been added to cvs
>  .B ebuild.output
>  A simple sourcing of the ebuild produces output; this breaks ebuild
> policy.
>  .TP
> +.B ebuild.missing
> +A package directory must at least contain one ebuild or be treecleaned.
> +.TP
>  .B ebuild.patches
>  PATCHES variable should be a bash array to ensure white space safety
>  .TP
> --
> 1.8.5.2
>
>
>
Looks fine.

-- 
Jesus Rivero (Neurogeek)
Gentoo Developer

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

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

[Tom Wijsman]

---
 bin/repoman           |  2 ++
 man/repoman.1         |  3 +++
 pym/repoman/checks.py | 10 ++++++++++
 3 files changed, 15 insertions(+)

diff --git a/bin/repoman b/bin/repoman
index 3263ceb..6754edd 100755
--- a/bin/repoman
+++ b/bin/repoman
@@ -318,6 +318,7 @@ qahelp = {
 	"EAPI.incompatible": "Ebuilds that use features that are only available with a different EAPI",
 	"EAPI.unsupported": "Ebuilds that have an unsupported EAPI version (you must upgrade portage)",
 	"SLOT.invalid": "Ebuilds that have a missing or invalid SLOT variable value",
+	"G2CONF.deprecated": "G2CONF is deprecated, see Gentoo bug #482084 and the GNOME team policies",
 	"HOMEPAGE.missing": "Ebuilds that have a missing or empty HOMEPAGE variable",
 	"HOMEPAGE.virtual": "Virtuals that have a non-empty HOMEPAGE variable",
 	"PDEPEND.suspect": "PDEPEND contains a package that usually only belongs in DEPEND.",
@@ -382,6 +383,7 @@ qawarnings = set((
 "dependency.badtilde",
 "DESCRIPTION.toolong",
 "EAPI.deprecated",
+"G2CONF.deprecated",
 "HOMEPAGE.virtual",
 "LICENSE.deprecated",
 "LICENSE.virtual",
diff --git a/man/repoman.1 b/man/repoman.1
index 2bf3765..7ec43d5 100644
--- a/man/repoman.1
+++ b/man/repoman.1
@@ -227,6 +227,9 @@ Syntax error in RESTRICT (usually an extra/missing space/parenthesis)
 .B SLOT.invalid
 Ebuilds that have a missing or invalid SLOT variable value
 .TP
+.B G2CONF.deprecated
+G2CONF is deprecated, see Gentoo bug #482084 and the GNOME team policies
+.TP
 .B SRC_URI.mirror
 A uri listed in profiles/thirdpartymirrors is found in SRC_URI
 .TP
diff --git a/pym/repoman/checks.py b/pym/repoman/checks.py
index 85aa065..c2608b0 100644
--- a/pym/repoman/checks.py
+++ b/pym/repoman/checks.py
@@ -799,6 +799,16 @@ class PortageInternalVariableAssignment(LineCheck):
 			e += ' on line: %d'
 		return e
 
+class DeprecateG2CONF(LineCheck):
+	repoman_check_name = 'G2CONF.deprecated'
+	re = re.compile(r'.*G2CONF.*')
+
+	def check(self, num, line):
+		"""Run the check on line and return error if there is one"""
+		m = self.re.match(line)
+		if m is not None:
+			return ("G2CONF on line %d is deprecated, see Gentoo bug #482084.")
+
 _base_check_classes = (InheritEclass, LineCheck, PhaseCheck)
 _constant_checks = None
 
-- 
1.8.5.2

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

[Jesus Rivero (Neurogeek)]

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

On Jan 15, 2014 7:08 PM, "Tom Wijsman" <tomwij@gentoo.org> wrote:
>
> ---
>  bin/repoman           |  2 ++
>  man/repoman.1         |  3 +++
>  pym/repoman/checks.py | 10 ++++++++++
>  3 files changed, 15 insertions(+)
>
> diff --git a/bin/repoman b/bin/repoman
> index 3263ceb..6754edd 100755
> --- a/bin/repoman
> +++ b/bin/repoman
> @@ -318,6 +318,7 @@ qahelp = {
>         "EAPI.incompatible": "Ebuilds that use features that are only
available with a different EAPI",
>         "EAPI.unsupported": "Ebuilds that have an unsupported EAPI
version (you must upgrade portage)",
>         "SLOT.invalid": "Ebuilds that have a missing or invalid SLOT
variable value",
> +       "G2CONF.deprecated": "G2CONF is deprecated, see Gentoo bug
#482084 and the GNOME team policies",
>         "HOMEPAGE.missing": "Ebuilds that have a missing or empty
HOMEPAGE variable",
>         "HOMEPAGE.virtual": "Virtuals that have a non-empty HOMEPAGE
variable",
>         "PDEPEND.suspect": "PDEPEND contains a package that usually only
belongs in DEPEND.",
> @@ -382,6 +383,7 @@ qawarnings = set((
>  "dependency.badtilde",
>  "DESCRIPTION.toolong",
>  "EAPI.deprecated",
> +"G2CONF.deprecated",
>  "HOMEPAGE.virtual",
>  "LICENSE.deprecated",
>  "LICENSE.virtual",
> diff --git a/man/repoman.1 b/man/repoman.1
> index 2bf3765..7ec43d5 100644
> --- a/man/repoman.1
> +++ b/man/repoman.1
> @@ -227,6 +227,9 @@ Syntax error in RESTRICT (usually an extra/missing
space/parenthesis)
>  .B SLOT.invalid
>  Ebuilds that have a missing or invalid SLOT variable value
>  .TP
> +.B G2CONF.deprecated
> +G2CONF is deprecated, see Gentoo bug #482084 and the GNOME team policies
> +.TP
>  .B SRC_URI.mirror
>  A uri listed in profiles/thirdpartymirrors is found in SRC_URI
>  .TP
> diff --git a/pym/repoman/checks.py b/pym/repoman/checks.py
> index 85aa065..c2608b0 100644
> --- a/pym/repoman/checks.py
> +++ b/pym/repoman/checks.py
> @@ -799,6 +799,16 @@ class PortageInternalVariableAssignment(LineCheck):
>                         e += ' on line: %d'
>                 return e
>
> +class DeprecateG2CONF(LineCheck):
> +       repoman_check_name = 'G2CONF.deprecated'
> +       re = re.compile(r'.*G2CONF.*')
> +
> +       def check(self, num, line):
> +               """Run the check on line and return error if there is
one"""
> +               m = self.re.match(line)
> +               if m is not None:
> +                       return ("G2CONF on line %d is deprecated, see
Gentoo bug #482084.")
Are you missing the line number interpolation here? or %d is supposed to be
returned?
> +
>  _base_check_classes = (InheritEclass, LineCheck, PhaseCheck)
>  _constant_checks = None
>
> --
> 1.8.5.2
>
>
Other than that, Looks good to me.

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

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

[Tom Wijsman]

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

On Wed, 15 Jan 2014 20:23:26 -0500
"Jesus Rivero (Neurogeek)" <neurogeek@gentoo.org> wrote:

> On Jan 15, 2014 7:08 PM, "Tom Wijsman" <tomwij@gentoo.org> wrote:
>
> > PortageInternalVariableAssignment(LineCheck): e += ' on line: %d'
> >                 return e
> >
> > +class DeprecateG2CONF(LineCheck):
> > +       repoman_check_name = 'G2CONF.deprecated'
> > +       re = re.compile(r'.*G2CONF.*')
> > +
> > +       def check(self, num, line):
> > +               """Run the check on line and return error if there
> > is
> one"""
> > +               m = self.re.match(line)
> > +               if m is not None:
> > +                       return ("G2CONF on line %d is deprecated,
> > see Gentoo bug #482084.")
>
> Are you missing the line number interpolation here? or %d is supposed
> to be returned?

That is filled in by the function which calls check(...), you can see
the same happen with the "e += ' on line: %d'" of the other check that
made it into the context of this patch; when testing this, it worked
properly in the repoman output.

-- 
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 3/3] Have repoman deprecate G2CONF for the GNOME team. (bug #482084).

[Mike Frysinger]

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

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

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

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

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

[Mike Frysinger]

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

btw, suggestion for commit summary/message:

	repoman: deprecate G2CONF

	Deprecate the G2CONF variable as requested by the GNOME team.

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

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