modernize cmake #37
modernize cmake #37
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
|
|
I signed it! |
|
CLAs look good, thanks! |
| set(CMAKE_EXPORT_COMPILE_COMMANDS ON) | ||
|
|
||
| #not used? | ||
| #list(APPEND CMAKE_MODULE_PATH "${CMAKE_SOURCE_DIR}/third_party/cmake") |
There was a problem hiding this comment.
I thought this was needed for the FindGFlags and FindGLog. Are you configuring with or without those?
There was a problem hiding this comment.
Oh good point! I have not tried all options yet.
| # - lower cmake version a bit ~3.7 to support older systems | ||
| # - fix abseil warnings or try to use SYSTEM / -isystem includes | ||
|
|
||
| cmake_minimum_required(VERSION 3.8) |
There was a problem hiding this comment.
Separate PR for increasing cmake min version.
What's the advantage of doing this? Removing version >= 3.8.0 checks?
There was a problem hiding this comment.
2.8.x is really too old it does not really support targets (even though they became available in 2.8.11 iirc) and it does not have support for newer standards and language features. I think it is best to lower the version to something that still supports at least c++14, but something else might be ok. It could be better to use something that is available in the current Ubuntu release or in Travis containers. Eventually it is up to you! Please don't insist to support CentOS 6:)
There was a problem hiding this comment.
It looks like 3.8.0 is only a year old. That seems a bit new to require.
Maybe something in the 3.4-3.7 range? What's the oldest version that supports the features you want to use?
| include(CMakeDependentOption) | ||
| include(CheckCXXCompilerFlag) |
There was a problem hiding this comment.
Separate PR to remove unneeded options.
There was a problem hiding this comment.
I am not sure if we will get on the same page here. I favor to rewrite the whole file on my branch and squash everything. I am willing to reason with you about every change, pass all tests and try to find the best solution. I do not want to create a meaningful commit with message for every line. That would be a too big time investment for me. Of course I will remove unrelated changes like in coder.i. And eventually smaller commits could be created when once the work is done.
There was a problem hiding this comment.
The problem is that one big diff makes things hard to review.
It doesn't have to be one change per line, just logically related changes with finer granularity than "fix everything related to cmake".
Maybe I can submit the fine-grained changes, and you keep merging until your change is empty. Would that work for you?
|
|
||
| option(WITH_GFLAGS "Use gflags to change command line flags." OFF) | ||
| add_feature_info(GFLAGS WITH_GFLAGS | ||
| "allows changing command line flags.") | ||
| add_feature_info(GFLAGS WITH_GFLAGS "allows changing command line flags.") |
There was a problem hiding this comment.
Is it ok to reformat everything to your style when I am done?
There was a problem hiding this comment.
Sure. Is there a standard CMake indentation style?
Please take a look at the current changes and let me know if they are something you like. If so I'll continue to work on my branch. This PR should eventually fix #35 #11 and maybe #6