Repository: conan-io/conan
Pull request #17076 (2024-09-27 15:13:48)

[feature] MSBuildDeps | Configuration and Platform keys for property sheets
# Description

### Context
This PR is an attempt to add a bit more customization to the `MSBuildDeps` class for large monorepos visual studio solutions. The current Visual Studio extension for conan is not practical for solutions with hundreds of projects. The ideal and more user friendly workflow in this case is to have a single conanfile that is maintain in the solution. To work effectively, this requires the user to create a new visual studio project (e.g., `conanproject.vcxproj`) and add it to its solution. With a bit of custom build step and build ordering magic in the `conanproject.vcxproj` file, the user can ensure that conan is ran at each build of its solution.

The problem arise when your configuration in your `conanproject.vcxproj` differ from the configurations selected of some other projects in the same solution. For CI purpose, you often find yourself in the position of having to do multiple configurations for different projects and having to mix them in the final build of the solution. To be able to correctly import the property sheets generated by conan in your other projects, you need to be able to set a new "Configuration" variable in your project `.vcxproj` to match to the one from conan.

This is hard to explain, please ask questions or clarifications if this is not clear enough!

### Solution
What worked very well for us was to overload the _condition method in the `MSBuildDeps` class and provide a custom `Configuration` variable name. The thing is, overloading a "private" method is not great and could be regarded as undefined behavior, so i'm proposing to make this into an additional customization point for everyone. I added customization point for platform as well as filename to extend the possibilities. In the issue described above, having only the `self.configuration_condition_key` variable solve the problem.

### TODO

- [ ] Review
- [ ] Add documentation - Documentation on this might confuse basic user, need feedback if needed.

Changelog: (Feature): Describe here your pull request
Docs: https://github.com/conan-io/docs/pull/XXXX

- [x] Refer to the issue that supports this Pull Request. **N/A**
- [x] If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
- [x] I've read the [Contributing guide](https://github.com/conan-io/conan/blob/develop2/.github/CONTRIBUTING.md).
- [x] 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.


CLAassistant: [![CLA assistant check](https://cla-assistant.io/pull/badge/not_signed)](https://cla-assistant.io/conan-io/conan?pullRequest=17076) <br/>Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our [Contributor License Agreement](https://cla-assistant.io/conan-io/conan?pullRequest=17076) before we can accept your contribution.<br/><sub>You have signed the CLA already but the status is still pending? Let us [recheck](https://cla-assistant.io/check/conan-io/conan?pullRequest=17076) it.</sub>
vermz99: Not sure, but this might partially help #16276 
memsharded: Thanks very much for your contribution @vermz99 

I like it that as-is, it is quite safe, in the sense that it won't break any existing usage.

But I'd like to understand a bit better what is the use case, if we could start with a (as simple as possible) test for the test suite, that illustrate the expected UX and usage, and does some basic checking of the output (like checking the generated .props to see they are what are expected), also to guarantee that things do not break in the future. (this could go to an ``test/integration/toolchains/microsoft``.
sykhro: > Not sure, but this might partially help #16276. @sykhro, any opinion?

I will think about it a bit - need to check what's the situation with different vendors. At the moment I'm using CMake everywhere and bypassing the problem entirely
memsharded: Hi @vermz99 

Did you check my above comment about adding some tests? 
This will really help to move this forward, please ask for advice or help with providing the tests, we can help with that, if you need.
AlexVermette-Eaton: @memsharded Hey James, sorry for the delay, I got half of it done and should be able to complete soon. I was thinking of validating that the generated props file contains the expected values for the key of Configuration and Platform as well as the filename. 

I'm not sure how to build a more extensive test case without actually using Visual Studio here.
memsharded: > Hey James, sorry for the delay, I got half of it done and should be able to complete soon. I was thinking of validating that the generated props file contains the expected values for the key of Configuration and Platform as well as the filename.

Great, sure, no hurries, I just wanted to follow up in case you needed some help with that.

This is enough, no need for a test that really uses VS.
The main idea would be to illustrate and capture in the test the UX of the ``conanfile.py``, how users define and use this, if it is intended to have a connection with custom ``settings.yml``, etc. Something that captures the use case without necessarily doing a full compilation.
vermz99: @memsharded Just added integration tests. I rolled back the changes related to filename since i could not find a use case. I also clarified the function related to filename to avoid confusion in the future.

🤔 Let me know what you think. I started with pretty heavy combinatory testings, but realized it might be too much for such a simple feature. 
memsharded: > 🤔 Let me know what you think. I started with pretty heavy combinatory testings, but realized it might be too much for such a simple feature.

Indeed, not necessary heavy combinatorics over this, just something simple, one success and one fail scenario to cover the basics use cases and ensure the feature is not broken in some refactor or other changes

> Also, not sure what is wrong with CI since I cannot access it.

Yes, we had to make it private for security reasons, but we are in the process to move it to public again, hopefully soon. Don't worry at the moment about failed builds, we will check them.

Patch:
diff --git a/conan/tools/microsoft/msbuilddeps.py b/conan/tools/microsoft/msbuilddeps.py
index a747c1c1c..01bf9f09e 100644
--- a/conan/tools/microsoft/msbuilddeps.py
+++ b/conan/tools/microsoft/msbuilddeps.py
@@ -96,6 +96,8 @@ class MSBuildDeps(object):
         self._conanfile = conanfile
         #: Defines the build type. By default, ``settings.build_type``.
         self.configuration = conanfile.settings.build_type
+        #: Defines the configuration key used to conditionate on which property sheet to import.
+        self.configuration_key = "Configuration"
         # TODO: This platform is not exactly the same as ``msbuild_arch``, because it differs
         # in x86=>Win32
         #: Platform name, e.g., ``Win32`` if ``settings.arch == "x86"``.
@@ -103,6 +105,8 @@ class MSBuildDeps(object):
                          'x86_64': 'x64',
                          'armv7': 'ARM',
                          'armv8': 'ARM64'}.get(str(conanfile.settings.arch))
+        #: Defines the platform key used to conditionate on which property sheet to import.
+        self.platform_key = "Platform"
         ca_exclude = "tools.microsoft.msbuilddeps:exclude_code_analysis"
         #: List of packages names patterns to add Visual Studio ``CAExcludePath`` property
         #: to each match as part of its ``conan_[DEP]_[CONFIG].props``. By default, value given by
@@ -124,14 +128,14 @@ class MSBuildDeps(object):
             save(generator_file, content)
 
     def _config_filename(self):
-        props = [("Configuration", self.configuration),
-                 ("Platform", self.platform)]
-        name = "".join("_%s" % v for _, v in props)
+        props = [self.configuration,
+                 self.platform]
+        name = "".join("_%s" % v for v in props)
         return name.lower()
 
     def _condition(self):
-        props = [("Configuration", self.configuration),
-                 ("Platform", self.platform)]
+        props = [(self.configuration_key, self.configuration),
+                 (self.platform_key, self.platform)]
         condition = " And ".join("'$(%s)' == '%s'" % (k, v) for k, v in props)
         return condition
 


Test patch:
diff --git a/test/integration/toolchains/microsoft/test_msbuilddeps.py b/test/integration/toolchains/microsoft/test_msbuilddeps.py
index 940bbe5f7..d43145cbe 100644
--- a/test/integration/toolchains/microsoft/test_msbuilddeps.py
+++ b/test/integration/toolchains/microsoft/test_msbuilddeps.py
@@ -28,6 +28,57 @@ def test_msbuilddeps_maps_architecture_to_platform(arch, exp_platform):
     assert expected_import in toolchain
 
 
+@pytest.mark.parametrize(
+    "build_type,arch,configuration,exp_platform",
+    [
+        ("Release", "x86", "Release - Test", "Win32"),
+        ("Debug", "x86_64", "Debug - Test", "x64"),
+    ],
+)
+@pytest.mark.parametrize(
+    "config_key,platform_key",
+    [
+        ("GlobalConfiguration", "GlobalPlatform"),
+        (None, None),
+    ],
+)
+def test_msbuilddeps_import_condition(build_type, arch, configuration, exp_platform, config_key, platform_key):
+    client = TestClient()
+    app = textwrap.dedent(f"""
+        from conan import ConanFile
+        from conan.tools.microsoft import MSBuildDeps
+        class App(ConanFile):
+            requires = ("lib/0.1")
+            settings = "arch", "build_type"
+            options = {{"configuration": ["ANY"],
+                       "platform": ["Win32", "x64"]}}
+
+            def generate(self):
+                ms = MSBuildDeps(self)
+                ms.configuration_key = "{config_key}"
+                ms.configuration = self.options.configuration
+                ms.platform_key = "{platform_key}"
+                ms.platform = self.options.platform
+                ms.generate()
+        """)
+    
+    # Remove custom set of keys to test default values
+    app = app.replace(f'ms.configuration_key = "{config_key}"', "") if config_key is None else app
+    app = app.replace(f'ms.platform_key = "{platform_key}"', "") if platform_key is None else app
+    config_key_expected = "Configuration" if config_key is None else config_key
+    platform_key_expected = "Platform" if platform_key is None else platform_key
+    
+    client.save({"app/conanfile.py": app, 
+                 "lib/conanfile.py": GenConanfile("lib", "0.1").with_package_type("header-library")})
+    client.run("create lib")
+    client.run(f'install app -s build_type={build_type} -s arch={arch} -o *:platform={exp_platform} -o *:configuration="{configuration}"')
+
+    assert os.path.exists(os.path.join(client.current_folder, "app", "conan_lib.props"))
+    dep = client.load(os.path.join(client.current_folder, "app", "conan_lib.props"))
+    expected_import = f"""<Import Condition="'$({config_key_expected})' == '{configuration}' And '$({platform_key_expected})' == '{exp_platform}'" Project="conan_lib_{configuration.lower()}_{exp_platform.lower()}.props"/>"""
+    assert expected_import in dep
+
+
 def test_msbuilddeps_format_names():
     c = TestClient()
     conanfile = textwrap.dedent("""
@@ -172,4 +223,4 @@ def test_msbuilddeps_relocatable(withdepl):
             text = c.load(fn)
             text = text.replace('\\', '/')
             dir = c.current_folder.replace('\\', '/')
-            assert dir not in text
+            assert dir not in text
\ No newline at end of file