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

Docs for using ament results in broken libraries by default #3192

Closed
tylerjw opened this issue Nov 16, 2022 · 13 comments
Closed

Docs for using ament results in broken libraries by default #3192

tylerjw opened this issue Nov 16, 2022 · 13 comments

Comments

@tylerjw
Copy link
Contributor

tylerjw commented Nov 16, 2022

From the ament docs on this site:
image
Reference: https://docs.ros.org/en/rolling/How-To-Guides/Ament-CMake-Documentation.html#adding-files-and-headers

From the colcon docs:
image
Reference: https://colcon.readthedocs.io/en/released/user/overriding-packages.html#how-to-make-it-easier-for-your-users-to-override

This is not the pit of success; by default, all libraries published into ROS, even those that try to use the ROS CMake tooling correctly by following the docs from docs.ros.org are broken. As someone who both uses libraries published into ROS based on the tooling or ROS and as someone trying to publish working libraries into ROS, this is really frustrating. In my opinion this sort of "broken by default but documented" sort of fix is sad, but burying it in the colon docs is much worse. Who knew the colcon docs is where you go to read about how to write CMake packages for ROS?

@clalancette
Copy link
Contributor

See #2915 , which fixes this.

@tylerjw
Copy link
Contributor Author

tylerjw commented Nov 16, 2022

Ignore me; I'd just like a system that works by default and to stop answering users' questions with "yes, I know, overrides still don't work in ROS2".

@clalancette
Copy link
Contributor

We're mostly there; once ecosystem packages switch to the new include paths, overrides should always work.

@tylerjw
Copy link
Contributor Author

tylerjw commented Nov 16, 2022

Requiring everyone to edit their cmake files seems like such a sad solution to this. Also, why is this documented in the colon docs. What does colcon have to do with how users write cmake?

@clalancette
Copy link
Contributor

Requiring everyone to edit their cmake files seems like such a sad solution to this.

See ros2/ros2#1150 for the other solutions that we rejected. It is a difficult problem to solve.

What does colcon have to do with how users write cmake?

It really has nothing at all to do with CMake. That's just the most common thing we use in ROS. You would have the same problem with any other build system. Since the problem is not specific to ROS, nor even to CMake, we thought it made more sense in the colcon docs. That said, the ROS 2 docs should also be updated with best practice, as in #2915.

@tylerjw
Copy link
Contributor Author

tylerjw commented Nov 16, 2022

The last time I read that thread; I had the misunderstanding that it was decided to just not fix this problem and that overriding was never going to be supported.

Today I discovered those docs in colcon docs while looking for something else.

I've been told many times that "colcon is not a build system, it is a meta-build system". It only invokes cmake, it has nothing to do with what you write in your cmake. I was confused about why it would have docs like this.

I would have assumed docs.ros.org was the right place to document this in the first place as that is where the docs for using ament_cmake are which does have to do with how users write cmake for publishing in the ROS ecosystem.

@tylerjw
Copy link
Contributor Author

tylerjw commented Nov 16, 2022

While the bug has nothing to do with CMake itself and instead with the results of a CMake install... the implication is that all library authors have to change how they use CMake for this to work.

@tylerjw
Copy link
Contributor Author

tylerjw commented Nov 16, 2022

Since the problem is not specific to ROS, nor even to CMake, we thought it made more sense in the colcon docs.

The implication here is that the primary users of colcon with CMake are not also users of ROS? Is that true?

@clalancette
Copy link
Contributor

The implication here is that the primary users of colcon with CMake are not also users of ROS? Is that true?

No, I didn't say that. The primary users of colcon are ROS 2 users with CMake.

However, there are other users, either ROS users with other build systems, or non-ROS users completely (Gazebo, for instance, also uses colcon heavily).

And any of those, if they use overlays, would run into similar issues. Since colcon is the system primarily concerned with overlays, that was the sensible place to put it.

That's not to say we shouldn't update the ROS 2 docs with the relevant information; we should. We even have a pull request open to do that. What is your point here?

@tylerjw
Copy link
Contributor Author

tylerjw commented Nov 16, 2022

What is your point here?

Just that it was a surprising place to find the docs. I see it is being updated so my complaint about it not being updated here is out of place.

I recognize that some problems are just hard, but this seems like a pit of failure where to correctly publish a library into ROS for users to use, one would have to know to read the colcon docs to find out how to fix their CMake.

Closing this issue as the issue with these docs is being fixed.

My other complaint is more of just me being sad that as there is only one way to do this correctly the tools (ament, colcon, buildfarm) couldn't have solved the problem instead of requiring changes to all the CMake files. This frustration has nothing to do with the docs, so it is unrelated and off-topic here.

@tylerjw tylerjw closed this as completed Nov 16, 2022
@clalancette
Copy link
Contributor

My other complaint is more of just me being sad that as there is only one way to do this correctly the tools (ament, colcon, buildfarm) couldn't have solved the problem instead of requiring changes to all the CMake files. This frustration has nothing to do with the docs, so it is unrelated and off-topic here.

One of the things that we've been trying to do is to use less ament-specific stuff, and more modern CMake stuff. So that led to the current implementation where the destination of these headers is kind of "open-coded".

Maybe that was the wrong call in this case, and we should have added ament_install_headers or something. I'll note that this would still require us to change every package to use it, but at least it would be familiar and we could hide the details there.

If you think that is a good idea, do feel free to open up an issue over at https://github.com/ament/ament_cmake, and we can consider it.

@tylerjw
Copy link
Contributor Author

tylerjw commented Nov 16, 2022

I do appreciate wanting to use standard CMake, and maybe my complaint is best directed at whoever is deciding that merged workspaces is the correct way to install packages we link against.

Has a not-merged install for the binary releases been considered? I know this goes against the default in Debian, so maybe that makes it a non-starter.

An ament_install_header solution might allow us to warn users who do not call it inside ament_package, which would make the transition to correct packaging less opt-in and more likely to be fixed across the ecosystem. I don't like that the current solution is both opt-in for it to work and that correct cmake is confusing without knowing all the internal details of cmake.

From a user's perspective, knowing they need to install their headers is enough. However, the fact that the correct cmake is still really hard to write correctly is not a problem of ROS, except that ROS chose CMake. Have we considered entirely different build systems?

@clalancette
Copy link
Contributor

Has a not-merged install for the binary releases been considered? I know this goes against the default in Debian, so maybe that makes it a non-starter.

We can discuss it, but it is an orthogonal question. Merged workspaces exist, people use them, and they are required in some cases (like on Windows). So we need to deal with overlays with merged workspaces regardless.

From a user's perspective, knowing they need to install their headers is enough. However, the fact that the correct cmake is still really hard to write correctly is not a problem of ROS, except that ROS chose CMake. Have we considered entirely different build systems?

colcon is entirely pluggable, so you should be able to implement a plugin for whatever build system you want. There's even colcon-meson and colcon-bazel already. The hard part here is integrating with the existing ecosystem, so that would mean somehow parsing the CMake for downstream packages that are depended on, and also interacting with ament (the index, the linters, and the equivalent to ament_cmake). If you are interested in such an undertaking, that would be the route I would suggest; implement it as a parallel solution to CMake/ament_cmake, and convince enough of the community to switch.

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

No branches or pull requests

2 participants