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

MSBuild: add argument to build() to opt-in autoconsumption of props files generated by MSBuildToolchain & MSBuildDeps #12817

Open
wants to merge 31 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
7a60623
MSBuild: ensure to consume props files generated by MSBuildToolchain …
SpaceIm Dec 31, 2022
5326ef5
rely on predictable props names from generators
SpaceIm Dec 31, 2022
2976776
adapt template & tests
SpaceIm Jan 1, 2023
f6da520
force PlatformToolset from msbuild command line
SpaceIm Jan 1, 2023
d0fc429
move /p options first
SpaceIm Jan 1, 2023
86762f7
allow to disable automatic injection of dependencies props
SpaceIm Jan 2, 2023
17d39c2
Revert "allow to disable automatic injection of dependencies props"
SpaceIm Jan 2, 2023
9ea12f8
Revert "move /p options first"
SpaceIm Jan 2, 2023
8730bae
Revert "force PlatformToolset from msbuild command line"
SpaceIm Jan 2, 2023
d3e48ba
Revert "adapt template & tests"
SpaceIm Jan 2, 2023
24d75e5
add force_import_generated_files argument to MSBuild.build()
SpaceIm Jan 2, 2023
a42e46f
typo
SpaceIm Jan 2, 2023
f42f32c
take into account namespace in props files
SpaceIm Jan 2, 2023
3d178a6
revert WindowsCE logic
SpaceIm Jan 2, 2023
68a2200
restore difference between msbuild_arch injected by MSBuild and the o…
SpaceIm Jan 2, 2023
04e69cd
typo
SpaceIm Jan 2, 2023
ab5295c
handle platform overridden as Win32 in MSBuild
SpaceIm Jan 2, 2023
88cf4c1
improve robustness
SpaceIm Jan 2, 2023
0941ed3
test force_import_generated_files=True flavor
SpaceIm Jan 3, 2023
b35577a
fix tests
SpaceIm Jan 3, 2023
7b8e82a
fix again
SpaceIm Jan 3, 2023
b4e0801
no mixup of unittest, parameterized and pytest
SpaceIm Jan 3, 2023
97cc0fe
more fix
SpaceIm Jan 3, 2023
501e227
fix again
SpaceIm Jan 3, 2023
c4f5a86
fix test again
SpaceIm Jan 3, 2023
c450449
minor change
SpaceIm Jan 3, 2023
7753e04
simplify test_msbuild
SpaceIm Jan 3, 2023
730cede
fix side effect in intel msbuild test
SpaceIm Jan 4, 2023
008c568
in test_msbuild, change PlatformToolset in project file to an old one…
SpaceIm Jan 4, 2023
ce846a5
cleanup
SpaceIm Jan 5, 2023
def7edc
factorize default platform computation in MSBuildToolchain & MSBuildDeps
SpaceIm Jan 5, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
98 changes: 95 additions & 3 deletions conan/tools/microsoft/msbuild.py
@@ -1,3 +1,9 @@
import os
import re
import xml.etree.ElementTree as ET

from conan.tools.microsoft.msbuilddeps import MSBuildDeps
from conan.tools.microsoft.toolchain import MSBuildToolchain
from conans.errors import ConanException


Expand All @@ -16,10 +22,21 @@ def msbuild_arch(arch):
'armv8': 'ARM64'}.get(str(arch))


def msbuild_arch_to_conf_arch(arch):
Copy link
Member

Choose a reason for hiding this comment

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

If used only once, then it is better to keep it local initially, until repetition pattern clearly arises (ideally at least 3 occurrences)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not exposed in __init__.py of microsoft folder, so it's not public according to conan documentation. What should I do? There are other non-public functions in this file actually.

return {
"Win32": "Win32",
"x86": "Win32",
"x64": "x64",
"ARM": "ARM",
"ARM64": "ARM64",
}.get(str(arch))


class MSBuild(object):
def __init__(self, conanfile):
self._conanfile = conanfile
self.build_type = conanfile.settings.get_safe("build_type")
self.configuration = conanfile.settings.get_safe("build_type")
# if platforms:
# msvc_arch.update(platforms)
arch = conanfile.settings.get_safe("arch")
Expand All @@ -28,10 +45,13 @@ def __init__(self, conanfile):
msvc_arch = conanfile.settings.get_safe("os.platform")
self.platform = msvc_arch

def command(self, sln, targets=None):
def command(self, sln, targets=None, force_import_generated_files=False):
cmd = ('msbuild "%s" /p:Configuration=%s /p:Platform=%s'
% (sln, self.build_type, self.platform))

if force_import_generated_files:
cmd += f" {self._force_import_generated_files_cmd_line_arg()}"

verbosity = msbuild_verbosity_cmd_line_arg(self._conanfile)
if verbosity:
cmd += " {}".format(verbosity)
Expand All @@ -48,11 +68,83 @@ def command(self, sln, targets=None):

return cmd

def build(self, sln, targets=None):
cmd = self.command(sln, targets=targets)
def build(self, sln, targets=None, force_import_generated_files=False):
cmd = self.command(sln, targets=targets, force_import_generated_files=force_import_generated_files)
self._conanfile.run(cmd)

@staticmethod
def get_version(_):
return NotImplementedError("get_version() method is not supported in MSBuild "
"toolchain helper")

def _get_concrete_props_file(self, root_props_file):
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure why the specific file needs to be passed, if the general toolchain file already contains conditionals to pick the right configuration one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if the general toolchain file already contains conditionals to pick the right configuration one.

Because it doesn't work.
I'm testing with several CCI recipes for the moment, and properties are ignored when they come only from props file (msbuild complains about PlatformToolset since it's not overridden, and when there is a hardcoded WholeProgramOptimization I can't disable it through a custom property in MSBuildToolchain). When I extract these values from toolchain file and inject them in command line, it works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(obviously, I would be more than happy if a smart solution not involving to parse xml files could be found)

Copy link
Member

Choose a reason for hiding this comment

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

I still see a lot of complexity in this functionality.
Wouldn't it be way more straightforward to just inject the <import> section in the vcxproj directly?

concrete_props_file = ""

root = ET.parse(root_props_file).getroot()
namespace = re.match('\{.*\}', root.tag)
namespace = namespace.group(0) if namespace else ""
importgroup_element = root.find(f"{namespace}ImportGroup")
if importgroup_element:
import_elements = importgroup_element.findall(f"{namespace}Import")
if len(import_elements) == 1:
concrete_props_file = import_elements[0].attrib.get("Project")
else:
expected_condition = (f"'$(Configuration)' == '{self.configuration}' And "
f"'$(Platform)' == '{msbuild_arch_to_conf_arch(self.platform)}'")
for import_element in import_elements:
if expected_condition == import_element.attrib.get("Condition"):
concrete_props_file = import_element.attrib.get("Project")
break

if concrete_props_file:
concrete_props_file = os.path.join(self._conanfile.generators_folder, concrete_props_file)

if not concrete_props_file or not os.path.exists(concrete_props_file):
raise ConanException(
f"MSBuildToolchain props file is missing for configuration={self.configuration} and "
f"platform={msbuild_arch_to_conf_arch(self.platform)}."
)

return concrete_props_file

def _get_msbuildtoolchain_properties(self, root_props_file):
properties = {}

# Get properties from props file of configuration and platform
concrete_props_file = self._get_concrete_props_file(root_props_file)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this can be avoided with ForceImportAfterCppTargets?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes maybe, I can try

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nop, it doesn't help :(

root = ET.parse(concrete_props_file).getroot()
namespace = re.match('\{.*\}', root.tag)
namespace = namespace.group(0) if namespace else ""
for propertygroup in root.iter(f"{namespace}PropertyGroup"):
if propertygroup.attrib.get("Label") == "Configuration":
for child in propertygroup:
propert_name = child.tag.replace(namespace, "")
properties[propert_name] = child.text
return properties

def _force_import_generated_files_cmd_line_arg(self):
cmd_args = []
props_paths = []

# MSBuildToolchan must be in generators for this MSBuild mode
msbuildtoolchain_file = os.path.join(self._conanfile.generators_folder, MSBuildToolchain.filename)
if not os.path.exists(msbuildtoolchain_file):
Copy link
Member

Choose a reason for hiding this comment

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

It is possible that some users want to inject only dependencies information, but they don't want to use the toolchain at all? Sounds very possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Il would make little sense. It would be like allowing CMake build helper to not consume conan_toolchain.cmake.

raise ConanException("Missing MSBuildToolchain, it should be added to generators")
props_paths.append(msbuildtoolchain_file)

# Properties of MSBuildToolchain must be extracted and passed manually through command line
# because they don't have precedence when props file is injected with /p:ForceImportBeforeCppTargets
properties = self._get_msbuildtoolchain_properties(msbuildtoolchain_file)
for k, v in properties.items():
cmd_args.append(f"/p:{k}=\"{v}\"")

# MSBuildDeps generator is optional
msbuilddeps_file = os.path.join(self._conanfile.generators_folder, MSBuildDeps.filename)
if os.path.exists(msbuilddeps_file):
props_paths.append(msbuilddeps_file)

# Inject root props generated by MSBuildToolchain & MSBuildDeps
if props_paths:
cmd_args.append(f"/p:ForceImportBeforeCppTargets=\"{';'.join(props_paths)}\"")

return " ".join(cmd_args)
17 changes: 7 additions & 10 deletions conan/tools/microsoft/msbuilddeps.py
Expand Up @@ -7,6 +7,7 @@
from jinja2 import Template

from conan.tools._check_build_profile import check_using_build_profile
from conan.tools.microsoft.toolchain import arch_to_vcproj_platform
from conans.errors import ConanException
from conans.util.files import load, save

Expand All @@ -19,6 +20,8 @@ class MSBuildDeps(object):

"""

filename = "conandeps.props"

_vars_props = textwrap.dedent("""\
<?xml version="1.0" encoding="utf-8"?>
<Project ToolsVersion="4.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
Expand Down Expand Up @@ -92,12 +95,7 @@ def __init__(self, conanfile):
self._conanfile = conanfile
self._conanfile.must_use_new_helpers = True # TODO: Remove 2.0
self.configuration = conanfile.settings.build_type
# TODO: This platform is not exactly the same as ``msbuild_arch``, because it differs
# in x86=>Win32
self.platform = {'x86': 'Win32',
'x86_64': 'x64',
'armv7': 'ARM',
'armv8': 'ARM64'}.get(str(conanfile.settings.arch))
self.platform = arch_to_vcproj_platform(str(conanfile.settings.arch))
ca_exclude = "tools.microsoft.msbuilddeps:exclude_code_analysis"
self.exclude_code_analysis = self._conanfile.conf.get(ca_exclude, check_type=list)
check_using_build_profile(self._conanfile)
Expand Down Expand Up @@ -251,7 +249,6 @@ def _conandeps(self):
""" this is a .props file including direct declared dependencies
"""
# Current directory is the generators_folder
conandeps_filename = "conandeps.props"
direct_deps = self._conanfile.dependencies.filter({"direct": True})
pkg_aggregated_content = textwrap.dedent("""\
<?xml version="1.0" encoding="utf-8"?>
Expand All @@ -262,12 +259,12 @@ def _conandeps(self):
""")
for req, dep in direct_deps.items():
dep_name = self._dep_name(dep, req.build)
filename = "conan_%s.props" % dep_name
dep_filename = "conan_%s.props" % dep_name
comp_condition = "'$(conan_%s_props_imported)' != 'True'" % dep_name
pkg_aggregated_content = self._dep_props_file("", conandeps_filename, filename,
pkg_aggregated_content = self._dep_props_file("", self.filename, dep_filename,
condition=comp_condition,
content=pkg_aggregated_content)
return {conandeps_filename: pkg_aggregated_content}
return {self.filename: pkg_aggregated_content}

def _package_props_files(self, dep, build=False):
""" all the files for a given package:
Expand Down
21 changes: 13 additions & 8 deletions conan/tools/microsoft/toolchain.py
Expand Up @@ -12,6 +12,15 @@
from conans.util.files import save, load


def arch_to_vcproj_platform(arch):
return {
"x86": "Win32",
"x86_64": "x64",
"armv7": "ARM",
"armv8": "ARM64",
}.get(arch)


class MSBuildToolchain(object):

filename = "conantoolchain.props"
Expand Down Expand Up @@ -52,27 +61,23 @@ def __init__(self, conanfile):
self.cflags = []
self.ldflags = []
self.configuration = conanfile.settings.build_type
self.platform = arch_to_vcproj_platform(str(conanfile.settings.arch))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've exposed platform attribute in MSBuildToolchain because it was exposed in MSBuildDeps and they share the same underlying logic, but I'm not sure it makes sense. Is it really customizable in MSBuild files?

Copy link
Member

Choose a reason for hiding this comment

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

I am ok with it, I think it is customizable, but in the worst case, it is not critical that it is exposed, just don't change it if you don't need to change it.

self.runtime_library = self._runtime_library(conanfile.settings)
self.cppstd = conanfile.settings.get_safe("compiler.cppstd")
self.toolset = self._msvs_toolset(conanfile)
self.properties = {}
check_using_build_profile(self._conanfile)

def _name_condition(self, settings):
def _name_condition(self):
props = [("Configuration", self.configuration),
# TODO: refactor, put in common with MSBuildDeps. Beware this is != msbuild_arch
# because of Win32
("Platform", {'x86': 'Win32',
'x86_64': 'x64',
'armv7': 'ARM',
'armv8': 'ARM64'}.get(settings.get_safe("arch")))]
("Platform", self.platform)]

name = "".join("_%s" % v for _, v in props if v is not None)
condition = " And ".join("'$(%s)' == '%s'" % (k, v) for k, v in props if v is not None)
return name.lower(), condition

def generate(self):
name, condition = self._name_condition(self._conanfile.settings)
name, condition = self._name_condition()
config_filename = "conantoolchain{}.props".format(name)
# Writing the props files
self._write_config_toolchain(config_filename)
Expand Down
Expand Up @@ -44,7 +44,7 @@ def test_use_msbuild_toolchain(self):
# Prepare the actual consumer package
self.t.save({"conanfile.py": conanfile_py,
"MyProject.sln": sln_file,
"MyApp/MyApp.vcxproj": myapp_vcxproj,
"MyApp/MyApp.vcxproj": myapp_vcxproj(),
"MyApp/MyApp.cpp": app,
'profile': self.profile},
clean_first=True)
Expand Down