Re: [gentoo-portage-dev] [PATCH] Another slot operator bug (bug 486580, try 2)

[Sebastian Luther]

Am 28.11.2013 15:47, schrieb Brian Dolbec:
> I like this one much better, thank you.
>
> Could you rebase the 2 patches together, tis one shows removing
> lines from the first.  Also see inline comments below :)

The removed lines are another large if-block that already existed.
This has been moved into _ignore_dependency() too.

>
> On Thu, 2013-11-28 at 11:34 +0100, SebastianLuther@×××.de wrote:
>> From: Sebastian Luther <SebastianLuther@×××.de>
>>
>> This time rebuilds are scheduled properly, but we might still
>> forget to install the package that caused the rebuild.
>>
>> URL: https://bugs.gentoo.org/486580 --- pym/_emerge/depgraph.py
>> | 48 +++++++++++------
>> .../tests/resolver/test_slot_conflict_rebuild.py   | 63
>> ++++++++++++++++++++++ 2 files changed, 95 insertions(+), 16
>> deletions(-)
>>
>> diff --git a/pym/_emerge/depgraph.py b/pym/_emerge/depgraph.py
>> index da2e604..0a998b5 100644 --- a/pym/_emerge/depgraph.py +++
>> b/pym/_emerge/depgraph.py @@ -2268,6 +2268,36 @@ class
>> depgraph(object): finally: self._dynamic_config._autounmask =
>> _autounmask_backup
>>
>> +	def _ignore_dependency(self, atom, pkg, child, dep, mypriority,
>> recurse_satisfied): +		""" +		In some cases, dep_check will
>> return deps that shouldn't +		be proccessed any further, so they
>> are identified and +		discarded here. Try to discard as few as
>> possible since +		discarded dependencies reduce the amount of
>> information +		available for optimization of merge order. +
>> Don't ignore dependencies if pkg has a slot operator dependency
>> on the child +		and the child has changed slot/sub_slot. +		""" +
>> slot_operator_rebuild = False +		if atom.slot_operator == '=' and
>> \ +			(pkg.root, pkg.slot_atom) in
>> self._dynamic_config._slot_operator_replace_installed and \ +
>> mypriority.satisfied and \ +			mypriority.satisfied is not child
>> and \ +			mypriority.satisfied.installed and \ +			not
>> child.installed and \ +			(child.slot !=
>> mypriority.satisfied.slot or child.sub_slot !=
>> mypriority.satisfied.sub_slot): +			slot_operator_rebuild = True
>> + +		return not atom.blocker and \ +			not recurse_satisfied and
>> \ +			mypriority.satisfied and \ +			mypriority.satisfied.visible
>> and \ +			dep.child is not None and \ +			not dep.child.installed
>> and \ +
>> self._dynamic_config._slot_pkg_map[dep.child.root].get( +
>> dep.child.slot_atom) is None and \ +			not slot_operator_rebuild
>> +
>
> Since slot_operator_rebuild is already set either true or false,
> would it be better if "not slot_operator_rebuild" is first in the
> return statement.  If it resolves False, it will be an early out
> for the remaining checks.  Personally I don't know which is the
> more common case.  But if it resolve False more often, it will be a
> slight speed improvement.  That goes for the rest of them too, they
> should be ordered in the most often to resolve False order.  Same
> for the if as well.

This should be more often False than True. recurse_satisfied in the
second block should be more often True than False.

In fact there's some duplication between the two blocks. Feel free to
clean this up after it has been pushed.


>
> /end tiny nitpick
>
>> def _wrapped_add_pkg_dep_string(self, pkg, dep_root,
>> dep_priority, dep_string, allow_unsatisfied): depth = pkg.depth +
>> 1 @@ -2357,14 +2387,7 @@ class depgraph(object): # discarded
>> dependencies reduce the amount of information # available for
>> optimization of merge order. ignored = False -			if not
>> atom.blocker and \ -				not recurse_satisfied and \ -
>> mypriority.satisfied and \ -				mypriority.satisfied.visible and
>> \ -				dep.child is not None and \ -				not dep.child.installed
>> and \ -
>> self._dynamic_config._slot_pkg_map[dep.child.root].get( -
>> dep.child.slot_atom) is None: +			if
>> self._ignore_dependency(atom, pkg, child, dep, mypriority,
>> recurse_satisfied): myarg = None try: myarg =
>> next(self._iter_atoms_for_pkg(dep.child), None) @@ -2467,14
>> +2490,7 @@ class depgraph(object): collapsed_parent=pkg,
>> collapsed_priority=dep_priority)
>>
>> ignored = False -				if not atom.blocker and \ -					not
>> recurse_satisfied and \ -					mypriority.satisfied and \ -
>> mypriority.satisfied.visible and \ -					dep.child is not None
>> and \ -					not dep.child.installed and \ -
>> self._dynamic_config._slot_pkg_map[dep.child.root].get( -
>> dep.child.slot_atom) is None: +				if
>> self._ignore_dependency(atom, pkg, child, dep, mypriority,
>> recurse_satisfied): myarg = None try: myarg =
>> next(self._iter_atoms_for_pkg(dep.child), None) diff --git
>> a/pym/portage/tests/resolver/test_slot_conflict_rebuild.py
>> b/pym/portage/tests/resolver/test_slot_conflict_rebuild.py index
>> 74f5cc1..e3c517d 100644 ---
>> a/pym/portage/tests/resolver/test_slot_conflict_rebuild.py +++
>> b/pym/portage/tests/resolver/test_slot_conflict_rebuild.py @@
>> -181,6 +181,69 @@ class SlotConflictRebuildTestCase(TestCase):
>> finally: playground.cleanup()
>>
>> +	def testSlotConflictForgottenChild(self): +		""" +		Similar to
>> testSlotConflictMassRebuild above, but this time the rebuilds are
>> scheduled, +		but the package causing the rebuild (the child) is
>> not installed. +		""" +		ebuilds = { + +			"app-misc/A-2" : { +
>> "EAPI": "5", +				"DEPEND": "app-misc/B:= app-misc/C", +
>> "RDEPEND": "app-misc/B:= app-misc/C", +			}, + +			"app-misc/B-2"
>> : { +				"EAPI": "5", +				"SLOT": "2" +			}, + +
>> "app-misc/C-1": { +				"EAPI": "5", +				"DEPEND":
>> "app-misc/B:=", +				"RDEPEND": "app-misc/B:=" +			}, +		} + +
>> installed = { +			"app-misc/A-1" : { +				"EAPI": "5", +
>> "DEPEND": "app-misc/B:1/1= app-misc/C", +				"RDEPEND":
>> "app-misc/B:1/1= app-misc/C", +			}, + +			"app-misc/B-1" : { +
>> "EAPI": "5", +				"SLOT": "1" +			}, + +			"app-misc/C-1": { +
>> "EAPI": "5", +				"DEPEND": "app-misc/B:1/1=", +				"RDEPEND":
>> "app-misc/B:1/1=" +			}, +		} + +		test_cases = ( +
>> ResolverPlaygroundTestCase( +				["app-misc/A"], +				success =
>> True, +				mergelist = ['app-misc/B-2', 'app-misc/C-1',
>> 'app-misc/A-2']), +		) + +		world = [] + +		playground =
>> ResolverPlayground(ebuilds=ebuilds, +			installed=installed,
>> world=world, debug=False) +		try: +			for test_case in
>> test_cases: +				playground.run_TestCase(test_case) +
>> self.assertEqual(test_case.test_success, True,
>> test_case.fail_msg) +		finally: +			playground.cleanup() + + def
>> testSlotConflictDepChange(self): """ Bug 490362
>