Environment setup can collide when .dsv files and scripts change the same env var

[jacobperron]

The new performance enhancements that involve preparing the environment in Python (#89) can subtly collide with packages that provide environments hooks as shell scripts (instead of the newer .dsv files). This can happen because the Python code is unaware any environment changes that might occur when sourcing arbitrary shell scripts.

In particular, I ran into this issue while updating ROS 2 Java packages to work with Dashing.
In an attempt to use .dsv files for setting the CLASSPATH variable, I ran into an issue that CLASSPATH previously set by ament_cmake packages are overwritten by the .dsv file-based implementation (e.g. rcljava's JARs are missing from CLASSPATH since they are appended via shell scripts earlier in the topological ordering of packages).

[dirk-thomas]

When prepending values to an env var we aim to not have a trailing separator. The rational is that in the past there were cases where consumers of the env vars didn't support that case.

A sourced script can perform the correct prepending. But the command generated for a dsv type like prepend-non-duplicate isn't aware of the shell logic and instead of resulting in a line like:

`FOO=new_value:$FOO`

it will (since the env var was not set before) generate a line like:

`FOO=new_value`

which will overwrite the change of the sourced script.

[jacobperron]

Just throwing out an idea, what if we always prepend and then do a post-processing step to check that all env vars that were set do not have a trailing :? And if they do, just unset them.

[dirk-thomas]

Should that post processing happen in the shell or in Python? The former would need to be implemented in all primary shells (sh, bat, ps1 atm). The later would require the overhead of another Python invocation.

How does the post-processing determine which env vars to check? Preferably without needing to discover all .dsv file again due to the necessary runtime.

Should the post-processing distinguish which trailing separators are actually coming from the "always prepending" or blindly remove them (potentially altering values unintentionally)?

[jacobperron]

Should that post processing happen in the shell or in Python?

I guess the former seems like the way to go performance-wise.

How does the post-processing determine which env vars to check? Preferably without needing to discover all .dsv file again due to the necessary runtime.

We could store this information in a temporary environment variable (which is just a list of all variables prepended to via dsv commands). Note, this could be unset at the end of the post-processing script to avoid environment pollution.

Should the post-processing distinguish which trailing separators are actually coming from the "always prepending" or blindly remove them (potentially altering values unintentionally)?

I'm not sure, but maybe it's safest to just remove the trailing separator if and only it is the only character (ie. if the contents of the variable are longer than one character, do nothing).
It seems like an odd case to want a standalone separator, but I don't know.

[dirk-thomas]

maybe it's safest to just remove the trailing separator if and only it is the only character

That might still break (poorly written) code which isn't capable of handling a trailing separator.

[jacobperron]

We know what the first prepended value should be if nothing else was prepended before the first .dsv file was processed. A best effort approach could be to compare the suffix with what we might expect to be there, and if they're equal, remove the trailing separator.

[dirk-thomas]

That sounds like a promising approach 👍

[jacobperron]

I'll try to implement this in colcon first and see how it goes.

[jacobperron]

I took a slightly different approach in colcon/colcon-core#254,

Instead of setting an environment variable to track changes in the shell scripts, I've done this tracking in Python and appended commands for cleanup.