Skip to content

Wjwwood/build tool colcon#169

Merged
wjwwood merged 7 commits intobuild_tool_colconfrom
wjwwood/build_tool_colcon
Apr 24, 2018
Merged

Wjwwood/build tool colcon#169
wjwwood merged 7 commits intobuild_tool_colconfrom
wjwwood/build_tool_colcon

Conversation

@wjwwood
Copy link
Copy Markdown
Member

@wjwwood wjwwood commented Apr 18, 2018

Request and recommended changes.

Based on the above information a decision has been made to pick `colcon` as the universal build tool which was not an easy one.
Based on the above information a decision has been made to pick `colcon` as the universal build tool.

The decision was made after considering the input of ROS 2 team members and some ROS 1 users.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happened to consulting ROS 2 users?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And "some ROS1 users" is terribly unspecific. ROS1 has been working by REPs, and OSRF religiously decreed "ROS1 will not be disrupted". So now you want to disrupt ROS1 without a REP?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happened to consulting ROS 2 users?

I am not sure what kind of feedback we would expect from this. colcon basically works the same way as ament_tools. Some of the command line options are named slightly different. If that is a major concern it would be easy to provide an executable named ament which provides the exact same CLI (I have a prototype of that locally from when I was exploring such options). But since the difference is really small (see http://colcon.readthedocs.io/en/latest/migration/ament_tools.html) I am not sure if that would even be valuable or more confusing.

Maybe you can clarify what other kind of feedback you would expect?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happened to consulting ROS 2 users?

I think feedback on this design document update is the solicitation for feedback from ROS 2 users and others. Though I don't know if it would impact the decision that's been made one way or the other.

And "some ROS1 users" is terribly unspecific.

That was intentional, because I didn't take time to ask if I could mention them directly, also I didn't feel it was material to the point.

So now you want to disrupt ROS1 without a REP?

Well, there isn't a REP to update or replace w.r.t. the build tool in ROS. It's more of a recommendation than a requirement, and not the same in my mind as the switch from rosbuild to catkin.

Other things which would actually be disruptive (require changes in existing code) would require REP's, like getting rid of the devel space, and deciding on whether or not to pursue those or not is apparently also blocking roll out of the universal build tool in ROS 1, so I don't it will be disruption without REP's personally, but I suppose we'll have to see.

To be fair, had we decided to use catkin_tools, roll out in ROS 1 would have just been fix a few final things and change documentation to recommend catkin_tools instead of catkin_make, but there would not have been a need for a REP in that case, in my opinion.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there is much feedback to provide from the point of view of a ROS 2 user. I'm actually in favour of the change in tool, especially because it is so non-disruptive. But I'm disappointed that I only get to provide that opinion after the decision has been made. I find it annoying that the OSRF assumes that existing ROS 2 users will have no valid or useful input into a high level decision such as this. We went from a state we were in a year ago to suddenly having a new tool and a decision made to use it without any notice.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I agree that community feedback before a decision would be nice, but as a member of the team I also understand the desire to get things moving. I’m certain your feedback is considered but it would have to be a significant amount of negative feedback to change the direction. And that was the case before the decision was made as well.

Based on the above information a decision has been made to pick `colcon` as the universal build tool.

The decision was made after considering the input of ROS 2 team members and some ROS 1 users.
The decision was not easy, as it was not unanimous, but the vast majority of input was either pro `colcon` or ambivalent.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me. 👍

In ROS 2 the concept of the *devel space* has intentionally been removed.
In the future it might be feasible to provide the concept of *symlinked installs* in ROS 1 to provide a similar benefit without the downsides.
Therefore not supporting this concept in the universal build tool is considered a viable path forward.
However, this design document will assume that the *devel space* will remain in ROS 1.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't this this article should that claim either. Maybe it would be better to just state the fact that ROS 1 currently supports the devel space and the universal build tool doesn't. But exclude any future decision and defer that to be dealt with on the ROS 1 side (maybe in a REP).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It wasn't my intention to claim either. I was just taking the "status quo" as the most likely future. I think that's the best assumption to take given the circumstances. Note an assumption is not an assertion or claim. Just what we have to assume until it changes.

I'm not sure how I would change this. Would you like me to remove it, or modify it some specific way?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I read the assumption slightly different. Like "we have thought about it and for now think it will stay that way". I think it should more like "at this point we are not making a decision how to move forward with this in ROS 1".

Maybe:

The direction forward in ROS 1 - if the new build tool needs to support the devel space or if the concept of the devel space is deprecated at some point (maybe even implicitly if the new build tool doesn't support it) - has not been decided yet and is out of scope of this article.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like "we have thought about it and for now think it will stay that way". I think it should more like "at this point we are not making a decision how to move forward with this in ROS 1".

Those are the same thing... basically "at this point we are not making a decision how to move forward with this in ROS 1, therefore it will stay that way until we do make a decision".

I'm just removing the sentence. The previous lines describe the situation sufficiently.

- In the future `colcon` will replace `catkin_make_isolated` and `catkin_make` as the recommended build tool for ROS 1.
- `colcon` will not support the *devel space* and will require packages to have install rules
- `catkin` will likely still support the *devel space*, though it might be removed at some point (that has not been decided yet)
- Therefore, it is possible that the default build tool for ROS 1 may not support the *devel space*, though legacy tools will continue to support it.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to mention that explicitly we should probably also clarify that a similar problem already exists. Some packages do only work in the devel space and some do only work in the install space (since these packages haven't been developed with both in mind and the developers / maintainers / users only use one of the two).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But currently that's not driven by the build tool (the build tool allows you to do either), so that's why I didn't include it. But I guess I can add a note about that.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wjwwood wjwwood added in review Waiting for review (Kanban column) in progress Actively being worked on (Kanban column) and removed in review Waiting for review (Kanban column) labels Apr 19, 2018
@wjwwood wjwwood added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Apr 24, 2018
@wjwwood
Copy link
Copy Markdown
Member Author

wjwwood commented Apr 24, 2018

I didn’t mean to let this linger, I guess I’ll merge since the approval.

@wjwwood wjwwood merged commit 8a233bc into build_tool_colcon Apr 24, 2018
@wjwwood wjwwood deleted the wjwwood/build_tool_colcon branch April 24, 2018 21:35
@tfoote tfoote removed the in review Waiting for review (Kanban column) label Apr 24, 2018
dirk-thomas added a commit that referenced this pull request Jun 15, 2018
…#168)

* update universal build tool article to reflect decision to use colcon

* fix spelling / missing word

* add next steps, implications, outlook

* fix spelling

* rephrase

* wording

* spelling

* Wjwwood/build tool colcon (#169)

* typo

* clarify how the decision was made

* add a few more items to the list of things needed to complete the colcon option

* clarify future plans for ROS 1

* typo

* note existing case for catkin packages and devel space versus install rules

* remove line about devel space

* remove cross compilation

* minor tweaks

* add progress section to link to relevant PRs

* fixups

* update archival time for ament_tools

* mention inferring metadata

* keep abstract sentence

* punctuation for readability
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.

5 participants