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

Set CMake Max Version #3691

Merged
merged 1 commit into from
Dec 17, 2020

Conversation

paul-elliott-arm
Copy link
Member

@paul-elliott-arm paul-elliott-arm commented Sep 18, 2020

Description

Make sure that CMake policy CMP0012 is set to NEW, without which FindPython3 and FindPython2 functionality becomes broken with CMake 3.18.2 if searching by location. This will be fixed in CMake 3.18.3, however this policy will remain the default, so this is a safe change. Ensure CMP0011 is set to NEW to avoid warnings being issued about policy push / pop with CMake 3.18.0 or newer.

This closes #3690

Status

READY

Requires Backporting

No (Newer version of FindPython3 not used in either LTS branch)

Todos

  • Tests

Steps to test or reproduce

Project should now build with CMake version 3.18.2, which it won't without this patch.

@paul-elliott-arm paul-elliott-arm added bug needs-review Every commit must be reviewed by at least two team members, Arm Contribution needs-backports Backports are missing or are pending review and approval. needs-ci Needs to pass CI tests labels Sep 18, 2020
@paul-elliott-arm paul-elliott-arm self-assigned this Sep 18, 2020
@paul-elliott-arm paul-elliott-arm removed the needs-backports Backports are missing or are pending review and approval. label Sep 18, 2020
@paul-elliott-arm paul-elliott-arm force-pushed the fix_cmake branch 2 times, most recently from fb75e89 to 9464f51 Compare September 24, 2020 15:48
CMakeLists.txt Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
@daverodgman daverodgman added the needs-reviewer This PR needs someone to pick it up for review label Oct 2, 2020
@gilles-peskine-arm gilles-peskine-arm removed the needs-ci Needs to pass CI tests label Oct 5, 2020
@ronald-cron-arm ronald-cron-arm self-requested a review October 7, 2020 15:40
d3zd3z
d3zd3z previously approved these changes Oct 26, 2020
Copy link
Contributor

@d3zd3z d3zd3z left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tested with cmake 3.17.4, 3.18.2, 3.18.3, and 3.18.4. I will also test with some older version of cmake that are supported.

d3zd3z
d3zd3z previously approved these changes Oct 27, 2020
CMakeLists.txt Outdated
@@ -17,6 +17,15 @@
#

cmake_minimum_required(VERSION 2.8.12)

# https://cmake.org/cmake/help/latest/policy/CMP0011.html
# Fix warnings in CMake >= 3.18.0. OLD is deprecated.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"OLD is deprecated."

From your comment below:
"This is necessary to avoid a warning when using any FindPython* on cmake > 3.18. The warning indicates this policy has to be set one way or the other, having it set to default is deprecated."

it seems that what is deprecated is "not setting the policy", not that OLD is deprecated.

From the commit message it seems both are deprecated.

Thus I'm confused. Could you clarify the comment ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue here is that that it is basically both. If its not set, then a warning is generated, and the OLD behaviour is used. The OLD behaviour is deprecated by definition, and may be removed in a future version of CMake, thus the only sensible thing to set it to is NEW. This is all detailed in the link, so I didn't want a massive comment, but will try and make it more clear.

CMakeLists.txt Outdated
# Fix warnings in CMake >= 3.18.0. OLD is deprecated.
cmake_policy(SET CMP0011 NEW)
# https://cmake.org/cmake/help/latest/policy/CMP0012.html
# We require this for FindPython3 to work with CMake 3.18.2, but this will also generate warnings in
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about:
"Setting the CMP0012 policy to NEW is required for FindPython3 to work with CMake 3.18.2 (bug in this particular version).
Otherwise, setting the CMP0012 policy is required for CMake versions >= 3.18.3 and a warning is generated if it is not the case."
Again, I am not completely sure about what is actually deprecated: not setting the policy or the OLD value of the policy, both ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, its both.

Make sure that CMP0012 is set to NEW, without which FindPython3 and
FindPython2 functionality becomes broken with CMake 3.18.2 if searching
by location. CMP0012 being set to OLD is also deprecated as of 3.18.3.
Ensure CMP0011 is set to NEW to avoid warnings and deprecated behaviour
being issued about policy push / pop with CMake 3.18.0 or newer.

Signed-off-by: Paul Elliott <paul.elliott@arm.com>
Copy link
Contributor

@d3zd3z d3zd3z left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've also tested against cmake 2.8.12.1, as long as #3897 is also brought in, it builds.

Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

Thanks for adding comments as requested by Ronald. I think the comments don't really need more details than their current version, considering they include links to the official CMake documentation which has all the details (and hopefully will be kept up-to-date).

@mpg mpg removed the needs-reviewer This PR needs someone to pick it up for review label Dec 8, 2020
Copy link
Contributor

@ronald-cron-arm ronald-cron-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for improving the comments. It is all clear to me now.

@ronald-cron-arm ronald-cron-arm merged commit 3c537fe into Mbed-TLS:development Dec 17, 2020
Sacha0 added a commit to JuliaLang/julia that referenced this pull request Dec 17, 2020
MBedTLS fails to build with CMake 3.18.2, due to a bug in
CMake 3.18.2 causing failure to find python3. For more information,
please see Mbed-TLS/mbedtls#3690 and
https://gitlab.kitware.com/cmake/cmake/-/issues/21204.

This pull request applies the MBedTLS patch that works around this
issue from Mbed-TLS/mbedtls#3691, which
has been merged into MBedTLS's development branch but not
yet into a tagged release.
Sacha0 added a commit to JuliaLang/julia that referenced this pull request Dec 17, 2020
MBedTLS fails to build with CMake 3.18.2, due to a bug in
CMake 3.18.2 causing failure to find python3. For more information,
please see Mbed-TLS/mbedtls#3690 and
https://gitlab.kitware.com/cmake/cmake/-/issues/21204.

This pull request applies the MBedTLS patch that works around this
issue from Mbed-TLS/mbedtls#3691, which
has been merged into MBedTLS's development branch but not
yet into a tagged release.
Sacha0 added a commit to JuliaLang/julia that referenced this pull request Dec 21, 2020
MBedTLS fails to build with CMake 3.18.2, due to a bug in
CMake 3.18.2 causing failure to find python3. For more information,
please see Mbed-TLS/mbedtls#3690 and
https://gitlab.kitware.com/cmake/cmake/-/issues/21204.

This pull request applies the MBedTLS patch that works around this
issue from Mbed-TLS/mbedtls#3691, which
has been merged into MBedTLS's development branch but not
yet into a tagged release. The latest minor release as of that
merge was 2.25.0, so it's probably safe to conjecture that this
patch will be live as of minor release 2.26.0; it's also
possible that this patch will land on patch releases for
2.24.0 and 2.25.0, but this author doesn't know.
Sacha0 added a commit to JuliaLang/julia that referenced this pull request Dec 21, 2020
MBedTLS fails to build with CMake 3.18.2, due to a bug in
CMake 3.18.2 causing failure to find python3. For more information,
please see Mbed-TLS/mbedtls#3690 and
https://gitlab.kitware.com/cmake/cmake/-/issues/21204.

This pull request applies the MBedTLS patch that works around this
issue from Mbed-TLS/mbedtls#3691, which
has been merged into MBedTLS's development branch but not
yet into a tagged release. The latest minor release as of that
merge was 2.25.0, so it's probably safe to conjecture that this
patch will be live as of minor release 2.26.0; it's also
possible that this patch will land on patch releases for
2.24.0 and 2.25.0, but this author doesn't know.
staticfloat pushed a commit to JuliaLang/julia that referenced this pull request Dec 21, 2020
MBedTLS fails to build with CMake 3.18.2, due to a bug in
CMake 3.18.2 causing failure to find python3. For more information,
please see Mbed-TLS/mbedtls#3690 and
https://gitlab.kitware.com/cmake/cmake/-/issues/21204.

This pull request applies the MBedTLS patch that works around this
issue from Mbed-TLS/mbedtls#3691, which
has been merged into MBedTLS's development branch but not
yet into a tagged release. The latest minor release as of that
merge was 2.25.0, so it's probably safe to conjecture that this
patch will be live as of minor release 2.26.0; it's also
possible that this patch will land on patch releases for
2.24.0 and 2.25.0, but this author doesn't know.
Sacha0 added a commit to JuliaLang/julia that referenced this pull request Dec 22, 2020
MBedTLS fails to build with CMake 3.18.2, due to a bug in
CMake 3.18.2 causing failure to find python3. For more information,
please see Mbed-TLS/mbedtls#3690 and
https://gitlab.kitware.com/cmake/cmake/-/issues/21204.

This pull request applies the MBedTLS patch that works around this
issue from Mbed-TLS/mbedtls#3691, which
has been merged into MBedTLS's development branch but not
yet into a tagged release. The latest minor release as of that
merge was 2.25.0, so it's probably safe to conjecture that this
patch will be live as of minor release 2.26.0; it's also
possible that this patch will land on patch releases for
2.24.0 and 2.25.0, but this author doesn't know.
KristofferC pushed a commit to JuliaLang/julia that referenced this pull request Dec 25, 2020
)

MBedTLS fails to build with CMake 3.18.2, due to a bug in
CMake 3.18.2 causing failure to find python3. For more information,
please see Mbed-TLS/mbedtls#3690 and
https://gitlab.kitware.com/cmake/cmake/-/issues/21204.

This pull request applies the MBedTLS patch that works around this
issue from Mbed-TLS/mbedtls#3691, which
has been merged into MBedTLS's development branch but not
yet into a tagged release. The latest minor release as of that
merge was 2.25.0, so it's probably safe to conjecture that this
patch will be live as of minor release 2.26.0; it's also
possible that this patch will land on patch releases for
2.24.0 and 2.25.0, but this author doesn't know.
staticfloat pushed a commit to JuliaLang/julia that referenced this pull request Jan 15, 2021
)

MBedTLS fails to build with CMake 3.18.2, due to a bug in
CMake 3.18.2 causing failure to find python3. For more information,
please see Mbed-TLS/mbedtls#3690 and
https://gitlab.kitware.com/cmake/cmake/-/issues/21204.

This pull request applies the MBedTLS patch that works around this
issue from Mbed-TLS/mbedtls#3691, which
has been merged into MBedTLS's development branch but not
yet into a tagged release. The latest minor release as of that
merge was 2.25.0, so it's probably safe to conjecture that this
patch will be live as of minor release 2.26.0; it's also
possible that this patch will land on patch releases for
2.24.0 and 2.25.0, but this author doesn't know.
@paul-elliott-arm paul-elliott-arm deleted the fix_cmake branch March 9, 2021 17:11
ElOceanografo pushed a commit to ElOceanografo/julia that referenced this pull request May 4, 2021
)

MBedTLS fails to build with CMake 3.18.2, due to a bug in
CMake 3.18.2 causing failure to find python3. For more information,
please see Mbed-TLS/mbedtls#3690 and
https://gitlab.kitware.com/cmake/cmake/-/issues/21204.

This pull request applies the MBedTLS patch that works around this
issue from Mbed-TLS/mbedtls#3691, which
has been merged into MBedTLS's development branch but not
yet into a tagged release. The latest minor release as of that
merge was 2.25.0, so it's probably safe to conjecture that this
patch will be live as of minor release 2.26.0; it's also
possible that this patch will land on patch releases for
2.24.0 and 2.25.0, but this author doesn't know.
staticfloat pushed a commit to JuliaLang/julia that referenced this pull request Dec 23, 2022
)

MBedTLS fails to build with CMake 3.18.2, due to a bug in
CMake 3.18.2 causing failure to find python3. For more information,
please see Mbed-TLS/mbedtls#3690 and
https://gitlab.kitware.com/cmake/cmake/-/issues/21204.

This pull request applies the MBedTLS patch that works around this
issue from Mbed-TLS/mbedtls#3691, which
has been merged into MBedTLS's development branch but not
yet into a tagged release. The latest minor release as of that
merge was 2.25.0, so it's probably safe to conjecture that this
patch will be live as of minor release 2.26.0; it's also
possible that this patch will land on patch releases for
2.24.0 and 2.25.0, but this author doesn't know.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug needs-review Every commit must be reviewed by at least two team members,
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CMake build appears to be broken with CMake 3.18.2
6 participants