-
Notifications
You must be signed in to change notification settings - Fork 991
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
base: develop
Are you sure you want to change the base?
Conversation
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 |
…& MSBuildDeps with highest precedence
b4aad0a
to
7a60623
Compare
09ac233
to
6c07827
Compare
6c07827
to
f6da520
Compare
- 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
conan/tools/microsoft/msbuild.py
Outdated
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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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".
conan/tools/microsoft/msbuild.py
Outdated
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}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no breaking change, I've not changed toolchain files.
Here is another proposal:
- add something like
props_autoinjection
argument tobuild()
method ofMSBuild
,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
- it collects root props of
- keep previous tests as is, an write other for the
props_autoinjection=True
flavor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 likeforce_imports
orforce_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 Conanbuild()
injection), so this should be a separate discussion if there is a use case for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -115,9 +115,6 @@ | |||
</ImportGroup> | |||
<ImportGroup Label="Shared"> | |||
</ImportGroup> | |||
<ImportGroup Label="PropertySheets"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
I don't understand. What am I missing? EDIT: ok there is a mixup of Unittest, parameterized and pytest. |
@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
@@ -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)) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
* 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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: avcxproj
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.
|
||
from conan.tools.microsoft.visual import vcvars_command | ||
from conan.tools.microsoft.visual import msvc_version_to_vs_ide_version, vcvars_command |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
@@ -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)) |
There was a problem hiding this comment.
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.
|
||
# 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): |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
@@ -16,10 +22,21 @@ def msbuild_arch(arch): | |||
'armv8': 'ARM64'}.get(str(arch)) | |||
|
|||
|
|||
def msbuild_arch_to_conf_arch(arch): |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
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): |
There was a problem hiding this comment.
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?
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.
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
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 |
related to #12155 (comment)
Changelog: (Fix): In
MSBuild
, add an argument tobuild()
method to opt-in automatic consumption of props files generated byMSBuildToolchain
&MSBuildDeps
, without requiring intrusive modification of vcxproj files.Docs: conan-io/docs#2885
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.