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 is changing too much within minor releases #534

Closed
jbohren opened this issue Oct 11, 2013 · 8 comments
Closed

Catkin is changing too much within minor releases #534

jbohren opened this issue Oct 11, 2013 · 8 comments
Assignees

Comments

@jbohren
Copy link
Member

jbohren commented Oct 11, 2013

First, the new "relocatable" features broke the orocos/rtt integration and required fixing. Now it looks like the changes to the build organization of catkin_make_isolated (see below) have just wasted several hours of my time because it broke our integration test harness (https://travis-ci.org/jhu-lcsr-forks/rtt/builds).

The test script would cd into the build directory for a specific package and call make explicitly to run make check unit tests. In the most recent catkin release (0.5.77) this procedure no longer works. We have to do this to run these tests because catkin_make_isolated --pkg rtt --make-args check isn't a valid sequence of arguments.

The changelog makes what changed pretty clear (so kudos to changelogs):

catkin_make_isolated:
separate devel and install build folders for plain cmake packages cleanly without polluting namespace (#532)

I think while #532 is a valid long-term concern, I don't think it's something that should have been rolled out in a patch update. The build of RTT along with its tests takes about 40 minutes to run each time, and since travis-ci doesn't allow postmortem shell access, it required running several builds before getting enough information about the environment that it was clear that catkin was the problem.

@smits @meyerj This is why the rtt ci builds were failing.

@dirk-thomas
Copy link
Member

About the "relocatable" feature I do agree. We could have kept that for Indigo. Consensus about it was in the development group that it was important enough to support those uses cases now (especially for cross compiling) and not only with the next distribution which will not be released until May 2014.

For the separated build directories I have to disagree. When they were not separated (which was the case in version 0.5.74) it resulted in pretty bad side effects when switching between install and non-install. That was a bug and it needed to get fixed.

Therefore the build folders was separated - first with a "_devel" and "install" suffix (which was done in version 0.5.75). That version was never available in the public repositories. During the phase were it was in shadow-fixed we improved the bug fix to use subfolders (what was done in #532 as you mentioned above) since that approach avoids any naming collision.

Regarding your invocation catkin_make_isolated --pkg rtt --make-args check can you please provide some more information on that? catkin_make_isolated allows to invoke arbitrary make targets using that syntax. E.g. invoking catkin_make_isolated --pkg roscpp --make-args roscpp works fine in a workspace containing roscpp.

@jbohren
Copy link
Member Author

jbohren commented Oct 11, 2013

Regarding your invocation catkin_make_isolated --pkg rtt --make-args check can you please provide some more information on that? catkin_make_isolated allows to invoke arbitrary make targets using that syntax. E.g. invoking catkin_make_isolated --pkg roscpp --make-args roscpp works fine in a workspace containing roscpp.

So this fails (no target 'check'):

catkin_make_isolated --install
catkin_make_isolated --pkg rtt --make-args check

This works:

catkin_make_isolated
catkin_make_isolated --pkg rtt --make-args check

This also works:

catkin_make_isolated --install
catkin_make_isolated --install  --pkg rtt --make-args check

This, in addition to the subject of this ticket (the recent build structure change) leads me to believe that the --install documentation is misleading since it doesn't simply cause the packages to get installed, but actually changes the way that (or place where) the packages get built.

@dirk-thomas
Copy link
Member

The first command does not work since you have not build the package rtt before (since --install used a different folder) [I am guessing rtt is a plain cmake package?].

Can you please point to the docs which should be updated? I only updated REP 137 (ros-infrastructure/rep#62) for now.

@jbohren
Copy link
Member Author

jbohren commented Oct 11, 2013

This line:

help='Causes each catkin package to be installed.')

Using the --install argument now does more than just install the packages, it builds them in different folders, which means all subsequent calls to catkin_make_isolated also need the --install argument in order to point to the correct sub-buildspace.

@dirk-thomas
Copy link
Member

What description do you think would make that behavior clear?

Causes each catkin package to be installed. For packages with build type 'cmake' the package is build in separate folders for install and non-install invocations.

I am not sure if such a detailed description actually makes sense in the CLI help.

@jbohren
Copy link
Member Author

jbohren commented Oct 11, 2013

Well the fact that they're put into different folders is irrelevant if you don't actually need to get into those folders. How about:

Installs packages to install space. Subsequent calls which re-use this build configuration must also use this argument.

@dirk-thomas
Copy link
Member

I don't think that it is actually related to --install. Instead a clarification for --pkg is necessary - similar to the one already in place for --from-pkg:

--pkg PKGNAME [PKGNAME ...]
Invoke "make" on specific packages (only after catkin_make_isolated has been invoked before with the same install flag)

@dirk-thomas
Copy link
Member

I have committed the proposed update to the command line help of the --pkg option.

I will mark the ticket as closed for now. If you think further clarification is necessary please provide more feedback on this ticket and it can be reopened.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants