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 <SebastianLuther@gmx.de>]

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.