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.

Sign up for GitHub

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

Fix build scope OS detection in tools.microsoft.visual.VCVars #15568

Merged
merged 4 commits into from
Feb 6, 2024
Merged

Fix build scope OS detection in tools.microsoft.visual.VCVars #15568

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c6fa5f3
Fix build scope OS detection in tools.microsoft.visual.VCVars
ravenexp Jan 31, 2024
25ac0f9
Merge branch 'develop2' into fix-vcvars-build-os-detect
memsharded Feb 5, 2024
46924e7
fix: Change the VCVars build OS detection method
ravenexp Feb 5, 2024
659260c
test: Add an integration test for the VCVars build OS detection
ravenexp Feb 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions conan/tools/microsoft/visual.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

from conan.internal import check_duplicated_generator
from conans.client.conf.detect_vs import vs_installation_path
from conans.client.subsystems import deduce_subsystem, WINDOWS
from conan.errors import ConanException, ConanInvalidConfiguration
from conan.tools.scm import Version
from conan.tools.intel.intel_cc import IntelCC
Expand Down Expand Up @@ -100,8 +101,10 @@ def generate(self, scope="build"):
"""
check_duplicated_generator(self, self._conanfile)
conanfile = self._conanfile
os_ = conanfile.settings.get_safe("os")
if os_ != "Windows":

# Derive subsystem name from the current scope.
subsystem = deduce_subsystem(conanfile, scope)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if subsystem != WINDOWS:
return

compiler = conanfile.settings.get_safe("compiler")
Expand Down