Skip to content
This repository has been archived by the owner on Jul 31, 2023. It is now read-only.

CMake: Replace ExternalProject_Add with FetchContent #290

Merged
merged 10 commits into from
Mar 14, 2019

Conversation

meastp
Copy link
Contributor

@meastp meastp commented Feb 28, 2019

This reduces the amount of code for including/building dependencies. The only drawback is an increase in the CMake version required.

@googlebot
Copy link

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.

ℹ️ Googlers: Go here for more info.

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@bogdandrutu
Copy link

@g-easy this looks good to me (from a person with 0 knowledge about cmake, but I like when code gets deleted). What do you think?

@meastp
Copy link
Contributor Author

meastp commented Feb 28, 2019

We already discussed it in #244, but I'd like @g-easy to look at it anyway :)

@g-easy
Copy link
Contributor

g-easy commented Feb 28, 2019

Please make this pass on Travis. :)

@meastp
Copy link
Contributor Author

meastp commented Feb 28, 2019

Well, I can't run the tools/format.sh script as I'm on Windows. Could you help me with that like last time? :)

The other failure is simply because the travis node's CMake version is too old:

CMake Error at CMakeLists.txt:15 (cmake_minimum_required):
CMake 3.11 or higher is required. You are running version 3.9.2

IS trusty in the travisci-config Ubuntu Trusty 14.04 ? That's a pretty old distro... Why aren't you using xenial?

@bogdandrutu
Copy link

@meastp you can create a small docker image map your clone directory, and run the script :) maybe we should have a "tools/format_docker.sh" which actually is useful not only for your usecase but also to make sure that we run that specific clang-format.

@meastp
Copy link
Contributor Author

meastp commented Feb 28, 2019

@meastp you can create a small docker image map your clone directory, and run the script :) maybe we should have a "tools/format_docker.sh" which actually is useful not only for your usecase but also to make sure that we run that specific clang-format.

What if travis could just push a commit to the pull request with the required changes if the tools/format.sh fails? :)

https://gist.github.com/willprice/e07efd73fb7f13f917ea

@g-easy
Copy link
Contributor

g-easy commented Feb 28, 2019

Regular Xenial ships cmake 3.5.1. I got this running locally with ubuntu:cosmic. https://launchpad.net/ubuntu/+source/cmake

@g-easy
Copy link
Contributor

g-easy commented Feb 28, 2019

@coryan Is the cmake 3.11 requirement okay with you?

@coryan
Copy link

coryan commented Feb 28, 2019

And Ubuntu:bionic is 3.10, and let's not get started on CentOS. I think 3.11 might be too high given the current versions on popular distros.

@meastp
Copy link
Contributor Author

meastp commented Feb 28, 2019

@coryan What about including FetchContent.cmake directly, then, as was suggested in #256 (comment)? It is not much code, the opencensus-cpp cmake-code is cleaner, and in a while when the popular distros get CMake >= 3.11, we can just delete FetchContent.cmake and FetchContent.cmake.in.

@g-easy
Copy link
Contributor

g-easy commented Mar 1, 2019

I think FetchContent.cmake is worth a try. What is the minimum version requirement with this option?

@googlebot
Copy link

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.

ℹ️ Googlers: Go here for more info.

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@meastp
Copy link
Contributor Author

meastp commented Mar 5, 2019

@coryan @g-easy It looks like this works with the old cmake versions. :)

@g-easy Could you please fix format.sh for me?

g-easy added a commit to g-easy/opencensus-cpp that referenced this pull request Mar 14, 2019
This file comes from upstream, so let's keep it as-is.
This file will be added in census-instrumentation#290.
g-easy added a commit that referenced this pull request Mar 14, 2019
This file comes from upstream, so let's keep it as-is.
This file will be added in #290.
Copy link
Contributor

@g-easy g-easy left a comment

Choose a reason for hiding this comment

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

Nice! Thanks!

@g-easy g-easy merged commit 71fd6a7 into census-instrumentation:master Mar 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants