[bug] msys2 subsystem should always unset ORIGINAL_PATH first

[paulharris]

Describe the bug

In short:

• Packages such as libvpx use the msys environment on Windows to build the package
• the msys /etc/profile script will NOT keep conan's carefully constructed PATH if ORIGINAL_PATH is set

The result is builds either work because the ORIGINAL_PATH already had the correct compiler paths (ie user was running conan from within an MSVC Developer CMD),
or
builds don't work because the compiler could not be found.

I previously did not understand this, and I "solved it" by hand-picking the environment vars that were kept (which did not include ORIGINAL_PATH).
#16118 and #16119
This couldn't be merged.

#12760
This describes the exact problem and correctly identifies that ORIGINAL_PATH is the culprit, but does not dive into exactly why it is a problem.

#16122
This is a merged PR that fixes problems when HOME and USERPROFILE both exist in the environment.

I suggest that we add code to the same place as #16122 to always remove ORIGINAL_PATH.
There is no need for this variable to be set for conan's purposes, and it disrupts the intended behaviour of inheriting the PATH from the environment that conan was executed within.

The code from /etc/profile, but only the PATH related bits

case "${MSYS2_PATH_TYPE:-minimal}" in
<snip>
  inherit)
    # Inherit previous path. Note that this will make all of the Windows path
    # available in current shell, with possible interference in project builds.
    ORIGINAL_PATH="${ORIGINAL_PATH:-${PATH}}"
    ;;
<snip>
esac

case "${MSYSTEM}" in
MINGW*|CLANG*|UCRT*)
  PATH="${MINGW_MOUNT_POINT}/bin:${MSYS2_PATH}${ORIGINAL_PATH:+:${ORIGINAL_PATH}}"
  ;;
*)
  PATH="${MSYS2_PATH}:/opt/bin${ORIGINAL_PATH:+:${ORIGINAL_PATH}}"
esac

The key part is: ORIGINAL_PATH="${ORIGINAL_PATH:-${PATH}}"
which will set ORIGINAL_PATH to PATH but only if ORIGINAL_PATH was originally unset.
So the environment's PATH is backed up into ORIGINAL_PATH, and then added into the final PATH.

I propose we add to conans/utils/windows.py def conan_expand_user(path): the code:

        if os.getenv("ORIGINAL_PATH"):
            del os.environ["ORIGINAL_PATH"]

I believe it will make conan behave the way we would expect.

How to reproduce it

No response

[memsharded]

Hi @paulharris

Thanks for your report.

I am trying to wrap my head around this issue.

It would be fantastic to have some links to any msys2 docs about the ORIGINAL_PATH variable and behavior.

Many thanks for the PR in #16269, it would also be much better to have a failing test that would evidence better the need for this change. You can grep @pytest.mark.tool("msys2 to see other tests using msys2, it is in our CI, so it is possible to have tests using msys2 there.

[paulharris]

I found this link that talks a little about ORIGINAL_PATH:
https://github.com/msys2/MSYS2-packages/blob/b18becd00a49f092c96fd002c8ff853bac797748/msys2-runtime/0032-Handle-ORIGINAL_PATH-just-like-PATH.patch#L4

I wasn't able to find any other documentation on ORIGINAL_PATH.
The intent seems to be (for "inherit" mode):

• When msys first starts (ie /usr/bin/bash --login), capture the PATH in ORIGINAL_PATH, then add some extra paths for msys's binaries in /usr/bin etc.
• When a msys shell is started from WITHIN msys (ie /usr/bin/bash --login), don't re-capture the PATH. Instead, use the previously captured ORIGINAL_PATH and again add the msys /usr/bin paths etc.

That way, bash --login will always have the original DOS paths, plus their standard ones added.

The /etc/profile in question is here:

• Set the ORIGINAL_PATH from PATH (but only if not already set): https://github.com/msys2/MSYS2-packages/blob/b18becd00a49f092c96fd002c8ff853bac797748/filesystem/profile#L38
• For MINGW environment: Set PATH to ORIGINAL_PATH plus extra mingw paths: https://github.com/msys2/MSYS2-packages/blob/b18becd00a49f092c96fd002c8ff853bac797748/filesystem/profile#L52
• or, for an MSYS environment: Set PATH to ORIGINAL_PATH plus extra msys paths: https://github.com/msys2/MSYS2-packages/blob/b18becd00a49f092c96fd002c8ff853bac797748/filesystem/profile#L61

I'm not sure how to write a test for this,
to demonstrate locally, you would do:

• open DOS prompt
• set ORIGINAL_PATH=blah
• activate conan env
• cd clone of conan-center-index\recipes\libvpx\all
• conan create . --user test --channel test1 --version 1.13.1 -pr:h=host-profile -pr:b=build-profile --build=missing
• It will fail as the build-environment's (during conan-build) will be /mingw64/bin:/usr/local/bin:/usr/bin:/bin:blah

I modified the conan-cache's copy of /etc/profile to print out the PATH values to confirm.
It is not documented by msys (that I could find) and leads to obscure problems due to assumptions conan is making.

The set ORIGINAL_PATH=x is not something you would ever do as a user, but it IS done by msys shells, and it poisons the future build environment of conan runs.

Conan is assuming that the internal msys env it uses for autotools will inherit the PATH that conan configures. When ORIGINAL_PATH is set, the paths inherited are not the conan paths, but the paths BEFORE conan was called.

---

This will be another one of those situations where you explain that it is up to the conan user to ensure the environment is clean and ready for build, but this is something that the typical user hasn't introduced, and won't realise it is poisoning their builds.

Conan2 works well running from within an msys shell, there are just a couple of things to improve and then we are all the way there.

If we can't clear the env variable as I suggest, could we instead check if the env variable is set and show a warning.

Is there a way in the profile to UNSET an environment variable? Perhaps this is something I (and other people) can add to their profile.

[memsharded]

Thanks for the feedback

https://github.com/msys2/MSYS2-packages/blob/b18becd00a49f092c96fd002c8ff853bac797748/msys2-runtime/0032-Handle-ORIGINAL_PATH-just-like-PATH.patch#L4

This looks like a patch/change in msys2 so this issue won't actually happen?

This will be another one of those situations where you explain that it is up to the conan user to ensure the environment is clean and ready for build, but this is something that the typical user hasn't introduced, and won't realise it is poisoning their builds.

Not really, I think I start to understand this better, and I understand how this can be more obscure, and it would be nice if this could be managed by Conan. Just to clarify, if I understood correctly, this doesn't happen when you are in cmd and launch msys2 unless you manually set ORIGINAL_PATH yourself, but it will happen if you are already inside msys2 and launch another msys2 subprocess, is this the case?

Is there a way in the profile to UNSET an environment variable? Perhaps this is something I (and other people) can add to their profile.

yes, profiles can do MYENV_VAR=! to unset variables, if you want to give it a try. Still, it might be worth trying to automate this, if this won't break other users.

[paulharris]

Thanks for the feedback

https://github.com/msys2/MSYS2-packages/blob/b18becd00a49f092c96fd002c8ff853bac797748/msys2-runtime/0032-Handle-ORIGINAL_PATH-just-like-PATH.patch#L4

This looks like a patch/change in msys2 so this issue won't actually happen?

Well that patch was applied to msys2, I only linked it because it was the only thing I could find that even mentioned ORIGINAL_PATH at all.

This will be another one of those situations where you explain that it is up to the conan user to ensure the environment is clean and ready for build, but this is something that the typical user hasn't introduced, and won't realise it is poisoning their builds.

Not really, I think I start to understand this better, and I understand how this can be more obscure, and it would be nice if this could be managed by Conan. Just to clarify, if I understood correctly, this doesn't happen when you are in cmd and launch msys2 unless you manually set ORIGINAL_PATH yourself, but it will happen if you are already inside msys2 and launch another msys2 subprocess, is this the case?

Yes, you understand correctly.
I set ORIGINAL_PATH manually as it nicely demonstrates how PATH can end up in an unexpected state due to another env variable.

You can see the behaviour in msys terminals.
Open an msys mintty terminal, and you see ORIGINAL_PATH with all the windows env PATH entries too.

export ORIGINAL_PATH=blahblah
bash --login
echo $PATH

Now PATH doesn't contain the windows env PATH entries, but instead contains blahblah.
The intent from msys is that ORIGINAL_PATH will be set when the very first bash --login is run, capturing the original windows PATH. Then, any subsequent bash --login inside that session will reset the path to that original environment PATH.

conan is spinning up a cmd, setting the PATH and then running bash --login.
The assumption is that bash will inherit cmd's PATH.
But, that doesn't happen as the environment has a record of the PATH prior to conan's cmd subprocess, in the form of the ORIGINAL_PATH that was in the environment the whole time.

If conan wants to set PATH and make msys inherit from that PATH, it needs to unset ORIGINAL_PATH.

Is there a way in the profile to UNSET an environment variable? Perhaps this is something I (and other people) can add to their profile.

yes, profiles can do MYENV_VAR=! to unset variables, if you want to give it a try. Still, it might be worth trying to automate this, if this won't break other users.

I can't imagine anyone would want the current behaviour.

[paulharris]

Note that ORIGINAL_PATH=! appeared to not work for me.
I put it in the [buildenv] section of my profile, but ffmpeg was not able to find cl.exe

I didn't investigate further, I suspect it is related to when the environment things are applied - I assume original_path=! would be applied after the msys env was spun up, so /etc/profile saw ORIGINAL_PATH with some content and didn't appropriate set PATH. Something like that.

[memsharded]

I have been checking this a bit more.
I'd say that this should be better applied in the subsystem management, and not in the general conan_expand_user(path): code, that is:

diff --git a/conans/client/subsystems.py b/conans/client/subsystems.py
index daba56c06..ac7ca25c7 100644
--- a/conans/client/subsystems.py
+++ b/conans/client/subsystems.py
@@ -82,6 +82,7 @@ def _windows_bash_wrapper(conanfile, command, env, envfiles_folder):
         # - CHERE_INVOKING is necessary to keep the CWD and not change automatically to the user home
         msys2_mode_env.define("MSYSTEM", _msystem)
         msys2_mode_env.define("MSYS2_PATH_TYPE", "inherit")
+        msys2_mode_env.unset("ORIGINAL_PATH")
         # So --login do not change automatically to the user home
         msys2_mode_env.define("CHERE_INVOKING", "1")
         path = os.path.join(conanfile.generators_folder, "msys2_mode.bat")

Can you please try to use this patch instead of yours and test it in your setup?

[paulharris]

I can confirm that your patch works for me!
Thanks very much for looking into it, much appreciated!