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

Added a package.xml file for REP-136 support #446

Merged
merged 4 commits into from
Jul 10, 2015

Conversation

mkoval
Copy link
Collaborator

@mkoval mkoval commented Jul 9, 2015

This pull request: (1) adds a package.xml file for DART and (2) installs it to share/dart. These are the minimal requirements, from REP-136 to build a third-party package as part of a Catkin workspace.

I am generally not a fan of putting these type of files in non-ROS packages, so I make this pull request with some trepidation. However, this is very useful if you are developing DART in parallel with ROS packages (e.g. the work I mentioned on DartLoader) because: (1) it allows you to easily link against DART without installing it into a system directory, (2) automatically re-builds DART when it is changed, and (3) insures that the DART build is done before building ROS packages that depend on it.

This doesn't add any dependencies and these files have no effect if you are not building DART in a Catkin workspace. The only visible change is that you will have an extra XML file in /share/dart.

@mkoval mkoval force-pushed the feature/REP-136 branch from 04a4d50 to 1432996 Compare July 9, 2015 02:49
@mxgrey
Copy link
Member

mxgrey commented Jul 9, 2015

ROS compatibility is only a good thing, especially since there isn't any downside to it 👍

@jslee02
Copy link
Member

jslee02 commented Jul 9, 2015

We can consider to use cmake variables for dart information (name, version, description) in pacakge.xml and configure it through cmake to centralize the information in CMakeLists.txt. There is an example: config.h.in.

@jslee02 jslee02 added this to the DART 5.1.0 milestone Jul 9, 2015
@mxgrey
Copy link
Member

mxgrey commented Jul 9, 2015

Right, since the file won't get installed until cmake is run, we may as well autogenerate it with cmake to automatically keep the versioning and other meta info up to date.

@mkoval
Copy link
Collaborator Author

mkoval commented Jul 9, 2015

@jslee02 Unfortunately, that won't work. Catkin examines the package.xml (to perform a topological sort on the packages is it is building) before it runs cmake. DART would never be built if we generate the file using cmake.

@mxgrey
Copy link
Member

mxgrey commented Jul 9, 2015

Oh, so this depends on DART already being in your catkin workspace with the package.xml written before cmake is ever run?

@mkoval
Copy link
Collaborator Author

mkoval commented Jul 9, 2015

The workflow is that you checkout DART into your workspace and run catkin_make_isolated (or it's replacement, catkin build). This: (1) searches for package.xml files in the workspace, (2) performs a topological sort using the dependencies specified in the package.xml files, and (3) calls cmake on each package in sequence (and, in the case of catkin build, in parallel when possible).

If the package.xml is not present, then Catkin will not identify DART as a package and, thus, catkin_make_isolated and catkin build will not build it.

@jslee02
Copy link
Member

jslee02 commented Jul 9, 2015

It sounds like Catkin builds DART from source that is in the workspace (some local directory) in general. Then, what if DART is already installed with package.xml? Catkin will search installed DART or still build from source in the workspace?

@mkoval
Copy link
Collaborator Author

mkoval commented Jul 9, 2015

Catkin is designed to chain workspaces, so the version of DART in the inner-most workspace wins. E.g. if DART both is in your workspace and installed, it will use the version in your workspace. If it is only installed locally, it will use that version.

I believe this is internally implemented by munging a bunch of environment variables during the build, e.g. CMAKE_PREFIX_PATH, INCLUDE_PATH, LIBRARY_PATH, and LD_LIBRARY_PATH.

@jslee02
Copy link
Member

jslee02 commented Jul 9, 2015

I think I get it. I was unclear why package.xml needs to be installed in share/dart. It seems package.xml in the root of DART is used when Catkin build it in the workspace, and package.xml in share/dart is used when Catkin use installed one.

Anyways, package.xml in the root doesn't have chance to be configured before Catkin process. We should make sure the meta data in package.xml is updated before every release. @mkoval Could you make simple note about this in package.xml? Thanks!

@mxgrey
Copy link
Member

mxgrey commented Jul 9, 2015

We may actually want the reminder to be in the CMakeLists.txt where the version variable is, so that we see the reminder whenever we go to update the canonical version number.

@jslee02
Copy link
Member

jslee02 commented Jul 9, 2015

It sounds better!

@scpeters
Copy link
Collaborator

scpeters commented Jul 9, 2015

Maybe add <buildtool_depend>pkg-config</buildtool_depend> as well?

@scpeters
Copy link
Collaborator

scpeters commented Jul 9, 2015

I have been maintaining several package.xml files to build gazebo, including one for dart-core.

I think there's not an easy way to specify a package.xml for dart-core, probably not a big deal. Or it would argue for splitting dart-core into a separate cmake package, but that's a different discussion (and I'm not proposing it here).

This comment is mainly an FYI, since the file looks pretty good.

@mkoval
Copy link
Collaborator Author

mkoval commented Jul 9, 2015

@scpeters Thanks! I added the pkg-config dependency.

I also don't think it's possible to make a separate package.xml file for dart-core unless it is built using separate CMakeLists.txt file. It may be possible to make this transparent to non-Catkin users by creating two parallel directories that contain separate package.xml files. Non-Catkin builds would use a CMakeLists.txt file in the parent directory that uses add_subdirectory to include both.

That being said, I don't think this is a good idea. It makes the DART build system more complicated just to work around the limitation that Catkin doesn't support optional dependencies.

@mkoval
Copy link
Collaborator Author

mkoval commented Jul 9, 2015

It looks like the Travis build is failing because it's failing to download Nlopt:

  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   316  100   316    0     0   6885      0 --:--:-- --:--:-- --:--:-- 28727
tar: This does not look like a tar archive
gzip: stdin: not in gzip format
tar: Child returned status 1
tar: Error is not recoverable: exiting now
ci/before_install_linux.sh: line 7: cd: nlopt-2.4.1/: No such file or directory
make: *** No targets specified and no makefile found.  Stop.

@jslee02
Copy link
Member

jslee02 commented Jul 9, 2015

It looks like the Travis build is failing because it's failing to download Nlopt:

Yeah, it seems MIT server is down for now. I'm testing if alternative server, github, works on Travis-CI.

@jslee02
Copy link
Member

jslee02 commented Jul 10, 2015

The MIT server is back. Building nlopt from the github generated tarball is not recommended so we will stay with the tarball in the MIT server.

jslee02 added a commit that referenced this pull request Jul 10, 2015
Added a package.xml file for REP-136 support
@jslee02 jslee02 merged commit f59ab70 into dartsim:master Jul 10, 2015
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