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

catkin_create_pkg/stack scripts #173

Closed
wants to merge 3 commits into from
Closed

catkin_create_pkg/stack scripts #173

wants to merge 3 commits into from

Conversation

tkruse
Copy link
Member

@tkruse tkruse commented Sep 18, 2012

I created catkin_create_stack and catkin_create_package scripts.

The purpose of this is threefold:

  • Allow easy generation of testcases for integration testing
  • Allow generation of documentation for how to use catkin
  • Allow users with almost no knowledge of cmake to get started quickly and write non-ugly cmake code, without having to read the catkin docs

This is similar to roscreate_pkg and roscreate_stacks

I am aware stacks will go away, but this can still be used for backporting to fuerte, if you decide so. Backporting would be a huge help in making catkin user experience better when switching from fuerte to groovy.

Implementation works, but is not complete, I would like to get feedback before I proceed. For completeness, it would be great to generate and validate more of the inputs, and I am almost sure that the way I interpret dependencies is not right.

The most value is in the comments and code commented in the generated CMakeLists.txt, as this is what most ROS users will heavily rely upon when creating wet packages. So this should be reviewed by several cmake + catkin experts.

One thing that could be changed is to make the package generation also a python API, so that other tools can also create catkin packages, e.g. think of an IDE plugin to create catkin packages.
I use empy for templating.

@dirk-thomas
Copy link
Member

It will not make sense when we move to packages-only in the near future. As soon as we have made that step it would be great if you could provide an equivalent for the new packages.

@tkruse
Copy link
Member Author

tkruse commented Sep 18, 2012

ok, removed anything related to stacks. Now it only has a script that creates catkin 'packages', meaning a CMakeLists.txt and a manifest.xml. The code is flexible, so just changing the name from manifest.xml.em to package.xml.em would generate that in the future, changing the internal structure would be similar.

I need this functionality to write more catkin unit tests anyway.

@dirk-thomas
Copy link
Member

Could you update the catkin_create_pkg script to generate a new package for the current catkin version?

@dirk-thomas dirk-thomas reopened this Sep 24, 2012
@tkruse
Copy link
Member Author

tkruse commented Sep 24, 2012

Okay, will do it tomorrow first thing.

On Mon, Sep 24, 2012 at 7:17 PM, Dirk Thomas [email protected]:

Could you update the catkin_create_pkg script to generate a new package
for the current catkin version?


Reply to this email directly or view it on GitHubhttps://github.com//pull/173#issuecomment-8826706.

@tkruse
Copy link
Member Author

tkruse commented Sep 25, 2012

Hi Dirk,

the way I see it, it would make most sense to add the creation
functionality to catkin_pkg. That's where the xml format is defined, and we
can use it there in unit test to test both parsing and generating against
each other.

I started doing so in a private branch of my fork:
https://github.com/tkruse/catkin_pkg/tree/templates

Currently I still use empy, but it seems to me that it might be more
suitable to use plain python string substitution.

Can you tell me what you think about putting the functionality within
catkin_pkg and dropping empy?
The actual script for users to create a package skeletton can remain in
catkin.

regards,
Thibault

On Mon, Sep 24, 2012 at 11:17 PM, Thibault Kruse
[email protected]:

Okay, will do it tomorrow first thing.

On Mon, Sep 24, 2012 at 7:17 PM, Dirk Thomas [email protected]:

Could you update the catkin_create_pkg script to generate a new package
for the current catkin version?


Reply to this email directly or view it on GitHubhttps://github.com//pull/173#issuecomment-8826706.

@tfoote
Copy link
Member

tfoote commented Sep 25, 2012

Hi Thibault,

Dropping empy seems reasonable as this is going to be very simple
templating. And it does make sense to have the create script generating
the package.xml files in the same place as the libraries for processing. I
don't think that it will bring in any extra dependencies to worry about.

On Tue, Sep 25, 2012 at 2:26 PM, tkruse [email protected] wrote:

Hi Dirk,

the way I see it, it would make most sense to add the creation
functionality to catkin_pkg. That's where the xml format is defined, and
we
can use it there in unit test to test both parsing and generating against
each other.

I started doing so in a private branch of my fork:
https://github.com/tkruse/catkin_pkg/tree/templates

Currently I still use empy, but it seems to me that it might be more
suitable to use plain python string substitution.

Can you tell me what you think about putting the functionality within
catkin_pkg and dropping empy?
The actual script for users to create a package skeletton can remain in
catkin.

regards,
Thibault

On Mon, Sep 24, 2012 at 11:17 PM, Thibault Kruse
[email protected]:

Okay, will do it tomorrow first thing.

On Mon, Sep 24, 2012 at 7:17 PM, Dirk Thomas [email protected]:

Could you update the catkin_create_pkg script to generate a new package
for the current catkin version?


Reply to this email directly or view it on GitHub<
https://github.com/ros/catkin/pull/173#issuecomment-8826706>.


Reply to this email directly or view it on GitHubhttps://github.com//pull/173#issuecomment-8870888.

Tully Foote
[email protected]
(650) 475-2827

@dirk-thomas
Copy link
Member

If simple string substitution is sufficient for the use case you can use the same "template" syntax as CMake's configure_file. That should be straight forward to substituted.

But may be it is also valuable to use the same mechanism for templates in catkin / catkin_pkg and not use individual solutions for each case. This might be fitting for each case but more overhead to read through all variants of doing templates. (Still simple configure_file-like substitution is an option...)

So the overall question is where we want to go for with catkin. It uses empy since string substitution is not sufficient in a lot of cases. Do we want to stick with empy even if it is not maintained for a long time or look for lightweight alternatives?

@wjwwood
Copy link
Member

wjwwood commented Sep 25, 2012

I would recommend just using Python's built-in format mini-language before
trying to string replace CMake's configure_file format with Python.

However, I don't think you can easily replace all of the uses of em in
catkin with string replacement. In some places non-trivial code is being
executed in the template files to generate the output.

For example:
https://github.com/ros/catkin/blob/master/cmake/em/order_packages.cmake.em

On Tue, Sep 25, 2012 at 3:30 PM, Dirk Thomas [email protected]:

If simple string substitution is sufficient for the use case you can use
the same "template" syntax as CMake's configure_file. That should be
straight forward to substituted.

But may be it is also valuable to use the same mechanism for templates in
catkin / catkin_pkg and not use individual solutions for each case. This
might be fitting for each case but more overhead to read through all
variants of doing templates. (Still simple configure_file-like substitution
is an option...)

So the overall question is where we want to go for with catkin. It uses
empy since string substitution is not sufficient in a lot of cases. Do we
want to stick with empy even if it is not maintained for a long time or
look for lightweight alternatives?


Reply to this email directly or view it on GitHubhttps://github.com//pull/173#issuecomment-8872712.

William Woodall
Willow Garage - Software Engineer
[email protected]

@dirk-thomas
Copy link
Member

feature moved to catkin_pkg (see ros-infrastructure/catkin_pkg#1)

@tkruse
Copy link
Member Author

tkruse commented Oct 17, 2012

rebased to latest master, refactored, added CMakeLists.txt, unit-tested

@dirk-thomas
Copy link
Member

As mentioned in ros-infrastructure/catkin_pkg#1 this should not go into catkin (but stay in catkin_pkg).

Implicitly catkin_pkg would depend on catkin which is somehow a circular dependency which will never make it into the manifests. Additionally catkin_pkg must anyway work for with multiple package.xml formats and catkin versions (in the future) when it is released distro-independent. So it should just keep multiple version-specific templates (when that becomes necessary) in catkin_pkg.

@tkruse
Copy link
Member Author

tkruse commented Oct 17, 2012

I do not see the circular dependency (other than general BC), and that
catkin_pkg will need to work with different package.xml variants still does
not convince me that it should know about catkin macros.

In fact, if catkin_pkg must know about catkin macros of each distro, that
is an implicit dependency to me. In particular if we want to extend the
CMakeLists.txt template with new values for new macros, the API would have
to be extended if that template lives in catkin_pkg.

However I don't think anyone else cares, and since I am just a contractor,
I will do as you say.

In that case the file structure and API should be future proof, IMO. This
means I would create a subfolder "groovy" for the templates and also make
distro-name a parameter of the API.

What about the catkin_create_pkg executable, do you want this in catkin_pkg
as well, with a parameter like --groovy, or should that go into catkin as I
planned?

On Wed, Oct 17, 2012 at 8:11 PM, Dirk Thomas [email protected]:

As mentioned in ros-infrastructure/catkin_pkg#1https://github.com/ros-infrastructure/catkin_pkg/issues/1this should not go into catkin (but stay in catkin_pkg).

Implicitly catkin_pkg would depend on catkin which is somehow a circular
dependency which will never make it into the manifests. Additionally
catkin_pkg must anyway work for with multiple package.xml formats and
catkin versions (in the future) when it is released distro-independent. So
it should just keep multiple version-specific templates (when that becomes
necessary) in catkin_pkg.


Reply to this email directly or view it on GitHubhttps://github.com//pull/173#issuecomment-9537705.

@dirk-thomas
Copy link
Member

I would keep the executable in catkin_pkg to. The logic and all templates will be there so should the CLI script. Keeping the templates in a subfolder is a good idea. Then adding modified templates in the future is easier.

@tkruse
Copy link
Member Author

tkruse commented Oct 17, 2012

So you also agree that catkin_create_pkg will have a mandatory parameter
for distro, so users will have to type

catkin_create_pkg foo --groovy ros_comm
or
catkin_create_pkg --hydro foo std_msgs

I woult not make it positional since that would be more confusing.

I see no reasonable way to infer for the user what distro he is using.

On Wed, Oct 17, 2012 at 11:27 PM, Dirk Thomas [email protected]:

I would keep the executable in catkin_pkg to. The logic and all templates
will be there so should the CLI script. Keeping the templates in a
subfolder is a good idea. Then adding modified templates in the future is
easier.


Reply to this email directly or view it on GitHubhttps://github.com//pull/173#issuecomment-9544650.

@dirk-thomas
Copy link
Member

If the environment has no distro set and there is more that one template, yes, the user has to specify it. A single template can be selected automatically and if the distro is set in the environment that should be the default if the arguments is not specified.

cwecht pushed a commit to cwecht/catkin that referenced this pull request Mar 20, 2018
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.

4 participants