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

add CMake option for symlink install #929

Merged
merged 2 commits into from
May 1, 2018
Merged

Conversation

dirk-thomas
Copy link
Member

In the discussion about the unified build tool the question came up if the devel space should be continued to be supported. In ROS 2 it has intentionally been removed due to the extra complexity and custom logic which have to be put into every package to support that custom concept. Instead ROS 2 offers a "symlink install" option which replaces most CMake install() calls with symlinks on platforms which support that. In the case we want to do the same in the future in ROS 1 I ported the symlink install logic from ament_cmake_core to catkin.

@NikolausDemmel
Copy link
Contributor

Interesting.

Assuming that the usual development-workspace workflow for catkin currently is to only work with the devel space, this change would allow to switch to symlink-install for development and use the install space instead, right? The devel space would probably still be there (e.g. for generated files, right?), but not directly used.

Are there any downsides of the symlink-install vs the devel workflow for development as far as you can tell?

@dirk-thomas
Copy link
Member Author

Assuming that the usual development-workspace workflow for catkin currently is to only work with the devel space, this change would allow to switch to symlink-install for development and use the install space instead, right?

Yes, the idea is that users would always use the install option and optional use symlink install to have the same benefit as the devel space.

The devel space would probably still be there (e.g. for generated files, right?), but not directly used.

Yes, if you don't specify an explicit location the devel directory is actually created as a subdirectory of build so it wouldn't be "directly visible".

Are there any downsides of the symlink-install vs the devel workflow for development as far as you can tell?

None, as far as I know. But the advantage is that the CMake code in ROS packages wouldn't need to handle the devel case separately. E.g. no need for a separate CFG_EXTRAS for *.develspace.*.

@NikolausDemmel
Copy link
Contributor

Cool, so there could be a long term goal of phasing out the devel space. But it would have to be really long term to not be disruptive. From my experience people working with non-released packages often don't bother to define install rules if they only work with the devel space anyway. If phasing out the devel space in ROS 1 is even desirable, one would need to make a convincing argument why it is necessary.

In any case, it would be great to get support for this in catkin_tools as well once it is merged here.

Copy link
Member

@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.

👍, I'm very much in favor of back porting this feature and recommending that people use install rules in all packages and then prefer install space with symlink install over the devel space when developing their code.

I share the concern of @NikolausDemmel, which is (hopefully I paraphrase correctly) that it let's us plan for a tick-tock style removal of the devel space in ROS 1, but it does not make it obvious that we should remove the devel space. We need to discuss that with the community in addition to having this feature before we decide. But I think that's what @dirk-thomas was saying too.

However, if we add the option and advertise it, most everyone might switch over to the install space only on the their own accord, and at that point it would be easier to justify going ahead and removing it. So the early the better on this feature, imo.

@NikolausDemmel
Copy link
Contributor

I share the concern of @NikolausDemmel, which is (hopefully I paraphrase correctly) that it let's us plan for a tick-tock style removal of the devel space in ROS 1, but it does not make it obvious that we should remove the devel space. We need to discuss that with the community in addition to having this feature before we decide.

This is exactly what I mean, just in much clearer words 👍.

@dirk-thomas dirk-thomas merged commit e1683e9 into kinetic-devel May 1, 2018
@dirk-thomas dirk-thomas deleted the symlink_install branch May 1, 2018 18:20
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