Skip to content

Conversation

@powderluv
Copy link
Contributor

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.

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor

mikkelfj commented Jun 2, 2022

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
Copy link
Contributor

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

I had a look at #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
Copy link
Contributor

mikkelfj commented Jun 3, 2022

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
Copy link
Contributor

mikkelfj commented Jun 9, 2022

@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
Copy link
Contributor

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

@mikkelfj mikkelfj force-pushed the master branch 4 times, most recently from a8e7a8a to d7f50b2 Compare November 1, 2022 10:48
@mikkelfj mikkelfj mentioned this pull request Nov 17, 2022
@mikkelfj
Copy link
Contributor

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.

@mikkelfj mikkelfj closed this Nov 21, 2022
@midokura-xavi92
Copy link
Contributor

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
Copy link
Contributor

Fair enough, thanks for the update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants