Support find_package and install

[meastp]

This is a work-in-progress - do not merge yet.

See #244

Unfortunately, I have to add support for find_package for third-party dependencies and install in one go, because the targets from third party dependencies messed up the install/export for opencensus.

I have tested this in Visual Studio 2017 with the open folder native cmake support. I did also successfully run the opencensus tests.

Are these changes acceptable to you? Is this the proper way to support find_package in the dependencies?

[googlebot]

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

[googlebot]

CLAs look good, thanks!

[meastp]

With the current structure, abseil and googletest targets have to be built prior to building any opencensus libs/tests. I think the "best practice" way to get around this is something like the answer in https://stackoverflow.com/questions/44990964/how-to-perform-cmakefind-package-at-build-stage-only . If I implement this - and building works properly - would this be acceptable to you?

[g-easy]

The approach in cmake/Findabseil.cmake doesn't seem right to me: we shouldn't be enumerating build targets inside of abseil.

@coryan, what do you think? Especially as a consumer of opencensus-cpp as a library. What's the right approach here?

[g-easy]

@meastp: Would you like to split out "Replace OPENCENSUS_INCLUDE_DIR with CMAKE_SOURCE_DIR" into its own PR? (and please convert to Unix line endings) If that doesn't break an out-of-tree example, I'd be happy to merge that.

[coryan]

The approach in cmake/Findabseil.cmake doesn't seem right to me: we shouldn't be enumerating build targets inside of abseil.

TL;DR; if you want to use ExternalProject_Add() then yes.

Longer version, these are the most common, (there are many more) choices for dependency management in CMake

1. You assume they are all installed and use find_package(), maybe with some helpers, to discover where they are. It works great, but the experience for a first-time user is terrible because they have to manually download and install everything before trying your library.

2. You download things with ExternalProject_Add(), much better experience for your first-time users, no manual installation of dependencies. Unfortunately find_package() runs at configuration time, and the outputs from ExternalProject_Add() are not available until build time, so you cannot rely on find_package() and need to manually list the targets from each dependency. Also, it does not compose well for libraries that use your library.

3. You use a package manager, such as vcpkg or Conan that will automatically download and "install" (in $HOME) things before configuration time, and then rely on find_package(). Great experience overall, it composes well, but requires your first-time users to install one more thing before trying your code (the package manager).

@coryan, what do you think? Especially as a consumer of opencensus-cpp as a library. What's the right approach here?

I believe, and this is just like My Opinion:tm:, that you should let folks configure how the dependencies are found. Your first-time users will prefer the simplicity of ExternalProject_Add(), the folks that create packages based on your library would prefer if you did not dynamically downloaded things.

[coryan]

Drive-by comments, take them with a grain of salt, feel free to ignore them too.

[coryan]

nit: be consistent IF or if everywhere (I think the trend is towards lowercase).

[coryan]

I think you want target_compile_definitions() in the targets that need this definition, not need to push it on everybody.

[coryan]

This is unusual... is that to avoid installing the headers twice?

[meastp]

I think so. To be honest I took that example from somewhere else. What is the proper way to do it? :)

[coryan]

CMake has a native module to find gtest:

https://cmake.org/cmake/help/latest/module/FindGTest.html

why not use it here?

[coryan]

Ah, because you want to download it.

[meastp]

Well, this is the rationale for changing from "inline" ExternalProject_Add to doing it via find_package

1. Adding install/export failed because I then also had to export the abseil targets, which seemed wrong to me
2. find_package is then a point of customization: in vcpkg I'll just delete the FindModule.cmake for abseil and googletest, and use the built-in packages for vcpkg
3. when abseil supports find_package directly (with a install/targets and a config-file) this approach will still work
4. for GTest the native module is missing gmock, unfortunately :( - else I would be happy to use that
5. the only thing that does not work properly is that I have to cmake build the abseil and googletest targets before I can build the opencensus. This is because (I think) ExternaProject_Add runs at build time (not configure time) and for some reason cmake is not smart enough to build the dependencies on demand.
   What I want is e.g. to build an opencensus test and automatically build abseil and gtest when they are required, while also supporting install/export for opencensus. I found an approach on stackoverflow, see the other reply. If this works, might that be okay? :)

[coryan]

nit: newline at end of file.

[coryan]

Is this the right copyright?

[meastp]

The approach in cmake/Findabseil.cmake doesn't seem right to me: we shouldn't be enumerating build targets inside of abseil.

TL;DR; if you want to use ExternalProject_Add() then yes.

Longer version, these are the most common, (there are many more) choices for dependency management in CMake

1. You assume they are all installed and use `find_package()`, maybe with some helpers, to discover where they are. It works great, but the experience for a first-time user is terrible because they have to manually download and install everything before trying your library.

2. You download things with `ExternalProject_Add()`, much better experience for your first-time users, no manual installation of dependencies.  Unfortunately  `find_package()` runs at configuration time, and the outputs from `ExternalProject_Add()` are not available until build time, so you cannot rely on `find_package()` and need to manually list the targets from each dependency.  Also, it does not compose well for libraries that use your library.

3. You use a package manager, such as `vcpkg` or `Conan` that will automatically download and "install" (in `$HOME`) things before configuration time, and then rely on `find_package()`. Great experience overall, it composes well, but requires your first-time users to install one more thing before trying your code (the package manager).

@coryan, what do you think? Especially as a consumer of opencensus-cpp as a library. What's the right approach here?

I believe, and this is just like My Opinion™️, that you should let folks configure how the dependencies are found. Your first-time users will prefer the simplicity of ExternalProject_Add(), the folks that create packages based on your library would prefer if you did not dynamically downloaded things.

I'm a beginner with cmake, so feel free to correct me, but:

The reason I did this to support find_package for the external dependencies was to be able to support install/export opencensus targets. I think that from cmake's pow the targets from the external dependencies were part of the build, and thus those targets had to be exported as well - I didn't think this made sense, so I wrote a kind of FindModule.cmake that also builds with ExternalProject_Add to support the current workflow.

However:
With the current structure, abseil and googletest targets have to be built manually, prior to building any opencensus libs/tests. I think the "best practice" way to get around this is something like the answer in https://stackoverflow.com/questions/44990964/how-to-perform-cmakefind-package-at-build-stage-only . If I implement this - and building works properly - would this be acceptable to you instead of going back to the current solution?

Also, to be able to support vcpkg, using find_package is kind of required (because the dependencies already exist as packages in vcpkg). (However, it is possible to patch opencensus cmake definition before building with vcpkg).

[g-easy]

@meastp: Regarding the out-of-tree example, I've updated https://github.com/census-ecosystem/opencensus-cpp-example to have a CMake version of the build system. Feel free to use that for testing from the point of view of a user of the opencensus-cpp library.

[g-easy]

Regarding https://stackoverflow.com/questions/44990964/how-to-perform-cmakefind-package-at-build-stage-only - this seems a little odd, but IMO much less bad than enumerating all of our dependencies' targets. What do other folks think? @coryan? @fancl20? @isaachier?

[g-easy]

you should let folks configure how the dependencies are found.

I agree with this.

How about if we add a knob via set() so the user can choose between three alternatives:

1. opencensus-cpp uses ExternalProject_Add() unconditionally. (and make this the default, because it's a good experience for new users)
2. opencensus-cpp calls find_package() unconditionally. (if a dependency is missing, the user has to install it. This is what packagers will want to use)
3. opencensus-cpp does nothing. The "outer workspace" has to import all the dependencies, bazel style. e.g. Currently opencensus-cpp-example imports abseil before opencensus-cpp, the latter uses the former.

[isaachier]

Regarding https://stackoverflow.com/questions/44990964/how-to-perform-cmakefind-package-at-build-stage-only - this seems a little odd, but IMO much less bad than enumerating all of our dependencies' targets. What do other folks think? @coryan? @fancl20? @isaachier?

CMake has historically been more interested in supporting existing patterns than setting a standard for third party dependencies. CMake 3.11 actually started supporting something similar to Bazel's http_archive, but it fetches at configuration time, which is better for CMake's configuration overall. See more about it here: https://cmake.org/cmake/help/latest/module/FetchContent.html#module:FetchContent.

[coryan]

@isaachier Thanks for the pointers to FetchContent I did not know about it. It seems that it only works with add_subdirectory() which unfortunately brings all the targets in the dependency into the (single, global) target namespace, so you have to avoid conflicts "somehow". Did I get that right? If so, it strikes me as similar to using git submodules.

[isaachier]

@coryan I imagine git submodules works just as well for this sort of thing, just a bit annoying to maintain correctly, especially with transitive dependencies. Actually, reading the documentation I linked above, I see there is a great example describing multiple projects that depend on each other and how the top-level project "wins" in terms of determining the transitive dependencies.

@g-easy, if you like the idea of FetchContent but not sure if you can use that new feature in CMake (only available in >= v3.11), you can actually just copy FetchContent.cmake and FetchContent.cmake.in into your project and it should work.

[g-easy]

Thanks for the pointer, @isaachier !

CMake 3.11 looks a bit too new today but that will change: https://packages.debian.org/search?keywords=cmake

Good to know we can just import the implementation though.

Whereas ExternalProject_Add() downloads at build time, the FetchContent module makes content available immediately, allowing the configure step to use the content in commands like add_subdirectory(), include() or file() operations.

I'm sorry but I don't understand how this helps. I think we still want the user to specify how opencensus-cpp should find its dependencies, e.g. find_package vs download its own.

[g-easy]

@meastp: In the interest of keeping things moving, would you like to split out the install (+ hdrs) part of this PR into its own PR, while we keep discussing find_package + dependencies here?

[isaachier]

@g-easy, I agree it is not necessarily a good solution for users to define their own versions. However, it allows them to override the version easily from the top-level CMakeLists.txt if you follow the FetchContent convention.

[meastp]

Sure, I'll work on headers and msvc nominmax while this is discussed, and submit them in their own PRs :)

[bogdandrutu]

@g-easy what is the status of this?

[g-easy]

@bogdandrutu Short term: waiting for headers and MSVC changes to be split out.

Longer term I'd like to see #256 (comment) implemented.

[meastp]

Hi,

I am working on separate changes, and experimented with replacing the current ExternalProject with FetchContent and this works very well and reduces the amount of code substantially. Are you interested in a PR with this? Since you have Bazel, requiring CMake >= 3.11 might be okay? :)

Then (because the amount of code is reduced) it will also be easier to implement an option to switch between FetchContent and find_package...