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

create build tool article #115

Merged
merged 16 commits into from
Mar 27, 2017
Merged

create build tool article #115

merged 16 commits into from
Mar 27, 2017

Conversation

dirk-thomas
Copy link
Member

This is a draft for an article describing the goal of a "universal" build tool. It describes the current state as well as the rational, goals, and use cases. The proposal section is intentionally left empty for now. It can be filled with the decision we made after some discussion.

It would be great to get feedback on the rational / goals / use cases first so that we are all on the same page before moving forward to deciding which path to take.

(Please feel free to fix spelling / improve wording directly.)

@dirk-thomas dirk-thomas added the in review Waiting for review (Kanban column) label Mar 3, 2017
@dirk-thomas dirk-thomas self-assigned this Mar 3, 2017
@tkruse
Copy link

tkruse commented Mar 4, 2017

  • It would be nice to announce this in a mailing list. I am not sure which one, https://groups.google.com/forum/#!forum/ros-sig-buildsystem or discourse
  • It would be nice to link to past discussions / articles about buildsystems in ROS (I can dig up links if that is desired)
  • Since this is not a REP, it would be nice to explain the decision-making process / planned timeline
  • Overall I missed some mentioning of ROS2 message generation / lower-middleware flexibility and why this could not be done easily with catkin. I assume this is an important force in the decision making, maybe that's wrong.

@dirk-thomas
Copy link
Member Author

dirk-thomas commented Mar 6, 2017

It would be nice to link to past discussions / articles about buildsystems in ROS (I can dig up links if that is desired)

That would be great to add. Since I am not sure which discussions you are referring to can you please either comment with the plain links here and I am happy to add them "somewhere" to the article or make a PR against this branch to make specific suggestions where / how to reference them. Thanks!

Since this is not a REP, it would be nice to explain the decision-making process / planned timeline

The first step is to get feedback on the "facts" and "goal" parts of the current draft. After that we have to describe possible paths forward in more details and gathering the pros and cons of each. Then we have to come up with a proposal. Only with these parts being "finished" I think a time estimate can be made.

It would be nice to announce this in a mailing list. I am not sure which one, https://groups.google.com/forum/#!forum/ros-sig-buildsystem or discourse

I certainly plan to announce this on the mailing list to receive feedback from a wider audience. For now I will wait a little bit until the article contains more information. I think it needs at least some of the missing information described in the previous paragraph to be helpful.

Overall I missed some mentioning of ROS2 message generation / lower-middleware flexibility and why this could not be done easily with catkin. I assume this is an important force in the decision making, maybe that's wrong.

I am not sure if I understand your comment. The article is about the build tool. It is not related to any message generation or middleware at all. As mentioned in the article this tool should even be usable for non-ROS use cases like Gazebo. Can you please clarify your feedback on this.

@tkruse
Copy link

tkruse commented Mar 6, 2017

Regarding ament, I mixed up something, ignore my comment.

@sbarthelemy
Copy link

Hi,

catkin allows users to build multiple packages within a single CMake context (using catkin_make).
While this is an efficient approach since all targets across all packages can be parallelized it comes
with a significant disadvantages.
Due to the single context all function names, targets and tests share a single namespace across all
packages and on a larger scale easily leads to collisions.
[...] ament does not provide that feature.

the "single CMake context" seems especially valuable to me during development of two projects/packages (eg. a lib and a closely related service): CMake will nicely parallelize incremental rebuilds, and also partial rebuilds:

while not fixed:
  vi this_buggy_library.cpp
  make this_service_failing_test && bin/this_service_failing_test

It would be great to keep support for this use case.

The point is not to build all the packages in a single CMake context (which indeed does not scale, especially with a federated development model), but to enable some of them to be in the same CMake context.

@dirk-thomas
Copy link
Member Author

@sbarthelemy This article is only about the build tool which does not make that decision. The build system ament_cmake is covered in a separate article which describes the rational why support for a single CMake context is being dropped: http://design.ros2.org/articles/ament.html#building-within-a-single-cmake-context I just created #116 to elaborate on the rational a bit more.

Basically it comes down to the following: if you build multiple packages within a single CMake context their decoupling is basically lost. The packages need to know about the internal structure of their dependencies: avoid duplicate symbols, avoid side effects, declare cross-package target level dependencies. At that point you might want to put those tightly coupled packages into a single package.

Also when considering the bigger picture the user trying to build a set of packages has no way of knowing if he can use this approach / if it is supported by the set of packages or not.

@sbarthelemy
Copy link

@dirk-thomas

Basically it comes down to the following: if you build multiple packages within a single CMake context their decoupling is basically lost.

I agree on this.

At that point you might want to put those tightly coupled packages into a single package.

I agree on this too.

Still, my experience is that there is pressure to split packages: separate interface from implementation, have finer package-level dependency descriptions (users of a package don't want to compile what they won't use, to install what they won't need),...
In the end of the day, packages tend to be smaller than the "comfortable development size".
The shared CMake context was a useful workaround.

Also when considering the bigger picture the user trying to build a set of packages has no way of knowing if he can use this approach / if it is supported by the set of packages or not.

Agreed.

Maybe a better solution would be to let a single build produce several packages. A bit like debian packages: a single source package build creates several binary packages (if I recall correctly).

Thus a big single source package might produce several tightly (coupled) binary packages.

@dirk-thomas
Copy link
Member Author

Maybe a better solution would be to let a single build produce several packages. A bit like debian packages: a single source package build creates several binary packages (if I recall correctly).

Thus a big single source package might produce several tightly (coupled) binary packages.

But where would the necessary information for how to split the package come from? Also each subpart would require the following meta information for packaging: name, version, maintainer, changelog, dependencies etc. (which is exactly what a package.xml file specifies).

For Debian this process is performed by a human by hand which is quite some work. I doubt any ROS maintainer would like to spend that much time to release a single package.

@tkruse
Copy link

tkruse commented Mar 7, 2017

This kind of discussion is not ideal on github. Rosbuild had the concept of stacks and packages. A stack was something like all packages living in a single VCS root. Those could in theory be build cleanly (enough) in one cmake context, but released individually. There should be an old thread about this.

@sbarthelemy
Copy link

@tkruse

This kind of discussion is not ideal on github.

indeed, and the matter is not really related to this pull request. Is there a better place?

@dirk-thomas

But where would the necessary information for how to split the package come from?

I'll try to find the time to make up a real life example.

@dirk-thomas
Copy link
Member Author

This kind of discussion is not ideal on github.

indeed, and the matter is not really related to this pull request. Is there a better place?

I don't think there is a discourse category for it yet. Hence I would recommend posting to the buildsystem mailing list: https://groups.google.com/forum/#!forum/ros-sig-buildsystem

@sloretz
Copy link
Contributor

sloretz commented Mar 10, 2017

The naming section sounds like ament_tools would stop being developed in favor of a new build tool with a name that didn't sound similar to ament_cmake. I think it would be fine for the tool to share a name with a build system. To a lot of developers the "tool" is just the commands they've memorized to build their code. Knowing the build tool is a different from the build system doesn't change their workflow.

Adding another build tool might increase the maintenance workload. The existing build tools catkin_make, catkin_make_isolated, and catkin_tools are all commonly used in ROS 1. While there is a goal to not disturb ROS 1, if the tool works with ROS 1 packages I think deprecating those tools in favor of the new one would go a long ways towards consolidating the work to the new tool.

Extensions adding extra flags to verbs might be too small to be worth supporting. Flags tend to be small changes to behavior. Supporting extending these means adding hooks throughout the logic without really knowing where hooks will be useful to future extension writers, or encouraging python monkey patching. If someone really wants a small change in behavior then it not being extensible may increase the chance they modify the tool and contribute the change upstream.

Copy link
Member

@dhood dhood left a comment

Choose a reason for hiding this comment

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

this reads well to me

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.

This article describes the steps from the current build tools to a single universal build tool which automates the process and works for various different use cases.
Copy link
Member

Choose a reason for hiding this comment

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

"current build tools" -> "current build tools used in the ROS ecosystem" (otherwise the scope is not clear)

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point: done in ae668e5


The tool needs to be able to build ROS 2 workspaces which can already be built using `ament_tools`.

#### Build Gazebo including dependencies
Copy link
Member

Choose a reason for hiding this comment

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

From the 'goal' section above I understood that this was just a 'nice-to-have'. It would be good to either mention again that this is a nice-to-have, or explain why it is that this is a requirement

Copy link
Member Author

Choose a reason for hiding this comment

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

In the goal section it was more meant as "since we already have all this functionality it should also work for this case". Would you have a suggestion to clarify that so that it doesn't look like a nice-to-have?

Copy link
Member

Choose a reason for hiding this comment

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

please see #118

@dirk-thomas
Copy link
Member Author

@sloretz

Adding another build tool might increase the maintenance workload. The existing build tools catkin_make, catkin_make_isolated, and catkin_tools are all commonly used in ROS 1. While there is a goal to not disturb ROS 1, if the tool works with ROS 1 packages I think deprecating those tools in favor of the new one would go a long ways towards consolidating the work to the new tool.

That is certainly the goal. The "new unified" build tool should work for both ROS 1 as well as ROS 2 (and hopefully other use cases) and we would recommend it to be used for both ROS versions. For backward compatibility the current ROS 1 tools will still be around but we wouldn't plan to spend any time on them (except urgent bug fixes of course).

The naming section sounds like ament_tools would stop being developed in favor of a new build tool with a name that didn't sound similar to ament_cmake. I think it would be fine for the tool to share a name with a build system. To a lot of developers the "tool" is just the commands they've memorized to build their code. Knowing the build tool is a different from the build system doesn't change their workflow.

Since the build tool is supposed to be used with multiple different build system which have different names I think the association with one of them by name is to be avoided (since users can a) often not distinguish what is build tool and what is build system and b) expect a build tool called ament_tools not to work for catkin).

Extensions adding extra flags to verbs might be too small to be worth supporting. Flags tend to be small changes to behavior. Supporting extending these means adding hooks throughout the logic without really knowing where hooks will be useful to future extension writers, or encouraging python monkey patching. If someone really wants a small change in behavior then it not being extensible may increase the chance they modify the tool and contribute the change upstream.

After having developed quite a few build systems and build tools in the past I have to disagree with you on this. A verb like build has to handle a lot of different things. Just take a look at the command line arguments for it in catkin_tools (https://github.com/catkin/catkin_tools/blob/master/catkin_tools/verbs/catkin_build/cli.py#L94-L193). The number of different aspects it has to handle is enormous. And I am sure people can easily come of with more ideas which it should handle additionally. In my opinion this is the exact opposite of the goal of the single responsibility principle. (Contributing to this code base sets a pretty high bar. I would expect a more modular code base to attract more external contributions.)

@clalancette
Copy link
Contributor

This looks pretty good to me. There will definitely have to be more clarification in the future as things evolve, but I think this is a good start.

@dirk-thomas
Copy link
Member Author

There will definitely have to be more clarification in the future as things evolve

Can you please clarify which aspects you have in mind?

@clalancette
Copy link
Contributor

In particular, the part at the end for the Proposal section, which is TBD.

@mikaelarguedas
Copy link
Member

All my minor comments and questions have been addressed by previous feedback, hence my +1 on the first comment.

Few points that could be clarified (maybe directly in the proposal):

  • What do we mean by multi-platform ? The platforms targeted by ament natively will be supported by the core of the tool? and then users can use extension points to add support for other platforms?
  • In the goal section: does "work with ROS1 packages as well as ROS2 packages" implies that no modification of their CMake will be necessary, right?
  • While it's possible to do cross-compilation with some of the existing tools it doesn't appear in this document at all, should this be taken into account in this document?
  • Will the proposal specify a way to provide meta information externally?

@dirk-thomas
Copy link
Member Author

What do we mean by multi-platform ? The platforms targeted by ament natively will be supported by the core of the tool? and then users can use extension points to add support for other platforms?

The "unified" build should certainly cover all platforms currently supported by the existing tools it aims to replace. If that is part of the "core" or realized in plugins is an implementation detail. The article doesn't prescribe that.

In the goal section: does "work with ROS1 packages as well as ROS2 packages" implies that no modification of their CMake will be necessary, right?

No, the build tool will not provide any such magic. It aims to support building ROS 1 packages like catkin_* does as well as ROS 2 package like ament_tools. Any mixture is the responsibility of the build system - not the build tool.

The idea as mentioned on the roadmap draft is to create a ROS 2 package named "catkin" which provides the same CMake API as catkin does in ROS 1 and map the calls to ament_cmake. That should allow to process existing ROS 1 packages for ROS 2 on a CMake level (!). If anything within such a package depends on ROS 1 names etc. that is a different story which can't be addressed by this CMake API shim.

While it's possible to do cross-compilation with some of the existing tools it doesn't appear in this document at all, should this be taken into account in this document?

I don't consider cross-compilation to be relevant for the build tool. The build sytem is responsible to support this. Maybe it needs to be invoked with certain parameters. So the build tools responsibilty is to allow invoking the build system of each package with arbitrary arguments. But I don't think this is specific to cross-compilation. The same requirement already arises from native builds. E.g. passing an individual build type to a specific package in the workspace.

Will the proposal specify a way to provide meta information externally?

I don't plan to specify the details about how the meta information is being provided. It could be yaml, xml, whatever else. The requirement is that it needs to be possible from outside of the source space. It could even be that different plugins of the build tool provide different mechanisms (locally for building a Gazebo workspace I have three different ways to provide these information).

@mikaelarguedas
Copy link
Member

So the build tools responsibilty is to allow invoking the build system of each package with arbitrary arguments.

Agreed, I think this should be a requirement for the new tool. Maybe some tools (catkin_tools?) allow that already so I guess it's implied in the "should provide a superset of the functionality provided by the existing tools " but it wasn't obvious to me when I read it. Hence the question

@wjwwood
Copy link
Member

wjwwood commented Mar 13, 2017

No, the build tool will not provide any such magic. It aims to support building ROS 1 packages like catkin_* does as well as ROS 2 package like ament_tools. Any mixture is the responsibility of the build system - not the build tool.

When you say "Any mixture" do you mean the shim you mentioned or do you mean a workspace, with two packages, one catkin and the other ament_cmake? In the latter case I think the build tool (or it's plugins) will need to deal with this issue. Either that or the proposal will need to include what we intend to do in the build systems to make them behave with one another.

The idea as mentioned on the roadmap draft is to create a ROS 2 package named "catkin" which provides the same CMake API as catkin does in ROS 1 and map the calls to ament_cmake.

Where is this being discussed? It sounds like an interesting idea, but I don't remember talking about it. Why is it necessary to have this shim? Also, how would the shim handle the required differences in the contents of the package.xml (think different dependencies beyond the build system)? Can't you just have catkin packages use catkin directly and let them coexist with ament_cmake packages? (this is perhaps off-topic, I can repost the question where ever this is being discussed once you point me to it)

I don't consider cross-compilation to be relevant for the build tool. The build sytem is responsible to support this.

I'm not convinced by this argument, because catkin_tools is mostly divested from the "build system" part of catkin and yet it had to be modified to not break DESTDIR and in other ways to support cross-compilation. I agree most of the responsibility lies mostly with cmake or python's setuptools or whatever underlying build system it is to support cross-compilation, but I also don't think the tool will be unaffected.

You could be making a separate argument that the "core" build tool doesn't need to handle this, but rather the catkin or ament specific plugin, but I think this just kicks the can down the road. In my opinion the proposal needs to address details about these plugins and how they'll work together. I think it's good to separate them conceptually, but both need to be represented in this document I think.

@dirk-thomas
Copy link
Member Author

No, the build tool will not provide any such magic. It aims to support building ROS 1 packages like catkin_* does as well as ROS 2 package like ament_tools. Any mixture is the responsibility of the build system - not the build tool.

When you say "Any mixture" do you mean the shim you mentioned or do you mean a workspace, with two packages, one catkin and the other ament_cmake? In the latter case I think the build tool (or it's plugins) will need to deal with this issue. Either that or the proposal will need to include what we intend to do in the build systems to make them behave with one another.

From my point of view the build tool identifies a package and decides how to process it. Afterwards it knows how to setup the environment to "make it available" to downstream packages.

A package using the CMake API shim is still an ament package since it uses ament_cmake under the hood (it registers itself at the ament resource index, it creates setup files like other ament packages, etc.).

The primary goal for the unified build tool is to support building a ROS 1 workspace and (unrelated from that !) a ROS 2 workspace. Mixing ROS 1 and ROS 2 packages in the same workspace might be technically possible since the build tool knows how to process them. But there is much more to it then just invoking the build: packages names might collide or be different, etc.

The idea as mentioned on the roadmap draft is to create a ROS 2 package named "catkin" which provides the same CMake API as catkin does in ROS 1 and map the calls to ament_cmake.

Where is this being discussed? It sounds like an interesting idea, but I don't remember talking about it. Why is it necessary to have this shim? Also, how would the shim handle the required differences in the contents of the package.xml (think different dependencies beyond the build system)? Can't you just have catkin packages use catkin directly and let them coexist with ament_cmake packages? (this is perhaps off-topic, I can repost the question where ever this is being discussed once you point me to it)

Currently the CMake code of every ROS 1 package we try to use with ROS 2 is being completely changed from catkin to ament_cmake. Imo most of that is unnecessary. We might need a couple of changes (e.g. names as well as a CMake call at the end where ament_package usually is invoked) but it would be beneficial to be able to keep all the rest as is. We have talked about it in verb discussions only. It is also mentioned on the (still internal) roadmap draft.

To fully work in a ROS 2 workspace it is not sufficient to just be able to build the catkin package with a tool which knows how to do that. A package using catkin will e.g. by definition not register itself at the ament resource index.

I don't consider cross-compilation to be relevant for the build tool. The build sytem is responsible to support this.

I'm not convinced by this argument, because catkin_tools is mostly divested from the "build system" part of catkin and yet it had to be modified to not break DESTDIR and in other ways to support cross-compilation. I agree most of the responsibility lies mostly with cmake or python's setuptools or whatever underlying build system it is to support cross-compilation, but I also don't think the tool will be unaffected.

Completely agreeing with you. The build tool will need to make sure not to be in the way of what is possible with the build system.

@iluetkeb
Copy link

iluetkeb commented Mar 17, 2017

@dirk-thomas The only argument on that page that I could identify as explaining why catkin has not been evolved is that it might cause regressions on ROS1. I do not agree with that argument, because I think there would have been ways to prevent that from happening.

However, that's all water under the bridge now. The decision has been made, you guys put a lot of effort into ament and improving upon catkin, and the result is nice, so it's here to stay.

I really didn't mean to re-start the discussion, the only reason I mentioned is because I thought you guys wanted to do it again. I now see that I was mistaken about that, and that that's not what you're doing.

@dirk-thomas
Copy link
Member Author

Thank you @ all for the feedback so far. Based on that I have tried to clarify the goals at the very top of the article as well as mentioned some more use cases which should continue to be supported (e619b85). I also described the highlevel steps necessary for the two possible approaches to give an idea what is involved to get there (a980b7c).

@tkruse
Copy link

tkruse commented Mar 18, 2017

It seems the decision on whether to use catkin_tools or ament_tools as a basis might have a personal/political aspect to it. Assuming we are professionals but also humans, might be worth mentioning that.

When I look at the commit-contributors of catkin_tools, I see:

jbohren      211 commits
wjwwood      204 commits 
...
dirk-thomas    4 commits

When I look at ament_tools:

dirk-thomas   127 commits
wjwwood        65 commits
... others less than 3 commits each

So some of the players here might have strong emotional attachments to one of those tools, not the other. Is that something to discuss at all?

@dirk-thomas
Copy link
Member Author

It seems the decision on whether to use catkin_tools or ament_tools as a basis might have a personal/political aspect to it. Assuming we are professionals but also humans, might be worth mentioning that.

Please see the two approaches current mentioned in the article. The first option describes "evolving catkin_tools" and the second option described "starting with more flexible architecture from scratch". Evolving from ament_tools is explicitly mentioned as not an reasonable option. So I am not sure what you are trying to imply with your statement and the statistics.

Imo the choice boils down to a software engineering decision. What is easier to achieve and / or what results in a better quality end result. The conclusion will likely be influenced by personal opinion. That is why the current draft explicitly doesn't decide to do either or but only describe two possible paths forward .

@tkruse tkruse mentioned this pull request Mar 18, 2017

One approach is to incrementally evolve one of the existing tools to satisfy the described goals.
Since `catkin_tools` is in many aspects the most complete build tool it should be one being evolved.
While `ament_tools` has a few features `catkin_tools` currently lacks (e.g. plain CMake support without a manifest, Windows support) the feature richness of `catkin_tools` makes it a better starting point.
Copy link

Choose a reason for hiding this comment

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

'Evolve ament' should be it's own section, even if it is rejected early on.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in e99e8a9


The following items are highlighting some of the necessary efforts (not a complete list):

- Refactor the software architecture of the existing code base to support the flexibility sketched by the extension points listed above.
Copy link

Choose a reason for hiding this comment

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

Which of the extension points are missing in catkin_tools?

Copy link
Member Author

Choose a reason for hiding this comment

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

@tkruse
Copy link

tkruse commented Mar 18, 2017

Regarding the commit statistics, I'll let them speak for themselves without further comment.

Another aspect I have not seen mentioned so far is the problem of catkin_tools not being the default build tool for ROS1 yet. This was planned in 2014 (https://groups.google.com/d/msg/ros-sig-buildsystem/UdgxD7AZTzM/k2az2aY1_24J), but did not happen until now (despite the many contributions of OSRF visible in the commit statistics).

The impact of catkin_tools remains the same though. When package developers start using catkin_tools only to build packages in isolation, those packages will not necessarily work with catkin_make, as far as I am aware. The same would also happen for creating a universal build tool from scratch. So being able to build catkin packages/workspaces does not automatically imply being compatible with catkin_make.

Is that something that should be mentioned somewhere in the document? I.e. will the universal build tool finally lead to deprecation of catkin_make?

- Environment setup of `ament` packages

- Rename the tool to use a name unrelated to one build system.
- Investigate if a feature like continued support of the *devel space* is feasible since it doesn't apply to other build system and might be complicated to separate without sacrificing usability.
Copy link

Choose a reason for hiding this comment

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

How is this specific to evolving catkin_tools? What would the 'from scratch' solution do w.r.t. the devel space?

Copy link
Member Author

Choose a reason for hiding this comment

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

A "from scratch" tool will hopefully encapsulate all devel space specific knowledge in a catkin / ROS 1 specific plugin. The question it it want to offer the feature at all or not is the same.

The following items are highlighting some of the necessary efforts (not a complete list):

- Refactor the software architecture of the existing code base to support the flexibility sketched by the extension points listed above.
- Move `catkin` specific concepts out of the core of the build tool into a catkin specific extension (e.g. manifest format, *devel space*).
Copy link

Choose a reason for hiding this comment

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

Is that aspect important to the success of the tool? Extracting functionality can be done at any time after release 1, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

One of the software goals says:

Not paying for what you don't use

So if I am building a ROS 2 workspace with this tools which doesn't feature a devel space I don't want the code being involved building the workspace having to deal with that concept.

## Possible Approaches

In terms of flexibility neither of the existing build tools can already support the superset of features described in this article.
There are two different paths possible to reach the goal of a universal build tool:
Copy link

Choose a reason for hiding this comment

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

What I suggested in a comment was a third way. Create a tool sitting on top of both ament_tools and catkin_tools, detecting the workspace nature, then invoking either of those tools. Basically treating catkin_tools and ament_tools as libraries (they can be forked as libraries without the CLI part). Possibly even enforce that catkin_tools as library is only used using Python2, and ament only with Python3. Once this runs, release it, then iteratively refactor moving common functionality to the universal layer. Should this approach be mentioned?

Copy link
Member Author

Choose a reason for hiding this comment

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

Since this approach doesn't meet the state goals (deduplicating the effort of maintaining multiple tools) I don't think it needs to be mentioned.

@tkruse
Copy link

tkruse commented Mar 23, 2017

What about migrating all ament-packages to be build with catkin_tools (extended for python 3 and windows)? This seems to also satisfy a lot of goals. The document could briefly touch upon that option.

@dirk-thomas
Copy link
Member Author

What about migrating all ament-packages to be build with catkin_tools (extended for python 3 and windows)? This seems to also satisfy a lot of goals. The document could briefly touch upon that option.

Any progress toward the goals is a good thing. But since there are arbitrary subsets possible I don't think it makes sense to enumerate them all.

@dirk-thomas
Copy link
Member Author

I have added some more feedback. The last commit (1db7d03) describes the re-prioritization and that the implementation of this goal is suspended for the foreseeable future.

If I could get a couple of final reviews on this I would like to go ahead and merge the article in the current state rather sooner then later.

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.

lgtm

It's a good starting point for discussion. Thanks for compiling it @dirk-thomas.

`catkin_make_isolated` supports building the following packages:

- ROS 1 `catkin` package with a `package.xml` file.
- Plain CMake packages with a `package.xml` file.
Copy link
Member

Choose a reason for hiding this comment

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

This looks like a copy-paste error?

Copy link
Member Author

Choose a reason for hiding this comment

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

The headline is certainly a copy-n-paste error. Good catch 👍

The supported build types are still correct, right?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, sorry I mean the labeling. The list looks ok.

Copy link
Member Author

Choose a reason for hiding this comment

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

Great, fixed: 70d11e4

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.