Push policy stack in InstallDependencies module

[DomClark]

Since #6780, we get this warning when running the install script:

CMake Warning (dev) in D:/a/lmms/lmms/cmake/modules/InstallDependencies.cmake:
  Policy CMP0011 is not set: Included scripts do automatic cmake_policy PUSH
  and POP.  Run "cmake --help-policy CMP0011" for policy details.  Use the
  cmake_policy command to set the policy and suppress this warning.

  The included script

    D:/a/lmms/lmms/cmake/modules/InstallDependencies.cmake

  affects policy settings.  CMake is implying the NO_POLICY_SCOPE option for
  compatibility, so the effects are applied to the including context.
Call Stack (most recent call first):
  D:/a/lmms/lmms/build/cmake/install/cmake_install.cmake:45 (INCLUDE)
  D:/a/lmms/lmms/build/cmake_install.cmake:112 (include)
This warning is for project developers.  Use -Wno-dev to suppress it.

That module previously set CMP0011, presumably to silence this error, but as I mentioned in #6780 (comment), this fix is incorrect. It only silences the warning, doing nothing to create the intended policy scope. (This is a deliberate choice by CMake - it silences the warning to support the use case where a helper module is included in order to set policies.) To create a policy scope for a file that has already been included without one, the cmake_policy(PUSH) and cmake_policy(POP) commands can be used.

(Apologies to @tresf - I should have thought of this when reviewing that PR.)

[tresf]

Thanks for the detailed explanation. I spent quite a while researching your comments from #6780 and reading the PR and what wasn't immediately obvious is that (from what I'm reading ) the install script comes before the main CMakeLists.txt, right? Otherwise, we'd set CMP0011 and CMP0057 project wide. If that's the case, maybe the comment wording could be improved, because I don't think I understood that at time of authoring #6780.

[DomClark]

The main CMakeLists.txt runs to generate both the build system and install script. The program is built, then the install script is run to copy the files to the appropriate location for use or packaging. It's not so much whether the install script comes before or after the main CMakeLists.txt (although it does technically come after), but rather that they run in separate processes and never the twain shall meet.

[tresf]

The main CMakeLists.txt runs to generate both the build system and install script. The program is built, then the install script is run to copy the files to the appropriate location for use or packaging. It's not so much whether the install script comes before or after the main CMakeLists.txt (although it does technically come after), but rather that they run in separate processes and never the twain shall meet.

Right, because it's interpreted at INSTALL(...) time. I'm not trying to delay this PR, but rather understand this approach a bit better... Why do we do this? Historically, we'd register the CMake functions globally and then call them when they're needed.

I realize this may cause a slight overhead when NOT installing since the scripts will need to be included/interpreted in advance, but it would also mean that any changes to scripts we call from INSTALL(...) would be a bit easier to test.

The reason I ask is because the PUSH() POP() behavior can be set by policy and the if() IN_LIST can be set project-wide, so this behavior can be influenced without the workarounds if we treat it as a regular include(...). Perhaps I'm over thinking this, I just wanted to offer the feedback now while we're talking about it.

[DomClark]

Right, because it's interpreted at INSTALL(...) time. [...] Why do we do this? Historically, we'd register the CMake functions globally and then call them when they're needed. [...] This behavior can be influenced without the workarounds if we treat it as a regular include(...).

There's no global state shared between configuration and installation scripts. Functions can't be shared, nor can policies or minimum required versions. It's a different script running in a different process. The code in InstallDependencies.cmake needs to run at installation time, so it's included in the installation script. The installation script doesn't include CMakeLists.txt, so no code in there will have any effect.

As for why we do this, I'm not sure of the exact details. It looks like it was mostly written by Lukas-W and Reflexe, who aren't around much these days. Ideally we would specify dependencies well enough in the configuration script that CMake's automatically generated installation script does the right thing without any custom code. However, that's probably far from trivial to fix up, and isn't the intent of this pull request.

the PUSH() POP() behavior can be set by policy

This is true, but the policy doesn't retroactively apply to files that have already been included. Setting policy CMP0011 in a file will not introduce a scope for that file, only for future inclusions.

[tresf]

There's no global state shared between configuration and installation scripts.

This is hard documentation to find. I found a question on StackOverflow confirming it: https://stackoverflow.com/a/62023303/3196753.

Ideally we would specify dependencies well enough in the configuration script that CMake's automatically generated installation script does the right thing without any custom code. However, that's probably far from trivial to fix up, and isn't the intent of this pull request.

Agreed.

the policy doesn't retroactively apply to files that have already been included

Right, I was referring to the former (stared state) scenario, which doesn't exist.

So now that I understand the behavior better, this PUSH() and POP() is superfluous, since it can't actually pollute the parent script. This means that calling PUSH() and POP() actually do nothing in this scenario, it's just additional unnecessary steps to suppress a warning.

That module previously set CMP0011, presumably to silence this error, but as I mentioned in #6780 (comment), this fix is incorrect. It only silences the warning, doing nothing to create the intended policy scope.

But in your own words, there's no shared state, right? So isn't this superfluous either way?

Lastly, and just asking here because documentation on this is sparse... is CMAKE_MINIMUM_REQUIRED(...) prohibited in INSTALL(...) scripts? If it's allowed, it seems like it could help influence both of these policies.

[DomClark]

So now that I understand the behavior better, this PUSH() and POP() is superfluous, since it can't actually pollute the parent script.

It can't pollute the configuration script, no, but that wasn't a concern, given that the configuration script already has that policy set anyway. It can still affect the rest of the installation script, however.

My concern is that the vast majority of the installation script is automatically generated, and not under our control. I have no idea if it is compatible with all settings of all policies, or, even if it is right now, whether it is guaranteed to remain so.

is CMAKE_MINIMUM_REQUIRED(...) prohibited in INSTALL(...) scripts? If it's allowed, it seems like it could help influence both of these policies.

It is allowed, yes. We could replace the cmake_policy(SET CMP0057 NEW) line with cmake_minimum_required(VERSION 3.9). I like this alternative - opting in to more modern CMake style in general feels cleaner than listing individual policies. Again, though, this won't introduce a policy scope because the file has already been included. If the policy scope isn't manually introduced, it will also pollute the installation script with far more policies than the current option.

it's just additional unnecessary steps to suppress a warning.

I admit that one motivating factor for this PR is to reduce terminal spam when running the install script, but warnings still exist for a reason. Policies that are set by one of our files for the benefit of that file shouldn't affect the script globally, as that's not our responsibility to control, and we can't predict what impact it will have. We also don't want to risk relying on it implicitly, and have things break if CMake enables CMP0011 by default (or we remove the file current under discussion, etc.).

[tresf]

My concern is that the vast majority of the installation script is automatically generated, and not under our control. I have no idea if it is compatible with all settings of all policies, or, even if it is right now, whether it is guaranteed to remain so.

Ok, you're referring to the built-in cmake installation script here, not our own. This is the part that I was missing. I agree, better not to poke the bear here.