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

Simplify CMake Policies #6778

Merged
merged 2 commits into from
Jul 26, 2023
Merged

Simplify CMake Policies #6778

merged 2 commits into from
Jul 26, 2023

Conversation

tresf
Copy link
Member

@tresf tresf commented Jul 23, 2023

Per #6758 (comment)

Notes:

Copy link
Member

@DomClark DomClark left a comment

Choose a reason for hiding this comment

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

We require a minimum of CMake 3.9 now, which means all of these policies will use the new behaviour by default (since CMake 3.9 includes the policies up to and including CMP0069). Thus the only line that still has an effect is the one that sets CMP0050 to OLD. Furthermore, I don't think we require that old behaviour any more: the policy value was introduced in #3583 because the old behaviour was used in the RemoteVstPlugin build scripts, but that use was since removed in ea15469. I can't find any other uses of the old behaviour, and LMMS configures and builds correctly without it (at least on my machine, using MSVC).

There are a couple of other cmake_policy commands elsewhere in the project. There are two in cmake/modules/InstallDependencies.cmake, which are also obsolete. Another is in plugins/CarlaBase/CMakeLists.txt, which already has a decent comment above it. Finally, there's one in cmake/modules/DefineInstallVar.cmake, which is still required, and lacks a comment.

@tresf
Copy link
Member Author

tresf commented Jul 26, 2023

Thanks @DomClark. For the sake of consistency and setting the standard, I'm including the vertabitim descriptions next to the policy, even for those that are seemingly obvious.

All other requests should be addressed.

I've added a placeholder in the main CMakeLists.txt file, since it'll help the rebase for when #6758 is merged, and will make it more obvious where this policy goes per #6758 (comment).

@tresf tresf merged commit 55937a9 into LMMS:master Jul 26, 2023
Comment on lines 5 to 7
IF(NOT CMAKE_VERSION VERSION_LESS 3.9)
CMAKE_POLICY(SET CMP0068 OLD)
CMAKE_POLICY(SET CMP0068 OLD) # RPATH settings on macOS do not affect install_name.
ENDIF()
Copy link
Member

Choose a reason for hiding this comment

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

We now require CMake 3.9, so isn't it possible to either remove the if or switch to the NEW behavior?

@tresf
Copy link
Member Author

tresf commented Jul 26, 2023

This wasn't merged, I mucked something up. 🤦 fixing now.

@tresf
Copy link
Member Author

tresf commented Jul 26, 2023

@PhysSong said:

👀

Yeah, pushed to the wrong branch 🤦. I disabled branch protection, reverted, just gotta use a new PR now.

New PR here: #6780

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants