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

[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 :)

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.

/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