fix the build dependencies to include recursive run dependencies from packages within the workspace#310
Conversation
… packages within the workspace
| run_depends_in_pkgs = set([d.name for d in depends if d.name in other_pkgs_by_names]) | ||
| while run_depends_in_pkgs: | ||
| # pick first element from sorted order to ensure deterministic results | ||
| pkg_name = sorted(run_depends_in_pkgs).pop(0) |
There was a problem hiding this comment.
This would be more efficient to call
run_depends_in_pkgs.sort()
run_depends_in_pkgs.pop(0)
And then you can get rid of run_depends_in_pkgs.remove(pkg_name)
Sorting an already sorted list is more efficient and this will avoid the copy as well.
There was a problem hiding this comment.
The variable is currently a set in order to avoid duplicates but a set can't be sorted. I am not sure if converting the data structure to a list (which can be sorted) makes sense since then it will require manual logic to deduplicate.
There was a problem hiding this comment.
Nevermind, I was thinking it was a list. The logic to deduplicate is definitely worth avoiding at the cost of a sort/copy.
|
One possible optimization but otherwise looks good to me. Seeing this in the script kind of makes me wonder if there might be a more centralized location for this sort of logic that might be shareable between the different scripts. But it is a kind of application specific sorting so might not be worth trying to centralize. |
2 participants
Reported in ros-simulation/gazebo_ros_pkgs#473 (comment)
I ran a test job with the branch of the pull request and
gazebowas added as a build dependency as desired.