-
Notifications
You must be signed in to change notification settings - Fork 10.2k
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 minimum to 2.8.12 #3094
Bump CMake minimum to 2.8.12 #3094
Conversation
From what I can tell the AppVeyor failure is unrelated. |
It has been a month since this PR has been submitted. @derekmauro can you take a quick look at it? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would appreciate this being merged, Github actions Ubuntu image has updated it's preinstalled CMake version, causing our builds to fail because of this GTest issue.
unsubscribe
…On Fri, Dec 4, 2020 at 8:08 PM Robert Chisholm ***@***.***> wrote:
***@***.**** approved this pull request.
Would appreciate this being merged, our CI has just automatically updated
CMake version and hit failure due to this.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#3094 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AO7RFJUAM3CI6XOELSLPIRDSTDX4VANCNFSM4TFDSKFQ>
.
|
@@ -1,7 +1,7 @@ | |||
# Note: CMake support is community-based. The maintainers do not use CMake | |||
# internally. | |||
|
|||
cmake_minimum_required(VERSION 2.8.8) | |||
cmake_minimum_required(VERSION 2.8.12) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Adding ... to version rage to future proof code when using cmake > 3.18 that explicitly
# deprecated 2.8.12 features
cmake_minimum_required(VERSION 2.8.12...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Version range was added in 3.12 to this command and the goal is to support 2.8.12 as a minimum.
Also, empty max range?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the linked docs:
The optional
<max>
version, if specified, must be at least the<min>
version and affects policy settings as described below. If the running version of CMake is older than 3.12, the extra...
dots will be seen as version component separators, resulting in the...<max>
part being ignored and preserving the pre-3.12 behavior of basing policies on<min>
.
With that being said I don't think the ...
makes much sense to add if no <max>
is being specified.
Ping. Lots of projects are stumbling on this bug on CI. |
Over 2 months ago, the 1 failing check contains 2 failed tests due to timeout. Given the number of projects that are stumbling due to this, could someone please rerun the checks, and if the timeout clears, please merge this PR? !!! Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This minor change looks good to me. It will certainly quiet many deprecation warnings that have been clogging many screens for over 2 months. Anyone up for closing this by Christmas?
…sion PiperOrigin-RevId: 349296827
Whohoo! |
This fixes the cmake deprecation warnings Ref: google/googletest#3094
This fixes the cmake deprecation warnings Ref: google/googletest#3094
Would you be willing to cherry-pick this change into the release branch (v1.10.x)? Thanks in advance. |
README says:
So I don't think there will be any more releases. |
Considering this critical bug fix took 3 months after reporting it, took an outside contributor to fix this tiny change, was reviewed by outside contributors, and wasn't merged for another 2 months, all this despite many users speaking up, this doesn't inspire sufficient confidence to me for a "Live at Head" policy. On further thought, it kinda makes me want to look for alternatives to Google Test if this is how it is maintained... |
Thanks for the perspective. Just because a large company puts resources into a project does not mean it is well maintained or will be in the future... |
I just started testing doctest a few days ago and it seems awesome indeed. The author fixed a bug I reported in less than a day despite not having access to the compiler in question. The only thing I'm missing a bit at the moment is a proper test adapter for VS (but it works well in VS Code). |
doctest is good. but google/benchmark uses googletest. |
Is there any suggestion as to when we might see this in a release? |
@LukeAI gtest doesn't have releases anymore. It's "live at head": https://github.com/google/googletest#live-at-head |
The relevant fix is: google/googletest#3094
Fixes #3040