conan-io / conan

Conan - The open-source C and C++ package manager
https://conan.io
MIT License
8.14k stars 970 forks source link

[feature] Expose `vs_ide_version()` in conan.tools.microsoft #11158

Closed SpaceIm closed 2 years ago

SpaceIm commented 2 years ago

vs_ide_version in visual.py is useful for recipes aiming to support both legacy compiler=Visual Studio and new compiler=msvc during conan v2 transition.

I've started to use it in few conan-center recipes with from conan.tools.microsoft.visual import vs_ide_version, but it would make sense to import this function the same way than is_msvc, is_msvc_static_runtime etc

In grpc recipe for example we could write something like this:

    def validate(self):
        if is_msvc(self) and tools.Version(vs_ide_version(self)) < "14":
            raise ConanInvalidConfiguration("gRPC can only be built with Visual Studio 2015 or higher.")
memsharded commented 2 years ago

I think this is a fair request, to help with the transition from 1.X->2.0 an abstraction for the version of VS is necessary to coexist msvc and Visual Studio settings.

memsharded commented 2 years ago

In any case, please don't use non documented imports in conan.tools.xxxx space. Basically every that cannot be imported with something like from conan.tools.xxx import yyy shouldn't be used in recipes, and can change at any time.

SpaceIm commented 2 years ago

Ok. For the record there are currently 3 recipes in conan-center using from conan.tools.microsoft.visual: grpc, libvpx & xapian-core. They import msvc_version_to_vs_ide_version, but actually to compute the same thing than vs_ide_version.

memsharded commented 2 years ago

@SpaceIm I am having a look at this, and I am thinking making this public will not help for this. This is the implementation:

    if compiler == "msvc":
        toolset_override = conanfile.conf.get("tools.microsoft.msbuild:vs_version", check_type=str)
        if toolset_override:
            visual_version = toolset_override
        else:
            visual_version = msvc_version_to_vs_ide_version(compiler_version)

It means that the version that it is returning is not the compiler one, which is the one that recipes want to check in the general case to validate() compiler support. In 2.0 ready tools, the IDE version has been decoupled, so it is possible that the IDE version is higher, but by using an older toolset the compiler version is still not enough to compile the thing.

SpaceIm commented 2 years ago

Would it be possible to provide an helper function returning standard IDE version associated with a given cl version, so that we can smoothly support both compiler=Visual Studio & compiler=msvc during conan v1 => v2 migration? Maybe it would be misleading for conan v2.

As I said for the migration, something like this:

    def validate(self):
        if self.settings.compiler == "Visual Studio" and tools.Version(self.settings.compiler.version) < "14":
            raise ConanInvalidConfiguration("gRPC can only be built with Visual Studio 2015 or higher.")

would become something like this:

    def validate(self):
        if is_msvc(self) and vs_ide_version_conan_v1(self) < "14":
            raise ConanInvalidConfiguration("gRPC can only be built with Visual Studio 2015 or higher.")

Maybe it could be the opposite: an helper function returning cl version for compiler=Visual Studio & compiler=msvc:

    def validate(self):
        if is_msvc(self) and vs_cl_version(self) < "190":
            raise ConanInvalidConfiguration("gRPC can only be built with Visual Studio 2015 or higher.")

But can we trust in conan-center stability of this cl version scheme?

memsharded commented 2 years ago

Yes, I think the compiler-version based approach if is_msvc(self) and vs_cl_version(self) < "190": would be the most aligned with Conan 2.0 (recall that in Conan 2.0, we use conan install . -s compiler=msvc -s compiler.version=191)

memsharded commented 2 years ago

Proposing a checker for more convenience: https://github.com/conan-io/conan/pull/11292

SpaceIm commented 2 years ago

thanks

SpaceIm commented 1 year ago

Actually, check_min_vs is too specific, it doesn't help when we want to test versions without raising. vs_ide_version is more versatile.

How to translate something like this?

if self.settings.compiler == "Visual Studio":
    msvc_folder = "vs2017" if Version(self.settings.compiler) >= "15" else "vs2013"
    [...]

Currently:

if is_msvc(self):
    if (self.settings.compiler == "Visual Studio" and Version(self.settings.compiler) >= "15") or \
       (self.settings.compiler == "msvc" and Version(self.settings.compiler) >= "191"):
        msvc_folder = "vs2017"
    else:
        msvc_folder = "vs2013"
    [...]

I would prefer

if is_msvc(self):
    msvc_folder = "vs2017" if vs_ide_version(self) >= "15" else "vs2013"
    [...]

Even when we want to raise, check_min_vs is not used in conan-center because usually we have more generic logic to handle all compilers min compatible version in a dictionary.

AbrilRBS commented 1 year ago

Regarding your last point @SpaceIm, #12880 should help with tha once merged and available in CCI :)

SpaceIm commented 1 year ago

@RubenRBS I appreciate, but vs_ide_version is more versatile. https://github.com/conan-io/conan/pull/12880 doesn't help for cases like https://github.com/conan-io/conan-center-index/pull/15265.