Fix conditional dependencies when one package uses manifest.xml#737
Merged
nuclearsandwich merged 2 commits intomasterfrom Apr 3, 2020
Merged
Fix conditional dependencies when one package uses manifest.xml#737nuclearsandwich merged 2 commits intomasterfrom
nuclearsandwich merged 2 commits intomasterfrom
Conversation
Contributor
Author
|
I didn't look too closely at the |
Contributor
Author
wjwwood
approved these changes
Mar 10, 2020
Contributor
wjwwood
left a comment
wjwwood
left a comment
There was a problem hiding this comment.
The logic makes sense as described, but I'm afraid I don't have the context to tell you if it should be fixed here or in rospkg...
I approve it, but you'll have to watch for issues next time I (or someone) releases rosdep.
Contributor
|
Also, can you have a look at the single test failure to see if it is related? For python 2.7 at least it appears to be passing on master but not this pr. |
Contributor
My gut feeling is that we should push this down as deep as we can so that we can think about conditions in as few places as possible. But to do so we'd need to pass the conditions through to the rospkg handler to forward to catkin_pkg? |
nuclearsandwich
approved these changes
Apr 2, 2020
Contributor
nuclearsandwich
left a comment
nuclearsandwich
left a comment
There was a problem hiding this comment.
I think this will have to do for now. I can't find a good place for this logic to live in rospkg.
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
nuclearsandwich
force-pushed
the
manifest_to_packagexml_conditions
branch
from
9103a1a to
d58a526
Compare
Contributor
|
@sloretz I pushed 0e28606 to address the test failure due to the export of an empty set (and sets in general) rather than lists. I don't think that it breaks anything within the recursive logic because we convert the interal call output to a set in the union() but I'd be grateful for a second set of eyes. |
Contributor
Author
|
@nuclearsandwich 0e28606 LGTM |
gstavrinos
pushed a commit
to gstavrinos/rosdep
that referenced
this pull request
Nov 12, 2020
* Add Suite3 option with Ubuntu Focal (ros-infrastructure#734) Signed-off-by: Shane Loretz <sloretz@openrobotics.org> * [update] Allow to process single ROS distro, fix 723 (ros-infrastructure#738) * [update] skip other distro if --rosdistro passed Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> * ignore argument if specified distro doesnt exist * address review comments * update help message for rosdistro Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> * Fix CI and reduce CI time (ros-infrastructure#739) * test newer python * use yaml.safe_load * pin PyYAML version for Python 3.4 * pass user flag to pip when needed * move slow jobs to the top to reduce CI time Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> * Strip Alpine's patch version from OS codename (ros-infrastructure#716) * Resolve Alpine os_version_type using OsDetect * Reduced Alpine OS VERSION to Major.Minor * improve support for null entries (ros-infrastructure#726) * support null for entire OS and not only OS version syntax like 'ubuntu: null' will now be raising ResolutionError with relevant error message instead of InvalidData with obscure error message will also cover syntax like: " ubuntu: '*': null bionic: [foobar] " Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> Co-authored-by: Scott K Logan <logans@cottsay.net> * Use DNF installer on RHEL 8 and newer (ros-infrastructure#713) RHEL/CentOS 8 uses DNF by default. * Updates to YUM and DNF (ros-infrastructure#640) Output YUM, DNF and RPM versions with `--all-versions` and fix the format guide's default PM for Fedora. * tests: don't assume euid != 0 (ros-infrastructure#703) If tests rely on the environment being a certain way, they should ensure it meets its expectations. Update the tests to ensure they're testing multiple euid conditions, and setting it explicitly where required. Fix ros-infrastructure#702 Signed-off-by: Kyle Fazzari <kyle@canonical.com> * openSUSE package query and install enhancements (ros-infrastructure#729) * Enable PIP installer for openSUSE * openSUSE package detection with RPM capabilities Packages sometimes get renamed and their old name is kept as an rpm capability (like an alias), so the additional flag `--whatprovides` is passed to the `rpm` query. * Fix conditional dependencies when one package uses manifest.xml (ros-infrastructure#737) * Fix conditional dependencies when one package uses manifest.xml Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Return rosdeps as a list to avoid breaking the interface. Co-authored-by: Steven! Ragnarök <steven@nuclearsandwich.com> * Guard next(inter) (ros-infrastructure#701) * guard next(inter) Fix ros-infrastructure#691 Signed-off-by: artivis <jeremie.deray@canonical.com> * Handle StopIteration with slightly less line noise. Co-authored-by: Steven! Ragnarök <steven@nuclearsandwich.com> * [Windows] Add console script entry point (ros-infrastructure#656) * Add console script entry point Add console script entry point for platforms (e.g. Windows) not supporting shebang. * remove scripts entry since we are using console_scripts. * fix bad merge. * fix SKIP_PYTHON_SCRIPTS case. Co-authored-by: Lou Amadio <ooeygui@users.noreply.github.com> * Depend on modules packages only to allow co-installability. (ros-infrastructure#750) When the rosdep modules package was split in [ros-infrastructure#731] the module dependencies weren't updated to only depend on modules packages themselves which prevents rosdep modules for python2 and python3 from actually being co-installable. I haven't yet audited to make sure there's no cli usage of these tools in the modules but it's Friday afternoon and I wanted to get this inked before I walked away. When I return to it I'll check for that before proceeding for reviews. * 0.19.0 * also install buildtool_export_depends (ros-infrastructure#753) not adding 'exec_depends' or 'build_export_depends' are both are included inside 'run_depends' Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> * Drop shebang from a non-executable file (ros-infrastructure#755) * add alias for Pop! OS (ros-infrastructure#757) * add alias for Pop! OS Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> Co-authored-by: Shane Loretz <sloretz@openrobotics.org> Co-authored-by: Mikael Arguedas <mikael.arguedas@gmail.com> Co-authored-by: Mark Hedley Jones <MarkHedleyJones@gmail.com> Co-authored-by: Scott K Logan <logans@cottsay.net> Co-authored-by: Kyle Fazzari <kyle@canonical.com> Co-authored-by: Bjar Ne <43565432+gleichdick@users.noreply.github.com> Co-authored-by: Jeremie Deray <deray.jeremie@gmail.com> Co-authored-by: Sean Yen <seanyen@microsoft.com> Co-authored-by: Lou Amadio <ooeygui@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #736
When rosdep finds a
manifest.xmlit usesrospkgto recursively find rosdep keys.rospkgdoesn't evaluate conditional dependencies, so all rosdep keys get returned even if packages use apackage.xml. This PR works around that by usingrospkgfor the first level of rosdeps, and then recursively using the logic to choose betweencatkin_pkgandrospkg.