-
Notifications
You must be signed in to change notification settings - Fork 421
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
Require CMake 3.10 and Check SSE support directly #506
Require CMake 3.10 and Check SSE support directly #506
Conversation
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.
Looks reasonable to me. Thanks, @sloretz for handling this.
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.
@jamiesnape can you give this your blessing? Seems reasonable to me, but I defer to your expertise.
Reviewed 1 of 1 files at r1.
Reviewable status: complete! all files reviewed, all discussions resolved
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.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @sloretz)
a discussion (no related file):
Blocking while we wait for @jamiesnape
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.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jamiesnape and @sloretz)
a discussion (no related file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
Blocking while we wait for @jamiesnape
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.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @sloretz)
CMakeLists.txt, line 91 at r1 (raw file):
if(CMAKE_SYSTEM_NAME STREQUAL "Emscripten") set(FCL_TARGET_SUPPORT_X64_SSE OFF) elseif(${CMAKE_VERSION} VERSION_GREATER_EQUAL "3.10.0")
According to the minimum required versions of CMake for this project if(... VERSION_GREATER_EQUAL ...) is not supported. Dropping support for old versions of CMake is probably an issue elsewhere.
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.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @jamiesnape and @sloretz)
CMakeLists.txt, line 91 at r1 (raw file):
Previously, jamiesnape (Jamie Snape) wrote…
According to the minimum required versions of CMake for this project if(... VERSION_GREATER_EQUAL ...) is not supported. Dropping support for old versions of CMake is probably an issue elsewhere.
What is the minimum version of cmake that supports VERSION_GREATER_EQUAL
? We recently bumped all the CI requirements to less antiquated versions of Mac OS and Ubuntu, could we simply do the same with CMake? Something that could be advanced here enough to provide the missing functionality, but doesn't require further changes to the current spelling (deferring strategic changes to the build infrastructure to subsequent PRs)?
CMakeLists.txt
Outdated
elseif(${CMAKE_VERSION} VERSION_GREATER_EQUAL "3.10.0") | ||
cmake_host_system_information(RESULT _has_sse QUERY HAS_SSE) | ||
cmake_host_system_information(RESULT _has_sse2 QUERY HAS_SSE2) | ||
if(_has_sse AND _has_sse2) | ||
set(FCL_TARGET_SUPPORT_X64_SSE ON) | ||
else() | ||
set(FCL_TARGET_SUPPORT_X64_SSE OFF) | ||
endif() | ||
else() | ||
set(FCL_TARGET_SUPPORT_X64_SSE ON) |
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_GREATER_EQUAL
was introduced in cmake 3.7 only. But we can easily invert the logics and use VERSION_LESS
that is available in cmake 2.8 as well:
elseif(${CMAKE_VERSION} VERSION_GREATER_EQUAL "3.10.0") | |
cmake_host_system_information(RESULT _has_sse QUERY HAS_SSE) | |
cmake_host_system_information(RESULT _has_sse2 QUERY HAS_SSE2) | |
if(_has_sse AND _has_sse2) | |
set(FCL_TARGET_SUPPORT_X64_SSE ON) | |
else() | |
set(FCL_TARGET_SUPPORT_X64_SSE OFF) | |
endif() | |
else() | |
set(FCL_TARGET_SUPPORT_X64_SSE ON) | |
elseif(${CMAKE_VERSION} VERSION_LESS "3.10.0") | |
set(FCL_TARGET_SUPPORT_X64_SSE ON) | |
else() # Decide based on actual host features (only supported since cmake 3.10) | |
cmake_host_system_information(RESULT _has_sse QUERY HAS_SSE) | |
cmake_host_system_information(RESULT _has_sse2 QUERY HAS_SSE2) | |
if(_has_sse AND _has_sse2) | |
set(FCL_TARGET_SUPPORT_X64_SSE ON) | |
else() | |
set(FCL_TARGET_SUPPORT_X64_SSE OFF) | |
endif() |
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.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @SeanCurtis-TRI and @sloretz)
CMakeLists.txt, line 91 at r1 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
What is the minimum version of cmake that supports
VERSION_GREATER_EQUAL
? We recently bumped all the CI requirements to less antiquated versions of Mac OS and Ubuntu, could we simply do the same with CMake? Something that could be advanced here enough to provide the missing functionality, but doesn't require further changes to the current spelling (deferring strategic changes to the build infrastructure to subsequent PRs)?
If the oldest platform is Bionic, then we can remove everything at
Lines 34 to 40 in b611d50
if(APPLE) | |
cmake_minimum_required(VERSION 3.0.0) | |
elseif(MSVC) | |
cmake_minimum_required(VERSION 3.1.3) | |
else() | |
cmake_minimum_required(VERSION 2.8.12) | |
endif() |
cmake_minimum_required(VERSION 3.10)
and remove the if()
statements here.
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.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @jamiesnape and @sloretz)
CMakeLists.txt, line 91 at r1 (raw file):
Previously, jamiesnape (Jamie Snape) wrote…
If the oldest platform is Bionic, then we can remove everything at
and replace it withLines 34 to 40 in b611d50
if(APPLE) cmake_minimum_required(VERSION 3.0.0) elseif(MSVC) cmake_minimum_required(VERSION 3.1.3) else() cmake_minimum_required(VERSION 2.8.12) endif() cmake_minimum_required(VERSION 3.10)
and remove theif()
statements here.
We've bumped CI to bionic and I don't see us going backwards at all. I'm game for bumping the CMake version and simplifying the code.
@SeanCurtis-TRI @jamiesnape It sounds like the minimum CMake version is going to be bumped to 3.10. Is that something you'd like to see in this PR, or is another one coming? I assumed the latter and removed the CMake version check in 5a87fff. |
I think adding a separate commit before the current one that makes the change to |
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.
Agreed.
Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @jamiesnape, @SeanCurtis-TRI, and @sloretz)
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@openrobotics.org>
5a87fff
to
f4e5530
Compare
The first commit 7c1dd82 now bumps the CMake requirement to 3.10, and the second commit f4e5530 checks SSE support |
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.
Reviewed 1 of 1 files at r2.
Dismissed @rhaschke from a discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @sloretz)
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.
I'll use my super powers to merge as two commits.
Reviewed 1 of 1 files at r2.
Reviewable status: complete! all files reviewed, all discussions resolved
Thanks for the PR! |
Require CMake 3.10 and check if SSE/SSE2 are supported directly.
Check if SSE/SSE2 are supported if the CMake version is new enough to do so.I think this would fix a build failure on armhf and aarch64:
@rhaschke Mind taking a look?
This change is