dvidelabs / flatcc

FlatBuffers Compiler and Library in C for C
Apache License 2.0
632 stars 180 forks source link

Newer CMake is going to drop <2.8.12 #217

Closed powderluv closed 1 year ago

powderluv commented 2 years ago

So update to 2.8.12.2 (in Ubuntu 14.04)

CentOS 7 has a way to install CMake3.

This avoids the warning in newer CMakes:

flatcc/CMakeLists.txt:5 (cmake_minimum_required): Compatibility with CMake < 2.8.12 will be removed from a future version of CMake.

mikkelfj commented 2 years ago

Thanks, but: There is a plan to include PR #169 after the next release. Would that solve the issue? A new release is long overdue and I do not feel confident changing the CMake version right before a release.

powderluv commented 2 years ago

Thanks for the quick response. Waiting for next release is not a problem since it is just a warning now. I think #169 may not fix this warning since newer CMake just wont support 2.8 so we have to decide if we want to support old cmake or new cmake in due course :D

midokura-xavi92 commented 2 years ago

Relying on ancient versions of CMake such as 2.8 has more serious implications. For example, CMake policy CMP0077 was introduced in 3.13 so that higher-level CMakeLists.txt can adjust option()s programmatically using set(). Otherwise, the settings from the following example would be ignored by flatcc:

cmake_minimum_required(VERSION 3.13.5)
project(test_project)
set(FLATCC_TEST OFF) # This will be ignored since flatcc currently uses cmake_minimum_required(VERSION 2.8)
add_subdirectory(flatcc)

Typically, CMake would then return the following warning message:

Policy CMP0077 is not set: option() honors normal variables.  Run "cmake
  --help-policy CMP0077" for policy details.  Use the cmake_policy command to
  set the policy and suppress this warning.

If flatcc does not bump cmake_minimum_required to 3.13, the only solution is to move all configurations as command line options e.g.:

$ cmake .. -D FLATCC_TEST=OFF

Which shifts responsibility to end users (who might be or not maintainers of the higher-level CMakeLists.txt), potentially causing confusion and issues.

mikkelfj commented 2 years ago

Historically, certain important Ubuntu versions did not trivially work with more recent CMake. I'm all for moving forward now. Especially since flatcc has just had a new release. As I mentioned, there is a rather large pending PR on upgrading CMake already. However, I cannot invest a lot of time in this for the time being. At the same time, the build system for flatcc needs to be migrated to GH Actions. Especially for *nix builds, but for consistency also Windows builds. Anyone willing to take on this, speak up.

EDIT: GH Actions do not support MSVC 2010 without special installation during build. I'm ready to move forward to something along MSVC 2013 or so.

midokura-xavi92 commented 2 years ago

As I mentioned, there is a rather large pending PR on upgrading CMake already.

I had a look at https://github.com/dvidelabs/flatcc/pull/171, but IMHO this PR introduces too many several architectural changes that affect many aspects of this project i.e., not only build-related stuff, but also integration with Github Actions, cross-compilation and introduction of new APIs such as flatcc_generate_sources, among others.

I would rather suggest to split the effort in #171 into smaller, independent PRs that make it easier for others to review. Therefore, as a first step, I plan to create a PR that identifies obsolete and deprecated CMake constructs within the project and fixes them with modern alternatives.

mikkelfj commented 2 years ago

I tend to agree, but I'd rather have GH Actions working first because the Travis build stopped working after they made a change. So there is no way to verify complex changes across several operating systems and compiler versions as it is. Thanks for having a look.

mikkelfj commented 2 years ago

@midokura-xavi92 don't let this stop you if you have something of interest. This is just the order of events I think makes the most sense.

midokura-xavi92 commented 2 years ago

@mikkelfj thank you very much for your support. I will then provide a few suggestions whenever possible.

mikkelfj commented 1 year ago

This was merged into PR #250 so closing this one. @midokura-xavi92 if you still have some input on CMake changes, the new GH Actions makes it easier to test changes.

midokura-xavi92 commented 1 year ago

Sorry for not bringing attention to this topic earlier - priorities were shifted to other projects, so no time could be dedicated to this suggestion, unfortunately, and I do not know when I could take back on it.

mikkelfj commented 1 year ago

Fair enough, thanks for the update.