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

[feature] tools.microsoft.msbuild:vs_version should be included in the default profile detect #14487

Closed
1 task done
p-groarke opened this issue Aug 15, 2023 · 9 comments · Fixed by #14578
Closed
1 task done

Comments

@p-groarke
Copy link

p-groarke commented Aug 15, 2023

What is your suggestion?

In conan 2, the new way of selecting an msvc toolset ABI is through compiler.version. Selecting the "build" msvc version is managed through [conf] tools.microsoft.msbuild:vs_version.

This tools.microsoft.msbuild:vs_version setting should be initialized and defaulted when running profile detect. It should be set at the same version as compiler.version by default. The "detected" visual studio, whatever the case may be.

So for example, in a VS 2022 context, I would expect running a conan profile detect to contain both.

compiler.version=193

[conf]
tools.microsoft.msbuild:vs_version=17

This brings back feature parity with conan 1, it allows flexibility and an "out-of-the-box" conan experience. Where the detected host msvc version doesn't need to be set by the callee. This is important, as vs_version isn't something easily controlled. It isn't something that needs to be controlled either, only the toolset matters.

[edit] This allows conan to run in multiple environments, not easily controlled (see, different teams, different products), all the while ensuring that as long as the selected toolset is installed, our recipes will work.

In addition, these are appropriate defaults. They do not change the current conan 2 behavior. Everything should work as before.

@jcar87 As per our discussion. Let me know if you need more details.

Have you read the CONTRIBUTING guide?

  • I've read the CONTRIBUTING guide
@memsharded
Copy link
Member

memsharded commented Aug 15, 2023

Hi @p-groarke

Thanks for the suggestion.

I'd like to clarify, just in case, that the default value for tools.microsoft.msbuild:vs_version is already the one matching the compiler version. So it is not necessary to add it to the profile, because it would be redundant. Only in the case that users wanted to use a different older toolset with a more modern IDE (a non default toolset version) it is necessary to set tools.microsoft.msbuild:vs_version, but this would be by definition an impossible to automatically detect case.

So I think defining this conf would not be needed, and it would only clutter the profiles, because it is already the default value. And the rule is that detected profiles should be minimal, otherwise the majority of users think that this conf is mandatory and are not happy that they think this has to be set, also for manually defined profiles.

@p-groarke
Copy link
Author

p-groarke commented Aug 15, 2023

Hey @memsharded , yes that's exactly the use case I was discussing with Julius. We rarely run the same VS version as the target toolset version.

On CI, it's always a more recent VS generating cached binaries to match a given ABI (ex, VS 2019 generating v142, v141, VS 2022 generating v143, v142, v141).

On dev PCs, any VS version goes as long as its >= than the targeted toolset. It really doesn't matter what VS version you use, ABI wise only the toolset matters.

By omitting this conf option from the detect profile, we can't automate this workflow. Configs we ship must only enforce toolsets (now compiler.version), not vs_version.

Doing it this way (ala conan 1) decouples our recipes from VS versions. It allows multiple teams, multiple CI images, multiple configs to work without any intervention. Using the conan 2 way, we'll have to implement some sort of logic to detect the VS version. This logic will need to be duplicated in all scenarios where we use conan. It's a pain, for something that's already builtin conan and should work out-of-the box.

Another way to think about this; conan 2 has it backwards. Detected profiles should probably only contain vs_version, and compiler.version can be passed at build time (or assumed the shipping toolset of vs_version).

[edit] Unless I'm missing something of course :)

@memsharded
Copy link
Member

Thanks for the feedback. I am afraid I might be missing something from the conversation you mention.

If you rely on the automatically detected profile (something not recommended in the docs in https://docs.conan.io/2/knowledge/guidelines.html and https://docs.conan.io/2/introduction.html#stable), then the compiler version and the IDE version will match, it is not possible that they diverge (Unless you are installing a more modern VS IDE without the matching default compiler and only installing an older toolset in it, is that the use case?)

I'll try to sync with @jcar87 to see what can I be missing from your conversation.

In the meantime, I'd like to suggest a potential alternative way of thinking: devs can set tools.microsoft.msbuild:vs_version in their global.conf file, as their default and that will apply to all profiles too, not only the default one, which might be very convenient. If there is variability among developers in the team, it means that it doesn't really belong to profiles, a "profile" is mostly a consensus or shared configuration among developers in a team and CI machines. If it is not something that devs agree "we all use VS 2022", then it is something that looks more like their own private conf than profile configuration

@memsharded
Copy link
Member

Hi @p-groarke

We are considering the following approach, which could provide both the desired behavior for your case and extended flexibility for other users as well:

  • Implement a conan_detect api
  • Inject the api to the rendering of the profiles and global.conf

In this way, you can define your profiles as:

[settings]
compiler.version=191

[conf]
tools.microsoft.msbuild:vs_version={{conan_detect.vs_ide_version}}

Explicitly defining it. This can have some side advantages as well, for example, it keeps working when installing a new VS IDE version, automatically using the new IDE. It can also completely eliminate the need to include(default) profile, which can be advantageous, and makes explicit in your profiles that you are relying on the autodetected current VS IDE in the developer machine. You will typically drop it in the "production" CI profiles.

As suggested above this approach could also work defining such value in the global.conf of the developers, so it is automatically applied to every profile.

Please let me know what you think.

@p-groarke
Copy link
Author

Hey @memsharded , I'm off for vacation tackling the fishies 🎣 so my responses will be sparse.

That sounds good :) conan_detect approach seems to fulfill the workflow (automagic detection of vs_version). I'll keep you in the loop once this starts getting tested.

Another thought I had : Lets say you only have 1 vs version installed, and we change compiler.version manually (to back-compile to an older toolset). Maybe that could automatically back-compile?

Anyhow, good day

@memsharded
Copy link
Member

That sounds good :) conan_detect approach seems to fulfill the workflow (automagic detection of vs_version). I'll keep you in the loop once this starts getting tested.

Fantastic. We will start to move forward the implementation of this new api

Another thought I had : Lets say you only have 1 vs version installed, and we change compiler.version manually (to back-compile to an older toolset). Maybe that could automatically back-compile?

Not sure what you mean here, could you please clarify?

Thanks!

@memsharded
Copy link
Member

Initial proposal to kick-off the technical and design discussions in #14578

@memsharded
Copy link
Member

Closed by #14578, it is now possible to do in profiles with jinja2 syntax:

{% set compiler, version = detect_api.detect_compiler() %}
{% set runtime, _ = detect_api.default_msvc_runtime(compiler) %}

[settings]
compiler={{compiler}}
compiler.version={{detect_api.default_compiler_version(compiler, version)}}
compiler.runtime={{runtime}}
compiler.cppstd={{detect_api.default_cppstd(compiler, version)}}

[conf]
tools.microsoft.msbuild:vs_version={{detect_api.default_msvc_ide_version(version)}}

@p-groarke
Copy link
Author

@memsharded Wow that was a quick turnaround. Thank you so much!

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

Successfully merging a pull request may close this issue.

3 participants