[Toaster] [PATCH 3/3] toaster: get all dependents for pkg for removal
Lerner, Dave
dave.lerner at windriver.com
Thu Mar 17 14:53:34 PDT 2016
Hi Elliot,
All but two recommendations implemented - see inline.
v1 vs v2 is attached to this email. The complete v2 will be sent out separately.
Thanks for the helpful review,
Dave
> -----Original Message-----
> From: Smith, Elliot [mailto:elliot.smith at intel.com]
> Sent: Wednesday, March 16, 2016 1:26 PM
> To: Lerner, Dave
> Cc: toaster at yoctoproject.org; Belen Barros Pena (Intel); WOOD, MICHAEL
> Subject: Re: [Toaster] [PATCH 3/3] toaster: get all dependents for pkg for removal
>
> Thanks for this, Dave. My comments are inline below.
>
> NB I have just done a code review, and haven't actually run the code.
>
>
> On 11 March 2016 at 16:29, Dave Lerner <dave.lerner at windriver.com> wrote:
>
>
> For customised image package removal change behavior.
> From:
> Only display the immediate dependents of the requested package
> to remove, not the full dependent list, that is dependents of
> dependents ...
> Do not remove the displayed dependents, just notify the user
> of the list.
> To:
> Display the complete dependent tree, traversing all reverse
> dependencies starting from the package to be removed and then it's
> dependents.
> Change the modal dialog to note that all of these dependents will
> be removed automatically.
>
> [YOCTO #9121]
>
> Signed-off-by: Dave Lerner <dave.lerner at windriver.com>
> ---
> bitbake/lib/toaster/toastergui/views.py | 98 +++++++++++++++++++++++++++++----
> 1 file changed, 86 insertions(+), 12 deletions(-)
>
> diff --git a/bitbake/lib/toaster/toastergui/views.py
> b/bitbake/lib/toaster/toastergui/views.py
> index 85ca9be..4442d33 100755
> --- a/bitbake/lib/toaster/toastergui/views.py
> +++ b/bitbake/lib/toaster/toastergui/views.py
> @@ -2538,6 +2538,67 @@ if True:
>
> return response
>
> + def _traverse_dependents(next_package_id, list, all_current_packages,
> tree_level):
> + """
> + Recurse through dependents (reverse dependency) tree up to a
> + depth of MAX_DEPENDENCY_TREE_DEPTH just in case their are circular
> + dependencies. A large system has 500 packages, exceeding a
> + dependency chain that long is unlikely.
> + Don't scan for dependencies for a package already in list.
> + Return the updated list with new dependencies.
> + """
> + MAX_DEPENDENCY_TREE_DEPTH = 500
> + if tree_level >= MAX_DEPENDENCY_TREE_DEPTH:
> + logger.warning(
> + "The number of dependents in the dependency tree"
> + "for this package exceeds " + MAX_DEPENDENDENCY_TREE_DEPTH +
> + " and the remaining dependents will not be removed")
> + return list, True
>
>
>
> This makes me a bit nervous, but I'm not sure if there's a better way to deal with it.
>
> Could we reduce the maximum depth of the tree, e.g. set it to the smaller of 500 or
> len(all_current_packages)? (I'm not sure what all_current_packages has in it, but if it
> means all packages in the image, then the max depth of a genuine dependency chain would
> be equal to the number of packages - 1.)
Good idea, fixed.
>
> +
> + package = CustomImagePackage.objects.get(id=next_package_id)
> + dependents = \
> + package.package_dependencies_target.annotate(
> + name=F('package__name'),
> + pk=F('package__pk'),
> + size=F('package__size'),
> + ).values("name", "pk", "size").exclude(
> + ~Q(pk__in=all_current_packages)
> + )
> +
> + truncated = False
> + for pkg in dependents:
> + if pkg in list:
> + # already seen, skip dependent search
> + continue
> +
> + list.append(pkg)
> + extension, truncated = _traverse_dependents(
>
>
>
> I was slightly confused by the use of extension, then realised that it's not actually
> used for anything. Maybe a comment there to that effect?
Sorry for the confusion, cruft got left in from the first implementation of the function that appended trees rather than single packages. Now 'extension' is removed, only one variable returned, what was formerly written to 'truncated'.
> I'm also not 100% sure about the structure of this, as the method is being used in two
> modes, each of which only cares about one of the return values; and the value we're
> actually interested in is an argument which is being updated in place. Though I can't
> think of a better alternative right now.
>
>
> + pkg["pk"], list, all_current_packages, tree_level+1)
> + if truncated:
> + break
> +
> + return list, truncated
Only one value returned, and a return of True, is the flag to exit the scan.
> +
> + def _get_all_dependents(package_id, all_current_packages, flat):
> + """
> + Returns the full list of dependents, also known as reverse
> + dependencies, for package_id, recursing through dependency
> + relationships.
> + Returns either list of dictionary items or a list of CustomImagePackage
> + model objects depending on arg flat=False
> + """
> + import itertools
>
>
>
> My preference would be for all imports to be at the top of the file.
>
Done
> + list = []
> + list, truncated = _traverse_dependents(
> + package_id, list, all_current_packages, 0)
>
>
>
> Rather than pass 0 and [] here, I would change the signature of _traverse_dependents to:
>
> def _traverse_dependents(next_package_id, all_current_packages, list=[], tree_level=0):
>
> Then call it here with:
>
>
> list, truncated = _traverse_dependents(
> package_id, all_current_packages)
>
I'd rather not declare the returned reverse dependency in the signature. Because I've changed the _traverse function so that only one parameter is returned, the exit early flag, shown above as the poorly named 'truncated'. The referenced reverse dependencies list then must be explicitly passed in so that the caller can see it. This seems cleaner than returning two values, one of which is ignored during the recursion, which as you noted is confusing and requires a comment, because just for declaring the empty reverse_package list in the signature. The resulting code looks much cleaner to me. Look for the V2 patch.
>
> +
> + # sort and remove duplicates
> + sortedlist = sorted(list, key=lambda x: x["name"])
> + list = [x[0] for x in itertools.groupby(sortedlist)]
>
>
>
> (This is the line you marked as unnecessary, as the list already contains unique
> elements.)
>
Removed.
> +
> + if flat:
> + list = [CustomImagePackage.objects.get(id=entry["pk"]) for entry in
> list]
>
>
>
> Could this be done with one query, e.g.
>
> if flat:
> ids = [entry['pk'] for entry in list]
> list = list(CustomImagePackage.objects.filter(id__in=ids))
>
> ?
> By the way, is the cast to list necessary, as the queryset is iterable and countable?
Within xhr_customrecipe_packages(), the new recursive call replaces two queries, each requiring a slightly different list:
1) to populate the modal dialog, a list of dictionaries with annotated field names, not model instances,
2) to apply removing the packages from the database, a list of model instances.
I've simplified _get_all_dependents and reduced coupling to always return the first format, list of dictionaries, and changed the code in the caller when model instances are needed, getting them with your more efficient query.
> I suggest changing the variable name "list" to something else, too.
>
>
>
> + return list
Fixed, renamed to rev_deps.
> @xhr_response
> def xhr_customrecipe_packages(request, recipe_id, package_id):
> @@ -2606,15 +2667,10 @@ if True:
> )
>
> # Reverse dependencies which are needed by packages that are
> - # in the image
> - reverse_deps = package.package_dependencies_target.annotate(
> - name=F('package__name'),
> - pk=F('package__pk'),
> - size=F('package__size'),
> - ).values("name", "pk", "size").exclude(
> - ~Q(pk__in=all_current_packages)
> - )
> -
> + # in the image. Recursive search providing all dependents,
> + # not just immediate dependents.
> + reverse_deps = _get_all_dependents(
> + package_id, all_current_packages, flat=False)
> total_size_deps = 0
> total_size_reverse_deps = 0
>
> @@ -2658,6 +2714,11 @@ if True:
>
> else:
> recipe.appends_set.add(package)
> + # Make sure that package is not in the excludes set
> + try:
> + recipe.excludes_set.remove(package)
> + except:
> + pass
> # Add the dependencies we think will be added to the recipe
> # as a result of appending this package.
> # TODO this should recurse down the entire deps tree
> @@ -2668,11 +2729,12 @@ if True:
>
> recipe.includes_set.add(cust_package)
> try:
> - # when adding the pre-requisite package make sure it's
> not in the
> - # excluded list from a prior removal.
> + # When adding the pre-requisite package, make
> + # sure it's not in the excluded list from a
> + # prior removal.
> recipe.excludes_set.remove(cust_package)
> except Package.DoesNotExist:
> - # Don't care if the package had never been excluded
> + # Don't care if the package had never been excluded
> pass
> except:
> logger.warning("Could not add package's suggested"
> @@ -2688,6 +2750,18 @@ if True:
> recipe.excludes_set.add(package)
> else:
> recipe.appends_set.remove(package)
> + all_current_packages = recipe.get_all_packages()
> + reverse_deps = _get_all_dependents(
> + package.pk, all_current_packages, flat=True)
> + for r in reverse_deps:
> + try:
> + if r.id in included_packages:
> + recipe.excludes_set.add(r)
> + else:
> + recipe.appends_set.remove(r)
> + except:
> + pass
> +
> return {"error": "ok"}
> except CustomImageRecipe.DoesNotExist:
> return {"error": "Tried to remove package that wasn't present"}
> --
> 1.9.1
>
> --
> _______________________________________________
> toaster mailing list
> toaster at yoctoproject.org
> https://lists.yoctoproject.org/listinfo/toaster
>
>
>
>
>
> --
>
> Elliot Smith
> Software Engineer
> Intel Open Source Technology Centre
V1 versus V2 changes. V2 patch and RR will be sent separately
==============================================================
commit f4555f162bd116ef1438123ee0bde5c850111145
Author: Dave Lerner <dave.lerner at windriver.com>
Date: Thu Mar 17 08:16:19 2016 -0500
toaster: recursive package dependency removal
Clean up per code review.
[Yocto #9151]
diff --git a/bitbake/lib/toaster/toastergui/views.py b/bitbake/lib/toaster/toastergui/views.py
index 4442d33..9944c8d 100755
--- a/bitbake/lib/toaster/toastergui/views.py
+++ b/bitbake/lib/toaster/toastergui/views.py
@@ -2538,22 +2538,27 @@ if True:
return response
- def _traverse_dependents(next_package_id, list, all_current_packages, tree_level):
+ def _traverse_dependents(next_package_id, rev_deps, all_current_packages, tree_level=0):
"""
- Recurse through dependents (reverse dependency) tree up to a
- depth of MAX_DEPENDENCY_TREE_DEPTH just in case their are circular
- dependencies. A large system has 500 packages, exceeding a
- dependency chain that long is unlikely.
- Don't scan for dependencies for a package already in list.
- Return the updated list with new dependencies.
+ Recurse through reverse dependency tree for next_package_id.
+ Limit the reverse dependency search to packages not already scanned,
+ that is, not already in rev_deps.
+ Limit the scan to a depth (tree_level) not exceeding the count of
+ all packages in the custom image, and if that depth is exceeded
+ return False, pop out of the recursion, and write a warning
+ to the log, but this is unlikely, suggesting a dependency loop
+ not caught by bitbake.
+ On return, the input/output arg rev_deps is appended with queryset
+ dictionary elements, annotated for use in the customimage template.
+ The list has unsorted, but unique elements.
"""
- MAX_DEPENDENCY_TREE_DEPTH = 500
- if tree_level >= MAX_DEPENDENCY_TREE_DEPTH:
+ max_dependency_tree_depth = all_current_packages.count()
+ if tree_level >= max_dependency_tree_depth:
logger.warning(
- "The number of dependents in the dependency tree"
- "for this package exceeds " + MAX_DEPENDENDENCY_TREE_DEPTH +
- " and the remaining dependents will not be removed")
- return list, True
+ "The number of reverse dependencies "
+ "for this package exceeds " + max_dependency_tree_depth +
+ " and the remaining reverse dependencies will not be removed")
+ return True
package = CustomImagePackage.objects.get(id=next_package_id)
dependents = \
@@ -2565,40 +2570,28 @@ if True:
~Q(pk__in=all_current_packages)
)
- truncated = False
for pkg in dependents:
- if pkg in list:
+ if pkg in rev_deps:
# already seen, skip dependent search
continue
- list.append(pkg)
- extension, truncated = _traverse_dependents(
- pkg["pk"], list, all_current_packages, tree_level+1)
- if truncated:
- break
+ rev_deps.append(pkg)
+ if (_traverse_dependents(
+ pkg["pk"], rev_deps, all_current_packages, tree_level+1)):
+ return True
- return list, truncated
+ return False
- def _get_all_dependents(package_id, all_current_packages, flat):
+ def _get_all_dependents(package_id, all_current_packages):
"""
- Returns the full list of dependents, also known as reverse
- dependencies, for package_id, recursing through dependency
+ Returns sorted list of recursive reverse dependencies for package_id,
+ as a list of dictionary items, by recursing through dependency
relationships.
- Returns either list of dictionary items or a list of CustomImagePackage
- model objects depending on arg flat=False
"""
- import itertools
- list = []
- list, truncated = _traverse_dependents(
- package_id, list, all_current_packages, 0)
-
- # sort and remove duplicates
- sortedlist = sorted(list, key=lambda x: x["name"])
- list = [x[0] for x in itertools.groupby(sortedlist)]
-
- if flat:
- list = [CustomImagePackage.objects.get(id=entry["pk"]) for entry in list]
- return list
+ rev_deps = []
+ _traverse_dependents(package_id, rev_deps, all_current_packages)
+ rev_deps = sorted(rev_deps, key=lambda x: x["name"])
+ return rev_deps
@xhr_response
def xhr_customrecipe_packages(request, recipe_id, package_id):
@@ -2669,8 +2662,7 @@ if True:
# Reverse dependencies which are needed by packages that are
# in the image. Recursive search providing all dependents,
# not just immediate dependents.
- reverse_deps = _get_all_dependents(
- package_id, all_current_packages, flat=False)
+ reverse_deps = _get_all_dependents(package_id, all_current_packages)
total_size_deps = 0
total_size_reverse_deps = 0
@@ -2751,8 +2743,9 @@ if True:
else:
recipe.appends_set.remove(package)
all_current_packages = recipe.get_all_packages()
- reverse_deps = _get_all_dependents(
- package.pk, all_current_packages, flat=True)
+ reverse_deps_dictlist = _get_all_dependents(package.pk, all_current_packages)
+ ids = [entry['pk'] for entry in reverse_deps_dictlist]
+ reverse_deps = CustomImagePackage.objects.filter(id__in=ids)
for r in reverse_deps:
try:
if r.id in included_packages:
More information about the toaster
mailing list