CMake Set temporarily the policy CMP0074 to OLD

[svenevs]

@SergioRAgostinho sorry for disappearing on this.

Ref: #2425

FWIW the warnings about CMP0048 and CMP0054 are subtly different. CMake is saying "hey I told you this would happen, but the OLD version is legit going away". For 0048 the main hiccup is what you want done with the dev part of the version number, project(PCL VERSION major.minor.patch.tweak) but they must all be integers. Having the "dev" is nice for strings but not for version comparison.

Anyway, I think freezing CMake stuff at this point is probably a good idea, and make 1.9.1 prioritize upgrading the cmake stuff. Having all of the policies fixed is nice, but at the end of the day we can probably fix the build system up before anything OLD truly gets deleted from CMake.

Other people should test the find_package(PCL) stuff besides me. It went fine on my end, but it's important other people test it...

For reference, the changes to PCLConfig.cmake are really just

# at the very top
if(POLICY CMP0074)
  cmake_policy(PUSH)
  cmake_policy(SET CMP0074 OLD)
endif()

# ... a bunch of whitespace auto-trimmed by my editor ...

# at the very bottom
if(POLICY CMP0074)
  cmake_policy(POP)
endif()

[claudiofantacci]

I will test this PR asap 😄.
Thanks for the effort.

I'm not sure that

if(POLICY CMP0074)
  # TODO:
  # 1. Find*.cmake modules need to be individually verified.
  # 2. PCLConfig.cmake needs to be changed.
  cmake_policy(SET CMP0074 OLD)
endif()

in the main CMakeLists.txt is necessary, but I don't think I will be able to test all possible combination of CMake configuration. In particular I reckon that an important test is by setting PCL_ALL_IN_ONE_INSTALLER to true.

[SergioRAgostinho]

For 0048 the main hiccup is what you want done with the dev part of the version number, project(PCL VERSION major.minor.patch.tweak) but they must all be integers. Having the "dev" is nice for strings but not for version comparison.

For version comparison we have a variable called PCL_VERSION_PLAIN which sets the tweak version number to 99 whenever we're in development mode. Have a look at #1905, specifically this commit 48729a8.

If I understand correctly, ultimately we might only need to rename PCL_VERSION_PLAIN to the actual PCL_VERSION and the current PCL_VERSION to something new called PCL_VERSION_STR. This should ensure the integer restriction is respected.

Anyway, I think freezing CMake stuff at this point is probably a good idea, and make 1.9.1 prioritize upgrading the cmake stuff. Having all of the policies fixed is nice, but at the end of the day we can probably fix the build system up before anything OLD truly gets deleted from CMake.

That's ok by me.

I'm not sure that

if(POLICY CMP0074)
  # TODO:
  # 1. Find*.cmake modules need to be individually verified.
  # 2. PCLConfig.cmake needs to be changed.
  cmake_policy(SET CMP0074 OLD)
endif()

in the main CMakeLists.txt is necessary, but I don't think I will be able to test all possible combination of CMake configuration.

It will explicitly suppress warnings and it will be a policy which we will know that we need to revisit once we're done with the next release. There should be no harm for the time being.

[SergioRAgostinho]

@svenevs please try to tone down your white space trimmer 😅

Good news is that we have something in the oven ready to be rolled out. #2376

[claudiofantacci]

It will explicitly suppress warnings and it will be a policy which we will know that we need to revisit once we're done with the next release. There should be no harm for the time being.

If that's the case, then ok 😄

I'm trying this PR right now. If you also want my feedback just wait few hours (or less).

[claudiofantacci]

Works just fine in my setup 👍

[SergioRAgostinho]

Gonna merge this and update the change log for the release.

Edit: Thanks @svenevs and @claudiofantacci

[bartoszek]

There is some issue with this PR.
When script reach return()

pcl/PCLConfig.cmake.in

Line 475 in f9cbe14

return()

it causes cmake_policy(POP) beeing skipped - resoulting in this error when building cloudcompare:

/usr/share/pcl-1.9/PCLConfig.cmake(523):  if(NOT PCL_TO_FIND_COMPONENTS )
/usr/share/pcl-1.9/PCLConfig.cmake(524):  return()
/usr/share/pcl-1.9/PCLConfig.cmake(9999):  CGAL_run_at_the_end_of_configuration(CMAKE_CURRENT_LIST_DIR MODIFIED_ACCESS /build/cloudcompare-git/src/cloudcompare/plugins/core/qPCL/PclUtils /build/cloudcompare-git/src/cloudcompare/plugins/core/qPCL/PclUtils/CMakeLists.txt /build/cloudcompare-git/src/cloudcompare/plugins/core/qPCL/PclUtils/CMakeLists.txt;/usr/share/pcl-1.9/PCLConfig.cmake )
/usr/lib/cmake/CGAL/CGAL_enable_end_of_configuration_hook.cmake(17):  if(NOT access STREQUAL MODIFIED_ACCESS OR value )
/usr/lib/cmake/CGAL/CGAL_enable_end_of_configuration_hook.cmake(20):  return()
CMake Error in /usr/share/pcl-1.9/PCLConfig.cmake:
  cmake_policy PUSH without matching POP
Call Stack (most recent call first):
  plugins/core/qPCL/PclUtils/CMakeLists.txt:4 (find_package)

[taketwo]

@svenevs @claudiofantacci why did we decide to use push/pop? The last relevant thought on this topic was from Claudio here:

Since both files will be (most likely) calld by include() and find_package() calls, we don't need to add policy(PUSH/POP) because is handeld automatically by CMake in that way. My suggestion was to use that anyway just to avoid any possible misuse, error, wrong integration by other people smile

The reason why I am asking is that these stack manipulations keep on bringing problems. Apart from above referenced #2625 (which we fixed in 1.9.1), there is a new bug report here. To be honest, I don't understand what's wrong there and have no idea how to fix it besides from getting rid of push/pop altogether. What do you think about this possibility? Note that in the meantime we are setting policy 74 to NEW, so ever less reasons to push/pop, right?

[claudiofantacci]

@taketwo you are right that we can get rid of the push/pop, especially if we are setting policy 74. I tried to understand the problem reported here, hopefully we go through it.

[svenevs]

why did we decide to use push/pop?

Because for that release we needed to set the policy to the old behavior (all of the *_ROOT variables). The current choice of logic and naming of configured variables means that users cannot wield *_ROOT to affect where PCL finds things in this config file.

However, if you remove the policy stuff the warnings are easy enough to ignore and it really makes no difference to me whether its kept or not. In reality all it means is that something users are supposed to have control over do not have that control. If you think it's buggy then just remove it. The implication: anybody using a cmake aware of CMP0074 but using an older policy stack (e.g., using cmake 3.13 and a minimum required version of 3.3) will always get a warning with find_package(PCL) about the fact that CMP0074 is not set.

Note that CMP0011 and CMP0074 are related, and in my humble opinion PCL should have zero desire to support CMP0011 being set to OLD. As @claudiofantacci pointed out to them, it's because they are setting their policy stack based on 2.6. If setting to NEW (or better, using a newer minimum required version) fixes it for them, then the answer should just be "we do not support CMP0011 being OLD".

---

Sorry for being unresponsive lately, things are going really poorly for me right now and it's not going to get better for the rest of the year :/ What I was hoping to do was just obliterate this file and do a proper config module in addition to exporting the PCL cmake targets. Maybe it would be helpful for me to outline what I wanted to do in a "future cmake issue"? I can outline the steps that I think should be taken and try and give counsel, but I'm worried about "being in charge of the issue" because I don't have time to implement any of it.

For example, do we really need the PCL All in One Installer? What's it's use case and is it even still being maintained? Is that the pre-built binary stuff for osx and for windows? Those are both pretty ancient, and these days if you want to enable pre-built binaries you should probably just get on conan (after getting a modern cmake build). Removing just the pcl all in one thing would greatly reduce the complexity of the config file.

[taketwo]

why did we decide to use push/pop?

Because for that release we needed to set the policy to the old behavior (all of the *_ROOT variables).

Yeah, this I understood. But I thought that cmake_policy is absolutely guaranteed to create a new stack frame when entering into find_package (thus making explicit manipulation not necessary). Was not aware of CMP0011.

---

it's not going to get better for the rest of the year :/

This should be over in a fortnight 😄

Maybe it would be helpful for me to outline what I wanted to do in a "future cmake issue"?

👍

do we really need the PCL All in One Installer?

AFAIK @UnaNancyOwen uses it to generate prebuilt binaries. The page you referenced is not updated, but we have binaries uploaded to Github releases.