Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix conditional dependencies when one package uses manifest.xml #737

Merged
merged 2 commits into from
Apr 3, 2020

Conversation

sloretz
Copy link
Contributor

@sloretz sloretz commented Jan 4, 2020

Fixes #736

When rosdep finds a manifest.xml it uses rospkg to recursively find rosdep keys. rospkg doesn't evaluate conditional dependencies, so all rosdep keys get returned even if packages use a package.xml. This PR works around that by using rospkg for the first level of rosdeps, and then recursively using the logic to choose between catkin_pkg and rospkg.

@sloretz sloretz self-assigned this Jan 4, 2020
@sloretz
Copy link
Contributor Author

sloretz commented Jan 4, 2020

I didn't look too closely at the rospkg code. Maybe an alternative fix is to make rospkg evaluate conditions here?

@sloretz
Copy link
Contributor Author

sloretz commented Mar 10, 2020

@wjwwood

Copy link
Contributor

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@wjwwood
Copy link
Contributor

wjwwood commented Mar 10, 2020

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.

@nuclearsandwich
Copy link
Contributor

I didn't look too closely at the rospkg code. Maybe an alternative fix is to make rospkg evaluate conditions here?

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?

Copy link
Contributor

@nuclearsandwich nuclearsandwich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will have to do for now. I can't find a good place for this logic to live in rospkg.

@nuclearsandwich nuclearsandwich force-pushed the manifest_to_packagexml_conditions branch from 9103a1a to d58a526 Compare April 2, 2020 19:29
@nuclearsandwich
Copy link
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.

@sloretz
Copy link
Contributor Author

sloretz commented Apr 2, 2020

@nuclearsandwich 0e28606 LGTM

@nuclearsandwich nuclearsandwich merged commit c53c951 into master Apr 3, 2020
@nuclearsandwich nuclearsandwich deleted the manifest_to_packagexml_conditions branch April 3, 2020 13:24
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 <[email protected]>

* [update] Allow to process single ROS distro, fix 723 (ros-infrastructure#738)

* [update] skip other distro if --rosdistro passed

Signed-off-by: Mikael Arguedas <[email protected]>

* ignore argument if specified distro doesnt exist

* address review comments

* update help message for rosdistro

Signed-off-by: Mikael Arguedas <[email protected]>

* 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 <[email protected]>

* 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 <[email protected]>

Co-authored-by: Scott K Logan <[email protected]>

* 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 <[email protected]>

* 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 <[email protected]>

* Return rosdeps as a list to avoid breaking the interface.

Co-authored-by: Steven! Ragnarök <[email protected]>

* Guard next(inter) (ros-infrastructure#701)

* guard next(inter)

  Fix ros-infrastructure#691

Signed-off-by: artivis <[email protected]>

* Handle StopIteration with slightly less line noise.

Co-authored-by: Steven! Ragnarök <[email protected]>

* [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 <[email protected]>

* 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 <[email protected]>

* 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 <[email protected]>

Co-authored-by: Shane Loretz <[email protected]>
Co-authored-by: Mikael Arguedas <[email protected]>
Co-authored-by: Mark Hedley Jones <[email protected]>
Co-authored-by: Scott K Logan <[email protected]>
Co-authored-by: Kyle Fazzari <[email protected]>
Co-authored-by: Bjar Ne <[email protected]>
Co-authored-by: Jeremie Deray <[email protected]>
Co-authored-by: Sean Yen <[email protected]>
Co-authored-by: Lou Amadio <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rosdep install python3 dependencies when a package has manifest.xml and package.xml
3 participants