build: Adding --extend argument to control workspace parent #58

jbohren · 2014-06-02T13:59:50Z

This adds --extend as an argument to catkin build which enables explicit chaining to/overlaying of another workspace. It is equivalent to sourcing the given workspace in a subshell building the current workspace, and then exiting that shell. For this to work properly, it requires that any variables defined in the current environment as a result of the previously-sourced workspace can be rolled back by their respective rollback hooks.

The --extend flag calls the load_workspace_environment() function at the beginning of the catkin build verb invocation which sources another workspace's environment before initiating the build process, but does not change the caller's environment. Calling catkin build --extend also implicitly enables --force-cmake. This makes it easy to switch the "parents" of a workspace.

This PR also prints out the $CMAKE_PREFIX_PATH used by the workspace at the beginning of the build for greater visibility.

decide on the argument name (should be based on standard name for this action as decided in rep#78
- --extend (current)
- --extend-ws (old)
- --overlay
- --chain
add documentation for this argument

jbohren · 2014-06-02T14:00:27Z

This is based on discussions in #48 and ros/catkin#643

jbohren · 2014-06-02T14:09:39Z

Here's an example usage:

# First, we start with an empty environment, and a source tree in the workspace
[moldy-crow:/tmp/ws]$ echo $CMAKE_PREFIX_PATH

[moldy-crow:/tmp/ws]$ tree
.
└── src
    └── foo
        ├── CMakeLists.txt
        └── package.xml

2 directories, 2 files
# Source hydro system install
[moldy-crow:/tmp/ws]$ source /opt/ros/hydro/setup.zsh 
[moldy-crow:/tmp/ws]$ echo $CMAKE_PREFIX_PATH        
/opt/ros/hydro
# Run `catkin build` normally
[moldy-crow:/tmp/ws]$ catkin build
Creating buildspace directory, '/tmp/ws/build' 
--------------------------------------------
CMAKE_PREFIX_PATH:           /opt/ros/hydro
Workspace:                   /tmp/ws
Buildspace:                  /tmp/ws/build
Develspace:                  /tmp/ws/devel
Installspace:                /tmp/ws/install
DESTDIR:                     None
--------------------------------------------
Isolate Develspaces:         False
Install Packages:            False
Isolate Installs:            False
--------------------------------------------
Additional CMake Args:       None
Additional Make Args:        None
Additional catkin Make Args: None
-------------------------------------------- 
Found '1' packages in 0.0 seconds. 
Starting ==> foo                                                                 
Finished <== foo [ 1.8 seconds ]                                                 
[build] Finished.                                                                
[build] Runtime: 1.8 seconds 
# Source the directory and check out the `$CMAKE_PREFIX_PATH`
[moldy-crow:/tmp/ws]$ source devel/setup.zsh 
[moldy-crow:/tmp/ws]$ echo $CMAKE_PREFIX_PATH        
/tmp/ws/devel:/opt/ros/hydro
# Now we can switch the base workspace to an underlay in my home directory (which is, itself chained on top of two others) and you see that the full `$CMAKE_PREFIX_PATH` is picked up by `catkin build`
[moldy-crow:/tmp/ws]$ catkin build --extend-ws ~/ws/hydro/underlay/devel   
---------------------------------------------------------------------------------------------------------------------------------------------------------------
CMAKE_PREFIX_PATH:           /home/jbohren/ws/hydro/underlay/devel:/home/jbohren/ws/hydro/underlay_isolated/install_isolated:/opt/ros/hydro
Workspace:                   /tmp/ws
Buildspace:                  /tmp/ws/build
Develspace:                  /tmp/ws/devel
Installspace:                /tmp/ws/install
DESTDIR:                     None
---------------------------------------------------------------------------------------------------------------------------------------------------------------
Isolate Develspaces:         False
Install Packages:            False
Isolate Installs:            False
---------------------------------------------------------------------------------------------------------------------------------------------------------------
Additional CMake Args:       -DCMAKE_PREFIX_PATH=/home/jbohren/ws/hydro/underlay/devel:/home/jbohren/ws/hydro/underlay_isolated/install_isolated:/opt/ros/hydro
Additional Make Args:        None
Additional catkin Make Args: None
--------------------------------------------------------------------------------------------------------------------------------------------------------------- 
Found '1' packages in 0.0 seconds. 
Starting ==> foo                                                                 
Finished <== foo [ 1.5 seconds ]                                                 
[build] Finished.                                                                
[build] Runtime: 1.5 seconds 
# We can source the new setup file in our workspace, and see that it's been switched to the new tree
[moldy-crow:/tmp/ws]$ source devel/setup.zsh 
[moldy-crow:/tmp/ws]$ echo $CMAKE_PREFIX_PATH 
/tmp/ws/devel:/home/jbohren/ws/hydro/underlay/devel:/home/jbohren/ws/hydro/underlay_isolated/install_isolated:/opt/ros/hydro
[moldy-crow:/tmp/ws]$

wjwwood · 2014-06-02T21:20:11Z

This is only intended to extend what is in the path or replace anything already in the environment?

It might be instructive to have an example where the workspace you specify with --extend-ws does not overlay /opt/ros/hydro since /opt/ros/hydro was already in the CPP in the environment.

jbohren · 2014-06-02T22:44:29Z

This is only intended to extend what is in the path or replace anything already in the environment?

If the catkin rollback behavior works, it will replace anything in the environment that gets overridden by the setup file.

It might be instructive to have an example where the workspace you specify with --extend-ws does not overlay /opt/ros/hydro since /opt/ros/hydro was already in the CPP in the environment.

I can do that...

In this example, I start off with an untouched ws, but with /home/jbohren/ws/hydro/rrm/devel:/opt/ros/hydro in my $CMAKE_PREFIX_PATH. Then I build it based on ~/ws/hydro/underlay/devel which is unrelated to the rrm workspace:

[moldy-crow:~]$ cd /tmp 
[moldy-crow:/tmp]$ mkdir -p ws/src
[moldy-crow:/tmp/ws]$ cd ws/src 
[moldy-crow:/tmp/ws/src]$ catkin_create_pkg foo
Created file foo/CMakeLists.txt
Created file foo/package.xml
Successfully created files in /tmp/ws/src/foo. Please adjust the values in package.xml.
[moldy-crow:/tmp/ws/src]$ cd ../
[moldy-crow:/tmp/ws]$ ls -l
total 4
drwxrwxr-x 3 jbohren jbohren 4096 Jun  2 18:37 src
[moldy-crow:/tmp/ws]$ echo $CMAKE_PREFIX_PATH 
/home/jbohren/ws/hydro/rrm/devel:/opt/ros/hydro
[moldy-crow:/tmp/ws]$ catkin build --extend-ws ~/ws/hydro/underlay/devel 
Creating buildspace directory, '/tmp/ws/build' 
---------------------------------------------------------------------------------------------------------------------------------------------------------------
CMAKE_PREFIX_PATH:           /home/jbohren/ws/hydro/underlay/devel:/home/jbohren/ws/hydro/underlay_isolated/install_isolated:/opt/ros/hydro
Workspace:                   /tmp/ws
Buildspace:                  /tmp/ws/build
Develspace:                  /tmp/ws/devel
Installspace:                /tmp/ws/install
DESTDIR:                     None
---------------------------------------------------------------------------------------------------------------------------------------------------------------
Isolate Develspaces:         False
Install Packages:            False
Isolate Installs:            False
---------------------------------------------------------------------------------------------------------------------------------------------------------------
Additional CMake Args:       -DCMAKE_PREFIX_PATH=/home/jbohren/ws/hydro/underlay/devel:/home/jbohren/ws/hydro/underlay_isolated/install_isolated:/opt/ros/hydro
Additional Make Args:        None
Additional catkin Make Args: None
--------------------------------------------------------------------------------------------------------------------------------------------------------------- 
Found '1' packages in 0.0 seconds. 
Starting ==> foo                                                                                                    
Finished <== foo [ 4.6 seconds ]                                                                                    
[build] Finished.                                                                                                   
[build] Runtime: 4.7 seconds 
[moldy-crow:/tmp/ws]$ source devel/setup.zsh 
[moldy-crow:/tmp/ws]$ echo $CMAKE_PREFIX_PATH 
/tmp/ws/devel:/home/jbohren/ws/hydro/underlay/devel:/home/jbohren/ws/hydro/underlay_isolated/install_isolated:/opt/ros/hydro

wjwwood · 2014-06-02T22:49:30Z

Ok, thanks, I'll look into the details more in a bit.

dirk-thomas · 2014-06-03T02:00:28Z

If the shell environment contains another workspace when invoking your command the implicit rollback is not 100% complete since changes from environment hooks are not reverted before the passed in workspaces are sourced.

Since I don't see a feasible way to achieve that and I would "force" the user to open a clean shell which has no workspace sourced.

We can discuss this in the upcoming meeting next week.

NikolausDemmel · 2014-06-03T08:21:06Z

@dirk-thomas: Just to make sure I understand, is the issue you mention exactly the same with sourcing another workspace in a shell where I previously had sourced an unrelated worspace? (I mean not in particular for workspace chaining but in general)

NikolausDemmel · 2014-06-03T10:54:36Z

If the shell environment contains another workspace when invoking your command the implicit rollback is not 100% complete since changes from environment hooks are not reverted before the passed in workspaces are sourced.

With this, would a workspace that has been built with --extend-ws in a "dirty" shell and is subsequently sourced in a fresh shell also still exhibit this problem of leftovers from the "incomplete rollback" in the previous shell?

dirk-thomas · 2014-06-03T15:38:13Z

@NikolausDemmel If you source a workspace B after another unrelated workspace A the environment is not the same as if you would have only sourced B.

All "general" variables handled in the _setup_util.py are rolled back cleanly - e.g. PATH, PYTHONPATH etc will be "clean".

But variables provided by environment hooks from workspace A won't be reset. If workspace B has the same environment hooks and if they overwrite the value it is ok but if they extend it there will still be leftovers of A in the environment.

Regarding the second question: the fresh shell where you subsequently source the workspace build from a "dirty" shell is not "dirty". But the "dirty" shell when building the workspace might effect the result of the build in undefined ways. So I would not consider the result to be 100% guaranteed equal to what you would get when you build from a "clean" shell.

jbohren · 2014-06-03T15:56:54Z

@dirk-thomas

If the shell environment contains another workspace when invoking your command the implicit rollback is not 100% complete since changes from environment hooks are not reverted before the passed in workspaces are sourced.

Since I don't see a feasible way to achieve that and I would "force" the user to open a clean shell which has no workspace sourced.

I think there's a general assumption by ROS users that if they have one catkin workspace sourced, and they source another one, it affects a clean switch from using one workspace to another. Aside from non-standard env-hooks, this is true.

If you want to prevent someone from calling --extend-ws when they have another workspace sourced, then you might as well prevent people from calling source setup.bash when they have another workspace sourced. I do not think this is a good idea to prevent people from doing this. Maybe it's reasonable to prevent novices from doing it.

As described in the planning doc for the upcoming videocon, I think catkin should provide tools for people to write good env hooks which clean themselves up.

dirk-thomas · 2014-06-03T17:47:23Z

@jbohren As mentioned before I don't think that making environment hook revertible is the right direction to go. Consider the recent issue when a workspace has been deleted it is simply impossible to rollback since the environment hooks are gone.

Instead we should consider approaches which do not require rolling back environment changes.

NikolausDemmel · 2014-06-03T18:42:53Z

Instead we should consider approaches which do not require rolling back environment changes.

While this is probably a good time to consider this, it sounds like this would imply that we need to change the way most people use catkin, which is only acceptable if it is an overall usability improvement for a large majority (keep this in mind).

A possible technical solution to making environment hooks revertible is restricting what environment hooks are allowed to do significantly and in such a way that they can be reverted even if the workspace is gone (can be done either by technical restrictions or by convention). Reverting could be achieved for example by naming conventions for environment variables that encode how they can be reverted. New kinds of such convertions could be add by third parties as python plugins to catkin-pkg, where rosdep installs the corresponding python package and the setup files warn if a env hook is executed for which corresponding python package is not present.

I have no idea how feasable for the kinds of env hooks that are currently used in the community.

jbohren · 2014-06-03T19:06:34Z

@jbohren As mentioned before I don't think that making environment hook revertible is the right direction to go. Consider the recent issue when a workspace has been deleted it is simply impossible to rollback since the environment hooks are gone.

We could also store the effects of env-hooks in the .catkin_workspace file.

dirk-thomas · 2014-06-03T19:50:57Z

... which again would be gone if you wipe the workspace...

jack-oquin · 2014-06-03T20:32:56Z

... which again would be gone if you wipe the workspace...

...which might be a good thing if the workspace has been reset.

I still think the root problem with our promiscuous use of environment variables is attempting to use global variables for per-workspace context.

jbohren · 2014-06-03T23:57:57Z

@jbohren As mentioned before I don't think that making environment hook revertible is the right direction to go. Consider the recent issue when a workspace has been deleted it is simply impossible to rollback since the environment hooks are gone.

We could also store the effects of env-hooks in the .catkin_workspace file.

... which again would be gone if you wipe the workspace...

Which is a lot less likely to happen if it's a hidden file that says # DO NOT EDIT OR REMOVE at the top of it. My point is that it seems like there are solutions here besides turning catkin into a command-line IDE.

My frustration with this argument about the env-hooks is that it seems like you simultaneously want to give people the flexibility to put whatever hooks they want into the setup files, but then you're letting that flexibility stand in the way of ease of use in the majority of use cases.

You can use your argument to justify preventing people from sourcing a setup file if one has already been sourced, which sounds like a huge pain in the ass. People want to be able to open a new shell, have a workspace get loaded automatically, and type rosmsg show or rosrun or roslaunch and have it run with the environment that they're currently working with.

dirk-thomas · 2014-06-04T00:17:02Z

I just don't think that a revised ROS workspace concept should include something for which we already foresee certain problematic cases - even if they are "a lot less likely to happen".

Let's try to just collect all these questions and options in the document for the upcoming meeting and then discuss it next week more efficiently in person.

jbohren · 2014-06-04T00:18:59Z

Let's try to just collect all these questions and options in the document for the upcoming meeting and then discuss it next week more efficiently in person.

Yep, doing that now.

wjwwood · 2014-06-09T23:19:20Z

From the discussion in the Google Hangout, we came up with these points:

Keep implicit chaining of workspaces from CMAKE_PREFIX_PATH
Allow override of chaining with argument, like the one proposed here
If the environment's CMAKE_PREFIX_PATH disagrees with with the ones passed by the arguments, still extend the ones provided by the argument, but warn that their build environment might be not what they wanted
Improve visibility of what is being built against by including that information in the beginning and at the end with a build summary (build: include build summary at the end of the build #63)

jack-oquin · 2014-06-10T16:10:44Z

If we were designing this from scratch, but knowing what we know now, I would not allow implicit chaining.

Given our history and the expectations of ROS users, I agree that it is too late now to make that a rule. So, adding explicit chaining with a warning when things don't look right seems like the best we can do.

Hence an unenthusiastic +1

jbohren · 2014-06-11T05:21:35Z

After some of the discussion of what a "workspace" is, I've changed the argument to just --extend instead of --extend-ws. I've also updated the documentation to describe the argument to --extend as a "directory containing catkin setup files" this way, no matter what we call such a directory, it's not blocking the merging of this feature.

jbohren · 2014-06-30T04:39:29Z

I've updated this to use the "result-space" term where it's appropriate.

jbohren · 2014-06-30T20:27:16Z

Is there anything blocking this getting merged?

wjwwood · 2014-06-30T20:39:14Z

I haven't had any time to test it, also I think there is no mention of it in the rst documentation. I don't think I will get to it today, but I'm getting close to a opening a big pull request on the catkin build verb, I'll make sure to integrate it before then.

jbohren · 2014-06-30T21:22:51Z

I haven't had any time to test it, also I think there is no mention of it in the rst documentation. I don't think I will get to it today, but I'm getting close to a opening a big pull request on the catkin build verb, I'll make sure to integrate it before then.

Ok. Cool. I'll add some doc text tonight.

wjwwood · 2014-07-08T19:32:00Z

catkin_tools/verbs/catkin_build/build.py

+        '-c', subcommand]
+
+    # Run the command to source the other environment and output all environment variables
+    blacklisted_keys = ('_','PWD')


Put a space after comma (PEP8)

wjwwood · 2014-07-08T19:40:45Z

Ok, I made some code style comments which I would like addressed before merging. I thought I had a style checker unit test setup, I guess not.

jbohren · 2014-07-08T20:25:52Z

Ok, I think I got all your requests.

wjwwood · 2014-07-09T00:24:54Z

catkin_tools/verbs/catkin_build/build.py

+    """
+    # Check to make sure result_space_path is a valid directory
+    if not os.path.isdir(result_space_path):
+        if quiet: 


Sorry for the pedantry, but you still have trailing whitespace on each of the if quiet: lines.

wjwwood · 2014-07-09T00:27:47Z

When giving bad input I get this:

% catkin build --extend ~/hydro
Traceback (most recent call last):
  File "/usr/local/bin/catkin", line 9, in <module>
    load_entry_point('catkin-tools==0.1.0', 'console_scripts', 'catkin')()
  File "/Users/william/devel/catkin_tools/catkin_tools/commands/catkin.py", line 192, in main
    sys.exit(args.main(args) or 0)
  File "/Users/william/devel/catkin_tools/catkin_tools/verbs/catkin_build/cli.py", line 161, in main
    load_resultspace_environment(opts.extend_path)
  File "/Users/william/devel/catkin_tools/catkin_tools/verbs/catkin_build/build.py", line 148, in load_resultspace_environment
    env_dict = get_resultspace_environment(result_space_path)
  File "/Users/william/devel/catkin_tools/catkin_tools/verbs/catkin_build/build.py", line 97, in get_resultspace_environment
    (result_space_path, '.catkin')
IOError: Cannot load setup file from path "/Users/william/hydro" because it is not a catkin-generated setup directory (missing .catkin file).

Maybe we should catch that error and give a non-traceback like error.

I know that I am not fully consistent on this every where, but it's better to fix it while we are here.

Thanks!

wjwwood · 2014-07-09T03:11:53Z

For the error processing I would try-catch the IOError around load_resultspace_environment and either sys.exit("{0}".format(exc)) or print("{0}".format(exc)) and then return 1. I think the sys.exit would be more consistent with the rest of the code.

jbohren · 2014-07-09T14:34:22Z

Maybe we should catch that error and give a non-traceback like error.

Added error catching / formatting.

Also, just so this doesn't get lost:

@wjwwood

So, what would you expect if I passed --extend-ws ""? I would expect it explicitly set a blank CPP, but in this case it will pass through the existing CPP. In fact I'm not sure if len(opts.extend_path) > 0 will ever be False when opts.extend_path is True.

Well, I feel like --extend "" should be invalid since you can't "extend" something that doesn't exist. Furthermore, in order to roll back environment variables, you need some setup file to run. Unless there's a setup file somewhere that rolls back but doesn't add anything to the environment, we'd need to implement this behavior. That's something we could do, but I feel like it's out of scope for this PR.

wjwwood · 2014-07-09T21:22:34Z

Ok, I take your point on the rolling back of variables, I don't think we should support that, at least not in this PR. However, I would then argue that --extend "" should be invalid, since the semantic is no longer clear. If I explicitly pass a value to --extend I don't expect it to behave as if I didn't. What do you think?

jbohren · 2014-07-09T21:27:04Z

Ok, I take your point on the rolling back of variables, I don't think we should support that, at least not in this PR. However, I would then argue that --extend "" should be invalid, since the semantic is no longer clear. If I explicitly pass a value to --extend I don't expect it to behave as if I didn't. What do you think?

I agree. As it stands, the current PR has the following behavior:

[moldy-crow:~/ws/hydro/cb_test]$ catkin build --extend ""
catkin build: error: argument --extend: Unable to extend workspace "": Cannot load setup file from path "" because it is not a directory.

wjwwood · 2014-07-09T21:27:34Z

I guess now I get this:

% catkin build --extend ""
catkin build: error: argument --extend: Unable to extend workspace "": Cannot load setup file from path "" because it is not a directory.

That is a reasonable error, though it could be more specific.

wjwwood · 2014-07-09T21:28:04Z

Yeah, I think that is OK to merge unless you want to make it more specific.

This enables the building of a workspace against an arbitrary realized catkin devel- or install-space which might not currently be sourced. This is equivalent to sourcing the setup files in the given directory in a subshell, building, and then exiting. - The argument to `--extend` should be a path containing valid catkin setup files - It prints out CMAKE_PREFIX_PATH at the top of the build command - Calling `--extend` also implicitly enables `--force-cmake` - Calling `--extend ""` (with an empty string) is invalid and will be reported in an error like specifying a path which doesn't exist.

jbohren · 2014-07-09T21:36:12Z

It now says:

[moldy-crow:~/ws/hydro/cb_test]$ catkin build --extend ""
catkin build: error: argument --extend: Unable to extend workspace "": Cannot load environment from resultspace "" because it does not exist.

Good to merge.

wjwwood · 2014-07-09T22:06:22Z

Cool. Looks good to me.

build: Adding --extend argument to control workspace parent

wjwwood · 2014-07-09T22:16:15Z

I touched it up slightly here: 3fc1c28

jbohren mentioned this pull request Jun 2, 2014

build: discussion of workspace "extending" behavior #48

Closed

jbohren mentioned this pull request Jun 3, 2014

Overriding CMAKE_PREFIX_PATH_AS_IS to control workspace chaining ros/catkin#643

Closed

jbohren closed this Jun 3, 2014

jbohren reopened this Jun 3, 2014

jbohren changed the title ~~build: Adding --extend-ws argument to control workspace parent~~ build: Adding --extend argument to control workspace parent Jun 11, 2014

wjwwood reviewed Jul 8, 2014
View reviewed changes

wjwwood reviewed Jul 9, 2014
View reviewed changes

wjwwood added a commit that referenced this pull request Jul 9, 2014

Merge pull request #58 from jbohren-forks/control-parent-ws

df67e04

build: Adding --extend argument to control workspace parent

wjwwood merged commit df67e04 into catkin:master Jul 9, 2014

wjwwood mentioned this pull request Jul 9, 2014

add code format/style test and fixup code style #81

Merged

build: Adding --extend argument to control workspace parent #58

build: Adding --extend argument to control workspace parent #58

Conversation

jbohren commented Jun 2, 2014

jbohren commented Jun 2, 2014

jbohren commented Jun 2, 2014

wjwwood commented Jun 2, 2014

jbohren commented Jun 2, 2014

wjwwood commented Jun 2, 2014

dirk-thomas commented Jun 3, 2014

NikolausDemmel commented Jun 3, 2014

NikolausDemmel commented Jun 3, 2014

dirk-thomas commented Jun 3, 2014

jbohren commented Jun 3, 2014

dirk-thomas commented Jun 3, 2014

NikolausDemmel commented Jun 3, 2014

jbohren commented Jun 3, 2014

dirk-thomas commented Jun 3, 2014

jack-oquin commented Jun 3, 2014

jbohren commented Jun 3, 2014

dirk-thomas commented Jun 4, 2014

jbohren commented Jun 4, 2014

wjwwood commented Jun 9, 2014

jack-oquin commented Jun 10, 2014

jbohren commented Jun 11, 2014

jbohren commented Jun 30, 2014

jbohren commented Jun 30, 2014

wjwwood commented Jun 30, 2014

jbohren commented Jun 30, 2014

wjwwood Jul 8, 2014

Choose a reason for hiding this comment

wjwwood commented Jul 8, 2014

jbohren commented Jul 8, 2014

wjwwood Jul 9, 2014

Choose a reason for hiding this comment

wjwwood commented Jul 9, 2014

wjwwood commented Jul 9, 2014

jbohren commented Jul 9, 2014

wjwwood commented Jul 9, 2014

jbohren commented Jul 9, 2014

wjwwood commented Jul 9, 2014

wjwwood commented Jul 9, 2014

jbohren commented Jul 9, 2014

wjwwood commented Jul 9, 2014

wjwwood commented Jul 9, 2014