Skip to content

Conversation

peci1
Copy link
Collaborator

@peci1 peci1 commented Jan 29, 2021

PR #752 broke building the repo in develspace using catkin build --link-devel. Before it was ok, it just wasn't possible to launch the simulator from the develspace (because the env-hooks were only taking care about installspace). We used this setup to build commsclient and similar environment-agnostic stuff in our solution images (because our packages are not correctly installable).

The build failure is caused by gcc not finding the generated protobuf .h files which are put in devel/.private/subt_ign/include.

This PR fixes the develspace build, and moreover, it also adds support for develspace in the env-hooks, so that it is now completely possible to run the simulation from devel space (I tested it on a more or less cleanly installed computer).

I'm not really sure about the ${CATKIN_DEVEL_PREFIX}/include item in catkin_package(INCLUDE_DIRS). It would be weird to add it there also in install builds. But I haven't found a way to distinguish develspace/installspace builds from within the main CMakeLists.txt... And string parsing of CMAKE_PREFIX_PATH is something I'd really like to avoid.

@osrf-jenkins
Copy link

Build finished. 21 tests run, 0 skipped, 1 failed.

@peci1
Copy link
Collaborator Author

peci1 commented Jan 29, 2021

Hmm, test subt_ign_Common FullWorldPath fails, but I haven't even touched Config.hh.in or its inputs (which, I now see, will need to be handled, but still). Based on the vague docs of FullWorldPath, I can't actually tell if I fixed it or broke it (if it's supposed to return full filesystem path or just path relative to the worlds dir).

@nkoenig nkoenig requested a review from mjcarroll February 10, 2021 16:26
@mjcarroll
Copy link
Contributor

I'm not really sure about the ${CATKIN_DEVEL_PREFIX}/include item in catkin_package(INCLUDE_DIRS). It would be weird to add it there also in install builds.

That's why I removed it in my PR. It seemed very strange to be there, but if it's helping your application, I don't think there is any harm in keeping it there.

@acschang
Copy link
Contributor

I love the ability to run the simulation from the devel space as opposed to the install space.

For a current work-around without this PR, you will be able to successfully build using a merged devel space (i.e. catkin config --merge-devel).

Signed-off-by: Michael Carroll <michael@openrobotics.org>
Signed-off-by: Michael Carroll <michael@openrobotics.org>
@mjcarroll
Copy link
Contributor

mjcarroll commented Feb 10, 2021

@peci1 I just built on top of your PR to fix the unit test as well as clean up the differences in master.

Edit: This works for me with colcon (equivalent to install space), let me know if it also works for your application.

@osrf-jenkins
Copy link

Build finished. No test results found.

@osrf-jenkins
Copy link

Build finished. 21 tests run, 0 skipped, 0 failed.

@osrf-jenkins
Copy link

Build finished. 21 tests run, 0 skipped, 0 failed.

@osrf-jenkins
Copy link

Build finished. 21 tests run, 0 skipped, 0 failed.

@peci1
Copy link
Collaborator Author

peci1 commented Feb 12, 2021

Thanks for the updates to this PR, @mjcarroll .

I found ign_migration_scripts was also spoiling the env hooks. It's actually hard to believe the subt workspace has worked so reliably until now, because the contents of etc/catkin/profile.d/ign.bash depended on whether subt_ign or ign_migration_scripts finish building first (they both wrote an env file with the same name) (not sure how is this ordering done during install time, though; but it's interesting that catkin doesn't complain about installing two files that override each other). I think the good oredering (subt_ign last) won probably just because ign_migration_scripts has no dependencies, so it almost always finishes earlier (I ran in the one instance when it finished later, that's actually how I found out this package should also be changed).

So I took env hooks from both ign_migration_scripts and subt_ign and renamed them according to the catkin docs suggestion with a numbered prefix. Originally, I wanted to give subt_ign lower priority than ign_migration_scripts, but as IGN_LAUNCH_CONFIG_PATH doesn't (yet) allow multiple paths, subt_ign has to be the last one. All other path variables can contain multiple paths, so I made them extend whatever already is in the environment. It would be nice to do for IGN_LAUNCH_CONFIG_PATH, too, but that waits for gazebosim/gz-launch#93.

The env-file renaming will probably not help people using devel space until they clean it (or manually delete devel/etc/catkin/profile.d/ign.*) because the non-renamed files will probably get ordered after the renamed ones and win over them. But I'd say that's a minor glitch not worth further effort.

I also tried to approach the hardcoded worlds path. I resorted to using rospack, as nobody gave me any better idea in https://answers.ros.org/question/370672/catkin-how-to-configure_file-differently-in-develspace-and-installspace/ (asked 12 days ago).

With these two more commits I was able to build subt workspace both with devel space and with install space (with non-shared build dir). subt_ign tests ran ok in both cases.

@mjcarroll mjcarroll merged commit 43fdd1f into osrf:master Feb 16, 2021
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