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_make_isolated now generates environment setup files without source packages #423

Closed
wants to merge 3 commits into from

Conversation

piyushk
Copy link

@piyushk piyushk commented May 17, 2013

See http://answers.ros.org/question/62901/should-catkin_make_isolated-generate-some-initial-setup-files-without-packages-in-src/ for context.

This patch includes the following changes:

  1. Adds setup environment files in devel_isolated. This behavior should only change iff there are no packages in the source directory.
  2. Minor bug fix inside get_python_path inside catkin builder.py (line 228)

…les without any source packages. Things do not work correctly at this commit.
@dirk-thomas
Copy link
Member

Why do you want the environment files be generated in the first place?

After adding packages to the workspace and building it again with catkin_make_isolated the generated environment is different. So a previously sourced setup.sh file (which was generated for the empty workspace) will be incomplete. You have to source the updated one again. With this I am not sure why an empty workspace should provide any environment files (except placebos in order to just be there).

Furthermore you can not assume that there is only a single entry in the CMAKE_PREFIX_PATH. This might only be true for standard cases.

…es through catkin_make_isolated when no source packages are present (ros#423). This is done by duplicating a bit of code from cmake/python.cmake
@piyushk
Copy link
Author

piyushk commented May 21, 2013

I'll try to explain my use case. I believe placebos are necessary to ensure that workspace chains are not broken. I have an automated script primarily designed for students unfamiliar with ROS that generates a rosbuild and a catkin workspace and overlays the rosbuild ws on top of the catkin one. I would prefer to chain the workspaces through catkin_make_isolated in case any plain cmake packages are inside the catkin workspace in the future. Currently there are no guarantees for the setup script to exist in devel_isolated which can break this chain and I have to manually fix it.

I agree that a previously sourced setup.sh file will be incomplete, but it was my understanding that this is true whenever packages are changed in src. For instance, If I add a new package to other existing packages in the src directory, I still need to resource devel_isolated/setup.bash (aka restart the terminal) to see updates for all the environment variables.

You are completely right about the CMAKE_PREFIX_PATH. I misinterpreted how catkin_make was handling it. I have updated the pull request.

Thanks!

@dirk-thomas
Copy link
Member

I do see your need to have these scripts available under devel_isolated.

But why should the builder script mimic the same behavior as the CMake invocation and generate the _setup_util.py file which all the non-existing paths? If the user has to source an updated file anyway why not make these generated files just simple relay scripts (without any paths to the devel_isolated folder which is empty anyway)? That would avoid duplicating logic like get_python_install_dir() etc.

@piyushk
Copy link
Author

piyushk commented May 21, 2013

I completely agree with trying to prevent logic duplication. Could you comment more on the relays. I can create the relays in my script, but in my opinion catkin_make_isolated should provide this functionality.

  1. I couldn't think of a good way for catkin_make_isolated to provide these relays. As far as I could tell catkin does not care about the system ROS installation while generating environment files, and simply manipulates environment variables to generate these files. As such I could not figure out a clean way to relay devel_isolated/system.* to the corresponding files in the system installation.
  2. Instead of creating a relay, I can generate much simpler environment files by sourcing the current environment variables (CMAKE_PREFIX_PATH, PYTHONPATH etc.), however I would lose additional environment hooks such as ROS_ROOT which the catkin script should be unaware about. Bringing in the _setup_util.py script provides these hooks.

If you have suggestions on setting up relays, then let me know and I'll update the pull request.

The current patch won't add any paths in devel_isolated into the environment variables. The setup_dir variable inside the _setup_util.py is empty, and is ignored by the rest of the script. However if CMAKE_PREFIX_PATH contains additional entries, they will be processed similar to /opt/ros/groovy

@dirk-thomas
Copy link
Member

I have updated your patch quite a bit. Please take a look at the new pull request #425 and check that these changes work for your use case.

dirk-thomas added a commit that referenced this pull request May 21, 2013
@dirk-thomas
Copy link
Member

Closing in favor of #425. Thank you for the patch!

dirk-thomas added a commit that referenced this pull request May 21, 2013
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.

None yet

2 participants