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

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