Fix logic which merges environment PATH variables. #449

mikepurvis · 2017-03-25T02:32:44Z

Compared to the previous implementation, this:

Preserves order of both old and new path values.
Is only applied to envvars which end in PATH.
Has tests which clarify exactly what its behaviour is.

Fixes #448.

mikepurvis · 2017-04-03T14:36:15Z

@wjwwood Thoughts on this?

(We've been running it from source in our infrastructure since Mar 25.)

wjwwood

Sorry it took me so long to get to this. I know it's hard to have these discussions over such long gaps. I'll make an effort to keep up with you on this issue until it's resolved.

wjwwood · 2017-04-03T23:38:56Z

tests/unit/test_jobs.py

+
+    # Validate that the known path was not moved from the existing order, and the unfamiliar
+    # path was correctly prepended.
+    assert job_env['PATH'] == '/bar/baz/bin:/usr/local/bin:/usr/bin'


This doesn't seem like what I'd expect from the deduplication... Isn't /usr/local/bin:/bar/baz/bin:/usr/bin more correct?

The logic is that it's introducing /usr/local/bin and /bar/baz/bin as new paths. It deduplicates first, and maintains order of new paths second, so /usr/local/bin is tossed as part of deduplication, and /bar/baz/bin is prefixed onto the existing paths.

What would be the logic that would lead to /bar/baz/bin:/usr/local/bin:/usr/bin?

I understand your logic, but what I'm talking about is expectation.

If I put myself in the shoes of the person setting /usr/local/bin:/bar/baz/bin, I might expect that the merged path might contain other stuff in between my entries, e.g. X:/usr/local/bin:Y:/bar/baz/bin:Z, but I would not expect my entries to end up reordered, where the first occurrence of /bar/baz/bin comes up before the first occurrence of /usr/local/bin.

I think that "append only if not in list" makes sense, and I think "remove duplicates in list and then prepend" makes sense. But what you've got here is "prepend only if not in list" which doesn't make sense to me because it can cause the order of contributed entries to switch.

The most naive, but correct in my opinion, is to just prepend with no deduplication.

That doesn't work though, since each dependency will independently include its own copy of the underlay chain. So if you imagine an underlay U containing packages A,B,C, and then a workspace being built which contains updated versions of the three packages, with A' and B' being the isolated resultspaces that have been built. When it comes time to build C (which depends on A and B), what you want in your env path vars is A':B':U or B':A':U, and what you definitely do not want is A':U:B':U or B':U:A':U, since then you'll have one or the other of the updated packages shadowed behind the version in the underlay.

In the real world, I don't think many/any packages are contributing more than one entry to any of the X_PATH vars, and even for those that do, relative order is maintained within entries that don't overlap with something already present. Given that, I think what is here is a reasonable compromise in simplicity without getting into an actual constraint satisfaction solver or whatever— particularly if it's constrained to the isolated case, which is a bit off the beaten track no matter how you slice things.

I see what you mean now. That's unresolvable I think as-is because each package brings a set of underlays which cannot be deduplicated at the "underlay" level. This might be something that can be addressed by a feature in ament (hopefully a back portable thing). Each package has two setup files, setup.*sh and localsetup.*sh. The latter only modifies the environment for that package. That means you can start with an underlay and know what each package brings to the table separately from their dependencies.

Anyways if you can remove this code path from the non-isolated case then this will have to be good enough I think.

The general merging case is much harder that this one— here we benefit from a bunch of assumptions, chiefly that all the envs being merged share a common underlay, and that since there are expected to be no duplicate files across the set of workspace resultspaces, the order they end up in in the overlay doesn't actually matter, as long as they're all present in the final list of paths, and all ahead of the underlay paths.

wjwwood · 2017-04-03T23:43:52Z

catkin_tools/jobs/utils.py

+
+    for overlay_env in overlay_envs:
+        for key, values_str in overlay_env.items():
+            if key.endswith('PATH'):


This is sooooo fragile. Imagine something like MY_CONFIG_PATH=file:///path/to/config.conf and that this is set by someone else as MY_CONFIG_PATH=file:///path/to/override_config.conf. This function will not handle that correctly. Obviously this is a made up example which is unlikely to happen, but if this function is being applied in all configurations of the tools, then if it does happen it will be broken and the user cannot even work around it.

So, is this function only being used in --env-cache or is it always being called, regardless of the tool configuration?

This function is used with and without --env-cache. It is required regardless because of the situation described in #382 (comment). In short, each new setup.bash blows away the environment that was present before it, so this function is required any time there are multiple isolated result spaces which must be merged together to form the environment for a package's build (the only other way I can think of to deal with this would be to somehow mutate the various setup files to "chain up" the resultspaces, as if they had been built one after the other like with catkin_make_isolated).

So, we could turn this off for non-isolated-resultspace builds. We could also set an explicit whitelist of envvars for it to act on (ROS_PACKAGE_PATH, CMAKE_PREFIX_PATH, CPATH, PYTHONPATH, PKG_CONFIG_PATH, PATH). Or both. Let me know your preference.

Ah, that's right thanks for the reminder and #382 (comment).

I guess this isn't as bad since it really only applies to isolated devel or install. That at least gives the user a way out, i.e. use a merged option.

I'm still worried about the visibility of this issue, but I don't see an effective way to raise this concern with the user. We could compare each matching env variable with a whitelist and let them know with a notice that this might not be how you expect this variable to be handled. But again, I don't see how to aggregate these and give them to the user in a place that they'll likely see, like at the end of the program.

So I guess what I'm saying is, leave it unless you can think of a way to try and notify the users when something unexpected might be happening.

In the implementation as currently proposed, the function is also getting invoked in the non-isolated case. It's not really doing anything, but it's there ready to trip up a user in the scenario you describe— even without having a colon in the variable, you could get behaviour like:

Package installed in the underlay sets FOO_PATH=bar

Package in the current merged/linked resultspace overrides that to FOO_PATH=baz

Environment setup for next package build merges those into FOO_PATH=baz:bar, when the intended behaviour was a straight up override, not a prepend.

Hence the suggestion of either a whitelist of keys where prepending is known to be correct behaviour, or else disabling this function altogether for the non-isolated case.

Oh, why is it necessary in the non-isolated case (just when I thought I was getting a handle on this thing)?

If it is "always on", then yeah I think a whitelist is the best alternative. I'd start with:

PATH

CPATH

LD_LIBRARY_PATH

DYLD_LIBRARY_PATH

DYLD_FALLBACK_LIBRARY_PATH

CMAKE_PREFIX_PATH

PKG_CONFIG_PATH

PYTHONPATH

And any others you can think of.

That being said, I don't think this can be a long term solution. I'm more convinced than ever that this mechanism is just fundamentally broken. I think we'll have to go back to setup.bash --extend like mechanics, despite the performance hit. At least I think it should be an option to operate completely with shell scripts, no env parsing/caching/manipulation by catkin_tools.

It isn't necessary in the non-isolated case; it was only made always on because I was thinking it would be non-harmful, and nice to keep a lesser-used piece of code in service to prevent it from rotting. I'm adding a commit which avoids the merge when there's only a single new env.

wjwwood · 2017-04-03T23:44:46Z

tests/unit/test_jobs.py

+    assert job_env['PATH'] == '/bar/baz/bin:/usr/local/bin:/usr/bin'
+
+    # Confirm that a key only in the original env persists.
+    assert job_env['FOO'] == 'foo'


It would be good to see a non-PATH env var that contains a : as a sanity check, especially since ROS_MASTER_URI tripped it up in the beginning if I remember correctly.

Sure, yeah; I'll modify the test.

Have added this to the "nonpaths" test down below.

wjwwood · 2017-04-03T23:45:52Z

catkin_tools/jobs/utils.py

+                    # here so that we can cheaply append to it, representing a prepend in the final
+                    # PATH var, and because we need to maintain the order of underlay paths.
+                    if key in job_env:
+                        values = job_env[key].split(':')


I have no delusions that this will work on Windows, but there is a placeholder for this in Python that is portable:

https://docs.python.org/2.7/library/os.html#os.pathsep

Cool, will do.

mikepurvis · 2017-04-04T02:46:25Z

catkin_tools/jobs/utils.py

+    if len(envs) > 1:
+        merge_envs(job_env, envs)
+    else:
+        job_env.update(envs[0])


Here's the logic which now avoids the merge function if not required by the presence of multiple resultspaces to extend.

- Use os.pathsep instead of a hard-coded colon. - Change override test to include colon values. - Disable merging when only one env to load.

wjwwood

lgtm

Thanks for spending time on the fix and iterating with me.

mikepurvis · 2017-04-04T03:41:09Z

Likewise!

mikepurvis force-pushed the fix-448 branch from a035d01 to a229a39 Compare March 25, 2017 02:40

Break out merge_envs to separate, tested function.

bee4f37

mikepurvis force-pushed the fix-448 branch from a229a39 to bee4f37 Compare March 25, 2017 03:11

mikepurvis mentioned this pull request Mar 28, 2017

Fail to build with --isolate-install and multiple source dependencies #451

Closed

5 tasks

wjwwood reviewed Apr 3, 2017

View reviewed changes

mikepurvis commented Apr 4, 2017

View reviewed changes

Review fixups.

593e650

- Use os.pathsep instead of a hard-coded colon. - Change override test to include colon values. - Disable merging when only one env to load.

mikepurvis force-pushed the fix-448 branch from c741ee9 to 593e650 Compare April 4, 2017 03:11

wjwwood approved these changes Apr 4, 2017

View reviewed changes

wjwwood merged commit 9ebe21e into catkin:master Apr 4, 2017

mikepurvis deleted the fix-448 branch April 4, 2017 03:41

mikepurvis mentioned this pull request Jun 4, 2017

Permit extending multiple workspaces #303

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix logic which merges environment PATH variables. #449

Fix logic which merges environment PATH variables. #449

mikepurvis commented Mar 25, 2017 •

edited

Loading

mikepurvis commented Apr 3, 2017 •

edited

Loading

wjwwood left a comment

wjwwood Apr 3, 2017

mikepurvis Apr 4, 2017 •

edited

Loading

wjwwood Apr 4, 2017

wjwwood Apr 4, 2017

mikepurvis Apr 4, 2017

wjwwood Apr 4, 2017

mikepurvis Apr 4, 2017

wjwwood Apr 3, 2017

mikepurvis Apr 4, 2017 •

edited

Loading

wjwwood Apr 4, 2017

mikepurvis Apr 4, 2017

wjwwood Apr 4, 2017

mikepurvis Apr 4, 2017

wjwwood Apr 4, 2017

wjwwood Apr 3, 2017

mikepurvis Apr 4, 2017

mikepurvis Apr 4, 2017

wjwwood Apr 3, 2017

mikepurvis Apr 4, 2017

mikepurvis Apr 4, 2017

wjwwood left a comment

mikepurvis commented Apr 4, 2017

Fix logic which merges environment PATH variables. #449

Fix logic which merges environment PATH variables. #449

Conversation

mikepurvis commented Mar 25, 2017 • edited Loading

mikepurvis commented Apr 3, 2017 • edited Loading

wjwwood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikepurvis Apr 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikepurvis Apr 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wjwwood left a comment

Choose a reason for hiding this comment

mikepurvis commented Apr 4, 2017

mikepurvis commented Mar 25, 2017 •

edited

Loading

mikepurvis commented Apr 3, 2017 •

edited

Loading

mikepurvis Apr 4, 2017 •

edited

Loading

mikepurvis Apr 4, 2017 •

edited

Loading