Re: [gentoo-portage-dev] [PATCH] repoman: new QA error: slot operator under '||' alternative

[Zac Medico]

On 06/18/2016 01:38 PM, Brian Dolbec wrote:
> Zac, what do you think?

I don't see any problems except for the need to do atom.slot_operator ==
'=', as mentioned in my previous reply.
-- 
Thanks,
Zac

Re: [gentoo-portage-dev] [PATCH] repoman: new QA error: slot operator under '||' alternative

[Brian Dolbec]

On Sat, 18 Jun 2016 20:37:06 +0100
Sergei Trofimovich  wrote:

> On Sat, 18 Jun 2016 11:02:53 -0700
> Brian Dolbec  wrote:
> 
> > > + if runtime:
> > > + try:
> > > + # to find use of ':=' in '||' we
> > > preserve
> > > + # tree structure of dependencies
> > > + hier_atoms =
> > > portage.dep.use_reduce(
> > > + mydepstr,
> > > + flat=False,
> > > + matchall=1,
> > > +
> > > is_valid_flag=pkg.iuse.is_valid_flag,
> > > + opconvert=True,
> > > + token_class=token_class)
> > > + except
> > > portage.exception.InvalidDependString as e:
> > > + hier_atoms = None
> > > + badsyntax.append(str(e))
> > > + check_slotop(hier_atoms, mytype,
> > > qatracker, ebuild.relative_path)
> > 
> > 
> > Is there a special reason this code can not be part of the
> > check_slotop()?  All it has is the one statement calling it's
> > internal recursive function.  Then all this code would would be
> > replaced here by the one check_slotop() which adds to badsyntax.  
> 
> I don't have special preference. I've followed example of what a
> check few lines before does to call check_missingslot(). I've sent a
> V2 patch w/o pulling it off to a separate helper.
> 
> If you think it should be better moved completely to a separate
> helper I'll move it there. As you see I have a very weak python fu.
> 
> If there is examples of better ways to pass that large context of
> variables please point me to it :)
> 

OK, V2 is better, but not quite what I had thought of.

The original check_sloptop() had 2 items, the _traverse_tree() and the
one staement calling it.  So, yes it is better that in that it doesn't
have the needless sub function.  But the _depend_checks() is getting
quite long already.  The new code you add in there makes it even longer
and adds even more branching.  It can lead to things being more
complicated for future changes.  What I meant was to move most of that
additional code into the pretty unused check_sloptop() and have it call
the _traverse_tree() from there.  That would simplify the
depends_check() and move the check_sloptop specific code to that
function.  Then the _traverse_tree sub-function would be utilized with
the check_slotop() better utilized.

 so:

+   if runtime:
+   check_slotop(...)

would be all that was added to _depends_check() directly.

Zac, what do you think?
-- 
Brian Dolbec 



pgpyqjwGVVJ0V.pgp
Description: OpenPGP digital signature

Re: [gentoo-portage-dev] [PATCH] repoman: new QA error: slot operator under '||' alternative

[Brian Dolbec]

On Sat, 18 Jun 2016 18:01:11 +0100
Sergei Trofimovich  wrote:

> From: Sergei Trofimovich 
> 
> A few links discussing what can go wrong with slot operator under
> '||':
> https://archives.gentoo.org/gentoo-dev/message/81a4e1a1f5767e20009628ecd33da8d4
> https://archives.gentoo.org/gentoo-dev/message/66ff016a055e57b6027abcd36734e0e3
> 
> The main problem here is how vdb gets updated when you have a
> dependency of style: RDEPEND="|| ( foo:= bar:= )"
> depending on what you have installed in system: only 'foo'/'bar' or
> both 'foo' and 'bar'.
> 
> I'm about to add actual test for some examples to gen-b0rk.
> 
> New repoman complains on them as follows:
> 
> RDEPEND="|| ( =not-broken/pkg1-subslot-0:=
> =not-broken/pkg1-subslot-1:0= )"
> 
> Yields:
> 
>   dependency.badslotop [fatal]  2
>ebuild-test/RDEPEND-any-of-slotop/RDEPEND-any-of-slotop-0.ebuild:
> RDEPEND: '=not-broken/pkg1-subslot-0:=' uses ':=' slot operator under
> '||' dep clause.
> ebuild-test/RDEPEND-any-of-slotop/RDEPEND-any-of-slotop-0.ebuild:
> RDEPEND: '=not-broken/pkg1-subslot-1:0=' uses ':=' slot operator
> under '||' dep clause.
> 
> CC: q...@gentoo.org
> CC: mgo...@gentoo.org
> Signed-off-by: Sergei Trofimovich 
> ---
>  repoman/pym/repoman/check_slotop.py| 32
> ++ .../repoman/modules/scan/depend/_depend_checks.py
> | 17  repoman/pym/repoman/qa_data.py
> |  2 ++ 3 files changed, 51 insertions(+)
>  create mode 100644 repoman/pym/repoman/check_slotop.py
> 


Hmm, this file should be in the module namespace, not in the base
repoman namespace.  It is not used anywhere else, so should be in the
module like _depend_checks.py

Another alternative is just add it to _depend_checks.py, it isn't a
very large file and is the only place this code is used.


> diff --git a/repoman/pym/repoman/check_slotop.py
> b/repoman/pym/repoman/check_slotop.py new file mode 100644
> index 000..3c3aec1
> --- /dev/null
> +++ b/repoman/pym/repoman/check_slotop.py
> @@ -0,0 +1,32 @@
> +# -*- coding:utf-8 -*-
> +# repoman: missing slot check
> +# Copyright 2016 Gentoo Foundation
> +# Distributed under the terms of the GNU General Public License v2
> +
> +"""This module contains the check used to find ':=' slot operator
> +uses in '||' style dependencies."""
> +
> +from portage.dep import Atom
> +
> +def check_slotop(dep_tree, mytype, qatracker, relative_path):
> + def _traverse_tree(dep_tree, is_under_any_of):
> + # leaf
> + if isinstance(dep_tree, Atom):
> + atom = dep_tree
> + if is_under_any_of and atom.slot_operator:
> +
> qatracker.add_error("dependency.badslotop", relative_path +
> + ": %s: '%s' uses ':=' slot
> operator under '||' dep clause." %
> + (mytype, atom))
> +
> + # branches
> + if isinstance(dep_tree, list):
> + if len(dep_tree) == 0:
> + return
> + # any-of
> + if dep_tree[0] == '||':
> + _traverse_tree(dep_tree[1:], True)
> + else:
> + for branch in dep_tree:
> + _traverse_tree(branch,
> is_under_any_of) +
> + _traverse_tree(dep_tree, False)
> diff --git
> a/repoman/pym/repoman/modules/scan/depend/_depend_checks.py
> b/repoman/pym/repoman/modules/scan/depend/_depend_checks.py index
> 4e1d216..1fd69d4 100644 ---
> a/repoman/pym/repoman/modules/scan/depend/_depend_checks.py +++
> b/repoman/pym/repoman/modules/scan/depend/_depend_checks.py @@ -4,6
> +4,7 @@ from _emerge.Package import Package 
>  from repoman.check_missingslot import check_missingslot
> +from repoman.check_slotop import check_slotop
>  # import our initialized portage instance
>  from repoman._portage import portage
>  from repoman.qa_data import suspect_virtual, suspect_rdepend
> @@ -117,6 +118,22 @@ def _depend_checks(ebuild, pkg, portdb,
> qatracker, repo_metadata): 
>   type_list.extend([mytype] * (len(badsyntax) -
> len(type_list))) 




> + if runtime:
> + try:
> + # to find use of ':=' in '||' we
> preserve
> + # tree structure of dependencies
> + hier_atoms = portage.dep.use_reduce(
> + mydepstr,
> + flat=False,
> + matchall=1,
> +
> is_valid_flag=pkg.iuse.is_valid_flag,
> + opconvert=True,
> + token_class=token_class)
> + except portage.exception.InvalidDependString
> as e:
> + hier_atoms = None
> +