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

also install buildtool_export_depends #753

Conversation

mikaelarguedas
Copy link
Contributor

not adding 'exec_depends' or 'build_export_depends' are both are included inside 'run_depends'

Fixes #752

Signed-off-by: Mikael Arguedas mikael.arguedas@gmail.com

not adding 'exec_depends' or 'build_export_depends' are both are included inside 'run_depends'

Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>
@mikaelarguedas
Copy link
Contributor Author

@nuclearsandwich can this be a candidate for the release underway? This is a rather serious bug currently preventing building ROS 2 workspaces on systems that don't already have python libraries installed. I opened this PR seeing that no release had been made for a while with the hope that this could be integrated before cutting a new release.

@nuclearsandwich
Copy link
Contributor

@nuclearsandwich can this be a candidate for the release underway?

This release is already stacked. Since most of the effort is PR review and QA I'd just as well prefer to cut a follow-up 0.19.1 once the PR has been merged.

@nuclearsandwich
Copy link
Contributor

The code change is easy enough to review and there's no problem there.

There was quite some discussion about rosdep handling doc_depends and which dependencies should be installed by default. I think the case for buildtool_export_depends being a default is in keeping with our other defaults, especially as the parallel fix in rosinstall_generator has been merged for some time. But I also want to call attention to #727 which I'd like to get wide feedback on, especially from folks like you @mikaelarguedas. Any change to the defaults will upset someone's apple cart and I think it would be good to have something in the pipeline so that different users can pick from available dependency types.

@mikaelarguedas
Copy link
Contributor Author

mikaelarguedas commented Apr 6, 2020

For sure I'm strongly in favor of #727 (or any other way to specify which dependency types to resolve) since it was brought to my attention at https://discourse.ros.org/t/generating-dev-and-runtime-artefacts-from-ros-packages/12448 👍 I didn't get a chance to test it though.

This release is already stacked. Since most of the effort is PR review and QA I'd just as well prefer to cut a follow-up 0.19.1 once the PR has been merged.

Do you think this has a chance to happen today ? or is it better to roll-back and release changes that exposed this problem in the meantime ?

@dirk-thomas
Copy link
Member

Related to ros-infrastructure/rospkg#191.

@nuclearsandwich
Copy link
Contributor

Do you think this has a chance to happen today ? or is it better to roll-back and release changes that exposed this problem in the meantime ?

Based on the question raised in ros-infrastructure/rospkg#191 (comment) this doesn't appear to be an open-and-shut case. I've put it on the discussion agenda for the ROS team tomorrow. The last comment in the other thread was asking the contributor to do a deeper assessment of the impact of the proposed change. I'm not sure that a decision will be made but I'm hoping we'll at least determine what info is needed to make an informed decision.

@mikaelarguedas
Copy link
Contributor Author

I'm not sure that a decision will be made but I'm hoping we'll at least determine what info is needed to make an informed decision.

Sounds good

The last comment in the other thread was asking the contributor to do a deeper assessment of the impact of the proposed change. I'm not sure that a decision will be made but I'm hoping we'll at least determine what info is needed to make an informed decision.

From my perspective, it makes sense to treat buildtool_depends and buildtool_export_depends the same way as they're both for the build platform and necessary for the build tool to successfully build a workspace.
Regarding if they should both be installed by default, I couldn't say. If the default/main use of rosdep is to get all dependences to build workspaces from source then most likely yes. If it's meant to be used on a target system to install runtime dependencies, then most likely no. But in that case a way to request all the deps needed for building workspaces should be provided.

@nuclearsandwich
Copy link
Contributor

From my perspective, it makes sense to treat buildtool_depends and buildtool_export_depends the same way as they're both for the build platform and necessary for the build tool to successfully build a workspace

This was the convincing argument for me. In today's discussion we agreed that rosdep should install buildtool_export_depends by default along with buildtool_depends.

Using rosdep with distinct host and target platforms is not a use case that rosdep currently supports but a PR like #727 could enable it in the future.

@nuclearsandwich nuclearsandwich merged commit c721d6a into ros-infrastructure:master Apr 7, 2020
@mikaelarguedas mikaelarguedas deleted the install_export_depends branch April 7, 2020 19:53
@mikaelarguedas
Copy link
Contributor Author

@nuclearsandwich Do you know when we can expect the follow-up release of rosdep including this fix ? 🙏

@ros-discourse
Copy link

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/announcement-trimmed-ros-gazebo-docker-images-released/14342/1

@mikaelarguedas
Copy link
Contributor Author

@nuclearsandwich friendly ping, is there a timeframe available for getting this fix released ?

@nuclearsandwich
Copy link
Contributor

Short answer no. I won't be able to get to a rosdep release this week or possibly next. But I'll try for the week after.

@mikaelarguedas
Copy link
Contributor Author

@nuclearsandwich friendly ping. This seem to still cause issues for people attenmpting to compile ROS2 from source using rosdep to install their workspace dependencies.

Is there a chance of getting this released soon ?

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>
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 doesnt install buildtool_export_depend dependencies
4 participants