Fix build scope OS detection in tools.microsoft.visual.VCVars #15568
Conversation
Get the build scope OS type using the common subsystems API.
Do not assume that the host profile OS is the same as
the build profile OS and do not generate "conanvcvars.bat"
on Unix build systems.
The same method of build OS detection is already used in
tools.env.EnvVars to generate the appropriate virtual environment
script type and in tools.cmake.toolchain to write correct file paths.
This change makes cross-compilation from Linux to Windows work
when using MSVC compilers running natively on Linux.
The original cross-compilation failure looks like:
ConanException: Cannot wrap command with different envs,['/build/Debug/generators/conanbuild.bat'] - ['/build/Debug/generators/conanbuild.sh']
and is caused by conanvcvars.bat script being generated unconditionally
and included into conanbuild.bat even when running on Linux.
|
|
There was a problem hiding this comment.
Thanks for your contribution.
While it seems it makes sense, I am concerned that this could have some risks, so we have to check it carefully.
The issue is that some Windows subsystems might need the VCVars generator if using msvc. And it is possible that these cases are not sufficiently covered by the test suite, so better double check them.
conan/tools/microsoft/visual.py
Outdated
| if os_ != "Windows": | ||
|
|
||
| # Derive subsystem name from the current scope. | ||
| subsystem = deduce_subsystem(conanfile, scope) |
There was a problem hiding this comment.
I have recovered some tests that were pending to migrate after 1.X -> 2.0 migration, and that test will reveal that this is breaking, actually breaking a lot of users that build with Windows subsystems and msvc as compiler.
I think we need to keep the conanfile.settings.get_safe("os") == "Windows", and in any case we could consider adding the conanfile.settings_build.get_safe("os")==Windows as an extra condition to avoid the Linux->Windows cross-build issue
There was a problem hiding this comment.
I'll change the patch to additionally check conanfile.settings_build.os instead, as you suggest.
I'm just wondering why deduce_subsystem() fails in this case, given that all other generators except VCVars use this function instead of inspecting the settings directly. This could point at a hidden problem in deduce_subsystem() itself.
There was a problem hiding this comment.
The problem is that deduce_subsystem() returning msys2 and still compiling with msvc and using vcvars.bat is a valid scenario. VCVars().generate() should create a conanvcvars.bat for this case too. Other generators are using the deduce_subsystem() mostly to adapt paths, but not to decide if the generator is used or not.
Use `conanfile.settings_build.os` in place of `deduce_subsystem()`.
|
I've added a new commit applying suggested changes. Should I squash and rebase on |
Not necessary, we squash at merge time. |
There was a problem hiding this comment.
Looks good now, do you think you could also add a test that covers this scenario?
It would be important to keep this working in the future and not break it.
The vcvars_test.py looks a good place to add it, I guess some test that explicitly -s:b os=Linux -s:h os=Windows ... would be good (check that the test fails without the fix). The tests in vcvars_test.py should serve as inspiration, but please let me know if you need help. Thanks!
Does this mean I should add an XFAIL test before the fix commit, then apply the fix commit, and then require the test to succeed? |
Not necessary, just check it locally, to make sure it fails, before committing, then apply your fix, and see it pass, then commit and push. Red-green strategy, just to make sure the test is fully covering the fix, but just ran locally while developing the test. |
Check that conanvcvars.bat is not created on Linux build systems.
|
I've checked locally on a Linux system that the new integration test fails without the fix and passes after the fix is re-applied. The test is only run on Linux systems. |
|
Thanks very much. This fix will be included in next release (2.1) |
Get the build scope OS type using the common subsystems API. Do not assume that the host profile OS is the same as the build profile OS and do not generate "conanvcvars.bat" on Unix build systems.
The same method of build OS detection is already used in tools.env.EnvVars to generate the appropriate virtual environment script type and in tools.cmake.toolchain to write correct file paths.
This change makes cross-compilation from Linux to Windows work when using MSVC compilers running natively on Linux.
The original cross-compilation failure looks like:
and is caused by conanvcvars.bat script being generated unconditionally and included into conanbuild.bat even when running on Linux.
Changelog: Bugfix: Fix build scope OS detection in
tools.microsoft.visual.VCVars.Docs: Omit