CMake Policy CMP0074

[svenevs]

This affects all of your find* modules. See CMP0074 documentation, this policy is introduced in CMake 3.12+.

Naively, the fix would just be

if (POLICY CMP0074)
  cmake_policy(SET CMP0074 NEW)
endif()

I looked specifically at the FindFLANN.cmake file, and it does appear to respect FLANN_ROOT. Here's the relevant CMake (v3.12.1) output:

CMake Warning (dev) at CMakeLists.txt:295 (find_package):
  Policy CMP0074 is not set: find_package uses <PackageName>_ROOT variables.
  Run "cmake --help-policy CMP0074" for policy details.  Use the cmake_policy
  command to set the policy and suppress this warning.

  Environment variable FLANN_ROOT is set to:

    /opt/spack/opt/spack/linux-fedora27-x86_64/gcc-7.3.1/flann-1.9.1-3qqgbuypp2goiwqpdmeuoiwa2untqskm

  For compatibility, CMake is ignoring the variable.
This warning is for project developers.  Use -Wno-dev to suppress it.

-- Checking for module 'flann>=1.7.0'
--   Found flann, version 1.9.1
-- Found FLANN: /opt/spack/opt/spack/linux-fedora27-x86_64/gcc-7.3.1/flann-1.9.1-3qqgbuypp2goiwqpdmeuoiwa2untqskm/lib/libflann_cpp.so (Required is at least version "1.7.0")
-- FLANN found (include: /opt/spack/opt/spack/linux-fedora27-x86_64/gcc-7.3.1/flann-1.9.1-3qqgbuypp2goiwqpdmeuoiwa2untqskm/include, lib: optimized;/opt/spack/opt/spack/linu$
-fedora27-x86_64/gcc-7.3.1/flann-1.9.1-3qqgbuypp2goiwqpdmeuoiwa2untqskm/lib/libflann_cpp.so;debug;/opt/spack/opt/spack/linux-fedora27-x86_64/gcc-7.3.1/flann-1.9.1-3qqgbuypp2
goiwqpdmeuoiwa2untqskm/lib/libflann_cpp.so)

For reference:

$ ls -R /opt/spack/opt/spack/linux-fedora27-x86_64/gcc-7.3.1/flann-1.9.1-3qqgbuypp2goiwqpdmeuoiwa2untqskm/lib/
/opt/spack/opt/spack/linux-fedora27-x86_64/gcc-7.3.1/flann-1.9.1-3qqgbuypp2goiwqpdmeuoiwa2untqskm/lib/:
libflann_cpp_s.a  libflann_cpp.so@  libflann_cpp.so.1.9@  libflann_cpp.so.1.9.1*  libflann_s.a  libflann.so@  libflann.so.1.9@  libflann.so.1.9.1*  pkgconfig/

/opt/spack/opt/spack/linux-fedora27-x86_64/gcc-7.3.1/flann-1.9.1-3qqgbuypp2goiwqpdmeuoiwa2untqskm/lib/pkgconfig:
flann.pc

I did a little snooping on Flann specifically, and that module appears to be respecting FLANN_ROOT, but it's also worth mentioning that my flann.pc also shows up in $PKG_CONFIG_PATH, so while the find_{path,library} are seemingly respecting FLANN_ROOT, it's unclear to me if anything here actually needs to change -- pkg-config // cmake // find modules are an area of CMake I'm not super comfortable with...

Other find modules are probably affected, I don't have time to check them all right now. This is a low priority issue, but the reason there is no pull request attached here is because a more thorough investigation of your vendored find modules needs to take place to actually set CMP0074 to NEW.

[taketwo]

We have 19 finder scripts, so this will take some effort to review. Definitely not before the 1.9.0 release.

[svenevs]

Yes, this is very low priority! I doubt many people are even using 3.12 yet 😉

[SergioRAgostinho]

Mac users probably are. Homebrew keeps things up to date. vcpkg users probably as well. Honestly it's mostly the Linux users who get stuck on old versions. 😅 But I definitely don't want to touch this before releasing.

[claudiofantacci]

This warning, and possibly other like this one, but for different targets, are generated by the Find<package>.cmake shipped with PCL because PCLConfig.cmake configures FLANN_ROOT (and many many other <package>_ROOT variables, in particular if PCL_ALL_IN_ONE_INSTALLER is true).

By my understanding, the new policy basically claims that <package>_ROOT variables are now reserved to CMake as hint paths to look for packages configuration and Find<package>.cmake files invoking find_package().

Having a quick look at PCLConfig.cmake, it seems to me that PCL needs quite some changes to comply with this policy. For examples FindFLANN.cmake uses FLANN_ROOT and ENV{FLANN_ROOT} to look directly for the headers and shared/static library.

For the time being, to remove the warning one could just enforce the OLD behaviour that is the default one anyway.

To completely address this issue, we need to check and refactor the variables with suffix _ROOT and eventually set the policy to NEW to the whole project.

[SergioRAgostinho]

I was wanting to focus on c++14 for the next release but seems likes reworking CMake might be more important. I'll ping you all for help after the release if you don't mind. It would be good to assemble a little task force.

[svenevs]

To completely address this issue, we need to check and refactor the variables with suffix _ROOT and eventually set the policy to NEW to the whole project.

This is a really good point. I asked about this on the cpplang slack and one of the CMake devs told me setting *_ROOT in the config file is discouraged. His words:

basically provide the necessary find modules themselves, but don't cache the results of your find calls when installing.

I will stew on this. At the very least, the currently cached values should probably just get a PCL_XXX_ROOT (prefix PCL_) to satisfy the policy. If the user setup an environment variable XXX_ROOT then presumably this environment variable will persist. If it's a configure argument, though, it will lead to trouble for them down the road (they'd need to provide the same -DXXX_ROOT="..." to the project consuming pcl)?

[taketwo]

I don't get the point behind this advice. I absolutely want PCLConfig to find exactly the same dependencies as were used during PCL build.

I will stew on this

Could you explain this expression for non-native speakers here? :)

[SergioRAgostinho]

Likely means that he will reflect on the topic for a while.

…

--

On Tuesday, Sep 11, 2018 at 7:42 PM, Sergey Alexandrov ***@***.*** ***@***.***)> wrote: I don't get the point behind this advice. I absolutely want PCLConfig to find exactly the same dependencies as were used during PCL build. > > I will stew on this > Could you explain this expression for non-native speakers here? :) — You are receiving this because you commented. Reply to this email directly, view it on GitHub (#2425 (comment)), or mute the thread (https://github.com/notifications/unsubscribe-auth/ABoUVgdDKVIXRbS6Wu72CBtIBERUrPaoks5uaAP_gaJpZM4Wgp9y).

[svenevs]

Yes, I meant I need to think about it more. Origin: stew (like beef stew) takes a few hours to cook, you kind of just let it sit there for a while :)

I will probably reach out to the mailing list to solicit advice, the slack conversation was brief and I didn't include much context. The person who told this to me is a cmake king whose advice is generally law ;)

[SergioRAgostinho]

For the time being, to remove the warning one could just enforce the OLD behaviour that is the default one anyway.

I'm open to enforcing this for now.

[claudiofantacci]

This is a really good point. I asked about this on the cpplang slack and one of the CMake devs told me setting *_ROOT in the config file is discouraged.

👍

At the very least, the currently cached values should probably just get a PCL_XXX_ROOT (prefix PCL_) to satisfy the policy.

Exactly, if we want to "minimize" refactoring and maintain the same logic and notation, prefixing PCL_ could be a good way to go.

I don't get the point behind this advice. I absolutely want PCLConfig to find exactly the same dependencies as were used during PCL build.

Yes, and it will be available to still do that 😄 The policy just says that <package>_ROOT is now reserved to CMake, just, as an example, like <package>_DIR is. As a consequence, there is the need of a refactoring, not a complete change in the configuration of PCL project.

I will probably reach out to the mailing list to solicit advice, the slack conversation was brief and I didn't include much context. The person who told this to me is a cmake king whose advice is generally law ;)

Ok thanks, just let us know. I'm confident that we understaood the problem and that a refactoring is all we need. Anyway, I will also discuss this topic here in my workplace with a big CMake contributor and let ou know asap.

I'm open to enforcing this for now.

Ok, I'll try to think about this. I need to understand whether it is better to enforce the policy project wise, or just Find<package>.cmake-wise with localized policy(PUSH) and policy(POP).

[taketwo]

Yes, and it will be available to still do that smile The policy just says that _ROOT is now reserved to CMake, just, as an example, like _DIR is

Yes, the policy does not talk about caching. But the quote from the CMake dev reads: "don't cache the results of your find calls when installing."

[claudiofantacci]

Yes, the policy does not talk about caching. But the quote from the CMake dev reads: "don't cache the results of your find calls when installing."

To me that means that you should not have in a configure file that is consumed by external projects targeting PCL, any variable in the form of <package>_ROOT. That for PCL implies that you can just have the same Config and scripts, but you have to refactor all <package>_ROOT variables in PCL_<package>_ROOT (for example).

I don't know whether we are saying the same thing or not actually ahahah 🤣