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

[question] Dealing with PATH-like env vars defined in profiles when using Windows subsystem (INCLUDE, msys2) #15908

Open
1 task done
SoShiny opened this issue Mar 20, 2024 · 4 comments · May be fixed by #15957
Open
1 task done
Assignees

Comments

@SoShiny
Copy link

SoShiny commented Mar 20, 2024

What is your question?

Hello,

This question/discussion is about passing PATH-like environment variables defined in the profile to a build in a Windows subsystem and the issues I had with it. I'd be happy about any pointers or comments you might have on this.
For the following I used Conan 2.3.0-dev (but 2.1.0 behaves the same) and Windows 11.

Observation

I am using Msys2 as a tool_requires which seems to be the go-to subsystem used in the CCI, but similar things will probably apply to WSL2 or Cygwin.

Msys2 can, and the CCI package does, inherit Window's PATH variable, and automatically converts it into a Unix-like path.
Other environment variables which are path-like, like INCLUDE, are not automatically converted (does not seem to be an option).

Adding to the INCLUDE variable via the Conan profile with INCLUDE+=(path)C:\a will lead to an INCLUDE variable which is a mix of Unix- and Windows paths and separators, which neither Msys2 nor Windows programs understands.
Adding to the INCLUDE variable with INCLUDE+=C:\a; will introduce a space and also break it.

I encountered this when trying to manage the use of a Fortran compiler of a particular version via the profile, but I can imagine this being an issue for less esoteric use cases.

Possible solution

A modification to the ProfileEnvironment to allow for something like a INCLUDE+=(win_path)C:\a option which will use the Environment.append with ; as the separator argument. Or, some more general syntax to replace the default separator (space) of the prepend and append methods.

Minimal example

For demonstration purposes, the Conan-invoking shell has the PATH=C:\SYSTEM\BIN1;C:\SYSTEM\BIN2; and INCLUDE=C:\SYSTEM\INCLUDE1;C:\SYSTEM\INCLUDE2; defined (simulates, e.g., VCVars).

Using the following default profile

[settings]
arch=x86_64
os=Windows

[buildenv]
mixed_path_issue/*:PATH=+(path)C:\PREPEND\BINDIR
mixed_path_issue/*:PATH+=(path)C:\APPEND\BINDIR

mixed_path_issue/*:INCLUDE=+(path)C:\PREPEND\INCLUDE
mixed_path_issue/*:INCLUDE+=(path)C:\APPEND\INCLUDE

and the conanfile.py

import textwrap
from conan import ConanFile
from conan.errors import ConanInvalidConfiguration


class MixedPathIssueConan(ConanFile):
    name = "mixed_path_issue"
    version = "1.0.0"
    settings = ("os", "arch")
    options = {"use_subsystem": [True, False]}
    default_options = {"use_subsystem": True}
    generators = ("VirtualBuildEnv",)

    def validate(self):
        if self.settings.os != "Windows":
            raise ConanInvalidConfiguration("It's about Windows subsystems.")

    def build_requirements(self):
        if self.options.get_safe("use_subsystem"):
            self.win_bash = True
            self.tool_requires("msys2/cci.latest")

    def build(self):
        if self.options.get_safe("use_subsystem"):
            self.run('echo "PATH=$PATH"')
            self.run('echo "INCLUDE=$INCLUDE"')
        else:
            with open("envinfo.bat", "w") as f:
                f.write(
                    textwrap.dedent(
                        """
                        @echo off
                        echo "PATH=%PATH%"
                        echo "INCLUDE=%INCLUDE%"
                        """
                    )
                )
            self.run("envinfo.bat")

prints the PATH and INCLUDE values used when running the Conan create command. The option use_subsystem switches from plain old CMD to Msys2.

Using the subsystem we end up with a good PATH, but broken INCLUDE:

conan create . -pr:a default -o mixed_path_issue/*:use_subsystem=True
PATH=/c/prepend/bindir:[...]:/c/SYSTEM/BIN1:/c/SYSTEM/BIN2:[...]:/c/append/bindir
INCLUDE=/c/prepend/include:C:\SYSTEM\INCLUDE1;C:\SYSTEM\INCLUDE2:/c/append/include

Not using the subsystem yields the expected result:

conan create . -pr:a default -o mixed_path_issue/*:use_subsystem=False
PATH=C:\PREPEND\BINDIR;C:\SYSTEM\BIN1;C:\SYSTEM\BIN2;[...];;C:\APPEND\BINDIR
INCLUDE=C:\PREPEND\INCLUDE;C:\SYSTEM\INCLUDE1;C:\SYSTEM\INCLUDE2;;C:\APPEND\INCLUDE

edit 1: The example previously confused appending with prepending (cosmetic edit, not relevant for issue)

Have you read the CONTRIBUTING guide?

  • I've read the CONTRIBUTING guide
@memsharded memsharded self-assigned this Mar 20, 2024
@memsharded
Copy link
Member

Hi @SoShiny

Thanks for your question, and thanks specially for the very good reproducible code, it really helped.

I have been playing with it a bit, and checking what is the root cause and if something could be done.

If you check inside the cache build folder, or if you build it locally with:

$ conan build . -pr:a profile -o use_subsystem=True

Then you will be able to see in the conanbuildenv-x86_64.sh:

export INCLUDE="/c/append/include:$INCLUDE:/c/prepend/include"
export PATH="/c/append/bindir:/c/users/user/msys27f2f094a41efb/p/bin/msys64/usr/bin:$PATH:/c/prepend/bindir"

And this is the file that conan is activating in self.run().

Then, no matter how the profile is defined, because the shell defined variables defined prior to Conan are not processed at all, just a $INCLUDE placeholder, that will be processed to whatever value is defined in the shell. It happens that the PATH one is automatically converted by the system, but it is not Conan that is changing it.

So this seems outside of what Conan can fix or change, and defining the INCLUDE env-var in the shell with the right format would be the way to go. Of course, it wouldn't be an issue if those values would be defined in the profile instead of defined in the shell before Conan.

Please let me know if this makes sense, thanks!

@SoShiny
Copy link
Author

SoShiny commented Mar 21, 2024

Hello @memsharded,

Thank you for responding in such a quick manner.

The point I am trying to make is exactly about how the export statements in the conanbuildenv-x86_64.sh are generated, and the lacking fine control over it.

Let me try to clarify this with a modified example.

Modified example

Let's first expand the default profile to use MSVC (v2022 in my case), and add a custom LIBPATH for the package using the append and prepend options without the (path).

[settings]
arch=x86_64
build_type=Release
compiler=msvc
compiler.cppstd=14
compiler.runtime=dynamic
compiler.version=193
os=Windows

[buildenv]
mixed_path_issue/*:PATH=+(path)C:\PREPEND\BINDIR
mixed_path_issue/*:PATH+=(path)C:\APPEND\BINDIR

mixed_path_issue/*:INCLUDE=+(path)C:\PREPEND\INCLUDE
mixed_path_issue/*:INCLUDE+=(path)C:\APPEND\INCLUDE

mixed_path_issue/*:LIBPATH=+C:\PREPEND\LIBPATH
mixed_path_issue/*:LIBPATH+=C:\APPEND\LIBPATH

Next we add the VCVars generator to the example conanfile.py, and also the compiler, and build_type setting (otherwise VCVars does not work). We also add prints for LIBPATH.

import textwrap
from conan import ConanFile
from conan.errors import ConanInvalidConfiguration


class MixedPathIssueConan(ConanFile):
    name = "mixed_path_issue"
    version = "1.0.0"
    settings = ("os", "compiler", "build_type", "arch")
    options = {"use_subsystem": [True, False]}
    default_options = {"use_subsystem": True}
    generators = ("VirtualBuildEnv", "VCVars")

    def validate(self):
        if self.settings.os != "Windows":
            raise ConanInvalidConfiguration("It's about Windows subsystems.")

    def build_requirements(self):
        if self.options.get_safe("use_subsystem"):
            self.win_bash = True
            self.tool_requires("msys2/cci.latest")

    def build(self):
        if self.options.get_safe("use_subsystem"):
            self.run('echo "PATH=$PATH"')
            self.run('echo "INCLUDE=$INCLUDE"')
            self.run('echo "LIBPATH=$LIBPATH"')
        else:
            with open("envinfo.bat", "w") as f:
                f.write(
                    textwrap.dedent(
                        """
                        @echo off
                        echo "PATH=%PATH%"
                        echo "INCLUDE=%INCLUDE%"
                        echo "LIBPATH=%LIBPATH%"
                        """
                    )
                )
            # "set var=value && echo %value%" does not work as one would expect in CMD
            # so I spin up another CMD.
            self.run("envinfo.bat")

Now, we don't set any additional PATH, INCLUDE, or LIBPATH variables manually in the Conan-invoking shell.
We will see that, we still end up with a broken INCLUDE variable in the subsystem, as it is in mixed Unix- and Windows-PATH notation (breaking the MSVC integration). Additionally, the LIBPATH is broken, because the additional paths were joined with spaces instead of some separator.

The generated conanbuildenv-x86_64.sh is nearly he same as before, just adding an export for LIBPATH.
The sourcing of the vcvarsall.bat happens before we enter into the subsystem (conanvcvars.bat is sourced in CMD).

conan build . -pr:a default -o mixed_path_issue/*:use_subsystem=True

Prints a proper PATH in Unix-format, because the $PATH was automatically converted by msys2, but $INCLUDE and $LIBPATH are not converted by msys2.

INCLUDE=/c/prepend/include:C:\Program Files (x86)\Microsoft Visual Studio\2022\BuildTools\VC\Tools\MSVC\14.34.31933\include;C:\Program Files (x86)\Microsoft Visual Studio\2022\BuildTools\VC\Tools\MSVC\14.34.31933\ATLMFC\include;C:\Program Files (x86)\Microsoft Visual Studio\2022\BuildTools\VC\Auxiliary\VS\include;C:\Program Files (x86)\Windows Kits\10\include\10.0.22621.0\ucrt;C:\Program Files (x86)\Windows Kits\10\\include\10.0.22621.0\\um;C:\Program Files (x86)\Windows Kits\10\\include\10.0.22621.0\\shared;C:\Program Files (x86)\Windows Kits\10\\include\10.0.22621.0\\winrt;C:\Program Files (x86)\Windows Kits\10\\include\10.0.22621.0\\cppwinrt:/c/append/include

LIBPATH=C:\PREPEND\LIBPATH C:\Program Files (x86)\Microsoft Visual Studio\2022\BuildTools\VC\Tools\MSVC\14.34.31933\ATLMFC\lib\x64;C:\Program Files (x86)\Microsoft Visual Studio\2022\BuildTools\VC\Tools\MSVC\14.34.31933\lib\x64;C:\Program Files (x86)\Microsoft Visual Studio\2022\BuildTools\VC\Tools\MSVC\14.34.31933\lib\x86\store\references;C:\Program Files (x86)\Windows Kits\10\UnionMetadata\10.0.22621.0;C:\Program Files (x86)\Windows Kits\10\References\10.0.22621.0;C:\Windows\Microsoft.NET\Framework64\v4.0.30319 C:\APPEND\LIBPATH

The problem

It is a common enough workflow to build something in a Windows subsystem using the MSVC compilers, and Conan does a good job allowing for that. The problem is that there is no way to add to the Windows-path-like variables, like INCLUDE and LIBPATH, using the profile which will work in the subsystem.

Possible solution

As we see in the output for LIBPATH in the example above, adding to LIBPATH nearly worked. If we just would be able to change the default joining character manually.
Instead of mixed_path_issue/*:LIBPATH+=C:\APPEND\LIBPATH we could write mixed_path_issue/*:LIBPATH+=(sep=;)C:\APPEND\LIBPATH to allow full control over how the appending glues the values together.

I believe, it would also be a rather simple code change in Conan. It would amount to adding to the logic in the ProfileEnvironment class.

If this a desired thing, I could attempt a merge request.

@memsharded
Copy link
Member

Thanks for the feedback.

I have been thinking about this, and to be honest, I am not sure yet what would be the best solution.

I think that there are cases where the real subsystem path, like /c/... will be needed, and there is not solution for that.

The approach of handling env-vars that are paths with a custom separator, instead of (path) sounds a bit more like a workaround rather than a real solution. I am not opposing to it, because I haven't been able to find any other solution. I have tried:

  • A reasonable way of having the msys2, specifically the launcher to convert the variables with cygpath to the desired format. This sounds like it should be the best solution, do exactly the same it does to PATH to the variables that we want. Unfortunately this doesn't seem neither built-in, nor easy in the launcher
  • To have a parametrization in Environment Conan class to decide if a path shouldn't be converted to the subsystem format.

I can't guarantee that it will be moved forward, but if you think it is relatively easy, please send a pull request with your proposal, and we will evaluate the risks and the impact on codebase maintainability. Many thanks!

@SoShiny
Copy link
Author

SoShiny commented Mar 26, 2024

Alright, I opened a pull request without documentation changes, as this is for evaluation.

With those changes the above example can be coerced to work by using a profile of the following form:

[settings]
arch=x86_64
build_type=Release
compiler=msvc
compiler.cppstd=14
compiler.runtime=dynamic
compiler.version=193
os=Windows

[buildenv]
mixed_path_issue/*:PATH=+(path)C:\PREPEND\BINDIR
mixed_path_issue/*:PATH+=(path)C:\APPEND\BINDIR

mixed_path_issue/*:INCLUDE=+(sep=;)C:\PREPEND\INCLUDE
mixed_path_issue/*:INCLUDE+=(sep=;)C:\APPEND\INCLUDE

mixed_path_issue/*:LIBPATH=+(sep=;)C:\PREPEND\LIBPATH
mixed_path_issue/*:LIBPATH+=(sep=;)C:\APPEND\LIBPATH

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants