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

Feature/msbuild toolchain generator #7754

Merged

Conversation

memsharded
Copy link
Member

@memsharded memsharded commented Sep 25, 2020

Changelog: Feature: Allow MSBuildToolchain custom configurations in generate() method.
Docs: conan-io/docs#1957

Note: the docs of this slipped into 1.32 release, the Docs PR is already merged.

Copy link
Contributor

@lasote lasote left a comment

Choose a reason for hiding this comment

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

Interesting. Some questions more related to the msbuild format than the feature itself.
Do you see other toolchains (like cmake) taking advantage of the generators declared there?
Could be an issue that the generator files are written twice if declared in the recipe?

EndGlobalSection
GlobalSection(ProjectConfigurationPlatforms) = postSolution
{6F392A05-B151-490C-9505-B2A49720C4D9}.Debug|x64.ActiveCfg = Debug|x64
Copy link
Contributor

Choose a reason for hiding this comment

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

Why these removed lines?

Copy link
Member Author

Choose a reason for hiding this comment

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

They were unused. Seems a legacy from a refactor or something. Dead code.

options = {"shared": [True, False]}
default_options = {"shared": False}
def toolchain(self):
tc = MSBuildToolchain(self)
if self.options["hello"].shared and self.settings.build_type == "Release":
tc.configuration = "ReleaseShared"
tc.generator.configuration = "ReleaseShared"
Copy link
Contributor

Choose a reason for hiding this comment

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

A Question: the generator configuration should follow always the configuration of the project or not necessarily? Should be the same by default or not really?
In other words, if the generator configuration is different, will the dependencies will be found for the configuration specified in the project?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, after conversation with users, it is very typical that the configuration of the project refers to the way it is linking dependencies. It could be both, of course, but users can do:

  • ReleaseStatic: I am building my app executable, linking it statically with its dependencies
  • ReleaseShared: My app executable links with shared libs.

While other users or other projects could define:

  • ReleaseDLL: I am building a shared library myself
  • ReleaseStatic: I am building a static library myself

So it seems that the system should be flexible to let the users define the logic they want differently to generators and toolchains.

Copy link
Contributor

Choose a reason for hiding this comment

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

Mhhh, so answering my questions...

  • The tc.configuration and tc.generator.configuration might be different.
  • We don't want to default the tc.generator.configuration with the value adjusted in the tc.configuration.

Did you mean the matrix is the following?:

  • ReleaseStatic (tc.configuration): I am building my app executable, linking it statically with its dependencies
  • ReleaseShared (tc.generator.configuration) : My app executable links with shared libs.

While other users or other projects could define:

  • ReleaseDLL (tc.configuration): I am building a shared library myself
  • ReleaseStatic (tc.generator.configuration): I am building a static library myself

Copy link
Member Author

Choose a reason for hiding this comment

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

I think in the second case, it would be both tc.configuration, assuming that dependencies in both cases do not change at all.

In the first case, it seems that tc.generator.configuration => "tc.configuration", in most cases both should be used, as it sounds difficult to consume that specific dependencies configuration unless your current project doesn't define that configuration. Though it might be possible to use the same toolchain file for different configurations, so maybe not in all cases.

Copy link
Contributor

@lasote lasote Sep 30, 2020

Choose a reason for hiding this comment

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

I'm so sorry but I don't get it hahahah. Could you do a Matrix of possible values of tc.configuration and tc.generator.configuration and the meaning?

Copy link
Member Author

Choose a reason for hiding this comment

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

The name of the config is irrelevant, it is just something a user will define. So lets define it from the other side:

  • toolchain() defines tc.configuration only. This means that generators will keep generating the same standard files, like zlib_release.cmake or zlib_release_x86_v140.props. The dependencies will be exactly the same, using standard Conan build_type settings.
    This would be the case if the current project have several custom configurations in cmake or Visual for building different variants, like building a shared library or a static library. The dependencies are the same (static, shared, or whatever), they do not change, but the current project build from the IDE can be different.

  • toolchain() defines generator.configuration only. This means that dependencies change based on some inputs, for example if the user wants to be creating an executable linked with static deps or with shared deps. The current toolchain files could be identical, and do not depend on the configuration, like building an executable, maybe the toolchain doesn't change at all: same generator, same architecture, etc.

  • toolchain() defines both generator.configuration and tc.configuration: Both the dependencies and the current toolchain will change based on the current configuration. Like if the previous case, but building that executable against shared or static libraries would require some extra flags to be defined by the toolchain, e.g. passing -PIE or some LinkTimeOptimization flag when building against static or shared libraries.

Please let me know if this makes sense.

@memsharded
Copy link
Member Author

Do you see other toolchains (like cmake) taking advantage of the generators declared there?

Yes, I think this is a pattern we might want to define. There is the question how to handle some generators passed in the command line, but at least the build-system integration generators, that are always necessary, could always be defined in the recipe.

Could be an issue that the generator files are written twice if declared in the recipe?

Yes, could be, maybe we need to differentiate "consumer" generators that can be passed in cli to other build-systems generator that need to be defined in the recipe. To think about and discuss.

@solvingj
Copy link
Contributor

Had good discussion about this today. Here are some takeaways for future reference:

To summarize the "new" logic being implemented:

The primary innovation of this work is to enable users to explicitly map conan settings to msbuild configurations. The secondary motivation is to enable users to add one simple unconditional import of conan_toolchain.props to their vcxproj file, and have all msbuild conditionals like configuration|platform for each group of conan-generated variables defined in the conan-generated files.

One-File-Per-Configuration

To do this, that single conan_toolchain.props file must either wrap all necessary conditional variables in msbuild conditional syntax, or put the variables in separate files and wrap additional .props import() statements in msbuild conditional syntax. In either case, Conan must now start generating text with msbuild conditional syntax. The current implementation uses the latter strategy (one-file-per-configuration).

Producing MSBuild Conditions

The current implementation takes a minimalistic approach to try to handle the common case, and do so with the least burden on the user. Thus, it exposes only the Configuration name to be arbitrarily defined by the user, while deducing the Platform from Conan settings. It then concatenates these together into a commonly used conditional string (eg. Release|x64).

Generated Filenames

The current implementation implicitly generates a unique filename that is based on the same variables that produce the condition string. This guarantees uniqueness and the user does not have to think about it.

Future Ideas
During discussion of this, the following mix of "realizations" and "opinions" formed (in no particular order):

  • Generators can use similar pattern once parameterized
  • Both toolchains and generators eventually need to handle multi-project solutions
    • two projects in same solution can have different toolchains
    • two projects in same solution can need different deps from a generator
      • the feature request for every-dep-in-separate prop was based on this case

Implementation Ideas for Future Ideas

For both toolchains and generators, it seems like we'll need to at least make one major addition:

  • Support generating multiple files for multi-project-solutions:
    • instead of just one:
      • conan_toolchain.props
      • conanbuildinfo.props
    • support:
      • conan_toolchain_project_1.props
      • conan_toolchain_project_2.props
      • conanbuildinfo_project_1.props
      • conanbuildinfo_project_2.props
    • easy to support, user just instantiates multiple generator instances
    • will need to support filename or suffix paramter
      • MSBuildToolchain(filename_suffix="project_1")
      • MSBuildGenerator(filename_suffix="project_1")

For generators, we had one novel idea so far:

  • Supporting a parameter for generators that lets users filter deps:
    • for the case of multi-project solutions
    • could be "include_deps" or "exclude_deps" (probably the former)
    • would be an improvement for the real-world case we already know about

For toolchains, it seems like there are only three variables:

  • platform_toolset
  • runtime
  • cppstd

But actually, we're planning to support arbitrary msbuild variables and preprocessor definitions, so users will need and want lots of flexibility in what is generated and under what msbuild conditions it applies. For this reason, I suggest generalizing the Toolchain's support for msbuild conditions. It's still a rough idea that needs to be played with, but for maximum generality, we could support something like this:

user_defined_content_dict = {
    "$(Configuration).startsWith('Release')": {
        "preprocessor_definitions": [
            "PUBLISHER=MYCORP",
            "COPYRIGHT_YEAR=2020",
        ],
        "$(Platform)' == 'x86'": {
            "preprocessor_definitions": [
                "ARCH=32-bit",
            ]
        }
        "$(Platform)' == 'x86_64'": {
            "preprocessor_definitions": [
                "ARCH=64-bit",
            ]
        }
    }
}

ToolchainGenerator(user_defined_content=user_defined_content_dict)

This doesn't look like something that would be reasonable or acceptable, but it's trying to push the boundaries on what would be possible for future thought.
After lookin at this, it reminds me that in previous toolchain work, we took out all the "build system conditionals" in favor of making the generated files much more declarative and readable, so that makes me question why the original primary goal of this toolchain is once again adding build-system-conditional syntax. Ahh, it's because MSBuild is a special case, and the case of importing a single file on a per-configuration basis requires this one use of msbuild conditional generation. However, for the case I just provided, we should instead just do the conditional logic in the recipe (in python).

@memsharded memsharded added this to the 1.33 milestone Dec 4, 2020
@memsharded memsharded marked this pull request as ready for review December 4, 2020 13:24
@memsharded memsharded assigned memsharded and unassigned czoido Jan 8, 2021
@memsharded memsharded merged commit 989d7ef into conan-io:develop Jan 8, 2021
@memsharded memsharded deleted the feature/msbuild_toolchain_generator branch January 8, 2021 12:26
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.

None yet

4 participants