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 string escaping in ADD_DEFINITIONS #10

Merged
merged 1 commit into from
Oct 16, 2018
Merged

Conversation

AMDmi3
Copy link
Contributor

@AMDmi3 AMDmi3 commented Oct 16, 2018

Recent CMake handles this on its own and does not need a lot of backslashes.

Recent CMake handles this on its own and does not need a lot of backslashes.
@viewizard
Copy link
Owner

Wait, since we are requiere cmake 2.8+. https://github.com/viewizard/astromenace/blob/master/CMakeLists.txt#L1
Will this ok for 2.8?

@AMDmi3
Copy link
Contributor Author

AMDmi3 commented Oct 16, 2018

Well it works for me, and AFAIK CMAKE_MINIMUM_REQUIRED makes CMake behave exactly as the version specified, which would mean it both works with latest CMake version and is still compatible with 2.8.

@viewizard
Copy link
Owner

I am just worry, this could be some kind of surprise, if we have 1 line that need 3.9+ or something. :-D
Sure, 2.8 will be compatible for cmake 3.9, but what about line that supported only by 3.9+ and cmake 2.8?
Btw, any idea, what cmake version have this fixes? 3.9+?

@AMDmi3
Copy link
Contributor Author

AMDmi3 commented Oct 16, 2018

Repeating myself, the assumption is that if it works with CMAKE_MINIMUM_REQUIRED(VERSION 2.8), it's compatible with CMake versions down to 2.8. Without this change though, the build is guaranteed broken with recent CMake versions for sure. Also, CMake 2.8 are long extinct:

Packaging status

@AMDmi3
Copy link
Contributor Author

AMDmi3 commented Oct 16, 2018

Update: the relevant policy is https://cmake.org/cmake/help/v3.12/policy/CMP0005.html.
It was introduced in CMake 2.6. So this change is compatible with and is required for CMake >= 2.6.

@viewizard viewizard merged commit 8cec36b into viewizard:master Oct 16, 2018
@AMDmi3 AMDmi3 deleted the patch-3 branch October 16, 2018 18:16
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