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

Install environment hooks into a package-specific directory #953

Merged
merged 6 commits into from
Sep 6, 2018

Conversation

paulbovbel
Copy link
Contributor

@paulbovbel paulbovbel commented Aug 20, 2018

Required for colcon/colcon-ros#36, related to colcon/colcon-ros#35

@dirk-thomas
Copy link
Member

I am not convinced this is the right way to address the problem. With the "slow" release cycle of catkin (it is released fairly infrequent due to the effort and impact) I would rather like to make sure that the approach is viable in the long term and in the various ways these scripts could be used.

  • In the case of the setup files from catkin (global per ws) having all scripts in a single directory is good in order to find / source them efficiently. Hence this is the current state - optimized for that use case.

  • In the case of the setup files colcon (per package) the scripts for a single package need to be identified. The need to read files and interpret their content in order to decide which other files to source seems to be sub optimal. If other tools want to reproduce the behavior they have to jump through the same hoops. For this use case it would be preferred if the scripts from a specific package would be installed in a package specific location (e.g. share/<pkgname>/...).

Both use cases should be supported efficiently. One possible approach would be to install these scripts in both locations - each optimized for one of the use cases. But I can't foresee yet if that will have other downsides down the road. Therefore I think this should be reconsidered with the bigger picture of general performance when sourcing setup file in mind.

@paulbovbel
Copy link
Contributor Author

paulbovbel commented Aug 25, 2018

The need to read files and interpret their content in order to decide which other files to source seems to be sub optimal.

It makes sense to me that there should be some sort of record of which package installed what into a global workspace path, but I'm all for a more optimal solution.

One possible approach would be to install these scripts in both locations - each optimized for one of the use cases

This doesn't feel particularly more optimal that a manifest, but it is certainly simpler/more obvious. Perhaps the CATKIN_INSTALL_INTO_PREFIX_ROOT parameter should control whether the hooks are even installed into <install_base>/etc/catkin?

Therefore I think this should be reconsidered with the bigger picture of general performance when sourcing setup file in mind.

I'd be happy to collaborate on/contribute a solution that would be acceptable, but I don't have a good understanding of the bigger picture, and would appreciate some guidance. Would it make sense to open a separate ticket in colcon-ros for the issue of overall performance? At the very least I can start by contributing some metrics from sourcing large workspaces built by catkin-tools vs colcon.

@paulbovbel
Copy link
Contributor Author

Adapted to install hooks into share/<pkgname>/profile.d

@paulbovbel paulbovbel changed the title Generate a manifest of env hooks for use by other build tools Install environment hooks into a package-specific directory Aug 28, 2018
@dirk-thomas
Copy link
Member

@ros/ros_team Please comment if you think is a good approach (or not).

@wjwwood
Copy link
Member

wjwwood commented Sep 5, 2018

Duplicating the files is not ideal, but I don't see a practical problem with it. It might be confusing to people who are trying to reverse engineer what catkin is doing, so more documentation might be in order, e.g. comments in the code where it is installed in two places, a marker file in the install space explaining what's going on, or documentation in the catkin documentation somewhere.

@dirk-thomas
Copy link
Member

more documentation might be in order, e.g. comments in the code where it is installed in two places, a marker file in the install space explaining what's going on, or documentation in the catkin documentation somewhere.

84c6c95 adds information about the two install destinations to the docblock which will also appear in the generated docs.

@wjwwood
Copy link
Member

wjwwood commented Sep 5, 2018

Looks ok to me.

@dirk-thomas
Copy link
Member

Thank you for the patch.

@dirk-thomas dirk-thomas merged commit 7b1e927 into ros:kinetic-devel Sep 6, 2018
@dirk-thomas
Copy link
Member

The CMake API docs have been regenerated in e086d89.

@paulbovbel
Copy link
Contributor Author

Thanks for chasing this @dirk-thomas!

@dirk-thomas
Copy link
Member

b1622e2 is a follow up to address warnings in the doc job.

@paulbovbel
Copy link
Contributor Author

paulbovbel commented Dec 3, 2018

@dirk-thomas, can I please request a tagged release of this package which includes this PR?

@dirk-thomas
Copy link
Member

It will likely be January until I get to do ROS 1 patch releases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants