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

Fix required CMake version when CMAKE_CXX_COMPILER_LAUNCHER is defined #566

Merged
merged 1 commit into from
Nov 29, 2023

Conversation

tokoro10g
Copy link
Contributor

@tokoro10g tokoro10g commented Nov 10, 2023

This PR fixes minimum CMake version in CMakeLists.txt

@tokoro10g
Copy link
Contributor Author

tokoro10g commented Nov 15, 2023

Related Issues & PRs

The issue is introduced by the version 20231031 0.10.0

Summary of Changes

This PR fixes minimum CMake version in CMakeLists.txt.
CMAKE_CXX_COMPILER_LAUNCHER CMake variable is available from version 3.4 according to the official documentation:
https://cmake.org/cmake/help/v3.4/variable/CMAKE_LANG_COMPILER_LAUNCHER.html#variable:CMAKE_%3CLANG%3E_COMPILER_LAUNCHER

Validation

Checked that the CMAKE_CXX_COMPILER_LAUNCHER variable is properly picked up by CMake older than 3.17.
(Ubuntu 20.04, cmake 3.16.3)


@twslankard Hi, could you review this patch?

Copy link
Collaborator

@Samahu Samahu left a comment

Choose a reason for hiding this comment

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

I agree with @tokoro10g, the CMAKE_CXX_COMPILER_LAUNCHER was introduced in CMake 3.4. so 3.10 should be covered.

@Samahu Samahu self-assigned this Nov 17, 2023
@tokoro10g
Copy link
Contributor Author

@Samahu
I updated the description of this PR to meet the requirements on CI. Could you rerun the check?

@Samahu Samahu merged commit 1f73d01 into ouster-lidar:master Nov 29, 2023
1 check failed
@Samahu
Copy link
Collaborator

Samahu commented Nov 29, 2023

@tokoro10g unfortunately GH or the script that checks PR description does not pick the changes, but nonetheless I went ahead and merged your proposed fix. Thanks for your contribution!

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