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

Consolidate and simplify CMAKE_POLICY entries #6780

Merged
merged 11 commits into from
Aug 3, 2023
Merged

Conversation

tresf
Copy link
Member

@tresf tresf commented Jul 26, 2023

Inspired by: #6758 (comment)

Supersedes: #6778

Quoting @DomClark:

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 tresf mentioned this pull request Jul 26, 2023
@tresf
Copy link
Member Author

tresf commented Jul 26, 2023

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

@DomClark It appears CMP0057 is still needed for mingw. Is this statement correct? I'm afraid how many rabbit holes I'll be jumping down.

@DomClark
Copy link
Member

It appears CMP0057 is still needed for mingw. Is this statement correct? I'm afraid how many rabbit holes I'll be jumping down.

I think the problem is that InstallDependencies.cmake is used in the install script, which is automatically generated and doesn't contain a cmake_minimum_required (or cmake_policy(VERSION)) command. As such, CMake runs it with old policy behaviours.
Either the policy needs to be set manually, or cmake_minimum_required/cmake_policy(VERSION) should be used.

Further complicating this is policy scope. In very old CMake versions, including files did not begin a new policy scope, so policies set in an included file would affect the caller. This is now controlled by CMP0011: you'll notice this policy was also set in that file, but I don't believe it would work as intended. By the time the policy is set, the file has already been included without a new policy scope, so any policies set in the remainder of the file will nevertheless influence the calling code.

With this in mind, I would aim for as few policy changes as possible, so I think just setting CMP0057 is the way to go. A comment explaining why this needs to be done wouldn't go amiss.

@Rossmaxx
Copy link
Contributor

Why the changes to the cmt submodule? Maybe a missing submodule update?

@tresf

This comment was marked as outdated.

@tresf
Copy link
Member Author

tresf commented Jul 31, 2023

Why the changes to the cmt submodule? Maybe a missing submodule update?

Fixed, thanks.

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.

4 participants