Repository: conan-io/conan
Pull request: Feature/toolchain msbuild cmd
Changelog: Feature: Provide a ``MSBuildCmd`` helper class that encapsulates calling MSBuild.
Docs: https://github.com/conan-io/docs/pull/1907

Close https://github.com/conan-io/conan/issues/7824
Close https://github.com/conan-io/conan/issues/7606

Problem statement:
[bug] MSBuild calls devenv by default, even if not available
### Environment Details
  * Operating System+version: Windows Server 2019
  * Compiler+version: Visual Studio 2017 Build Tools 15.9.2.0
  * Conan version: 
  * Python version: 3.7.0

### Steps to reproduce
On a system with only Visual Studio build tools installed, run a Conan build using the MSBuild build helper using the default setting for upgrade_project (Defaults to true)

Because Visual Studio build tools does not install devenv, this setting will fail the build. Disabling the setting (explicitly setting it to false) allows it to succeed.

[feature] MSBuild command line helper
So far the toolchain PoC is there, but there is no command line helper.

Hints from issue discussion:
Hi @UnderSampled 

I am not sure this is a bug, but instead by design. Conan tries to provide a consistent behavior. Running one thing or another different one based on the existence of a tool in the system is a bit fragile, and this is the reason the default behavior assumes that it exists and will error otherwise, and there is an opt-out to not use it.

We are now starting a new approach of integrating with the build systems, using toolchain files and trying to simplify what the build helpers do. I agree that maybe it makes sense to change the default, and only use ``/devenv upgrade`` when users explicitly opt in to use it. But we cannot change this default now in the current build helper, it will change in the new toolchain one.

This is the ongoing work around the toolchain for MSBuild: https://github.com/conan-io/conan/pull/7674

The build helper is not addressed yet, but it will be in following iterations. It will not run /devenv by default, unless explicitly requested. If it is explicitly requested, then, /devenv must be installed in the system.
Hi @UnderSampled 

The new helper that is going to be released together with the toolchain will not call devenv by default: https://github.com/conan-io/conan/pull/7941
In fact it is not even implemented yet, it might be added later (opt-in? opt-out?) depending on users feedback.
**Discussion:**

The mapping of Conan settings to MSBuild is more or less:

- ``settings.arch`` => ``Platform``
- ``settings.build_type`` => ``Configuration``

Where ``Configuration|Platform`` is what can be selected in the IDE to switch config, and typically is ``Release|x64`` or ``Debug|x86`` or similar.

There are other things that cannot be switched from the IDE:
- ``compiler.runtime`` => MT/MD/MTd...
- ``compiler.toolset`` => v140, v141,....

But the previous ``MSBuild`` helper can use some command line switches to inject and change for example the toolset on the fly, with ``/p:PlatformToolset=xxx``.
This introduces an assymetry, that the Conan ``build()`` is able to build a configuration that the user is not able to build as a developer from their IDE. Or is it expected that developers will call Visual from the command line with ``msbuild...... /p:PlatformToolset``? I dont think so

I would suggest:
- What a developer cannot change from the IDE, is hardcoded at ``conan install`` time. Cannot be switched by the build-helper.
- If a project wants to make variability available to developers, they could introduce custom configurations. Conan toolchains can allow custom logic to map from Conan settings to custom configurations (PoC already in: https://github.com/conan-io/conan/pull/7754)
- Check how this applies to other toolchains, specially CMake one.

Feedback appreciated team!


Any command-line-wrapper for a build system should allow the passing of arbitrary build system variables (`/p` or `-D` or whatever).  Otherwise, it's just a really bad helper and users would very frequently resort to constructing their own command-line strings and sending them to `self.run()`. If they do that, then the build helper has lost any protection and usefulness it intended to give. 

In response to one of your bullets, no, not all build system variables are candidates for creating additional configurations in the project.  While thinking about one priority and design challenge here (trying to deliver some safety and guarantees around Visual Studio's unique "configuration" model), it seems we lost sight of some fundamentals here. 

If you need something concrete, "cosmetic string substitution" is a ubiquitous category of usage for  `msbuild` properties which is perfectly reasonable and necessary and doesn't conflict with the other goals.  People inject strings into builds and applications for all kinds of cosmetic reasons, such as putting the GIT commit hash or SVN revision in the "about" box of a program, or date built, etc.  It's very common to pass this in as a `msbuild` property and then have the `vcxproj` pass convert this to a preprocessor definition which is rendered in the code.  Very often, sometimes this is only passed in the CI builds, and the `vcxproj` has some default string value like `local` or `dev` or just an empty string.  

So, this just leaves the question of: 
- Should users be forced to put all such cases of build system variable definitions in the toolchain() method?

I think the answer is an obvious no. A command-line wrapper should stand on it's own and be able to do fundamental translations like this.  Also, not all recipes will want to use the toolchain. 
> Any command-line-wrapper for a build system should allow the passing of arbitrary build system variables (/p or -D or whatever). Otherwise, it's just a really bad helper and users would very frequently resort to constructing their own command-line strings and sending them to self.run(). If they do that, then the build helper has lost any protection and usefulness it intended to give.

I agree it is a good thing that command line wrappers allow passing all kind of things that are possible in the command line. And this should be doable with our new build helpers. 

My objection is that this shouldn't be the default (and possibly only) mechanism for injecting Conan necessary configuration and variability that is mandatory in the developer flow. It is not that the developer can build, run and debug with a toolset defined or a runtime defined. And if there is something that fails for a package for a given configuration, they must be able to develop and test that configuration as easily as possible. That part should be in the toolchain, so developer native experience matches what will be created when they do a local ``conan create``. Then of course, there might be some extra information and somethings that might be only injected in CI, but the basic build configuration shouldn't be part of it.
It seems to me that build system variables should be supported in both the build helper and the toolchain, and I can see different uses for each. 

However, the above statement is only true assuming that the `toolchain.props` files will one-day be produced in the users working directory when calling `conan install` just like generator files are today with `-g`.  Otherwise (if `toolchain` files are never available outside the `build` directory in the conan cache) then there doesn't seem to be any difference between passing them using the `MSBuild` helper and the `toolchain.props` file.  
After speaking with @memsharded it seems that the `toolchain.props` is generated in the users working directory when using `conan install`.  So that's the important part.  The ad-hoc addition of toolchains via some command line flag like `-g` is done for generators is a separate feature and not critical (although may still make sense)

Commit message: Update conans/client/build/msbuild.py

Co-authored-by: Javier G. Sogo <jgsogo@gmail.com>

Patch to review:
diff --git a/conans/client/build/msbuild.py b/conans/client/build/msbuild.py
index 60ca59c0dc1..486ee5322e7 100644
--- a/conans/client/build/msbuild.py
+++ b/conans/client/build/msbuild.py
@@ -5,6 +5,7 @@
 from conans.client import tools
 from conans.client.build.visual_environment import (VisualStudioBuildEnvironment,
                                                     vs_build_type_flags, vs_std_cpp)
+from conans.client.toolchain.msbuild import MSBuildCmd
 from conans.client.tools.env import environment_append, no_op
 from conans.client.tools.intel import intel_compilervars
 from conans.client.tools.oss import cpu_count
@@ -19,6 +20,27 @@
 
 
 class MSBuild(object):
+    def __new__(cls, conanfile, *args, **kwargs):
+        """ Inject the proper MSBuild base class in the hierarchy """
+
+        # If already injected, create and return
+        if MSBuildHelper in cls.__bases__ or MSBuildCmd in cls.__bases__:
+            return super(MSBuild, cls).__new__(cls)
+
+        # If not, add the proper CMake implementation
+        if hasattr(conanfile, "toolchain"):
+            msbuild_class = type("CustomMSBuildClass", (cls, MSBuildCmd), {})
+        else:
+            msbuild_class = type("CustomMSBuildClass", (cls, MSBuildHelper), {})
+
+        return msbuild_class.__new__(msbuild_class, conanfile, *args, **kwargs)
+
+    @staticmethod
+    def get_version(settings):
+        return MSBuildHelper.get_version(settings)
+
+
+class MSBuildHelper(object):
 
     def __init__(self, conanfile):
         if isinstance(conanfile, ConanFile):
@@ -156,7 +178,7 @@ def get_command(self, project_file, props_file_path=None, targets=None, upgrade_
                 self._output.warn("Use 'platforms' argument to define your architectures")
 
         if output_binary_log:
-            msbuild_version = MSBuild.get_version(self._settings)
+            msbuild_version = MSBuildHelper.get_version(self._settings)
             if msbuild_version >= "15.3":  # http://msbuildlog.com/
                 command.append('/bl' if isinstance(output_binary_log, bool)
                                else '/bl:"%s"' % output_binary_log)
diff --git a/conans/client/toolchain/msbuild.py b/conans/client/toolchain/msbuild.py
index 5b5784fb236..91a030fcdb9 100644
--- a/conans/client/toolchain/msbuild.py
+++ b/conans/client/toolchain/msbuild.py
@@ -2,10 +2,48 @@
 import textwrap
 from xml.dom import minidom
 
+from conans.client.toolchain.visual import vcvars_arch, vcvars_command
+from conans.client.tools import msvs_toolset
 from conans.errors import ConanException
 from conans.util.files import save, load
 
 
+class MSBuildCmd(object):
+    def __init__(self, conanfile):
+        self._conanfile = conanfile
+        self.version = conanfile.settings.get_safe("compiler.version")
+        self.vcvars_arch = vcvars_arch(conanfile)
+        self.build_type = conanfile.settings.get_safe("build_type")
+        msvc_arch = {'x86': 'x86',
+                     'x86_64': 'x64',
+                     'armv7': 'ARM',
+                     'armv8': 'ARM64'}
+        # if platforms:
+        #    msvc_arch.update(platforms)
+        arch = conanfile.settings.get_safe("arch")
+        msvc_arch = msvc_arch.get(str(arch))
+        if conanfile.settings.get_safe("os") == "WindowsCE":
+            msvc_arch = conanfile.settings.get_safe("os.platform")
+        self.platform = msvc_arch
+
+    def command(self, sln):
+        vcvars = vcvars_command(self.version, architecture=self.vcvars_arch,
+                                platform_type=None, winsdk_version=None,
+                                vcvars_ver=None)
+        cmd = ('%s && msbuild "%s" /p:Configuration=%s /p:Platform=%s '
+               % (vcvars, sln, self.build_type, self.platform))
+        return cmd
+
+    def build(self, sln):
+        cmd = self.command(sln)
+        self._conanfile.run(cmd)
+
+    @staticmethod
+    def get_version(settings):
+        return NotImplementedError("get_version() method is not supported in MSBuild "
+                                   "toolchain helper")
+
+
 class MSBuildToolchain(object):
 
     def __init__(self, conanfile):
@@ -36,6 +74,7 @@ def format_macro(k, value):
 
         runtime = self._conanfile.settings.get_safe("compiler.runtime")
         cppstd = self._conanfile.settings.get_safe("compiler.cppstd")
+        toolset = msvs_toolset(self._conanfile.settings)
         runtime_library = {"MT": "MultiThreaded",
                            "MTd": "MultiThreadedDebug",
                            "MD": "MultiThreadedDLL",
@@ -53,13 +92,17 @@ def format_macro(k, value):
                   <LanguageStandard>{}</LanguageStandard>
                 </ClCompile>
               </ItemDefinitionGroup>
+              <PropertyGroup Label="Configuration">
+                <PlatformToolset>{}</PlatformToolset>
+              </PropertyGroup>
             </Project>
             """)
         preprocessor_definitions = ";".join([format_macro(k, v)
                                              for k, v in self.preprocessor_definitions.items()])
         # It is useless to set PlatformToolset in the config file, because the conditional checks it
         cppstd = "stdcpp%s" % cppstd if cppstd else ""
-        config_props = content.format(preprocessor_definitions, runtime_library, cppstd)
+        toolset = toolset or ""
+        config_props = content.format(preprocessor_definitions, runtime_library, cppstd, toolset)
         config_filepath = os.path.abspath(config_filename)
         self._conanfile.output.info("MSBuildToolchain created %s" % config_filename)
         save(config_filepath, config_props)
diff --git a/conans/client/toolchain/visual.py b/conans/client/toolchain/visual.py
new file mode 100644
index 00000000000..4f126ad8372
--- /dev/null
+++ b/conans/client/toolchain/visual.py
@@ -0,0 +1,75 @@
+import os
+
+from conans.client.tools.win import vs_installation_path
+from conans.errors import ConanException
+
+
+def vcvars_command(version, architecture=None, platform_type=None, winsdk_version=None,
+                   vcvars_ver=None, start_dir_cd=True):
+    """ conan-agnostic construction of vcvars command
+    https://docs.microsoft.com/en-us/cpp/build/building-on-the-command-line
+    """
+    # TODO: This comes from conans/client/tools/win.py vcvars_command()
+    cmd = []
+    if start_dir_cd:
+        cmd.append('set "VSCMD_START_DIR=%%CD%%" &&')
+
+    # The "call" is useful in case it is called from another .bat script
+    cmd.append('call "%s" ' % vcvars_path(version))
+    if architecture:
+        cmd.append(architecture)
+    if platform_type:
+        cmd.append(platform_type)
+    if winsdk_version:
+        cmd.append(winsdk_version)
+    if vcvars_ver:
+        cmd.append("-vcvars_ver=%s" % vcvars_ver)
+    return " ".join(cmd)
+
+
+def vcvars_path(version):
+    # TODO: This comes from conans/client/tools/win.py vcvars_command()
+    vs_path = vs_installation_path(version)
+    if not vs_path or not os.path.isdir(vs_path):
+        raise ConanException("VS non-existing installation: Visual Studio %s" % version)
+
+    if int(version) > 14:
+        vcpath = os.path.join(vs_path, "VC/Auxiliary/Build/vcvarsall.bat")
+    else:
+        vcpath = os.path.join(vs_path, "VC/vcvarsall.bat")
+    return vcpath
+
+
+def vcvars_arch(conanfile):
+    """
+    computes the vcvars command line architecture based on conanfile settings (host) and
+    settings_build
+    :param conanfile:
+    :return:
+    """
+    # TODO: This comes from conans/client/tools/win.py vcvars_command()
+    settings_host = conanfile.settings
+    try:
+        settings_build = conanfile.settings_build
+    except AttributeError:
+        settings_build = settings_host
+
+    arch_host = str(settings_host.arch)
+    arch_build = str(settings_build.arch)
+
+    arch = None
+    if arch_build == 'x86_64':
+        arch = {'x86': "amd64_x86",
+                'x86_64': 'amd64',
+                'armv7': 'amd64_arm',
+                'armv8': 'amd64_arm64'}.get(arch_host)
+    elif arch_build == 'x86':
+        arch = {'x86': 'x86',
+                'x86_64': 'x86_amd64',
+                'armv7': 'x86_arm',
+                'armv8': 'x86_arm64'}.get(arch_host)
+
+    if not arch:
+        raise ConanException('vcvars unsupported architectures %s-%s' % (arch_build, arch_host))
+
+    return arch
diff --git a/conans/client/tools/win.py b/conans/client/tools/win.py
index 93328e37159..ce9b056332b 100644
--- a/conans/client/tools/win.py
+++ b/conans/client/tools/win.py
@@ -180,8 +180,8 @@ def build_sln_command(settings, sln_path, targets=None, upgrade_project=True, bu
         self.run(command)
     """
     conan_v2_behavior("'tools.build_sln_command' is deprecated, use 'MSBuild()' helper instead")
-    from conans.client.build.msbuild import MSBuild
-    tmp = MSBuild(settings)
+    from conans.client.build.msbuild import MSBuildHelper
+    tmp = MSBuildHelper(settings)
     output = default_output(output, fn_name='conans.client.tools.win.build_sln_command')
     tmp._output = output
 
diff --git a/conans/test/functional/toolchain/test_msbuild.py b/conans/test/functional/toolchain/test_msbuild.py
index 224ae1a0bec..6b2716d3f8a 100644
--- a/conans/test/functional/toolchain/test_msbuild.py
+++ b/conans/test/functional/toolchain/test_msbuild.py
@@ -3,7 +3,7 @@
 import textwrap
 import unittest
 
-
+from conans.client.toolchain.visual import vcvars_command
 from conans.client.tools import vs_installation_path
 from conans.test.utils.tools import TestClient
 
@@ -96,15 +96,16 @@
     <WholeProgramOptimization>true</WholeProgramOptimization>
     <CharacterSet>Unicode</CharacterSet>
   </PropertyGroup>
+  <!-- Very IMPORTANT this should go BEFORE the Microsoft.Cpp.props -->
+  <ImportGroup Label="PropertySheets">
+    <Import Project="..\conan\conan_Hello.props" />
+    <Import Project="..\conan\conan_toolchain.props" />
+  </ImportGroup>
   <Import Project="$(VCTargetsPath)\Microsoft.Cpp.props" />
   <ImportGroup Label="ExtensionSettings">
   </ImportGroup>
   <ImportGroup Label="Shared">
   </ImportGroup>
-  <ImportGroup Label="PropertySheets">
-    <Import Project="..\conan\conan_Hello.props" />
-    <Import Project="..\conan\conan_toolchain.props" />
-  </ImportGroup>
   <ImportGroup Label="PropertySheets" Condition="'$(Configuration)|$(Platform)'=='Debug|Win32'">
     <Import Project="$(UserRootDir)\Microsoft.Cpp.$(Platform).user.props"
     Condition="exists('$(UserRootDir)\Microsoft.Cpp.$(Platform).user.props')"
@@ -218,7 +219,7 @@
 class WinTest(unittest.TestCase):
 
     conanfile = textwrap.dedent("""
-        from conans import ConanFile, MSBuildToolchain
+        from conans import ConanFile, MSBuildToolchain, MSBuild
         class App(ConanFile):
             settings = "os", "arch", "compiler", "build_type"
             requires = "hello/0.1"
@@ -233,6 +234,10 @@ def toolchain(self):
                 else:
                     tc.preprocessor_definitions["DEFINITIONS_CONFIG"] = "Release"
                 tc.write_toolchain_files()
+
+            def build(self):
+                msbuild = MSBuild(self)
+                msbuild.build("MyProject.sln")
         """)
 
     app = textwrap.dedent("""
@@ -316,22 +321,18 @@ def test_toolchain_win(self):
 
         # Run the configure corresponding to this test case
         client.run("install . %s -if=conan" % (settings, ))
-        self.assertIn("conanfile.py: MSBuildToolchain created "
-                      "conan_toolchain_release_win32.props", client.out)
-        vs_path = vs_installation_path("15")
-        vcvars_path = os.path.join(vs_path, "VC/Auxiliary/Build/vcvarsall.bat")
+        self.assertIn("conanfile.py: MSBuildToolchain created conan_toolchain_release_win32.props",
+                      client.out)
+        client.run("build . -if=conan")
 
-        cmd = ('set "VSCMD_START_DIR=%%CD%%" && '
-               '"%s" x86 && msbuild "MyProject.sln" /p:Configuration=Release' % vcvars_path)
-        client.run_command(cmd)
         self.assertIn("Visual Studio 2017", client.out)
         self.assertIn("[vcvarsall.bat] Environment initialized for: 'x86'", client.out)
         self._run_app(client, "x86", "Release")
         self.assertIn("AppMSCVER 17!!", client.out)
         self.assertIn("AppCppStd 17!!!", client.out)
 
-        cmd = ('set "VSCMD_START_DIR=%%CD%%" && '
-               '"%s" x86 && dumpbin /dependents "Release\\MyApp.exe"' % vcvars_path)
+        vcvars = vcvars_command(version="15", architecture="x86")
+        cmd = ('%s && dumpbin /dependents "Release\\MyApp.exe"' % vcvars)
         client.run_command(cmd)
         # No other DLLs dependencies rather than kernel, it was MT, statically linked
         self.assertIn("KERNEL32.dll", client.out)
@@ -363,23 +364,15 @@ def test_toolchain_win_debug(self):
         client.run("install . %s -if=conan" % (settings, ))
         self.assertIn("conanfile.py: MSBuildToolchain created conan_toolchain_debug_x64.props",
                       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
-        cmd = ('set "VSCMD_START_DIR=%%CD%%" && '
-               '"%s" x64 && '
-               'msbuild "MyProject.sln" /p:Configuration=Debug /p:PlatformToolset="v140"'
-               % vcvars_path)
-        client.run_command(cmd)
+        client.run("build . -if=conan")
         self.assertIn("Visual Studio 2017", client.out)
         self.assertIn("[vcvarsall.bat] Environment initialized for: 'x64'", client.out)
         self._run_app(client, "x64", "Debug")
         self.assertIn("AppMSCVER 15!!", client.out)
         self.assertIn("AppCppStd 14!!!", client.out)
 
-        cmd = ('set "VSCMD_START_DIR=%%CD%%" && '
-               '"%s" x64 && dumpbin /dependents "x64\\Debug\\MyApp.exe"' % vcvars_path)
+        vcvars = vcvars_command(version="15", architecture="amd64")
+        cmd = ('%s && dumpbin /dependents "x64\\Debug\\MyApp.exe"' % vcvars)
         client.run_command(cmd)
         self.assertIn("MSVCP140D.dll", client.out)
         self.assertIn("VCRUNTIME140D.dll", client.out)


Code review comments:
diff_hunk: @@ -96,15 +96,16 @@
     <WholeProgramOptimization>true</WholeProgramOptimization>
     <CharacterSet>Unicode</CharacterSet>
   </PropertyGroup>
+  <!-- Very IMPORTANT this should go BEFORE the Microsoft.Cpp.props -->
path: conans/test/functional/toolchain/test_msbuild.py
text: @user1:
```suggestion
  <!-- Very IMPORTANT this should go BEFORE the Microsoft.Cpp.props. If it goes after, the Toolset definition is ignored -->
```