-
Notifications
You must be signed in to change notification settings - Fork 986
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
Feature/msvc #8201
Feature/msvc #8201
Conversation
If it is possible I would add the compatibility by default to the Visual Studio compiler if we want to encourage users to move to this |
is it possible to use a trick similar to Intel's |
yes, something similar to that in package ID but it should be applied by default in conan 1.X IMO |
to clarify, are we proposing one-way (one-directional) compatibility layer?
will be able to use packages from:
but not vice versa |
return | ||
|
||
compatible = self.clone() | ||
version = compatible.settings.compiler.version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
version = compatible.settings.compiler.version | |
version = compatible.settings.get_safe("compiler.version") |
|
||
compatible = self.clone() | ||
version = compatible.settings.compiler.version | ||
runtime = compatible.settings.compiler.runtime |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
runtime = compatible.settings.compiler.runtime | |
runtime = compatible.settings.get_safe("compiler.runtime") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nop, at this point, using the msvc
compiler, the compiler.runtime
must be defined, otherwise I want it to raise, not to continue silently. Why would you want/allow this to be undefined here? It is not possible in the settings (it doesn't have a None
value)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's very possible settings.yml
might be modified by the user, either manually, or by running conan config install
. user may add None value or remove compiler.runtime
sub-setting completely. IMO code should be reliable for custom settings, as we're allowing them and promote it as a first-class feature (especially for Enterprise users).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So far this would be an unsupported case, the msvc
compiler always has a runtime
, it is not possible to compile without it. What does a None
value means? What is the use case? It is important to code new features initially more restrictive/limited scope, and learn from use cases, better than be relaxed, allow use cases that we completely ignore and that might have other implications. I strongly prefer to evolve from the basic "canonical" use case, and account for variants and special use cases later.
compatible = self.clone() | ||
version = compatible.settings.compiler.version | ||
runtime = compatible.settings.compiler.runtime | ||
runtime_type = compatible.settings.compiler.runtime_type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
runtime_type = compatible.settings.compiler.runtime_type | |
runtime_type = compatible.settings.get_safe("compiler.runtime_type") |
_visuals = {'19.0': '14', | ||
'19.1': '15', | ||
'19.2': '16'} | ||
compatible.settings.compiler.version = _visuals[version] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
may raise, need a sanity check (maybe deserves a test)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, this is a preliminary implementation, this needs to be improved.
runtime_type = compatible.settings.compiler.runtime_type | ||
|
||
compatible.settings.compiler = "Visual Studio" | ||
version = str(version)[:4] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
may raise, need a sanity check (maybe deserves a test)
@@ -48,6 +48,15 @@ def get_generator(conanfile): | |||
|
|||
compiler = conanfile.settings.get_safe("compiler") | |||
compiler_version = conanfile.settings.get_safe("compiler.version") | |||
|
|||
if compiler == "msvc": | |||
version = compiler_version[:4] # Remove the latest version number 19.1X if existing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this line (and the line below) may raise, need sanity checks (and maybe tests)
@@ -15,6 +15,14 @@ def _check_cppstd(settings): | |||
cppstd = settings.get_safe("cppstd") | |||
compiler_cppstd = settings.get_safe("compiler.cppstd") | |||
|
|||
if compiler == "msvc": | |||
# This is hardcoded at the moment, because the minimum defined version is 19.0, with c++14 | |||
# TODO: When older versions of msvc are defined in settings, cppstd can be None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# TODO: When older versions of msvc are defined in settings, cppstd can be None | |
# TODO: Decide if we want to force the user to provide the 'cppstd' value always |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have better removed the None value in settings leaving: cppstd: [14, 17, 20]
. Not defining it will raise an error. Current design was especulative, assuming older VS versions. For the current versions cppstd: [14, 17, 20]
is the correct definition, lets wait to handed this in the settings_preprocessor until we really have the problem because defining older versions.
In that case we might want to add something like cppstd: [undefined, 14, 17, 20]
instead of None
, in this way, it is still explicit and doesn't allow not defining it. The internal helpers like cppstd_flag
should account for this, but this can wait.
1929->19.29
1929->19.29 protobuf:shared=True compiler.cppstd=14
1929->19.29 protobuf:shared=True compiler.cppstd=14 MT
1929->19.29 protobuf:shared=True compiler.cppstd=14 MT 14cppstd
[no ci] conan-io/conan#8201 conan-io/conan#3311 1929->19.29 protobuf:shared=True compiler.cppstd=14 MT 14cppstd [no ci] conan profile compiler=msvc
Changelog: Feature: Add new
msvc
compiler setting and preliminary support inconan.tools.cmake
generator and toolchain.Docs: conan-io/docs#1991
Close #3573
It is providing a
compatible_packages
approach to compatibility with Visual Studio compiler.conan.tools.microsoft
following in a different PR