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

Always install package.xml file #78

Merged
merged 3 commits into from
Jun 22, 2021
Merged

Always install package.xml file #78

merged 3 commits into from
Jun 22, 2021

Conversation

fmauch
Copy link
Collaborator

@fmauch fmauch commented Jun 21, 2021

As this package's build type is plain cmake, catkin is not necessarily
available at build time. Hence, checking for catkin before installing the
package.xml doesn't make any sense as this will never work for the buildfarm builds.

Installig the package.xml file in every case won't hurt in non-ROS environments.

When the package.xml file is not installed, toolings like rospack will not be
able to find the package.

@puck-fzi @urmahp opinions on this?

As this package's build type is plain cmake, catkin is not necessarily
available at build time. Hence, checking for catkin before installing the
package.xml doesn't make any sense as this will never work for the buildfarm builds.

Installig the package.xml file in every case won't hurt in non-ROS environments.

When the package.xml file is not installed, toolings like rospack will not be
able to find the package.
@puck-fzi
Copy link
Contributor

LGTM.

I don't see any problems with always installing.
The check for catkin or ament_cmake seemed more like a workaround to see if the system already supports ROS or ROS2. But not necessarily if the lib should be used in that context anyway

@fmauch
Copy link
Collaborator Author

fmauch commented Jun 21, 2021

Thanks for looking at this @gavanderhoorn, @puck-fzi

I just don't quite understand why the noetic builds fail with this change, while they succeed with the master branch atm.

@gavanderhoorn
Copy link
Contributor

What about registering the package in the ROS 2 ament/resource index?

@fmauch
Copy link
Collaborator Author

fmauch commented Jun 21, 2021

What about registering the package in the ROS 2 ament/resource index?

From what I understand the docs this would be an alternative to installing the resource folder to the package's share location as we currently do.

However, I assume that we should still install the package.xml file, so this won't be a true alternative to this PR, right?

Also, I'd like to keep this repository as independent from ROS/ROS2 as possible (also with as little CMake conditionals as possible.)

Also, we need a solution working for ROS1.

P.S.: I am myself not very familiar to ROS2 (yet), so please correct me if I am saying something stupid about ROS2 :-)

@gavanderhoorn
Copy link
Contributor

gavanderhoorn commented Jun 21, 2021

From what I understand the docs this would be an alternative to installing the resource folder to the package's share location as we currently do.

I'm not sure I understand. The ament index is basically a registry containing information on which locations should be considered when looking for certain resources. This was introduced to reduce the amount of fs crawling which is the default in ROS 1 (by rospkg fi).

For packages to be 'findable' by ROS 2 tooling, registration in the index is required.

The rationale for doing that with the package in this repository would seem to be similar to what you're doing in this PR (ie: to be able to offer resources which can be located relative to the package, instead of by absolute paths).

However, I assume that we should still install the package.xml file, so this won't be a true alternative to this PR, right?

it's not an alternative to anything :)

It's similar in purpose (see above).

If the client library is purely a CMake project and you don't want to offer resources which should be locatable relative to a "ROS package" (whether ROS 1 or ROS 2), then there's no point in registering. But seeing as you're going to install your manifest, I believe you do want to do that.

Note that "registration" basically comes down to placing a marker file in a certain location (like this).

@fmauch
Copy link
Collaborator Author

fmauch commented Jun 21, 2021

Note that "registration" basically comes down to placing a marker file in a certain location (like this).

Thanks for the elaboration.

@fmauch
Copy link
Collaborator Author

fmauch commented Jun 21, 2021

After spending some time debugging this, the problem seems that as soon as find_package(catkin QUIET) is removed from the CMakeLists.txt file rosdep in the downstream workspace seems to not recognize this package anymore (if being built with colcon) and instead installs the released binary apt package. Then, there seem to be shadowing issues with the installed binary package...

When changing the builder to catkin_tools the released version doesn't get installed and everything works as expected.

I am unsure how to proceed at this point...

@gavanderhoorn
Copy link
Contributor

gavanderhoorn commented Jun 21, 2021

After spending some time debugging this, the problem seems that as soon as find_package(catkin QUIET) is removed from the CMakeLists.txt file rosdep in the downstream workspace seems to not recognize this package anymore (if being built with colcon) and instead installs the released binary apt package. Then, there seem to be shadowing issues with the installed binary package...

on systems with ROS 2, that's because the package is not registered with the ament index, which is what rosdep uses there.

Well, it's slightly more complicated then that. The whole rosdep with the resource index etc is "not nice" right now.

See also the lines in the CMake boilerplate I linked earlier.

@fmauch
Copy link
Collaborator Author

fmauch commented Jun 22, 2021

To bring this forward, I will set the builder for noetic to catkin tools, as this error seems to only occur when using an underlay workspace that has been built using colcon.

Building against the released package or against the package being in the same workspace or using an underlay workspace built with catkin tools seems to work.

@fmauch fmauch merged commit c23ddfd into master Jun 22, 2021
@fmauch fmauch deleted the install_package_xml branch June 22, 2021 12:53
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.

None yet

3 participants