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

removing toolset from msbuild generator and toolchain #7825

Merged

Conversation

memsharded
Copy link
Member

@memsharded memsharded commented Oct 6, 2020

Changelog: Fix: Remove toolset variability from the msbuild generator and MSBuildToolchain.
Docs: Omit

From: #7824

@memsharded memsharded added this to the 1.31 milestone Oct 6, 2020
@memsharded memsharded requested review from jgsogo and SSE4 October 6, 2020 13:10
client.out)
vs_path = vs_installation_path("15")
vcvars_path = os.path.join(vs_path, "VC/Auxiliary/Build/vcvarsall.bat")

# FIXME: This is cheating, pass the toolset on the command line, nothing that devs would do
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure I understand what is cheating here? I guess it's legal to pass toolset on command line, and developers can do that, correct?

Copy link
Member Author

@memsharded memsharded Oct 6, 2020

Choose a reason for hiding this comment

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

99% of developers will double click on the solution to open it, not build it in the command line with msbuild command, don't you think? It is not that it is not doable, but that it will fit in the developers flow. Building from the command line for Visual definitely doesn't fit a vast majority of cases IMHO.

Copy link
Contributor

Choose a reason for hiding this comment

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

@solvingj WDYT?
I understand, it might not be common for majority of users, but still possible and legal.
still I believe most of CI scripts invoke msbuild or devenv, so it should be supported case IMO.

Copy link
Member Author

Choose a reason for hiding this comment

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

It will be managed by the toolchain, so still very possible in CI. The only thing is that it will not be possible to build different toolset configurations in the same run, with the same build. You can do different conan install with different toolsets and still build with them, just not build sequentially for different toolsets.

What we are really trying to avoid as much as possible here is to have something that is done only in CI, but developers cannot easily replicate. The problem is that if we add the toolset variability to the generated files, we need the build helper to pass the toolset definition in the command line. If the build helper passes that definition in the command line, then developers are no longer able to reproduce the build from their IDE, which is a major goal for this integration approach. We cannot have a build from the build() method that a developer cannot achieve as a consumer, with their native tools.

Copy link
Contributor

Choose a reason for hiding this comment

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

so, if toolchain from msbuild is in general a .props file, then:

  1. conan flow runs msbuild project.sln -p conan.props in inject toolchain via command line
  2. developer locally opens project.sln via IDE and...
    2.1. developer loads conan.props via property manager into the solution?
    2.2. developer opens solution with conan.props somehow already injected by conan?
    2.3. developer needs to use command line to inject toolchain file (conan.props)

what is the best workflow here? IMO 2.1
for 2.2, I guess this is something we're going away (e.g injecting conanbuildinfo.cmake was proven to be a bad practice for many reasons).
for 2.3, if for CMake we think that command line is okay, why do we think it's not okay for msbuild then? don't we expect feature parity here? even for CMake, modern IDEs (Visual Studio, CLion, VSCode, etc.) allow to directly open CMakeLists.txt, so how would you include you toolchain in such case?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, 2.1: the flow is to add the properties file to the visual project. This can be done from the IDE or editing the .vcxproj. I think this flow has several advantages:

  • It is completely explicit, no magic.
  • The addition of the .props file needs to be done only once, as the .vcxproj is typically commited to source control
  • It can be done without command line (besides Conan commands). Note that typically msbuild is NOT in the path, so developer needs to open a matching VS-prompt. More complicated than double click on the .sln

From our experience Microsoft and Visual Studio developers are extremely reluctant to use command line, and this is already complicated for Conan adoption. Companies sysdmins/devops/build-responsibles are going to large extends to avoid their developers to touch the terminal.

We should optimize for user stories that are as close to the current dev experience as possible. I see a common user story in Msft/VS devs is:

  • Get the source code, with git clone or similar
  • Double click the .sln to open the project

Here, we aim to only introduce an extra step, the conan install of dependencies. Some will use MSBuild tasks or plugins, others will add a .bat file in the root to double click (that contains the conan install).

In the cmake user story, it is more like:

  • Get the source code, git clone
  • cmake .. something something # to create the project
  • OpenIDE, editor, build command line...

So the user is already doing the cmake command, it is very necessary, a majority of cmake users will call cmake themselves to generate the project. In this case, we should optimize so users can keep doing the cmake command as close as their current one as possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

okay, in that case, if I open solution and inject conan.props (via PropertyManager or manually), it will update RuntimeLibrary accordingly, right?
same way, I could edit property file via PropertyManager (or via text editor) and update the RuntimeLibrary to the value I need.
everything without command line, only via IDE.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, as the conan_toolchain.props is injected, doing a conan install ... -s compiler.runtime=MT or similar will update the toolchain and it will be taken into account.

same way, I could edit property file via PropertyManager (or via text editor) and update the RuntimeLibrary to the value I need.

It depends on the evaluation order. It seems that including the toolchain in the right position, it will have priority and define the final value of the toolset.

@solvingj
Copy link
Contributor

solvingj commented Oct 6, 2020

@SSE4 sorry to be a bit late to the discussion, but I will add what I can to what @memsharded said (there are some good points).

There are a few factors that distinguish these scenarios. I'll cut those factors down to what I consider the cases we're trying hardest to address with this discussion. Of note, we use the word "most" frequently and loosely because there are exceptions to every statement.

  • mostly focused on enterprise/private cases
  • supporting both local IDE flow and CLI/CI flow
  • company implementing Conan across non-trivial codebase

As @memsharded said, most enterprise IDE users assume that all necessary "Configurations" are predefined with all the valid project-specific variables included. And, most CI workflows with msbuild simply specify the building of an existing configuration. Indeed, even without Conan involved, one of the challenges companies face when passing a bunch of properties and variables to the build via the msbuild CLI during a CI job is that it becomes harder to reproduce on the user-side. Of course people still do it, and have .bat wrappers that people can run locally to reproduce CI when custom MSBuild CLI args are necessary, but it's definitely more reproducible and discoverable from Conan's perspective to bake the state into the props files on disk, and then work on the props import strategy and advice.

Regarding import strategy, in terms of real-world implementation of Conan, here is something else to think about. Most large-scale implementations will need to support a period of time where the projects can be built successfully both without Conan, and with Conan. Many will want time to verify that the new binaries from Conan builds are identical/equivalent to ones built without it, and it takes a long time to work through all possible builds with Conan. It doesn't happen over night. In these cases, most implementers will choose some strategy that involves making Conan props imports conditional. It might be based on environment variable, command-line property, or whether or not the file exists. The "else" condition will usually be to fall back to whatever existing dependency strategy they are using prior to Conan. All of that is fairly easy to do in MSBuild language, and I've talked to many implementers who have done this.

The point is that, in these specific cases I outlined above, it will be rare for users to manually import a Conan props file on the fly (as described in 2.1) for production development. There will more likely be a team implementing Conan which will gradually add conditional imports to all projects throughout the codebase, and in many cases automate the running of Conan in the background without the users intervention (similar to cmake-conan).

@memsharded
Copy link
Member Author

The point is that, in these specific cases I outlined above, it will be rare for users to manually import a Conan props file on the fly (as described in 2.1) for production development.

To clarify, as I see 2.1, the import is not on the fly. It is added by the first developer adding Conan support to the .vcxproj. Other users will just open the project from the IDE, and rely on the conditional, the automation to do the conan install or whatever. But it is not a "dynamically" added import. The import is hardcoded in the .vcxproj

@SSE4
Copy link
Contributor

SSE4 commented Oct 7, 2020

so, in summary, we want to use 2.1, but it may look like:
2.1.1. direct props import
2.1.2. conditional conan props import (check environment variable), possibly with fallback to non-conan dependencies
2.1.2. conditional conan props import (check props file exists), possibly with fallback to non-conan dependencies
2.1.3. conditional conan props import (check command line property), possibly with fallback to non-conan dependencies

here it's not important how props was imported by user - either via Property Manager, or via direct editing of .vcxproj file, or via some script or extension, in either way we will have the same result here - conan props import line is hard-coded into the .vcxproj file.

in any case, user is able to edit RuntimeLibrary property (and others) of conan props file (yes, of that specific file, or globablly, doesn't matter much). yes, it's more clicks than changing Debug->Release, but I still believe it's incorrect to call it "cheating" - it's uncommon, but still legit and documented feature available from the IDE. I would call something "cheating" only if we use something undocumented, e.g. hacking directly into the internals of Visual Studio.

do we still have any problems with it? probably, the very first developer has to inject props file and then push these changes to the source control (if they ever use source control system). what will happen to other developers who update .vcxproj file from the source control? there are few possible scenarios here:

a. conan.props is not checked in into the repository, only import line
a.1. developers just remove import line locally - might be boring for them
a.2. developers use conditional import and ignore it
a.3. developers have to run some conan command line (potentially as post-build step) to get conan.props locally generated - will it match the conan.props the very first developer had? not necessarily
b. conan.props is checked in into the repository, but not dependencies
b.1. developers get references with absolute paths to conan deps which they (potentially) have to fix somehow (e.g. via text editor)
b.2. developers get references with relative paths to conan deps which might not exist - they have to run some conan command line (potentially as post-build step) to get dependencies available - same potential problem as a.3, deps installed might not match deps the first developer installed
c. conan.props is checked in into the repository, with dependencies in relative sub-directories
c.1. usually, this should work out of the box for most of developers, except the fact most developers won't want to check in dependencies into the SCM, due to the huge size, and due to the necessity to update them from time to time

I am not sure which is the right scenario here (maybe all of them), @solvingj if you could clarify would be nice

@memsharded
Copy link
Member Author

in any case, user is able to edit RuntimeLibrary property (and others) of conan props file (yes, of that specific file, or globablly, doesn't matter much). yes, it's more clicks than changing Debug->Release, but I still believe it's incorrect to call it "cheating" - it's uncommon, but still legit and documented feature available from the IDE. I would call something "cheating" only if we use something undocumented, e.g. hacking directly into the internals of Visual Studio.

Sure, users can do a bunch of stuff, but it is impossible that we can manage any possible change that the user can do in the project.

I am not fully sure that what I am communicating succesfully what I am proposing here in this PR. Let me try to clarify:

Lets say that we have a specific toolchain file for every different conan setting:

# assume same Release Visual Studio 15 x64
$ conan install . -s compiler.toolset=v140 -s compiler.runtime=MT -s compiler.cppstd=14
# generates conan_toolchain_Release_x64_v140_MT_14.props
$ conan install . -s compiler.toolset=v141 -s compiler.runtime=MD -s compiler.cppstd=11
# generates conan_toolchain_Release_x64_v141_MD_11.props
....

Those files will have inside something like:

<ClCompile>
        <RuntimeLibrary>MultithreadedDLL</RuntimeLibrary>
        <LanguageStandard>cppstd14</LanguageStandard>
         <PlatformToolset>v141</PlatformToolset>
</ClCompile>

And these files will be included from the .vcxproj with something like:

<ImportGroup Label="PropertySheets" 
   Condition="'$(Configuration)|$(Platform)'=='Release|x64' and $(PlatformToolset)=='v140' 
    and $(LanguageStandard) == "cppstd14"....>
    <Import Project="conan_toolchain_Release_x64_v140_MT_14.props" />
</ImportGroup>

This would be completely useless, as it requires as a condition the value that it will be setting in the toolchain file.

So my proposal here is:

  • As default, conan will generate $(Configuration)|$(Platform) variability only, which are the things that the user can easily change in the IDE selectors:

    conan_toolchain_Release_x64.props
    conan_toolchain_Debug_win32.props
    ....
    

    All the other values, cppstd, runtime, toolset are constant inside the toolchain. They are defined at conan install time,
    not by the project. Exactly the same for options, preprocessor definitions or variables that the toolchain can change and
    provide a different value based on settings/options, etc.

  • There is nothing that forces VS projects to use the toolchains. This is only if they want this configuration injection by Conan, otherwise they are free to use whatever they want and change it however they want (as long as they use the right dependencies, of course)

  • Conan will allow custom configurations for $(Configuration), and the toolchain can generate, for lets say, configurations ReleaseStatic140, DebugShared141, etc:

    conan_toolchain_ReleaseStatic140_x64.props
    conan_toolchain_DebugShared141_win32.props
    ....
    

    In this case, the value of the toolset could be directly hardcoded in the main .vcxproj file as well, because there is a configuration for it, and in that case the toolset will be defined repeated in both places. But the rest of the toolchain things will be constants again.

  • I propose a similar default pattern for generator dependencies via msbuild generator, as it would be easier to understand. msbuild generator will produce by default something like:

    conan_deps_boost.props #what users can easily import
    conan_deps_boost_Release_x64.props
    conan_deps_boost_Debug_win32.props
    ....
    
  • The generators will most likely have the possibility to be configured in the same way (ongoing proposal) in the toolchain() method as well. It will then be possible to customize, and have the generators encode the desired variability, which might include listening to hardcoded values in the project configuration, producing more granular "multi" configurations:

    conan_deps_boost.props #what users can easily import
    conan_deps_boost_Release_x64_v140_MT_cppstd11.props
    conan_deps_boost_Debug_win32_v141_MD_cppstd14.props
    ....
    

Please let me know if this makes sense.

@solvingj
Copy link
Contributor

solvingj commented Oct 8, 2020

I've written two long responses (this is my third) to this PR, and then stopped short of posting them because I had significant realizations at the end each time. I started over each time. Hopefully these are my final thoughts.

Note: We probably should have started the discussion in the original issue instead of here because it had the original justification, which I only realized just now: #7824 But, it doesn't make sense to jump over there now since all the comments are here for historical purposes, so I'll continue here.

Edit: Also, after writing this, I thought it was important to come back here and add a preface. The perspectives below come from visual studio projects and solutions in private enterprise codebases. While there are open-source projects with visual studio build systems defined, they are not subject to the scalability and manageability challenges as enterprise projects, which are the focus of this conversation. They're not being ignored, but it's too difficult to talk about both cases in one discussion.

I think the important question that got lost was: "What is the benefit of adding the MSBuild Toolchain in your Conan Recipe?"

Without Toolchain
Currently, if you use the MSBuild helper inside Conan, it will take the settings.arch and settings.build_type and use them to choose the "platform/configuration". If configuration names are unique or custom, the user will have to add the logic to map Conan's build_type to the .sln or .vcxproj configurtation field. In any case, there will always a mapping process from Conan settings to Visual Studio "platform/configuration". If we just stopped there, users would be guaranteed to get all the settings exactly as they were defined in the "platform/configuration" chosen. This is actually the intuitive and desirable outcome for many users.

So, we should pause here and say that a stripped down MSBuild helper which just calls msbuild.exe and chooses/passes "platform/configuration" satisfies the "reproducibility" requirement for a large number of these cases all by itself. So, this is a good thing, and this is an option I think we should advertise as the "maximum hands-off approach". We should just make the following tradeoff very clear for users:

  • You get strong guarantee's Conan won't magically affect your build settings, only the vcxproj settings matter
    • This is maximizes intuitiveness and the ease-of-reproducibility
  • You are responsible for ensuring that your profile settings all match the vcxproj "platform/configuration" they map to
    • If there's a mismatch by mistake, the binary will be "labeled wrong" in the package id
    • but it will still be the binary expected, and trivially reproducible locally

With Toolchain
So, then the advantage of using the toolchain is for users who WANT the settings from the "conan profiles" to become the new authority on what settings make it into the build. I can imagine at least one user category which might want to do this long-term, and that is people (such as build engineers) who are managing automated builds of many projects with Conan, and always want to declare all their build configurations in Conan profiles as a "generic/declarative" source. Outside of this use cases, I think there are a lot of ad-hoc/corner cases where users might want to override whats in the .vcxproj from Conan, but whenever there's a need to run a particular build long-term, most cases would just add a configuration for it in the .vcxproj.

So, people in the position stated above can choose to use the toolchain in their recipes, with a significant, but easy to understand tradeoff:

  • You get strong guarantee's that the settings in the conan profile are the ones used by MSBuild
    • This is what you want from toolchains as a build engineer
  • You are responsible for ensuring that your profile settings all match the vcxproj "platform/configuration" they map to
    • If there's a mismatch by mistake, the binary will be "labeled correctly" in package id
    • but it will not be the binary expected, and not trivially reproducible locally

If you compare the two, you find this important conclusion:

  • The user will always have the responsibility of ensuring that the settings in the profile match the settings in the "platform/configuration" they map to.

Differences
Thus, in this design, there would be two differences between recipes which used toolchains, and those that did not:

  1. The consequences of mismatches between conan profile and "platform/configuration" are different:
    • In theory, people would probably say that mismatches in either case are unacceptible
    • In practice, mistakes happen, and this way teams can choose which consequences they would prefer to risk
  2. Only recipes with toolchains would allow ad-hoc builds which are not pre-defined as "platform/configuration" vcxproj
    • While I said this discussion should remain focused on enterprise case, I think this characteristic is distinctly valuable and relevant in the open-source recipe case, but I won't go into further detail here.

Summary
So, in summary I recommend making the toolchains provide strong guarantee about the Conan settings and force as many of them into the build as possible, and the alternative to just "not opt-in" to using them in recipes. It's a very simple story.

As a final note, I would never expect or want the compiler.runtime to be treated one way, and compiler.toolset treated a different way. This PR removes the guarantee on the PlatformToolset but leaves in the guarantee on compiler.runtime. I would want strong guarantee (inject conan settings into my build) or no guarantee (don't inject any settings into my build), not "some".

@memsharded
Copy link
Member Author

Hi @solvingj

I agree with the analysis and the conclusions. Those are 2 different modes of operation, that define different purposes and goals, and which approach is decided by users, as a tradeoff.

As a final note, I would never expect or want the compiler.runtime to be treated one way, and compiler.toolset treated a different way. This PR removes the guarantee on the PlatformToolset but leaves in the guarantee on compiler.runtime. I would want strong guarantee (inject conan settings into my build) or no guarantee (don't inject any settings into my build), not "some".

This is one of the great reasons I wanted to do this. I think there is a misunderstanding in the scope of the other variables in the current implementation. The runtime and the cppstd cannot be passed in the command line, so in the past they were generated on the fly in a temporary .props file that was passed in the command line. This PR is not removing the PlatformToolset, and leaving the cppstd and the runtime, it is exactly putting the 3 things in the exact same place. So far the toolset was configured as variable in the filename, and conditioned in the .vcxproj, while the runtime and cppstd are defined in the corresponding toolchain.props file:

            <!-- conan_toolchain_release_x64_v140.props -->
            <Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
              <ItemDefinitionGroup>
                <ClCompile>
                  <PreprocessorDefinitions>
                     {};%(PreprocessorDefinitions)
                  </PreprocessorDefinitions>
                  <RuntimeLibrary>{}</RuntimeLibrary>
                  <LanguageStandard>{}</LanguageStandard>
                </ClCompile>
              </ItemDefinitionGroup>
            </Project>

This PR is the first step, but not the complete thing yet. Next step (I am working on it on another branch, maybe I should have put everything together) is to have the toolchain.props file defining:

            <!-- conan_toolchain_release_x64.props -->
            <Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
              <ItemDefinitionGroup>
                <ClCompile>
                  <PreprocessorDefinitions>
                     {};%(PreprocessorDefinitions)
                  </PreprocessorDefinitions>
                  <RuntimeLibrary>{}</RuntimeLibrary>
                  <LanguageStandard>{}</LanguageStandard>
                </ClCompile>
              </ItemDefinitionGroup>
              <PropertyGroup Label="Configuration">
                <PlatformToolset>{}</PlatformToolset>
              </PropertyGroup>
            </Project>

I think the difference in these 2 files is what summarizes my proposal

@solvingj
Copy link
Contributor

solvingj commented Oct 8, 2020

Ok, that makes sense and sounds good to me now. The last props file seems like the one I want, with just one thing missing. I notice it has the placeholder for preprocessor definitions, but I don't see a placeholder for arbitrary MSBuild properties. Maybe you just haven't gotten to that yet.
Great work.
@SSE4 do you have additional questions/concerns?

@SSE4
Copy link
Contributor

SSE4 commented Oct 8, 2020

@solvingj yes, thank you very much, now it's much more clear

@memsharded memsharded merged commit c9a5093 into conan-io:develop Oct 13, 2020
@memsharded memsharded deleted the fix/toolchain_msbuild_remove_toolset branch October 13, 2020 09:38
@memsharded memsharded added this to Done in Toolchain Oct 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Toolchain
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants