Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bump CMake requirement to 3.10, don't enable C language #1624

Merged
merged 2 commits into from
Jul 21, 2020
Merged

Bump CMake requirement to 3.10, don't enable C language #1624

merged 2 commits into from
Jul 21, 2020

Conversation

tambry
Copy link
Contributor

@tambry tambry commented Jul 18, 2020

Using a version range enables the policies up to the current version.
This fixes warnings due to deprecated policies in newer versions. Testing shows there to be no policy changes that are problematic.

A fallback is retained for versions <3.11 to emulate the version range behaviour.

@gabime
Copy link
Owner

gabime commented Jul 18, 2020

This fixes warnings due to deprecated policies in newer versionsmo

Can you provide more details about the warnings?

@tambry
Copy link
Contributor Author

tambry commented Jul 19, 2020

This fixes warnings due to deprecated policies in newer versionsmo

Can you provide more details about the warnings?

For example when CMAKE_INTERPROCEDURAL_OPTIMIZATION is ON:

CMake Warning (dev) at CMakeLists.txt:148 (add_library):
  Policy CMP0069 is not set: INTERPROCEDURAL_OPTIMIZATION is enforced when
  enabled.  Run "cmake --help-policy CMP0069" for policy details.  Use the
  cmake_policy command to set the policy and suppress this warning.

  INTERPROCEDURAL_OPTIMIZATION property will be ignored for target 'spdlog'.
This warning is for project developers.  Use -Wno-dev to suppress it.

@gabime
Copy link
Owner

gabime commented Jul 19, 2020

I am concerned it would be hard to test new features that might be affected by policy. It would mean testing on the full range of policies instead of just 3.11

@tambry
Copy link
Contributor Author

tambry commented Jul 19, 2020

It would mean testing on the full range of policies instead of just 3.11

The current code is equivalent to 3.2...3.11 anyway. Per your thinking you're either already testing that range currently, which I doubt, or it'd be sufficient to simply test with 3.18.

@gabime
Copy link
Owner

gabime commented Jul 19, 2020

Right, but now it would be worse. The range of possible policies would be 3.2...3.11 and 3.12..3.18

@tambry
Copy link
Contributor Author

tambry commented Jul 19, 2020

Sure, but I don't see why testing with CMake 3.18 wouldn't suffice? It'd cover all the policies.

Alternatively, it would in my opinion be sensible to bump the minimum to something more recent. LLVM very recently bumped the minimum to 3.13 for the next release.

@gabime
Copy link
Owner

gabime commented Jul 19, 2020

It'd cover all the policies.
Are you sure? Does a given cmake policy is always backward compatible with all previous ones?

@tambry
Copy link
Contributor Author

tambry commented Jul 20, 2020

Are you sure? Does a given cmake policy is always backward compatible with all previous ones?

Good point, it's not that guaranteed that'd be the case. But that's unlikely and even more unlikely to affect spdlog, since spdlog's use of CMake isn't very weird.

Running CMake with -Wdev on spdlog still reveals CMP0069 as the only policy to be affected by bumping the CMake policy range to 3.2...3.18. I see no harm in this bump. If there are issues someone will report them and we'll be more ready for the time we bump the minimum version due to having testing with the newer policies enabled.

@gabime
Copy link
Owner

gabime commented Jul 20, 2020

If minimum version 3.13 is good enough for llvm it is certainly good enough for me. Lets just raise it to 3.13

@gabime gabime closed this Jul 20, 2020
@gabime gabime reopened this Jul 20, 2020
* Reduces the range of possible version we'd need to test with.
* Enables newer policies reducing possible deprecation warnings from new policies.
* Allows removing some code for compatibility with older versions.
* Coincides with LLVM's bump to requiring CMake 3.13.
@tambry tambry changed the title Enable CMake policies up to 3.18 Bump CMake requirement to 3.13, don't enable C language Jul 20, 2020
@tambry
Copy link
Contributor Author

tambry commented Jul 20, 2020

I've changed the PR to instead bump the CMake requirement to 3.13.

I also added another commit to not enable the C language in CMake. That doesn't seem to be necessary and not enabling C results in a significant initial configure time speedup.

@gabime
Copy link
Owner

gabime commented Jul 20, 2020

Actually it is necessary under some platforms. see #1442

@tambry
Copy link
Contributor Author

tambry commented Jul 20, 2020

Actually it is necessary under some platforms. see #1442

I believe that has been fixed in CMake 3.4 by this commit, which changed FindThreads to work when the C language isn't enabled.

@gabime
Copy link
Owner

gabime commented Jul 20, 2020

oh. good to know

@gabime
Copy link
Owner

gabime commented Jul 20, 2020

on second thought. 3.13 seems bit too aggressive to me. especially since major distros still use older versions (http://lists.llvm.org/pipermail/llvm-dev/2020-April/140585.html)

spdlog doesn't seem to actually require the C language.
Not enabling it results in a significant initial configure time speedup.
@tambry
Copy link
Contributor Author

tambry commented Jul 21, 2020

How about CMake 3.10 then? That'd allow removing the policy setting and C language enablement workarounds.

@gabime
Copy link
Owner

gabime commented Jul 21, 2020

Sounds better. Thanks!

@tambry
Copy link
Contributor Author

tambry commented Jul 21, 2020

Already updated the PR for CMake 3.10. 😉

@gabime gabime changed the title Bump CMake requirement to 3.13, don't enable C language Bump CMake requirement to 3.10, don't enable C language Jul 21, 2020
@gabime gabime merged commit ae02fba into gabime:v1.x Jul 21, 2020
@tambry tambry deleted the cmake_range branch July 21, 2020 17:14
bachittle pushed a commit to bachittle/spdlog that referenced this pull request Dec 22, 2022
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.

2 participants