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

New msbuild generator that creates 1 .props file per dependency (multi-configuration) #7035

Merged
merged 15 commits into from Jun 3, 2020

Conversation

memsharded
Copy link
Member

@memsharded memsharded commented May 16, 2020

Changelog: Feature: New experimental msvc generator that generates a .props file per dependency and is also multi-configuration.
Docs: conan-io/docs#1732

Close #7017

Many thanks to @birsoyo!!!

#tags: slow

A bit of context/vision:

  • This generator generates 1 .props file per dependency
  • The user needs to manually add the dependencies they want for the different subprojects. It is impossible to do this automatically.
  • They can add the per-package general .props file for all the configurations (add the conan_zlib.props, not each individual configuration conan_zlib_release_v141.props), as the generator accounts for the conditional logic to differentiate between Platforms, Toolsets and build_type
  • As the goal is to move to a developer experience that is exactly the same with and without Conan, it doesn't make sense to have the MSBuild helper to inject specific .props file for subprojects, it seems that the only way is let the user define their dependencies.
  • If we want the toolchain() to automate the injection of the global conan_deps.props which adds all the direct dependencies, it could be doable, but not sure we want to do it.

Copy link
Member

@jgsogo jgsogo left a comment

I like creating one .props file per requirement, but I need more explanations about how this generator is supposed to work and if it works out of the box or the user needs to manually add these property sheets to the projects in the solution.

return name.lower(), condition

def _multi(self, name_multi, name_conf, condition):
# read the existing mult_filename or use the template if it doesn't exist
Copy link
Member

@jgsogo jgsogo May 18, 2020

Choose a reason for hiding this comment

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

I really don't like reading and modifying files, it is against the generator paradigm we have until now where all the information for the generator is provided by Conan and all files could be generated using a template

Copy link
Member Author

@memsharded memsharded May 18, 2020

Choose a reason for hiding this comment

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

This is not new. The visual_studio_multi generator has been doing this for a very long time. It is the only way to avoid load errors, or you need to conan install all the configurations before you can open the IDE, and that was very annoying. Furthermore, the use case here was more dynamic, in cmake_multi only the build_type is involved, but here there were requested changes based on the toolset too.

Copy link
Member

@jgsogo jgsogo May 18, 2020

Choose a reason for hiding this comment

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

OMG, VS multi generators are evil then

Copy link
Member Author

@memsharded memsharded May 18, 2020

Choose a reason for hiding this comment

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

Yes, a bit, but on the good side, it works surprisingly well, because it is XML, so we haven't had big issues. I'd say that it works more smoothly than the cmake_multi one.

conans/test/functional/generators/msvc_test.py Outdated Show resolved Hide resolved
conans/test/functional/generators/msvc_test.py Outdated Show resolved Hide resolved
conans/client/generators/__init__.py Outdated Show resolved Hide resolved
@memsharded memsharded changed the title Feature/msvc generator New msbuild generator that creates 1 .props file per dependency (multi-configuration) May 18, 2020
@memsharded memsharded requested a review from danimtb May 26, 2020
Copy link
Member

@jgsogo jgsogo left a comment

Many minor suggestions.

Need someone using Windows to test it.


Not needed for this PR, but I want to comment it here. In the CMake world we are planning to generate a file with the variables and then the file for the cmake_find_package generator or the cmake one that includes that one. I don't know if we ever will need that same flexibility here, but implementation can be almost the same:

  • File conan_zlib_release_x64_v141.props contains only the <PropertyGroup Label="ConanVariables"> section with all the <Conanzlib.... variables.
  • File conan_zlib.props imports the proper conan_zlib_xxxxx.props file and then populates the different PropertyGroup and ItemDefinitionGroup.

conans/client/generators/msbuild.py Show resolved Hide resolved
conans/client/generators/msbuild.py Outdated Show resolved Hide resolved
conans/test/functional/generators/msvc_test.py Outdated Show resolved Hide resolved
conans/test/functional/generators/msvc_test.py Outdated Show resolved Hide resolved
conans/client/generators/msbuild.py Outdated Show resolved Hide resolved
conans/client/generators/msbuild.py Outdated Show resolved Hide resolved
conans/client/generators/msbuild.py Outdated Show resolved Hide resolved
conans/client/generators/msbuild.py Outdated Show resolved Hide resolved
conans/client/generators/msbuild.py Outdated Show resolved Hide resolved
conans/client/generators/msbuild.py Outdated Show resolved Hide resolved
@memsharded
Copy link
Member Author

@memsharded memsharded commented May 26, 2020

Not needed for this PR, but I want to comment it here. In the CMake world we are planning to generate a file with the variables and then the file for the cmake_find_package generator or the cmake one that includes that one. I don't know if we ever will need that same flexibility here, but implementation can be almost the same:

Totally agree. When we come up with a pattern for cmake (1 file with all vars, 1 file per dependency), we will replicate the same pattern here

@memsharded memsharded requested a review from jgsogo May 26, 2020
Copy link
Member

@jgsogo jgsogo left a comment

Looking good! ...but I need some Windows to actually test it works without flaws.

I really think that moving everything but the ConanVariables from the conan_zlib_xxxxxx.props file to the conan_zlib.props one will reduce code duplication and will make generated files easier to follow (even if those files are not intended to be read by the user). I think this doesn't need to wait for the CMake refactor.

conans/client/generators/msbuild.py Outdated Show resolved Hide resolved
conans/client/generators/msbuild.py Outdated Show resolved Hide resolved
conans/client/generators/msbuild.py Show resolved Hide resolved
conans/client/generators/msbuild.py Outdated Show resolved Hide resolved
conans/client/generators/msbuild.py Outdated Show resolved Hide resolved
conans/client/generators/msbuild.py Outdated Show resolved Hide resolved
conans/client/generators/msbuild.py Show resolved Hide resolved
memsharded and others added 5 commits May 26, 2020
Co-authored-by: Javier G. Sogo <jgsogo@gmail.com>
Co-authored-by: Javier G. Sogo <jgsogo@gmail.com>
Co-authored-by: Javier G. Sogo <jgsogo@gmail.com>
Co-authored-by: Javier G. Sogo <jgsogo@gmail.com>
Co-authored-by: Javier G. Sogo <jgsogo@gmail.com>
@memsharded
Copy link
Member Author

@memsharded memsharded commented Jun 1, 2020

I really think that moving everything but the ConanVariables from the conan_zlib_xxxxxx.props file to the conan_zlib.props one will reduce code duplication and will make generated files easier to follow (even if those files are not intended to be read by the user). I think this doesn't need to wait for the CMake refactor.

Done. Please review again @jgsogo

@memsharded memsharded requested a review from jgsogo Jun 1, 2020
jgsogo
jgsogo approved these changes Jun 2, 2020
Copy link
Member

@jgsogo jgsogo left a comment

I really like it this way, and I think this is a pattern to follow for other generators. Cool!

I'd like @danimtb to try it running Visual Studio.

danimtb
danimtb approved these changes Jun 3, 2020
Copy link
Member

@danimtb danimtb left a comment

Nothing relevant. It works as expected with Visual Studio 2019 even with debug/release configurations

conans/client/generators/msbuild.py Show resolved Hide resolved
conans/client/generators/msbuild.py Outdated Show resolved Hide resolved
conans/client/generators/msbuild.py Show resolved Hide resolved
conans/client/generators/msbuild.py Outdated Show resolved Hide resolved
conans/client/generators/msbuild.py Outdated Show resolved Hide resolved
@memsharded memsharded merged commit 4321cf5 into conan-io:develop Jun 3, 2020
2 checks passed
@memsharded memsharded deleted the feature/msvc_generator branch Jun 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants