Experimental CMake support #238

g-easy · 2018-11-21T03:52:00Z

This is addressing issue #86

Rolling my changes up as a PR to make reviews easier.

Factor out into an opencensus_lib() function.

On case insensitive systems like Windows.

g-easy · 2018-11-21T03:56:31Z

Two comments from @coryan that weren't resolved on the commit:

Namespacing: a later commit namespaced all the OpenCensus targets. Is this sufficient for now?

g-easy · 2018-11-21T03:57:17Z

I get googletest via ExternalProject, and only if OpenCensus is being built with tests (i.e. when developing OC, not when developing an app that uses OC). Is this reasonable for now?

coryan

Generally it looks good, some suggestions that you can ignore if they make no sense.

cmake/README.md

coryan · 2018-11-21T14:46:29Z

examples/CMakeLists.txt

+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+# add_subdirectory(grpc) TODO


Instead of using grpc as a subdirectory consider using it via externalproject_add like you do for Abseil. That way you do not get their targets in your namespace, and you would be testing a configuration that more closely resembles what you get via find_package().

I agree.

But this is examples/grpc/ which is a hello world client + server.

I haven't gotten as far as adding the grpc integration yet. It will probably happen in a followup PR.

coryan · 2018-11-21T14:47:33Z

cmake/OpenCensusDeps.cmake

+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+if(BUILD_TESTING)


Why not use externalproject_add() for googletest too?

I thought I was. If BUILD_TESTING is enabled, I do the usual dance starting on L19 with configure_file, which uses cmake/googletest.CMakeLists.txt which has the externalproject_add() call.

It works with more CMake generators.

meastp · 2018-11-27T19:14:49Z

Hi,

This is great, thank you for working on this! :)

I have experimented with adding opencensus-cpp to vcpkg and building it. Currently only static opencensus-cpp configurations are supported.

I then use vcpkg's version of gtest and abseil, both of which are included with find_package.

I have made some changes:

IF(MSVC)
  add_definitions(-DNOMINMAX) 
ENDIF()

This fixes compilation errors on MSVC.

Replaced the ${OPENCENSUS_INCLUDE_DIR} with the built in ${CMAKE_SOURCE_DIR} (not ${CMAKE_CURRENT_SOURCE_DIR})
I have added support for find_package for opencensus-cpp ( install( TARGETS ... ) and install ( EXPORT ...).
When doing this I also added include files for the targets ( added a HEADERS argument when using the opencensus_lib function).

I would like to:

Add google benchmarks (vcpkg also supports google benchmark, so it should be easy to add).
Precisely specify which link libraries are PRIVATE to the implementation and PUBLIC (exposed to clients) when using target_link_libraries.
With support for find_package added, I think the add_library(${PROJECT_NAME}::${NAME} ALIAS ${_NAME}) should be removed.

How do you feel about this? Which changes are you willing to merge? :)

coryan · 2018-11-27T19:19:26Z

@meastp

IF(MSVC)
  add_definitions(-DNOMINMAX) 
ENDIF()

Shouldn't that be target_compile_definitions() for the right targets?

I have added support for find_package for opencensus-cpp ( install( TARGETS ... ) and install ( EXPORT ...).

👍

meastp · 2018-11-27T20:13:39Z

@meastp
IF(MSVC)
  add_definitions(-DNOMINMAX) 
ENDIF()
Shouldn't that be target_compile_definitions() for the right targets?

Ah, you're right, of course, thanks :) I'm reading up on modern cmake to be able to contribute here, and I overlooked this function.

I think that the distinction between PRIVATE and PUBLIC library link dependencies from each target is needed for this to make sense, though. (I can do this if there is interest, but I want to know which changes are acceptable to you first - see previous reply/message)

g-easy · 2018-11-28T05:57:07Z

I have experimented with adding opencensus-cpp to vcpkg and building it. Currently only static opencensus-cpp configurations are supported.

I then use vcpkg's version of gtest and abseil, both of which are included with find_package.

Awesome!

I have added support for find_package for opencensus-cpp ( install( TARGETS ... ) and install ( EXPORT ...).
When doing this I also added include files for the targets ( added a HEADERS argument when using the opencensus_lib function).

This is great, opencensus-cpp needs to support find_package at some point.

I would like to:

Add google benchmarks (vcpkg also supports google benchmark, so it should be easy to add).

I'd definitely like Benchmark support (I think there's a TODO for it). Is there an ExternalProject version of this for people who don't use vcpkg?

Precisely specify which link libraries are PRIVATE to the implementation and PUBLIC (exposed to clients) when using target_link_libraries.

I think part of this is done, at least with respect to which libraries we expose in the ::namespace, right?

With support for find_package added, I think the add_library(${PROJECT_NAME}::${NAME} ALIAS ${_NAME}) should be removed.

I don't know enough about CMake yet to be able to make an informed decision here, let's discuss it.

How do you feel about this? Which changes are you willing to merge? :)

This sounds great, and I'm willing to consider all of it. I'm going to have some questions about some of the changes.

If I merge this PR soon, would you be willing to send PRs for the stuff above?

meastp · 2018-11-28T06:47:29Z

This sounds great, and I'm willing to consider all of it. I'm going to have some questions about some of the changes.

If I merge this PR soon, would you be willing to send PRs for the stuff above?

Great, I'll send PRs for the changes when this is merged, and we can discuss each change separately (and in more detail) :)

meastp · 2018-11-28T06:52:32Z

I'm wondering if using object libraries for some of the targets/components would be more appopriate? There are quite a lot of libraries (.lib) generated on build. What do you think?

ref. https://gitlab.kitware.com/cmake/community/wikis/doc/tutorials/Object-Library

coryan · 2018-11-28T13:31:22Z

@meastp @g-easy should we move this discussion to a separate bug?

I'm wondering if using object libraries for some of the targets/components would be more appopriate? There are quite a lot of libraries (.lib) generated on build. What do you think?

If you folks decide to use object libraries remember that position-indepedent code is not enabled by default for them:

https://cmake.org/cmake/help/v3.13/prop_tgt/POSITION_INDEPENDENT_CODE.html

You have to explicitly set the property if you are building them into shared objects. My recommendation is that you make the first round of changes you proposed, and do the object library changes (if @g-easy approves) afterwards.

meastp · 2018-11-28T13:46:20Z

@meastp @g-easy should we move this discussion to a separate bug?

I'm wondering if using object libraries for some of the targets/components would be more appopriate? There are quite a lot of libraries (.lib) generated on build. What do you think?

If you folks decide to use object libraries remember that position-indepedent code is not enabled by default for them:

https://cmake.org/cmake/help/v3.13/prop_tgt/POSITION_INDEPENDENT_CODE.html

You have to explicitly set the property if you are building them into shared objects. My recommendation is that you make the first round of changes you proposed, and do the object library changes (if @g-easy approves) afterwards.

Sure, I'm just wondering if there is another way to structure the targets that do not result in so many library files (especially for shared library builds). I'm not opposed to delaying that discussion 'till after the rest of the changes.

If there are better ways to achieve this goal / other best practices that should be followed, I'm eager to learn about them. :)

bogdandrutu

Ship it.

g-easy · 2018-11-29T04:42:29Z

@meastp I've opened #244 to track all the followups suggested in this PR. :)

g-easy added 6 commits November 20, 2018 18:20

Experimental CMake support.

3713b5a

Namespace all targets as opencensus_*.

5a20ea1

Factor out into an opencensus_lib() function.

build -> .build so it doesn't collide with BUILD.

c536c71

On case insensitive systems like Windows.

Use CMAKE_CXX_STANDARD_REQUIRED.

8ffcd3f

Split up cmake/CMakeLists.txt

64a0a25

Remove guards, configure cmake-format instead.

d6f4b00

coryan reviewed Nov 21, 2018

View reviewed changes

Use ctest in quickstart.

706c4df

It works with more CMake generators.

g-easy mentioned this pull request Nov 26, 2018

Any plan for cmake supporting? #86

Open

g-easy requested a review from bogdandrutu November 28, 2018 05:59

Merge branch 'master' into cmake

7c4c3ed

g-easy requested review from helly25 and removed request for bogdandrutu November 28, 2018 09:25

coryan approved these changes Nov 28, 2018

View reviewed changes

bogdandrutu approved these changes Nov 28, 2018

View reviewed changes

Merge branch 'master' into cmake

8fb004e

g-easy merged commit bd876f6 into census-instrumentation:master Nov 29, 2018

g-easy deleted the cmake branch November 29, 2018 04:35

g-easy mentioned this pull request Nov 29, 2018

CMake followups #244

Open

16 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Experimental CMake support #238

Experimental CMake support #238

g-easy commented Nov 21, 2018

g-easy commented Nov 21, 2018

g-easy commented Nov 21, 2018

coryan left a comment

coryan Nov 21, 2018

g-easy Nov 22, 2018

coryan Nov 21, 2018

g-easy Nov 22, 2018 •

edited

Loading

meastp commented Nov 27, 2018 •

edited

Loading

coryan commented Nov 27, 2018

meastp commented Nov 27, 2018

g-easy commented Nov 28, 2018

meastp commented Nov 28, 2018

meastp commented Nov 28, 2018 •

edited

Loading

coryan commented Nov 28, 2018

meastp commented Nov 28, 2018 •

edited

Loading

bogdandrutu left a comment

g-easy commented Nov 29, 2018

Experimental CMake support #238

Experimental CMake support #238

Conversation

g-easy commented Nov 21, 2018

g-easy commented Nov 21, 2018

g-easy commented Nov 21, 2018

coryan left a comment

Choose a reason for hiding this comment

coryan Nov 21, 2018

Choose a reason for hiding this comment

g-easy Nov 22, 2018

Choose a reason for hiding this comment

coryan Nov 21, 2018

Choose a reason for hiding this comment

g-easy Nov 22, 2018 • edited Loading

Choose a reason for hiding this comment

meastp commented Nov 27, 2018 • edited Loading

coryan commented Nov 27, 2018

meastp commented Nov 27, 2018

g-easy commented Nov 28, 2018

meastp commented Nov 28, 2018

meastp commented Nov 28, 2018 • edited Loading

coryan commented Nov 28, 2018

meastp commented Nov 28, 2018 • edited Loading

bogdandrutu left a comment

Choose a reason for hiding this comment

g-easy commented Nov 29, 2018

g-easy Nov 22, 2018 •

edited

Loading

meastp commented Nov 27, 2018 •

edited

Loading

meastp commented Nov 28, 2018 •

edited

Loading

meastp commented Nov 28, 2018 •

edited

Loading