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

SpaceIm · 2022-12-31T14:14:51Z

Changelog: (Fix): In MSBuild, add an argument to build() method to opt-in automatic consumption of props files generated by MSBuildToolchain & MSBuildDeps, without requiring intrusive modification of vcxproj files.
Docs: conan-io/docs#2885

Refer to the issue that supports this Pull Request.
If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
I've read the Contributing guide.
I've followed the PEP8 style guides for Python code.
I've opened another PR in the Conan docs repo to the develop branch, documenting this one.

^{Note: By default this PR will skip the slower tests and will use a limited set of python versions. Check here how to increase the testing level by writing some tags in the current PR body text.}

SpaceIm · 2022-12-31T14:47:11Z

I've tested with libsodium recipe (without this ugly workaround https://github.com/conan-io/conan-center-index/blob/4e0dc05121ed9999dcd3ed8841844757f6495ce0/recipes/libsodium/all/conanfile.py#L137-L171 -mandatory without the fix in this PR-, and a fix in custom configuration of its MSBuildToolchain) and it works like a charm, now vc runtime can be honored (as well as multiprocessing build).

…& MSBuildDeps with highest precedence

SpaceIm · 2022-12-31T19:08:26Z

see these conan-center PR for current workarounds:
conan-io/conan-center-index#15041
conan-io/conan-center-index#15043
conan-io/conan-center-index#15046
conan-io/conan-center-index#15050

It's more or less always the same boilerplate (boilerplate we could remove with this PR):
https://github.com/conan-io/conan-center-index/blob/4a2cbbba7fce7da91a656890744f3463c7429846/recipes/libsodium/all/conanfile.py#L130-L145
https://github.com/conan-io/conan-center-index/blob/22d52120bb27d81d551ada9255e012755b023276/recipes/xz_utils/all/conanfile.py#L104-L128
https://github.com/conan-io/conan-center-index/blob/110eb267a9776031a5189abd06254b07c88abf25/recipes/libmicrohttpd/all/conanfile.py#L137-L151
https://github.com/conan-io/conan-center-index/blob/a4ba448dc6c474198d669242ea5e1e626349581e/recipes/zimg/all/conanfile.py#L93-L108

- relocatable shared lib on macOS - put source code under src folder - use rm_safe - delete fPIC if Windows only - allow other build type than Release & Debug for msvc - add VirtualBuildEnv for autotools build (since there is msys2 in build requirements for MinGW) - remove PkgConfigDeps. It's used to discover gnutls only, but it's disabled by the recipe currently. - more future proof way to use MSBuild helper (see conan-io/conan#12817) - disable whole program optimization in msvc build

memsharded

There are different things proposed in this PR, and they probably require separate discussions:

Using /p:ForceImportBeforeCppTargets to automatically inject property files: At least requires an opt-out, but probably an opt-in is better, we need to discuss this.
Passing the PlatformToolset in the command line is hiding if the PlatformToolset definition from the toolchain works.
Making visible for users the vs_toolset() function

It seems that the best would be to make the p:ForceImportBeforeCppTargets available as opt-in, and cover it with new test, instead of modifying existing tests.

memsharded · 2023-01-01T22:04:54Z

conan/tools/microsoft/msbuild.py

+            props_path = os.path.join(self._conanfile.generators_folder, props_file)
+            if os.path.exists(props_path):
+                props_paths.append(props_path)
+        if props_paths:


This cannot be made default, because that will be forcing the injection of whole dependencies into the current project, and there are many users that don't want that, but manual addition of individual dependencies into different subprojects.

At least, it should have an opt-out to allow users defining it.

So keep this logic for conantoolchain.props, but allow to not autoconsume conandeps.props?

Regarding what users want, I'm wondering: users who manually edit their MSBuild project files, are they really using MSBuild conan helper? Or just MSBuildToolchain & MSBuildDeps?
This PR is really about providing the same high level logic in this toolchain/helper family than all other families in conan v2. It's the only family where the build helper does not automatically consume its toolchains. There is no opt-in or opt-out in Autotools helper to not globally inject AutotoolsDeps, so why a different behavior in MSBuild helper?

From the feedback from users using this integration, it is very common that they have different subprojects and they want to inject different dependencies to different subprojects. If MSBuild pass conandeps.props by default, this cannot be done.

There are definitely other build systems that do the same, CMake uses the same pattern, users need to define find_package() and target_link_libraries() to the things that they want to use, it is not automagically applied to the whole project. For sure that is a built-in construct, but this level of control is what can be achieved with the MSBuild integration. The fact that it is not possiblel to do it in Autotools, for example, shouldn't be used as the design goal for it.

I mostly agree for *Deps generators (even thought it's transparent in case of CMake), but MSBuild is the only build helper not consuming automatically & transparently its "glue toolchain".

memsharded · 2023-01-01T22:06:34Z

conan/tools/microsoft/msbuild.py

-        cmd = ('msbuild "%s" /p:Configuration=%s /p:Platform=%s'
-               % (sln, self.build_type, self.platform))
+        cmd = (f'msbuild "{sln}" /p:Configuration={self.build_type} '
+               f"/p:Platform={self.platform} /p:PlatformToolset={self.toolset}")


This adds back the PlatformToolset as argument only, and that doesn't work for user consumption, that will only be loading the toolchain. That configuration needs to be in the toolchain file.

I've not removed PlatformToolset from toolchain (actually I've not modified content of files generated by toolchains). It's added as an argument to msbuild because when conantoolchain.props is injected with /p:ForceImportBeforeCppTargets, I've seen that PlatformToolset from this toolchain was ignored.

Actually, from my experimentation when conantoolchain.props is injected with /p:ForceImportBeforeCppTargets:

Everything under ItemDefinitionGroup is honored.

Everything under PropertyGroup Label="Configuration" (PlatformToolset & custom properties) is ignored by MSBuild.

This is why I inject /p:PlatformToolset in msbuild command line (and if I could, I would provide a properties argument to msbuild build helper to enforce properties in command line).

https://learn.microsoft.com/en-us/cpp/build/project-property-inheritance

When you set a property with property pages, it's also important to pay attention to where you set it. If you set a property to "X" in a .props file, but the property is set to "Y" in the project file, then the project will build with the property set to "Y".

https://learn.microsoft.com/en-us/visualstudio/msbuild/msbuild-properties

MSBuild lets you set properties on the command line by using the -property (or -p) switch. These global property values override property values that are set in the project file. This includes environment properties, but does not include reserved properties, which cannot be changed.

The problem with /p:ForceImportBeforeCppTargets is the developer experience. Most developers with VS will just double click the project or solution Whatever is being passed should be in the vcxproj by default, it cannot rely on cli args to work. This is probably the most massive use case of this integration, there are companies out there using it, and hundreds of developers do this daily. So this cannot be changed if this doesn't keep working as-is (this is why I suggest an opt-in for ConanCenter recipes, which are typically not opened by developers to touch the source code)

There is no breaking change, I've not changed toolchain files.

Here is another proposal:

add something like props_autoinjection argument to build() method of MSBuild, False by default.

add properties dict attribute to MSBuild

when props_autoinjection is False, current behavior is kept, so no breaking change at all.

when props_autoinjection is True:

it collects root props of MSBuildToolchain & MSBuildDeps and inject them to /p:ForceImportBeforeCppTargets.

it also injects /p:<property>=<value> to msbuild command line

keep previous tests as is, an write other for the props_autoinjection=True flavor.

Regarding properties, to avoid duplication between toolchain and MSBuild, there is a more complex solution: gather all properties by parsing all props of a specific configuration programmatically, and inject them in command line if props_autoinjection is True.

Yes, sorry, I meant the /p:ForceImportBeforeCppTargets is a breaking change when applied to conandeps.props. For the toolchain, the main issue is that it can easily be hiding errors for developers opening the IDE

Yes, I think this approach would be better. Just a couple of suggestions:

Make explicit the intention, props_autoinjection is a bit generic, something like force_imports or force_import_generated_files

Generic property injection is a different feature and MSBuildToolchain already provides injection of properties via toolchain (so they work for developers opening the IDE, without needing the Conan build() injection), so this should be a separate discussion if there is a use case for it.

Generic property injection is a different feature and MSBuildToolchain already provides injection of properties via toolchain (so they work for developers opening the IDE, without needing the Conan build() injection), so this should be a separate discussion if there is a use case for it.

As I said in #12817 (comment), properties defined in toolchain files are not honored when injected through /p:ForceImportBeforeCppTargets, so for me it's related. /p:ForceImportBeforeCppTargets would be an internal detail of force_import_generated_files =True, the goal of this argument is to honor everything defined in these props, by any means.

memsharded · 2023-01-01T22:12:49Z

conans/test/functional/toolchains/microsoft/test_msbuilddeps.py

@@ -115,9 +115,6 @@
  </ImportGroup>
  <ImportGroup Label="Shared">
  </ImportGroup>
-  <ImportGroup Label="PropertySheets">


This test is testing exactly that different subprojects can depend on individual props, without needing to link all dependencies, please leave this test not modified.

This reverts commit 86762f7.

This reverts commit d0fc429.

This reverts commit f6da520.

This reverts commit 2976776.

this argument is disabled by default. When enabled, it automatically injects props files generated by MSBuildToolchain and MSBuildDeps. A new attribute configuration is added to MSBuild, because we have to extract properties from MSBuildToolchain for the configuration/platform.

- relocatable shared lib on macOS - put source code under src folder - use rm_safe - delete fPIC if Windows only - allow other build type than Release & Debug for msvc - add VirtualBuildEnv for autotools build (since there is msys2 in build requirements for MinGW) - remove PkgConfigDeps. It's used to discover gnutls only, but it's disabled by the recipe currently. - more future proof way to use MSBuild helper (see conan-io/conan#12817) - disable whole program optimization in msvc build

SpaceIm · 2023-01-03T11:34:43Z

E                   TypeError: test_toolchain_win_vs2017() missing 5 required positional arguments: 'compiler', 'version', 'runtime', 'cppstd', and 'force_import_generated_files'

I don't understand. What am I missing?

EDIT: ok there is a mixup of Unittest, parameterized and pytest.

SpaceIm · 2023-01-03T21:38:34Z

@memsharded I've updated a functional test_msbuild test to test both flavors of force_import_generated_files.

… to really test that it can be overridden

SpaceIm · 2023-01-05T12:58:48Z

conan/tools/microsoft/toolchain.py

@@ -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))


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?

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.

* avoid code duplication in test package * several fixes & improvements - relocatable shared lib on macOS - put source code under src folder - use rm_safe - delete fPIC if Windows only - allow other build type than Release & Debug for msvc - add VirtualBuildEnv for autotools build (since there is msys2 in build requirements for MinGW) - remove PkgConfigDeps. It's used to discover gnutls only, but it's disabled by the recipe currently. - more future proof way to use MSBuild helper (see conan-io/conan#12817) - disable whole program optimization in msvc build

memsharded

I am still seeing quite a complexity in this PR that I feel it is not great for future issues and maintenance.
For all company usage, adding the .props files to the projects is necessary anyway.
For ConanCenter usage, I think there are way simpler alternatives:

Patching the vcxproj to inject the .props files, in the recipe
Implementing the injection of .props files in the vcxproj with a helper.
In all cases, the functionality should be oriented to get to the desired recommended state: a vcxproj that imports the .props files in the right location. Having to parse the xml of the projects to extract import files, to extract properties, to pass them in command line is too complex IMO.

Asking for further reviews.

memsharded · 2023-01-11T13:16:07Z

conans/test/functional/toolchains/microsoft/test_msbuild.py


-from conan.tools.microsoft.visual import vcvars_command
+from conan.tools.microsoft.visual import msvc_version_to_vs_ide_version, vcvars_command


Please do not modify existing tests. There is a risk of breaking some functionality. The feature that you are proposing is a opt-in, so the previous tests shouldn't be modified, and adding a new test that uses the new functionality is the way to go.

It's deeply modified because this test was not easily extendable, but it tests the same things (and even more, properties injection was not tested actually, and it allows to test other Visual Studio versions than 14, it's an issue for local tests). It seems weird to add a separate test with a risk to have discrepancies between both flavors.
But yeah if you want I can move this to another file, it's just a lot of duplicated code (specially this big vcxproj file).

memsharded · 2023-01-11T13:18:00Z

conan/tools/microsoft/toolchain.py

@@ -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))


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.

memsharded · 2023-01-11T13:20:11Z

conan/tools/microsoft/msbuild.py

+
+        # 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):


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.

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

memsharded · 2023-01-11T13:21:48Z

conan/tools/microsoft/msbuild.py

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


+def msbuild_arch_to_conf_arch(arch):


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

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.

memsharded · 2023-01-11T13:24:19Z

conan/tools/microsoft/msbuild.py

        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):


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?

SpaceIm · 2023-01-11T21:23:09Z

I am still seeing quite a complexity in this PR that I feel it is not great for future issues and maintenance.
For all company usage, adding the .props files to the projects is necessary anyway.

It's necessary for company usage when they work in Visual Studio IDE, but it can be an issue if they want to package an external dependency with build files not under their control, like conan-center.

For ConanCenter usage, I think there are way simpler alternatives:

Patching the vcxproj to inject the .props files, in the recipe

It's what I've done in several recipes to fix them (and it was not obvious to find where this file should be injected in vcxproj files, it's very important actually and not documented in conan doc). And before I did, all packages were broken (even the template written by conan-center maintainers) due to the confusion introduced by this discrepancy behavior between MSBuildToolchain/MSBuild and other build helpers families. It's a good indication that it's not intuitive and there is something wrong.

* avoid code duplication in test package * several fixes & improvements - relocatable shared lib on macOS - put source code under src folder - use rm_safe - delete fPIC if Windows only - allow other build type than Release & Debug for msvc - add VirtualBuildEnv for autotools build (since there is msys2 in build requirements for MinGW) - remove PkgConfigDeps. It's used to discover gnutls only, but it's disabled by the recipe currently. - more future proof way to use MSBuild helper (see conan-io/conan#12817) - disable whole program optimization in msvc build

santhonisz · 2023-01-31T00:41:51Z

Just wondering if an alternate/more viable solution to this could be something along the lines of #3859 (comment)?

This would be also be a similar approach to what is suggested for vcpkg: https://vcpkg.io/en/docs/users/buildsystems/msbuild-integration.html#import-props-and-targets

CLAassistant · 2023-05-18T09:09:15Z

All committers have signed the CLA.

SpaceIm marked this pull request as draft December 31, 2022 14:15

SpaceIm mentioned this pull request Dec 31, 2022

libsodium: fix several regressions of #13792 conan-io/conan-center-index#14998

Merged

3 tasks

MSBuild: ensure to consume props files generated by MSBuildToolchain …

7a60623

…& MSBuildDeps with highest precedence

SpaceIm force-pushed the fix/msbuild-consume-generators branch from b4aad0a to 7a60623 Compare December 31, 2022 16:36

rely on predictable props names from generators

5326ef5

SpaceIm marked this pull request as ready for review December 31, 2022 17:11

SpaceIm mentioned this pull request Dec 31, 2022

libsodium: implement better workaround for the lack of automatic interaction between MSBuildToolchain & MSBuild conan-io/conan-center-index#15041

Merged

3 tasks

SpaceIm mentioned this pull request Dec 31, 2022

[question] What is the equivalent of conans.tools.MSBuild.build() upgrade_project in conan.tools.microsoft.MSBuid? #12155

Open

1 task

SpaceIm changed the title ~~MSBuild: ensure to consume props files generated by MSBuildToolchain & MSBuildDeps with highest priority~~ MSBuild: autoconsume props files generated by MSBuildToolchain & MSBuildDeps Jan 1, 2023

SpaceIm force-pushed the fix/msbuild-consume-generators branch 2 times, most recently from 09ac233 to 6c07827 Compare January 1, 2023 09:07

SpaceIm added 2 commits January 1, 2023 10:13

adapt template & tests

2976776

force PlatformToolset from msbuild command line

f6da520

SpaceIm force-pushed the fix/msbuild-consume-generators branch from 6c07827 to f6da520 Compare January 1, 2023 09:14

move /p options first

d0fc429

memsharded requested changes Jan 1, 2023

View reviewed changes

SpaceIm added 8 commits January 2, 2023 12:17

allow to disable automatic injection of dependencies props

86762f7

Revert "allow to disable automatic injection of dependencies props"

17d39c2

This reverts commit 86762f7.

Revert "move /p options first"

9ea12f8

This reverts commit d0fc429.

Revert "force PlatformToolset from msbuild command line"

8730bae

This reverts commit f6da520.

Revert "adapt template & tests"

d3e48ba

This reverts commit 2976776.

typo

a42e46f

take into account namespace in props files

f42f32c

improve robustness

88cf4c1

SpaceIm added 3 commits January 3, 2023 11:19

test force_import_generated_files=True flavor

0941ed3

fix tests

b35577a

fix again

7b8e82a

SpaceIm added 5 commits January 3, 2023 18:38

no mixup of unittest, parameterized and pytest

b4e0801

more fix

97cc0fe

fix again

501e227

fix test again

c4f5a86

minor change

c450449

SpaceIm added 3 commits January 4, 2023 00:06

simplify test_msbuild

7753e04

fix side effect in intel msbuild test

730cede

in test_msbuild, change PlatformToolset in project file to an old one…

008c568

… to really test that it can be overridden

SpaceIm changed the title ~~MSBuild: autoconsume props files generated by MSBuildToolchain & MSBuildDeps~~ MSBuild: add argument to build() to opt-in autoconsumption of props files generated by MSBuildToolchain & MSBuildDeps Jan 5, 2023

SpaceIm added 2 commits January 5, 2023 13:31

cleanup

ce846a5

factorize default platform computation in MSBuildToolchain & MSBuildDeps

def7edc

SpaceIm commented Jan 5, 2023

View reviewed changes

SpaceIm requested a review from memsharded January 5, 2023 23:00

memsharded reviewed Jan 11, 2023

View reviewed changes

memsharded requested review from czoido, franramirez688, jcar87 and RubenRBS January 11, 2023 13:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

SpaceIm commented Dec 31, 2022 •

edited

SpaceIm commented Dec 31, 2022 •

edited

SpaceIm commented Dec 31, 2022 •

edited

memsharded left a comment

memsharded Jan 1, 2023

SpaceIm Jan 2, 2023

memsharded Jan 2, 2023

SpaceIm Jan 2, 2023 •

edited

memsharded Jan 1, 2023

SpaceIm Jan 2, 2023 •

edited

SpaceIm Jan 2, 2023 •

edited

SpaceIm Jan 2, 2023 •

edited

memsharded Jan 2, 2023

SpaceIm Jan 2, 2023

SpaceIm Jan 2, 2023 •

edited

memsharded Jan 2, 2023

SpaceIm Jan 2, 2023

memsharded Jan 1, 2023

SpaceIm commented Jan 3, 2023 •

edited

SpaceIm commented Jan 3, 2023

SpaceIm Jan 5, 2023

memsharded Jan 11, 2023

memsharded left a comment

memsharded Jan 11, 2023

SpaceIm Jan 11, 2023 •

edited

memsharded Jan 11, 2023

memsharded Jan 11, 2023

SpaceIm Jan 11, 2023

memsharded Jan 11, 2023

SpaceIm Jan 11, 2023

memsharded Jan 11, 2023

SpaceIm commented Jan 11, 2023

santhonisz commented Jan 31, 2023

CLAassistant commented May 18, 2023 •

edited


		from conan.tools.microsoft.visual import vcvars_command
		from conan.tools.microsoft.visual import msvc_version_to_vs_ide_version, vcvars_command

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


		def msbuild_arch_to_conf_arch(arch):

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

Are you sure you want to change the base?

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

Conversation

SpaceIm commented Dec 31, 2022 • edited

SpaceIm commented Dec 31, 2022 • edited

SpaceIm commented Dec 31, 2022 • edited

memsharded left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SpaceIm Jan 2, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SpaceIm Jan 2, 2023 • edited

Choose a reason for hiding this comment

SpaceIm Jan 2, 2023 • edited

Choose a reason for hiding this comment

SpaceIm Jan 2, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SpaceIm Jan 2, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SpaceIm commented Jan 3, 2023 • edited

SpaceIm commented Jan 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

memsharded left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SpaceIm Jan 11, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SpaceIm commented Jan 11, 2023

santhonisz commented Jan 31, 2023

CLAassistant commented May 18, 2023 • edited

SpaceIm commented Dec 31, 2022 •

edited

SpaceIm commented Dec 31, 2022 •

edited

SpaceIm commented Dec 31, 2022 •

edited

SpaceIm Jan 2, 2023 •

edited

SpaceIm Jan 2, 2023 •

edited

SpaceIm Jan 2, 2023 •

edited

SpaceIm Jan 2, 2023 •

edited

SpaceIm Jan 2, 2023 •

edited

SpaceIm commented Jan 3, 2023 •

edited

SpaceIm Jan 11, 2023 •

edited

CLAassistant commented May 18, 2023 •

edited