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

Add Conan package info to visualstudio,msbuild generators #7645

Merged
merged 4 commits into from Nov 25, 2020

Conversation

thorntonryan
Copy link
Contributor

@thorntonryan thorntonryan commented Sep 2, 2020

Add CONAN_PACKAGE_NAME and CONAN_PACKAGE_VERSION to the property file generated by the visual_studio and msbuild generators, similar to what you see in the CMake generator.

Fixes #7177

Changelog: Feature: Add Conan package name and version to Visual Studio generator properties file.
Docs: Omit

  • 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.

@CLAassistant
Copy link

CLAassistant commented Sep 2, 2020

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

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

Please have a look to the comments.

I would also strongly encourage to check and start using the msbuild generator instead. It is more powerful and flexible than this one, and it is very likely that it will become the only one in Conan 2.0.

conans/client/generators/visualstudio.py Outdated Show resolved Hide resolved
@memsharded
Copy link
Member

Thanks very much for contributing to Conan. Please check the CLA above, it needs to be signed.

@thorntonryan

This comment has been minimized.

@thorntonryan thorntonryan changed the title Add Conan package info to visualstudio generator Add Conan package info to visualstudio,msbuild generators Sep 3, 2020
@thorntonryan thorntonryan marked this pull request as draft September 11, 2020 20:03
@thorntonryan

This comment has been minimized.

@thorntonryan thorntonryan marked this pull request as ready for review October 9, 2020 02:42
@thorntonryan
Copy link
Contributor Author

thorntonryan commented Oct 13, 2020

@memsharded , I think the test cases are working now.

I haven't tested the full test suite (I don't have the right environment), but hacking the affected tests to run on my machine (i.e. VS2019 w/ no gcc) seems to work.

@memsharded memsharded added this to the 1.31 milestone Oct 13, 2020
Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

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

Hi @thorntonryan

I have realized that we were missing the transitive dependencies problem, and thus the solution might not be that evident, this would only work for 1st level deps. Please have a look at the comments and let me know.

@@ -87,6 +87,10 @@ def _deps_props(self, name_general, deps):
template = textwrap.dedent("""\
<?xml version="1.0" encoding="utf-8"?>
<Project ToolsVersion="4.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
<PropertyGroup Label="Conan-PackageInfo">
Copy link
Member

Choose a reason for hiding this comment

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

I am reviewing this, and while it seems correct, I might be missing something.

All other variables are appended with the package name like includedirs_pkgname, because otherwise different packages will overwrite each other values.

The generated .props files will include transitively other .props files if there are transitive dependencies, which is the common case. In this case the ConanPackageName and ConanPackageVersion properties will be overwritten, and only 1 value accesible.

It would be necessary to have a complete case, in which the consumption, usage pattern is shown. Why and how a consumer will need this data? Wouldn't it need the data from all transitive dependencies that are being included?

I would say yes, in that case, the solution is a bit more complicated, and we would need at least some cumulative variable or something similar.

Please tell me what you think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

James, thank you for your input. I think you raise a valid concern.

My goal is to recover something analogous to:

sections.append(cmake_package_info(name=self.conanfile.name,
version=self.conanfile.version))

From the CMake generator, which generates a single:

### Definition of global aggregated variables ###

set(CONAN_PACKAGE_NAME MyPkg)
set(CONAN_PACKAGE_VERSION 0.1)

In the top level conanbuildinfo.cmake

I could be mistaken, but I don't think the CMake generator worries about the transitive dependencies either.

My goal was to generate ConanPackageName and ConanPackageVersion only once in the top level property file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Our real use case, is not unsurprisingly, a little lot more complicated.

We have a single top level conanfile.py that captures the shared build/runtime requirements for a variety of tools/utilities used for the development of embedded software -- think something akin to build-essential but with a lot of our home grown utilities.

To produce these tools, we have a single top level Visual Studio solution with a variety of sub projects that depend on these build / runtime requirements. Each sub project imports the generated conanbuildinfo.props.

One project is for a tool that produces artifacts for troubleshooting / debugging embedded software. Another project contains a mock/example embedded code that we compile and then produce build artifacts for.

We would like to package the artifacts for our example project with the same version as the high level project that produced them -- that way we can say PkgBuildEssential@August2020 produced PkgExampleCode@August2020 and keep the two in sync.

This lets downstream tools consuming these example build artifacts obtain good reference files from the latest "internal build essentials" release.

A good spot for pinning a top level version seemed to be the top level conanfile.py for the project. And we could get there if the visual_studio and msbuild generators behaved more like the cmake generator and adding something analogous to CONAN_PACKAGE_NAME and CONAN_PACKAGE_VERSION into one of the generated property files.

As @puetzk noted in the parent issue. We're already taking this approach (i.e. driving top level project information down into sub projects) on other projects built around CMake generators. We were just surprised we couldn't adopt the same approach for .NET projects. Hence the issue and PR :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to the doc:

One conan_deps.props Visual Studio properties file, importing all the direct dependencies, in this example both conan_zlib.props and conan_poco.props.

conan_deps.props seemed like the top level property file. But a different property file would work just as well if it's a bad fit logically / conceptually.

In fact, that's how we're currently working around the issue -- by defining a custom generator that produces yet another property file containing this top level information.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I think I understand. It seems that you want the name + version of the current package, not the dependencies, and this is why I was probably confused. Did I understand it right now?

In that case, the place to implement this is most likely the toolchain(), and the MSBuildToolchain(). It is experimental, but it totally sounds like the right place to be defined. Have you had a chance to have a look at them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I think I understand. It seems that you want the name + version of the current package, not the dependencies, and this is why I was probably confused. Did I understand it right now?

Correct. It sounds like we're on the same page now. I hadn't looked too closely at the new msbuild generator and didn't see any references to the toolchain capabilities from the generator page. This must've been what was being hinted at earlier. Sorry I didn't make the connection.

In that case, the place to implement this is most likely the toolchain(), and the MSBuildToolchain(). It is experimental, but it totally sounds like the right place to be defined. Have you had a chance to have a look at them?

conan_toolchain.props does seem like a better fit. Would you rather I put <PropertyGroup Label="Conan-PackageInfo"> there?

Copy link
Member

Choose a reason for hiding this comment

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

Hi @thorntonryan

yes, definitely, I suggest moving this name-version info to the generated conan_toolchain.props from the MSBuildToolchain(), this is the new proposed integration with build systems and where we are putting our efforts.

@memsharded memsharded modified the milestones: 1.31, 1.32 Oct 29, 2020
@thorntonryan
Copy link
Contributor Author

@memsharded , I've updated the PR but I have a few observations about MSBuildToolchain():

The doc says:

The MSBuildToolchain will generate two files after a conan install command or before calling the build() method when the package is building in the cache:

But based on my testing, the files are not produced when calling conan install.

I added:

from conans import ConanFile, MSBuildToolchain

class BuildToolsConanFile(ConanFile):
    name = "BuildTools"
    version = "2020.07.08"
    settings = "build_type"
    generators = (
        "msbuild"
    )

def toolchain(self):
    tc = MSBuildToolchain(self)
    tc.write_toolchain_files()

I think my code is in the right place, but the toolchain method never gets called so I'm wondering if there's still something I'm not understanding about how MSBuildToolchain() is expected to be used.

And unrelated nit:

From the msbuild generator doc:

One properties file for each dependency and transitive dependency, like conan_zlib.props, conan_openssl.props*and *conan_poco.props. These files will transitively import other files, in this case as the poco package depends on openssl, the conan_poco.props will import conan_openssl.props file.

We actually have an internal conan package called toolchain. If I understand the intent behind MSBuildToolchain, then you probably want to guard against creating a package with that reserved name to avoid collisions with the property files produced by msbuild generator for each transitive dependency.

Otherwise I think you can have an opportunity for the conan_toolchain.props file to be overwritten by the toolchain file (or vice versa) or even prevent a toolchain file from being written.

@thorntonryan thorntonryan marked this pull request as draft November 17, 2020 17:25
@memsharded
Copy link
Member

Hi @thorntonryan

It seems your toolchain method is non indented correctly, so it is understood as a global, unrelated method, not a recipe one. Try:

class BuildToolsConanFile(ConanFile):
    name = "BuildTools"
    version = "2020.07.08"
    settings = "build_type"
    generators = (
        "msbuild"
    )

    def toolchain(self):
        tc = MSBuildToolchain(self)
        tc.write_toolchain_files()

We actually have an internal conan package called toolchain. If I understand the intent behind MSBuildToolchain, then you probably want to guard against creating a package with that reserved name to avoid collisions with the property files produced by msbuild generator for each transitive dependency.

Haven't thought of that 😮 We might need to think another name, but collision will always be a possibility... lets try to make it at least less likely

@thorntonryan
Copy link
Contributor Author

It seems your toolchain method is non indented correctly, so it is understood as a global, unrelated method, not a recipe one.

That did it! Ok, I'll work to get unit tests added.

@memsharded
Copy link
Member

Did PR #8073 to address the conflict name issue.

@thorntonryan
Copy link
Contributor Author

Did PR #8073 to address the conflict name issue.

I think that would work to mitigate against collisions with existing packages.

Even if someone created a package "conantoolchain" and required it, that would result in a transitive dependency property file of conan_conantoolchain.props, which differs from your conantoolchain.props.

@thorntonryan thorntonryan marked this pull request as ready for review November 17, 2020 22:35
client.run('install . -s os=Windows -s compiler="Visual Studio" -s compiler.version=15'
' -s compiler.runtime=MD')

conan_toolchain_props = client.load("conan_toolchain.props")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given #8073, likely needs rebased/changed to:

-conan_toolchain_props = client.load("conan_toolchain.props")
+conan_toolchain_props = client.load("conantoolchain.props")

@memsharded memsharded merged commit e15dc79 into conan-io:develop Nov 25, 2020
2 checks passed
@memsharded
Copy link
Member

I merged latest changed from develop, moved the test and fixed a bug in the test (using Pk2g name), and it passed CI.

It has been merged, it will be in next 1.32 release. Thanks very much for your contribution!

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.

[feature] Add Conan package name and version to visual studio generator prop file
3 participants