text edits by tkruse · Pull Request #119 · ros2/design

tkruse · 2017-03-12T12:31:21Z

Mostly suggestions to keep focus on change, and add more context. None of it is essential, I was writing while reading.

I added package-managers for larger audiences.

I started structuring of essential and non-essential features.

Feel free to cherry pick what you like and close PR without merge without further discussion.

Some comments independent from my PR (but less chat in the main PR)

The goal is not motivated yet. I hint at issues with the current situation, but without explaining those issues, it is difficult to make a decision solving them.
I noticed rosbuild was not mentioned much so far, nor tools relying on the ROS_PACKAGE_PATH. Not sure if that is important for the discussion
while packaging is out of the scope for the tool, I believe the package manifest provides packaging data, might be worth mentioning (Also because of catkin_simple)
it might be helpful to compare the "anatomy" of catkin and ament files, to see the problems in migrating from one to the other
Devel_spaces not mentioned yet
I think catkin_isolated and catkin_auto can also build non-catkin cmake packages, no? The current text suggests otherwise

The realistic alternative approaches should be more detailed.
So far I see:

keeping catkin packages same vs. requiring migration
evolving catkin_tools vs. evolving ament_tools ('from scratch' is equivalent to one of those)

tkruse · 2017-03-13T01:04:43Z

More things I notice missing in goals, maybe because they seem too obvious:

Cross-compilation support
Cross plattform compatibility
cmake support as a constraint

dirk-thomas

The goal is not motivated yet. I hint at issues with the current situation, but without explaining those issues, it is difficult to make a decision solving them.

The goal is to consolidate the effort from the various build tools, avoid duplicate work and create a single tool which covers all the current use cases (and hopefully more).

I noticed rosbuild was not mentioned much so far, nor tools relying on the ROS_PACKAGE_PATH. Not sure if that is important for the discussion

This is a good example why the unified build tool needs to be very modular. The ROS_PACKAGE_PATH is a ROS specific concept and therefore not relevant in non-ROS use cases. Therefore the core part of the unified build tool should not know about this. Only a ROS specific plugin should introduce the concept of that environment variable.

it might be helpful to compare the "anatomy" of catkin and ament files, to see the problems in migrating from one to the other

Again, this is related to the build system and imo out of scope for this build tool article. From the build tools point of view it treats both ROS build system as separate and doesn't care how they operate (and if they are similar or how they are different).

Devel_spaces not mentioned yet

The devel space is a build system concept (only in catkin). The core of the unified build tool therefore doesn't care about it. The catkin specific extension might or might not expose that feature - that is up to the implementation to decide.

keeping catkin packages same vs. requiring migration

The tool only decides how the build systems of multiple packages are being invoked in the right order. There is conceptionally nothing to migrate.

evolving catkin_tools vs. evolving ament_tools ('from scratch' is equivalent to one of those)

Trying to refactoring an existing code base of significant size to a more modular architecture is a big effort. Designing a modular architecture from scratch with the experience and knowledge about the existing systems and then porting over existing features is very different. Therefore I don't agree with your statement. Which one to choose is a different question 😉

Cross-compilation support

This boils down to being in the realm of the build system again. Consider the build tool only a fancy "command line invocation improver". It doesn't do anything for cross-compilation which the build system can't do.

Cross plattform compatibility, cmake support as a constraint

The universal build tool needs to cover all use cases the current build tools already cover, that includes but is not limited to:

Build catkin packages with Python 2 on Linux and OS X
Build ament packages with Python 3 on Linux, OS X and Windows
Support processing packages in parallel
Support packages without a manifest file (e.g. eProsima's FastRTPS)
Provide the usability of catkin_tools with config and profile
many many more

dirk-thomas · 2017-03-13T18:41:10Z

articles/101_build_tool.md

-The "manual" approach to build a set of packages consists of building all packages in their topological order one by one.
-For each package the documentation usually describes what the dependencies are, how to setup the environment to build the package as well as how to setup the environment afterwards to use the package.
-From an efficiency point of view that manual process does not scale well.
+Today, the ROS1 ecosystem has the deprecated rosbuild and the catkin_make build systems for this purpose.


This sentence seem incomplete. Should there be something between "the ROS1 ecosystem has the deprecated rosbuild and" and "the catkin_make build systems for this purpose"?

No, the sentence should be complete. It is equivalent to: The ros1 ecosystem has 2 things for this purpose. 1. The deprecated rosbuild build system. 2. The catkin cmake build system.

dirk-thomas · 2017-03-13T18:45:12Z

articles/101_build_tool.md

-This article describes the steps from the current build tools used in the ROS ecosystem to a single universal build tool which automates the process and works for various different use cases.
+For historical reasons those buildsystems are incompatible with each other, which causes diverse issues in the present.
+
+This article describes the steps from the current build tools used in the ROS ecosystem to a single universal build tool.


I think this comes from a misunderstanding of the difference of build tool and build system. The fact that ROS 1 and ROS 2 use different build systems is described in another article (http://design.ros2.org/articles/ament.html). It provides the rational why the CMake API as well as the feature set is different.

The "univeral" build tool described in this article will no change any of that. The goal of the universal build tool is solely to reduce duplication of efforts across the various existing build tools and aim to even support use cases beyond pure ROS applications.

dirk-thomas · 2017-03-13T18:45:16Z

articles/101_build_tool.md

-For each package the documentation usually describes what the dependencies are, how to setup the environment to build the package as well as how to setup the environment afterwards to use the package.
-From an efficiency point of view that manual process does not scale well.
+Today, the ROS1 ecosystem has the deprecated rosbuild and the catkin_make build systems for this purpose.
+The ROS2 ecosystem has the ament_tools build system.


ament_tools is a build tool - not a build system. Distinguishing these two is crucial for this article to make clear what the scope is.

dirk-thomas · 2017-03-13T18:46:24Z

articles/101_build_tool.md

+* ROS 2 packages
+* other software sources when sufficient meta information is externally provided.
+
+The last point allows to build non-ROS dependencies of ROS packages (e.g. Gazebo including its dependencies).


To clarify: this does not imply that you can or can not mix ROS 1 and ROS 2 packages within the same workspace. Supporting that is only related to the build system.

Clarifying it in the document would be good. I had no idea.

dirk-thomas · 2017-03-13T18:48:04Z

articles/101_build_tool.md

+Examples are Gnu Make, cmake, python setuptools.
+
+A build tool schedules and invokes the build systems for separate source trees (packages) in topological order of dependency.
+Example are rosbuild, catkin_make, ament.


I would suggest using the name ament_tools here to not confuse this with ament_cmake.

dirk-thomas · 2017-03-13T18:51:53Z

articles/101_build_tool.md

-It determines the dependency graph and invokes the package specific build tool of each package in topological order.
-The build tool itself should know as little as possible about the build system used for a specific package.
-Just enough in order to know how to setup the environment for it, invoke the build, and setup the environment to use the built package.
+A build tool defines a manifest format for packages to follow to define the invocation of the respective build tool, and the dependencies to other packages.


I disagree that a build tool must be tighly coupled to a manifest format. A build tool needs the information which can e.g. be provided by a manifest file like the package.xml files in ROS. But that is not the only possible source. It should be possible to obtain the necessary information from any arbitrary source, e.g. an out-of-source configuration file (in whatever format you like), or like rosdep pull the information from a URL.

The tight coupling of the current build tools to the ROS manifest is one big reason why they can't be used in application outside the ROS ecosystem (e.g. Gazebo).

Is it a goal not to depend on any fixed manifest format? Then that would belong in the goals section (or somewhere else in the document, anyway)

removed my change, original text changed changed upstream

dirk-thomas · 2017-03-13T18:54:31Z

articles/101_build_tool.md


-The functionality to setup these environment variables can be provided by either the build tool or the build system.
-In the latter case the build tool only needs to know how the build system exposes the environment setup in order to reuse it.
+The build tool make it easy to extend the environment to run code from installed packages, or to build other packages on top of installed ones.


I think the edit looses an important point of the previous state: either the build tool or the build system can provide significant parts of this environment setup. And it describes the benefit if most of that is performed by the build system rather than the build tool.

So it would be a "nice-to-have" feature to extract the responsibility for the environment from the build-tool? If so it should be kept, could be marked more explicitly as a possible goal.
I removed it because it looked like a technical remark, not something related to a goal.

restored original text

dirk-thomas · 2017-03-13T18:55:36Z

articles/101_build_tool.md


+### rosbuild
+
+Rosbuild was the first build system for ROS, it was based on Makefiles.


rosbuild is both a build system as well as a build tool.

wjwwood · 2017-03-13T20:33:40Z

The devel space is a build system concept (only in catkin). The core of the unified build tool therefore doesn't care about it. The catkin specific extension might or might not expose that feature - that is up to the implementation to decide.

It doesn't have to be in the requirements or goals, but a proposal for what we should do and how that fails to address the devel space concept in catkin would be incomplete in my opinion.

Cross-compilation support
This boils down to being in the realm of the build system again. Consider the build tool only a fancy "command line invocation improver". It doesn't do anything for cross-compilation which the build system can't do.

It still may need to be addressed in some way by the tool. Especially if the tool has to connect two different build systems differently in the context of a cross-compile. You can hand wave it away as "should be in a plugin", but again that can be absent from the goals and requirements, but probably ought to be addressed in a proposal, even if just to say "it should not require anything extra out of the tool because ...".

tkruse · 2017-03-16T14:14:34Z

Regarding all buildsystem-specific things: I believe they should be mentioned in the document exactly to make other people know why the goal is not simply to create a unified build-tool instead. The reason might be obvious to you, but others will not see this easily.

I am not sure if I should try editing according to the comments, or if this was too far off the mark anyway. Please advise. Close this PR without further comments if it served it's purpose in your eyes.

dirk-thomas · 2017-03-16T15:40:24Z

I am not sure if I should try editing according to the comments, or if this was too far off the mark anyway. Please advise

I will leave this up to you. If you would like to update the PR with the goal to get it merged as a whole that is fine by me. Otherwise I can go through the patch and cherry pick the changes which are good as is into the main PR.

tkruse · 2017-03-16T15:42:59Z

I might find time on this weekend.

…* sections closer together

tkruse · 2017-03-18T02:32:43Z

rebased, added commit, squashed one commit for restoring original text

Those always apply, but in some cases they don't, and that list is not exhaustive anyway.

tkruse · 2017-03-18T02:58:25Z

Added more commits, mostly tidying up the requirements section

dirk-thomas

In general please use the same spelling / case for names like "ROS 1", "ROS 2", "CMake", Python, etc.

I am not sure how to proceed with this PR. Every iteration other things are added / changed / moved which makes it very difficult to iterate on this. I am open for suggestions how this should be reviewed and merged (either as a whole or in smaller pieces).

dirk-thomas · 2017-03-20T20:10:34Z

articles/101_build_tool.md

+The rosbuild tool enabled this workflow first and was a critical factor in the success of ROS.

-The "manual" approach to build a set of packages consists of building all packages in their topological order one by one.
-For each package the documentation usually describes what the dependencies are, how to setup the environment to build the package as well as how to setup the environment afterwards to use the package.


This important information what is involved is lost in the modification.

dirk-thomas · 2017-03-20T20:10:59Z

articles/101_build_tool.md

+The ROS1 ecosystem has catkin (and the deprecated rosbuild) for this purpose.
+The ROS2 ecosystem has ament.
+
+Both catkin and ament toolsets have 2 conceptually separate parts, a build tool part that deals with a single package at a time, and a build system part that deals with invoking build tools in the right order.


a build system part that deals with invoking build tools

This seems to be swapped.

dirk-thomas · 2017-03-20T20:16:23Z

articles/101_build_tool.md

 In the ROS ecosystem [bloom](http://wiki.ros.org/bloom) is used to generate the required metadata and then platform dependent tools like `dpkg-buildpackage` build binary packages.

-## Build Tool vs. Build System
+## Build Tool vs. Build System vs. Package manager


Since this article isn't about package managers I don't see why this should be added here. Maybe move it to the "Out of scope" section instead.

dirk-thomas · 2017-03-20T20:17:14Z

articles/101_build_tool.md


-The build system on the other hand operates on a single package.
-In the case a package uses e.g. `CMake` the responsibility of the tool comes down to invoke the common steps `cmake`, `make`, `make install` for this package.
-As another example for a package using `Autotools` the steps could look like `configure`, `make`, `make install`.


The example steps for invoking these build systems is lost in the modification.

dirk-thomas · 2017-03-20T20:18:23Z

articles/101_build_tool.md


 The tool invokes CMake only a single time and uses CMake's `add_subdirectory` function to process all packages in a single context.
+
+The tool uses CMake's `include` function to process all packages in a single cmake context.


This sentence directly contradicts the previous one.

dirk-thomas · 2017-03-20T20:20:03Z

articles/101_build_tool.md

 ### catkin_tools

-[catkin_tools](https://catkin-tools.readthedocs.io/) is provided by a standalone Python package used to build ROS 1 packages.
+[catkin_tools](https://catkin-tools.readthedocs.io/) is provided by a standalone Python package used to build catkin packages.


I think the statement that it builds ROS 1 packages is more accurate since it also supports plain CMake packages.

dirk-thomas · 2017-03-20T20:21:00Z

articles/101_build_tool.md

+Removing such restrictions will be an ongoing process.

-### Software Criteria
+#### Build Environment Setup


This is not an individual feature but a strict requirement in order to build package automatically on-top of each other. Therefore I don't think it should be moved that far down.

tkruse · 2017-03-23T12:57:44Z

Since I suggest large changes, but those are mostly about style, I would be okay with closing this PR and you cherry-pick whatever you like (which might be nothing). This is quicker than talking back and forth.

dirk-thomas · 2017-03-23T19:18:21Z

I tried to integrate some of the changes and feedback from this. Please see a92be6e I will close this PR. Please make further suggestions and contributions in a separate PR.

dirk-thomas reviewed Mar 13, 2017

View reviewed changes

tkruse added 13 commits March 18, 2017 11:04

Suggestion for a better preface

25aa54d

shortening of goals, less means better focus

65ecec1

Reduce definition to one sentence and one example

1cca639

Enlarge Features section

70cd99f

Change wording for *_cmake

ca825d1

consistently name 'build tools' catkin_make and ament_tools

348c94d

Add rosbuild for completeness, reduce catkin text

001aa9f

Move catkin_tools above ament in historical order and to keep catkin-…

fbe6b5a

…* sections closer together

Drop in a short line about cmake for context and consistency

1f51abc

rework introduction

cb21ea9

fix examples

62fd92e

fix rosbuild description

a75a8e1

add cross-compilation support

3c87baf

tkruse added 4 commits March 18, 2017 11:51

repeat non-homogenity of workspaces

8328fc3

Move Extension description inside extension point section

0218287

Remove software principles. They are not helping.

6987981

Those always apply, but in some cases they don't, and that list is not exhaustive anyway.

Moved all individual features into one subsection of requirements

6864a55

dirk-thomas reviewed Mar 20, 2017

View reviewed changes

dirk-thomas added a commit that referenced this pull request Mar 23, 2017

integrate some feedback from #119

a92be6e

dirk-thomas closed this Mar 23, 2017


		### rosbuild

		Rosbuild was the first build system for ROS, it was based on Makefiles.


		The tool invokes CMake only a single time and uses CMake's `add_subdirectory` function to process all packages in a single context.

		The tool uses CMake's `include` function to process all packages in a single cmake context.

Conversation

tkruse commented Mar 12, 2017

Uh oh!

tkruse commented Mar 13, 2017

Uh oh!

dirk-thomas left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wjwwood commented Mar 13, 2017

Uh oh!

tkruse commented Mar 16, 2017

Uh oh!

dirk-thomas commented Mar 16, 2017

Uh oh!

tkruse commented Mar 16, 2017

Uh oh!

tkruse commented Mar 18, 2017

Uh oh!

tkruse commented Mar 18, 2017

Uh oh!

dirk-thomas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tkruse commented Mar 23, 2017

Uh oh!

dirk-thomas commented Mar 23, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

dirk-thomas left a comment •

edited

Loading