<div dir="ltr">It might be a good idea to do a v2, but you can wait until I've had a better look through the other code if you like, in case there's anything else.<div><br></div><div>Elliot</div></div><div class="gmail_extra"><br><div class="gmail_quote">On 16 March 2016 at 15:51, Lerner, David M (Wind River) <span dir="ltr"><<a href="mailto:dave.lerner@windriver.com" target="_blank">dave.lerner@windriver.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Elliot,<br>
I realized this morning that a line in _get_all_dependents() is benign but not required.  The impact is performance - an unnecessary pass through the list of reverse dependendencies.  I don't know if that justifies a V2 patch or not.<br>
<br>
See the details below, inline.<br>
Dave<br>
<div><div class="h5"><br>
> -----Original Message-----<br>
> From: Dave Lerner [mailto:<a href="mailto:dave.lerner@windriver.com">dave.lerner@windriver.com</a>]<br>
> Sent: Friday, March 11, 2016 10:29 AM<br>
> To: <a href="mailto:toaster@yoctoproject.org">toaster@yoctoproject.org</a>; <a href="mailto:belen.barros.pena@linux.intel.com">belen.barros.pena@linux.intel.com</a>; WOOD, MICHAEL<br>
> Cc: Lerner, Dave<br>
> Subject: [PATCH 3/3] toaster: get all dependents for pkg for removal<br>
><br>
> For customised image package removal change behavior.<br>
> From:<br>
>  Â  Â Only display the immediate dependents of the requested package<br>
>  Â  Â to remove, not the full dependent list, that is dependents of<br>
>  Â  Â dependents ...<br>
>  Â  Â Do not remove the displayed dependents, just notify the user<br>
>  Â  Â of the list.<br>
> To:<br>
>  Â  Â Display the complete dependent tree, traversing all reverse<br>
>  Â  Â dependencies starting from the package to be removed and then it's<br>
>  Â  Â dependents.<br>
>  Â  Â Change the modal dialog to note that all of these dependents will<br>
>  Â  Â be removed automatically.<br>
><br>
> [YOCTO #9121]<br>
><br>
> Signed-off-by: Dave Lerner <<a href="mailto:dave.lerner@windriver.com">dave.lerner@windriver.com</a>><br>
> ---<br>
>  bitbake/lib/toaster/toastergui/views.py | 98 +++++++++++++++++++++++++++++----<br>
>  1 file changed, 86 insertions(+), 12 deletions(-)<br>
><br>
> diff --git a/bitbake/lib/toaster/toastergui/views.py<br>
> b/bitbake/lib/toaster/toastergui/views.py<br>
> index 85ca9be..4442d33 100755<br>
> --- a/bitbake/lib/toaster/toastergui/views.py<br>
> +++ b/bitbake/lib/toaster/toastergui/views.py<br>
> @@ -2538,6 +2538,67 @@ if True:<br>
><br>
>  Â  Â  Â  Â  return response<br>
><br>
> +  Â  def _traverse_dependents(next_package_id, list, all_current_packages, tree_level):<br>
> +  Â  Â  Â  """<br>
> +  Â  Â  Â  Recurse through dependents (reverse dependency) tree up to a<br>
> +  Â  Â  Â  depth of MAX_DEPENDENCY_TREE_DEPTH just in case their are circular<br>
> +  Â  Â  Â  dependencies. A large system has 500 packages, exceeding a<br>
> +  Â  Â  Â  dependency chain that long is unlikely.<br>
> +  Â  Â  Â  Don't scan for dependencies for a package already in list.<br>
> +  Â  Â  Â  Return the updated list with new dependencies.<br>
> +  Â  Â  Â  """<br>
> +  Â  Â  Â  MAX_DEPENDENCY_TREE_DEPTH = 500<br>
> +  Â  Â  Â  if tree_level >= MAX_DEPENDENCY_TREE_DEPTH:<br>
> +  Â  Â  Â  Â  Â  logger.warning(<br>
> +  Â  Â  Â  Â  Â  Â  Â  "The number of dependents in the dependency tree"<br>
> +  Â  Â  Â  Â  Â  Â  Â  "for this package exceeds " + MAX_DEPENDENDENCY_TREE_DEPTH +<br>
> +  Â  Â  Â  Â  Â  Â  Â  " and the remaining dependents will not be removed")<br>
> +  Â  Â  Â  Â  Â  return list, True<br>
> +<br>
> +  Â  Â  Â  package = CustomImagePackage.objects.get(id=next_package_id)<br>
> +  Â  Â  Â  dependents = \<br>
> +  Â  Â  Â  Â  Â  package.package_dependencies_target.annotate(<br>
> +  Â  Â  Â  Â  Â  Â  Â  name=F('package__name'),<br>
> +  Â  Â  Â  Â  Â  Â  Â  pk=F('package__pk'),<br>
> +  Â  Â  Â  Â  Â  Â  Â  size=F('package__size'),<br>
> +  Â  Â  Â  Â  Â  ).values("name", "pk", "size").exclude(<br>
> +  Â  Â  Â  Â  Â  Â  Â  ~Q(pk__in=all_current_packages)<br>
> +  Â  Â  Â  Â  Â  )<br>
> +<br>
> +  Â  Â  Â  truncated = False<br>
> +  Â  Â  Â  for pkg in dependents:<br>
> +  Â  Â  Â  Â  Â  if pkg in list:<br>
> +  Â  Â  Â  Â  Â  Â  Â  # already seen, skip dependent search<br>
> +  Â  Â  Â  Â  Â  Â  Â  continue<br>
<br>
</div></div>The scan prevents duplicate packages in the list and therefor the groupby list line below to make list entries unique is not required.<br>
<span class=""><br>
> +  Â  Â  Â  Â  Â  list.append(pkg)<br>
> +  Â  Â  Â  Â  Â  extension, truncated = _traverse_dependents(<br>
> +  Â  Â  Â  Â  Â  Â  Â  pkg["pk"], list, all_current_packages, tree_level+1)<br>
> +  Â  Â  Â  Â  Â  if truncated:<br>
> +  Â  Â  Â  Â  Â  Â  Â  break<br>
> +<br>
> +  Â  Â  Â  return list, truncated<br>
> +<br>
> +  Â  def _get_all_dependents(package_id, all_current_packages, flat):<br>
> +  Â  Â  Â  """<br>
> +  Â  Â  Â  Returns the full list of dependents, also known as reverse<br>
> +  Â  Â  Â  dependencies, for package_id, recursing through dependency<br>
> +  Â  Â  Â  relationships.<br>
> +  Â  Â  Â  Returns either list of dictionary items or a list of CustomImagePackage<br>
> +  Â  Â  Â  model objects depending on arg flat=False<br>
> +  Â  Â  Â  """<br>
> +  Â  Â  Â  import itertools<br>
> +  Â  Â  Â  list = []<br>
> +  Â  Â  Â  list, truncated = _traverse_dependents(<br>
> +  Â  Â  Â  Â  Â  package_id, list, all_current_packages, 0)<br>
> +<br>
> +  Â  Â  Â  # sort and remove duplicates<br>
> +  Â  Â  Â  sortedlist = sorted(list, key=lambda x: x["name"])<br>
> +  Â  Â  Â  list = [x[0] for x in itertools.groupby(sortedlist)]<br>
<br>
</span>The above line is should not be necessary, the list entries are already unique.<br>
<br>
> +<br>
<div class="HOEnZb"><div class="h5">> +<br>
> +  Â  Â  Â  if flat:<br>
> +  Â  Â  Â  Â  Â  list = [CustomImagePackage.objects.get(id=entry["pk"]) for entry in list]<br>
> +  Â  Â  Â  return list<br>
><br>
>  Â  Â  @xhr_response<br>
>  Â  Â  def xhr_customrecipe_packages(request, recipe_id, package_id):<br>
> @@ -2606,15 +2667,10 @@ if True:<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  )<br>
><br>
>  Â  Â  Â  Â  Â  Â  Â  Â  # Reverse dependencies which are needed by packages that are<br>
> -  Â  Â  Â  Â  Â  Â  Â  # in the image<br>
> -  Â  Â  Â  Â  Â  Â  Â  reverse_deps = package.package_dependencies_target.annotate(<br>
> -  Â  Â  Â  Â  Â  Â  Â  Â  Â  name=F('package__name'),<br>
> -  Â  Â  Â  Â  Â  Â  Â  Â  Â  pk=F('package__pk'),<br>
> -  Â  Â  Â  Â  Â  Â  Â  Â  Â  size=F('package__size'),<br>
> -  Â  Â  Â  Â  Â  Â  Â  ).values("name", "pk", "size").exclude(<br>
> -  Â  Â  Â  Â  Â  Â  Â  Â  Â  ~Q(pk__in=all_current_packages)<br>
> -  Â  Â  Â  Â  Â  Â  Â  )<br>
> -<br>
> +  Â  Â  Â  Â  Â  Â  Â  # in the image. Recursive search providing all dependents,<br>
> +  Â  Â  Â  Â  Â  Â  Â  # not just immediate dependents.<br>
> +  Â  Â  Â  Â  Â  Â  Â  reverse_deps = _get_all_dependents(<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  package_id, all_current_packages, flat=False)<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  total_size_deps = 0<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  total_size_reverse_deps = 0<br>
><br>
> @@ -2658,6 +2714,11 @@ if True:<br>
><br>
>  Â  Â  Â  Â  Â  Â  else:<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  recipe.appends_set.add(package)<br>
> +  Â  Â  Â  Â  Â  Â  Â  # Make sure that package is not in the excludes set<br>
> +  Â  Â  Â  Â  Â  Â  Â  try:<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  recipe.excludes_set.remove(package)<br>
> +  Â  Â  Â  Â  Â  Â  Â  except:<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  pass<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  # Add the dependencies we think will be added to the recipe<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  # as a result of appending this package.<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  # TODO this should recurse down the entire deps tree<br>
> @@ -2668,11 +2729,12 @@ if True:<br>
><br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  recipe.includes_set.add(cust_package)<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  try:<br>
> -  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  # when adding the pre-requisite package make sure it's not<br>
> in the<br>
> -  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  #  Â excluded list from a prior removal.<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  # When adding the pre-requisite package, make<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  # sure it's not in the excluded list from a<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  # prior removal.<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  recipe.excludes_set.remove(cust_package)<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  except Package.DoesNotExist:<br>
> -  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  #  Â Don't care if the package had never been excluded<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  # Don't care if the package had never been excluded<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  pass<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  except:<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  logger.warning("Could not add package's suggested"<br>
> @@ -2688,6 +2750,18 @@ if True:<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  recipe.excludes_set.add(package)<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  else:<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  recipe.appends_set.remove(package)<br>
> +  Â  Â  Â  Â  Â  Â  Â  all_current_packages = recipe.get_all_packages()<br>
> +  Â  Â  Â  Â  Â  Â  Â  reverse_deps = _get_all_dependents(<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  <a href="http://package.pk" rel="noreferrer" target="_blank">package.pk</a>, all_current_packages, flat=True)<br>
> +  Â  Â  Â  Â  Â  Â  Â  for r in reverse_deps:<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  try:<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  if <a href="http://r.id" rel="noreferrer" target="_blank">r.id</a> in included_packages:<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  recipe.excludes_set.add(r)<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  else:<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  recipe.appends_set.remove(r)<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  except:<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  pass<br>
> +<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  return {"error": "ok"}<br>
>  Â  Â  Â  Â  Â  Â  except CustomImageRecipe.DoesNotExist:<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  return {"error": "Tried to remove package that wasn't present"}<br>
> --<br>
> 1.9.1<br>
<br>
</div></div></blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="gmail_signature"><div dir="ltr"><div>Elliot Smith<br>Software Engineer<br>Intel Open Source Technology Centre</div></div></div>
</div>