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

Layered diagrams #121

Closed
wants to merge 2 commits into from
Closed

Layered diagrams #121

wants to merge 2 commits into from

Conversation

tkruse
Copy link

@tkruse tkruse commented Mar 18, 2017

In the spirit of a picture saying more than a thousand words, this table art might help.

Quick link to rendered result:
https://github.com/tkruse/design/blob/build_tool2/articles/101_build_tool.md

Because there is confusion about homogenous workspaces as a goal, I split up goal into 2 phases. Labelling could of course be different, such as humble/ambitious, or primary/secondary, ... I am dispassionate about that labelling.

@tkruse tkruse changed the base branch from gh-pages to build_tool March 18, 2017 03:44
| Workspace | ROS2 ament workspace | ROS1 catkin workspace | ROS1 rosbuild workspace |
| Build tool | ament_tools | catkin_tools, catkin_make, cmi | rosbuild |
| Build system | ament_cmake, cmake, python | catkin, cmake | rosbuild |
| Package | ROS2 package, cmake/python project | ROS1 package, cmake project | ros1 rosbuild package |
Copy link
Member

Choose a reason for hiding this comment

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

The visual presentation is certainly helpful. But I think the information is presented too early in the document. Especially since at that point the terms of build tools and build systems haven't been clearly defined. And because the table mixes the terminology and doesn't distinguish between those which imo confused the reader.

Copy link
Author

Choose a reason for hiding this comment

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

Feel free to move to another section or suggest one


(Note catkin_make cannot build pure cmake projects)

### Phase 1 goal: unify build systems (not rosbuild)
Copy link
Member

Choose a reason for hiding this comment

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

If someone would like to implement an extension to the unified build tool to support rosbuild that would be absolutely feasible. There this shouldn't be excluded like this.

Copy link
Author

Choose a reason for hiding this comment

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

would "rosbuild not shown" be ok for you?

4. Create DAG of packages for building the workspace
5. Invoke buildsystem plugin command for each package in build order

### Phase 2 goal: unify workspaces
Copy link
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 this article to outline such a phase. The current draft briefly mentions the intrinsic complexity of mixing workspaces like that which are beyond the build tools control: see https://github.com/ros2/design/pull/115/files#diff-361c5cb5b98ec5e7f690f2dbce2cd7fdR186 Also even if all build tools and build systems do the technical part of mixing them well there are other aspects like name spacing of ROS 1 and ROS 2 packages, their rosdep keys, etc. Therefore I think this is exceeding the scope of this article.

Copy link
Author

Choose a reason for hiding this comment

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

In the comments of the main PR @wjwwood seems to argue for such a goal #115 (comment), that's why I added it.

dirk-thomas added a commit that referenced this pull request Mar 23, 2017
@dirk-thomas
Copy link
Member

I added some information from this in 7ae99af Please comment / open another PR for additional changes.

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.

2 participants