search

PointCloudLibrary / pcl

Point Cloud Library (PCL)
https://pointclouds.org/
Other
9.83k stars 4.6k forks source link

CMake Policy CMP0074 #2425

Closed svenevs closed 5 years ago

svenevs commented 6 years ago

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 commented 6 years ago

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

svenevs commented 6 years ago

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

SergioRAgostinho commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

--

On Tuesday, Sep 11, 2018 at 7:42 PM, Sergey Alexandrov <notifications@github.com (mailto:notifications@github.com)> 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 (https://github.com/PointCloudLibrary/pcl/issues/2425#issuecomment-420378361), or mute the thread (https://github.com/notifications/unsubscribe-auth/ABoUVgdDKVIXRbS6Wu72CBtIBERUrPaoks5uaAP_gaJpZM4Wgp9y).

svenevs commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 🤣

taketwo commented 5 years ago

I believe we are saying different things and have different interpretations of what the guy meant. Thus :+1: if @svenevs will further clarify this through the mailing list.

svenevs commented 5 years ago

The implication of the advice was entirely removing the *_ROOT variables from PCLConfig.cmake. The rename was a proposition to satisfy the policy.

I will reach out to the mailing list today and post back tomorrow if I get responses :) depending on responses I will likely just add a hot fix PR too set the policy to old for the imminent release.

svenevs commented 5 years ago

Update: no bites (maybe I asked poorly). How imminent is the imminent release? Could we just commit to doing OLD on this policy on say Sunday if nobody responds on the list?

taketwo commented 5 years ago

I think we should commit to OLD. Even if you get a response by Sunday, we don't want to rework finder and config scripts in a rush and squeeze it in the upcoming release. There is a lot of potential for subtle bugs that can only be discovered by many people trying to use PCL on many systems.

SergioRAgostinho commented 5 years ago

My ideas was to go with OLD now as well. The effort to adopt the new policy starts after that, alongside whatever we identify as "good practices which are currently not being enforced".

claudiofantacci commented 5 years ago

My two cents here: go for OLD with policy(PUSH) & policy(POP) and then tackle this issue with proper time and test after the release.

taketwo commented 5 years ago

Any specific reason to use push/pop? We typically have:

if(POLICY xyz)
  cmake_policy(SET xyz OLD)
endif()
claudiofantacci commented 5 years ago

It is just a convenient way to isolate the code that needs a temporary cmake_policy and will be modified in the near future, as well as controlling the policy stack of the project. It may not be the case here, but if you set a policy project wide you, or contributors, may end up adding even more code that eventually will need to be updated when the policy will changed. policy(PUSH) and policy(POP) is used automatically when a policy is set in files calld by include() and find_package(), hence it may be implicitly call in our particular setting. As a strictly personal opinion, I also find it a little more under my control since a policy(PUSH) must be followed by a policy(POP) 😄.

SergioRAgostinho commented 5 years ago

It may not be the case here, but if you set a policy project wide you, or contributors, may end up adding even more code that eventually will need to be updated when the policy will changed.

This is interesting and desirable. Remains to understand the difference in effort from adopting the simple project wide setting vs the push/pop the policy wherever is needed.

svenevs commented 5 years ago

@claudiofantacci I'm not sure that cmake_policy(PUSH) and cmake_policy(POP) make sense here. We're electing to OLD because sifting through the whole project to find out what may / may not need to change is a big task, we just want a band-aid. (To be very clear, as you will read, I am confused / asking for clarification on where you think the PUSH|POP should go).

From an old but relevant mailing list post

if(POLICY CMP<NNNN>)
  cmake_policy(PUSH)
  cmake_policy(SET CMP<NNNN> OLD)
endif()

# ...
# other code where the policy is set to OLD
# ...

if(POLICY CMP<NNNN>)
  cmake_policy(POP)
endif()

The problem is that in our case this means finding / checking every find_package call to see if we should OLD or NEW there, or alternatively doing a PUSH at the very beginning and a POP at the very end of the root CMakeLists.txt. Which is the same as just setting it to OLD at the top without the PUSH|POP, right?

Does this make sense / do people agree? This one is kind of confusing because (as stated) find_package does introduce a new "policy stack". From CMP0074:

This policy provides compatibility with projects that have not been updated to avoid using <PackageName>_ROOT variables for other purposes.

So um. Do we set it in PCLConfig.cmake too? That's the file that's 100% violating this. If I'm understanding how these call stacks actually work, I think we do need to set it here. But I'm getting really turned around :cry:

claudiofantacci commented 5 years ago

@svenevs setting the policy project wide will not remove the warning for people targeting PCL within their project. The policy is not exported in the PCL configuration file if it set in the main CMakeLists.

Here is the output of git grep _ROOT (you can use the find feature of your browser to highlight the _ROOT substring of the following output):

$ git grep _ROOT
CHANGES.md:* Corrected PCL_ROOT in PCLConfig.cmake
CHANGES.md:* define PCL_ROOT environment variable using the NSIS installer
PCLConfig.cmake.in:    set(BOOST_ROOT "${PCL_ROOT}/3rdParty/Boost")
PCLConfig.cmake.in:    set(EIGEN_ROOT "${PCL_ROOT}/3rdParty/Eigen")
PCLConfig.cmake.in:  elseif(NOT EIGEN_ROOT)
PCLConfig.cmake.in:    get_filename_component(EIGEN_ROOT "@EIGEN_INCLUDE_DIRS@" ABSOLUTE)
PCLConfig.cmake.in:    set(QHULL_ROOT "${PCL_ROOT}/3rdParty/Qhull")
PCLConfig.cmake.in:  elseif(NOT QHULL_ROOT)
PCLConfig.cmake.in:    get_filename_component(QHULL_ROOT "@QHULL_INCLUDE_DIRS@" PATH)
PCLConfig.cmake.in:  if(NOT OPENNI_ROOT AND ("@HAVE_OPENNI@" STREQUAL "TRUE"))
PCLConfig.cmake.in:  if(NOT OPENNI2_ROOT AND ("@HAVE_OPENNI2@" STREQUAL "TRUE"))
PCLConfig.cmake.in:  if(NOT ENSENSO_ROOT AND ("@HAVE_ENSENSO@" STREQUAL "TRUE"))
PCLConfig.cmake.in:  if(NOT davidSDK_ROOT AND ("@HAVE_DAVIDSDK@" STREQUAL "TRUE"))
PCLConfig.cmake.in:    set(FLANN_ROOT "${PCL_ROOT}/3rdParty/Flann")
PCLConfig.cmake.in:  elseif(NOT FLANN_ROOT)
PCLConfig.cmake.in:    get_filename_component(FLANN_ROOT "@FLANN_INCLUDE_DIRS@" PATH)
PCLConfig.cmake.in:    if(EXISTS "${PCL_ROOT}/3rdParty/VTK/lib/cmake")
PCLConfig.cmake.in:      set(VTK_DIR "${PCL_ROOT}/3rdParty/VTK/lib/cmake/vtk-@VTK_MAJOR_VERSION@.@VTK_MINOR_VERSION@" CACHE PATH "The directory containing VTKConfig.cmake")
PCLConfig.cmake.in:      set(VTK_DIR "${PCL_ROOT}/3rdParty/VTK/lib/vtk-@VTK_MAJOR_VERSION@.@VTK_MINOR_VERSION@" CACHE PATH "The directory containing VTKConfig.cmake")
PCLConfig.cmake.in:# PCLConfig.cmake is installed to PCL_ROOT/cmake
PCLConfig.cmake.in:  get_filename_component(PCL_ROOT "${PCL_DIR}" PATH)
PCLConfig.cmake.in:# PCLConfig.cmake is installed to PCL_ROOT/share/pcl-x.y
PCLConfig.cmake.in:  get_filename_component(PCL_ROOT "${CMAKE_CURRENT_LIST_DIR}/../.." ABSOLUTE)
PCLConfig.cmake.in:if(EXISTS "${PCL_ROOT}/include/pcl-${PCL_VERSION_MAJOR}.${PCL_VERSION_MINOR}/pcl/pcl_config.h")
PCLConfig.cmake.in:  set(PCL_INCLUDE_DIRS "${PCL_ROOT}/include/pcl-${PCL_VERSION_MAJOR}.${PCL_VERSION_MINOR}")
PCLConfig.cmake.in:  set(PCL_LIBRARY_DIRS "${PCL_ROOT}/@LIB_INSTALL_DIR@")
PCLConfig.cmake.in:  if(EXISTS "${PCL_ROOT}/3rdParty")
PCLConfig.cmake.in:  endif(EXISTS "${PCL_ROOT}/3rdParty")
PCLConfig.cmake.in:elseif(EXISTS "${PCL_ROOT}/include/pcl/pcl_config.h")
PCLConfig.cmake.in:  set(PCL_INCLUDE_DIRS "${PCL_ROOT}/include")
PCLConfig.cmake.in:  set(PCL_LIBRARY_DIRS "${PCL_ROOT}/lib")
PCLConfig.cmake.in:  if(EXISTS "${PCL_ROOT}/3rdParty")
PCLConfig.cmake.in:  endif(EXISTS "${PCL_ROOT}/3rdParty")
PCLConfig.cmake.in:else(EXISTS "${PCL_ROOT}/include/pcl-${PCL_VERSION_MAJOR}.${PCL_VERSION_MINOR}/pcl/pcl_config.h")
PCLConfig.cmake.in:endif(EXISTS "${PCL_ROOT}/include/pcl-${PCL_VERSION_MAJOR}.${PCL_VERSION_MINOR}/pcl/pcl_config.h")
cmake/Modules/FindEigen.cmake:    HINTS ${PC_EIGEN_INCLUDEDIR} ${PC_EIGEN_INCLUDE_DIRS} "${EIGEN_ROOT}" "$ENV{EIGEN_ROOT}"
cmake/Modules/FindFLANN.cmake:          HINTS ${PC_FLANN_INCLUDEDIR} ${PC_FLANN_INCLUDE_DIRS} "${FLANN_ROOT}" "$ENV{FLANN_ROOT}"
cmake/Modules/FindFLANN.cmake:             HINTS ${PC_FLANN_LIBDIR} ${PC_FLANN_LIBRARY_DIRS} "${FLANN_ROOT}" "$ENV{FLANN_ROOT}"
cmake/Modules/FindFLANN.cmake:             HINTS ${PC_FLANN_LIBDIR} ${PC_FLANN_LIBRARY_DIRS} "${FLANN_ROOT}" "$ENV{FLANN_ROOT}"
cmake/Modules/FindG2O.cmake:    ${G2O_ROOT}/lib/Debug
cmake/Modules/FindG2O.cmake:    ${G2O_ROOT}/lib
cmake/Modules/FindG2O.cmake:    $ENV{G2O_ROOT}/lib/Debug
cmake/Modules/FindG2O.cmake:    $ENV{G2O_ROOT}/lib
cmake/Modules/FindG2O.cmake:    ${G2O_ROOT}/lib/Release
cmake/Modules/FindG2O.cmake:    ${G2O_ROOT}/lib
cmake/Modules/FindG2O.cmake:    $ENV{G2O_ROOT}/lib/Release
cmake/Modules/FindG2O.cmake:    $ENV{G2O_ROOT}/lib
cmake/Modules/FindG2O.cmake:  ${G2O_ROOT}
cmake/Modules/FindG2O.cmake:  $ENV{G2O_ROOT}
cmake/Modules/FindG2O.cmake:  ${G2O_ROOT}/include
cmake/Modules/FindG2O.cmake:  $ENV{G2O_ROOT}/include
cmake/Modules/FindGLEW.cmake:      $ENV{GLEW_ROOT}/include
cmake/Modules/FindGLEW.cmake:      $ENV{GLEW_ROOT}/lib
cmake/Modules/FindGTSAM.cmake:# GTSAM_DIR (or GTSAM_ROOT): (Optional) The install prefix OR source tree of gtsam (e.g. /usr/local or src/gtsam)
cmake/Modules/FindGTSAM.cmake:# Use GTSAM_ROOT or GTSAM_DIR equivalently
cmake/Modules/FindGTSAM.cmake:if(GTSAM_ROOT AND NOT GTSAM_DIR)
cmake/Modules/FindGTSAM.cmake:  set(GTSAM_DIR "${GTSAM_ROOT}")
cmake/Modules/FindGtest.cmake:    HINTS "${GTEST_ROOT}" "$ENV{GTEST_ROOT}"
cmake/Modules/FindGtest.cmake:    HINTS "${GTEST_ROOT}" "$ENV{GTEST_ROOT}"
cmake/Modules/FindOpenNI.cmake:            HINTS ${PC_USB_10_INCLUDEDIR} ${PC_USB_10_INCLUDE_DIRS} "${USB_10_ROOT}" "$ENV{USB_10_ROOT}"
cmake/Modules/FindOpenNI.cmake:               HINTS ${PC_USB_10_LIBDIR} ${PC_USB_10_LIBRARY_DIRS} "${USB_10_ROOT}" "$ENV{USB_10_ROOT}"
cmake/Modules/FindOpenNI.cmake:                "${OPENNI_ROOT}"
cmake/Modules/FindOpenNI.cmake:                "$ENV{OPENNI_ROOT}"
cmake/Modules/FindOpenNI.cmake:                   "${OPENNI_ROOT}"
cmake/Modules/FindOpenNI.cmake:                   "$ENV{OPENNI_ROOT}"
cmake/Modules/FindOpenNI2.cmake:            HINTS ${PC_USB_10_INCLUDEDIR} ${PC_USB_10_INCLUDE_DIRS} "${USB_10_ROOT}" "$ENV{USB_10_ROOT}"
cmake/Modules/FindOpenNI2.cmake:               HINTS ${PC_USB_10_LIBDIR} ${PC_USB_10_LIBRARY_DIRS} "${USB_10_ROOT}" "$ENV{USB_10_ROOT}"
cmake/Modules/FindQhull.cmake:          HINTS "${QHULL_ROOT}" "$ENV{QHULL_ROOT}" "${QHULL_INCLUDE_DIR}"
cmake/Modules/FindQhull.cmake:             HINTS "${QHULL_ROOT}" "$ENV{QHULL_ROOT}"
cmake/Modules/FindQhull.cmake:             HINTS "${QHULL_ROOT}" "$ENV{QHULL_ROOT}"
cmake/Modules/NSIS.template.in:  InstallDir "@CPACK_NSIS_INSTALL_ROOT@\@CPACK_PACKAGE_INSTALL_DIRECTORY@"
cmake/Modules/NSIS.template.in:  !define MUI_STARTMENUPAGE_REGISTRY_ROOT "SHCTX" 
cmake/Modules/NSIS.template.in:  WriteRegExpandStr ${env_hklm} PCL_ROOT "$INSTDIR"
cmake/Modules/NSIS.template.in:  DeleteRegValue ${env_hklm} PCL_ROOT
cmake/Modules/NSIS.template.in:  StrCmp "$INSTDIR" "@CPACK_NSIS_INSTALL_ROOT@\@CPACK_PACKAGE_INSTALL_DIRECTORY@" 0 +2
cmake/Modules/NSIS.template.in:      StrCpy $INSTDIR "@CPACK_NSIS_INSTALL_ROOT@\@CPACK_PACKAGE_INSTALL_DIRECTORY@"
cmake/pcl_all_in_one_installer.cmake:    get_filename_component(BOOST_ROOT "${Boost_INCLUDE_DIR}" PATH)
cmake/pcl_all_in_one_installer.cmake:    get_filename_component(BOOST_ROOT "${BOOST_ROOT}" PATH)
cmake/pcl_all_in_one_installer.cmake:    get_filename_component(EIGEN_ROOT "${EIGEN_INCLUDE_DIRS}" PATH)
cmake/pcl_all_in_one_installer.cmake:    get_filename_component(QHULL_ROOT "${QHULL_INCLUDE_DIRS}" PATH)
cmake/pcl_all_in_one_installer.cmake:    get_filename_component(FLANN_ROOT "${FLANN_INCLUDE_DIRS}" PATH)
cmake/pcl_all_in_one_installer.cmake:    get_filename_component(VTK_ROOT "${VTK_DIR}" PATH)
cmake/pcl_all_in_one_installer.cmake:    get_filename_component(VTK_ROOT "${VTK_ROOT}" PATH)
cmake/pcl_all_in_one_installer.cmake:        get_filename_component(VTK_ROOT "${VTK_ROOT}" PATH)
cmake/pcl_all_in_one_installer.cmake:            DIRECTORY "${${DEP}_ROOT}/"
cmake/pcl_cpack.cmake:    set(CPACK_NSIS_INSTALL_ROOT "$PROGRAMFILES64")
cmake/pcl_cpack.cmake:    set(CPACK_NSIS_INSTALL_ROOT "$PROGRAMFILES32")
cmake/pcl_utils.cmake:      set(INCLUDE_INSTALL_ROOT
cmake/pcl_utils.cmake:      set(INCLUDE_INSTALL_ROOT "include") # Android, don't put into subdir
cmake/pcl_utils.cmake:    set(INCLUDE_INSTALL_DIR "${INCLUDE_INSTALL_ROOT}/pcl")
cmake/uninstall_target.cmake.in:message(STATUS "Uninstalling \"@CMAKE_INSTALL_PREFIX@/@INCLUDE_INSTALL_ROOT@\"")
cmake/uninstall_target.cmake.in:if(EXISTS "@CMAKE_INSTALL_PREFIX@/@INCLUDE_INSTALL_ROOT@")
cmake/uninstall_target.cmake.in:        ARGS "-E remove_directory \"@CMAKE_INSTALL_PREFIX@/@INCLUDE_INSTALL_ROOT@\""
cmake/uninstall_target.cmake.in:            "Problem when removing \"@CMAKE_INSTALL_PREFIX@/@INCLUDE_INSTALL_ROOT@\"")
cmake/uninstall_target.cmake.in:else(EXISTS "@CMAKE_INSTALL_PREFIX@/@INCLUDE_INSTALL_ROOT@")
cmake/uninstall_target.cmake.in:        "Directory \"@CMAKE_INSTALL_PREFIX@/@INCLUDE_INSTALL_ROOT@\" does not exist.")
cmake/uninstall_target.cmake.in:endif(EXISTS "@CMAKE_INSTALL_PREFIX@/@INCLUDE_INSTALL_ROOT@")
doc/advanced/content/distcc.rst:      -- ROS_ROOT /opt/ros/diamondback/ros
doc/tutorials/content/bspline_fitting.rst:  $ ./bspline_fitting ${PCL_ROOT}/test/bunny.pcd
doc/tutorials/content/building_pcl.rst:Let's say PCL is placed under /PATH/TO/PCL, which we will refer to as PCL_ROOT::
doc/tutorials/content/building_pcl.rst:  $ cd $PCL_ROOT
doc/tutorials/content/building_pcl.rst:Under ${PCL_ROOT}/cmake/Modules there is a list of FindXXX.cmake files
doc/tutorials/content/building_pcl.rst:environment variable named **XXX_ROOT** to find headers and libraries.
doc/tutorials/content/building_pcl.rst:* **BOOST_ROOT**: for boost libraries with value `C:/Program Files/boost-1.4.6` for instance
doc/tutorials/content/building_pcl.rst:* **CMINPACK_ROOT**: for cminpack with value `C:/Program Files/CMINPACK 1.1.13` for instance
doc/tutorials/content/building_pcl.rst:* **QHULL_ROOT**: for qhull with value `C:/Program Files/qhull 6.2.0.1373` for instance
doc/tutorials/content/building_pcl.rst:* **FLANN_ROOT**: for flann with value `C:/Program Files/flann 1.6.8` for instance
doc/tutorials/content/building_pcl.rst:* **EIGEN_ROOT**: for eigen with value `C:/Program Files/Eigen 3.0.0` for instance
doc/tutorials/content/sources/vfh_recognition/FindFLANN.cmake:          HINTS ${PC_FLANN_INCLUDEDIR} ${PC_FLANN_INCLUDE_DIRS} "${FLANN_ROOT}" "$ENV{FLANN_ROOT}"
doc/tutorials/content/sources/vfh_recognition/FindFLANN.cmake:             HINTS ${PC_FLANN_LIBDIR} ${PC_FLANN_LIBRARY_DIRS} "${FLANN_ROOT}" "$ENV{FLANN_ROOT}"
doc/tutorials/content/sources/vfh_recognition/FindFLANN.cmake:       HINTS ${PC_FLANN_LIBDIR} ${PC_FLANN_LIBRARY_DIRS} "${FLANN_ROOT}" "$ENV{FLANN_ROOT}"
gpu/octree/src/cuda/octree_host.cu:        const static int MAX_LEVELS_PLUS_ROOT = 11;
gpu/octree/src/cuda/octree_host.cu:        int paths[MAX_LEVELS_PLUS_ROOT];          
gpu/octree/src/cuda/radius_search.cu:                MAX_LEVELS_PLUS_ROOT = 11,
surface/src/3rdparty/opennurbs/opennurbs_zlib.cpp:#define OPENNURBS_ZLIB_OUTPUT_ROOT_DIR "."
surface/src/3rdparty/opennurbs/opennurbs_zlib.cpp:#pragma comment(lib, "\"" OPENNURBS_ZLIB_OUTPUT_ROOT_DIR "/" OPENNURBS_CONFIGURATION_DIR "/" OPENNURBS_ZLIB_FILE_NAME "\"")

As you can see all the relevant <package>_ROOT variable violating CMP0074 are set in pcl_all_in_one_installer.cmake and PCLConfig.cmake.in (the latter exported to target PCL by external projects). My suggestion is to add the policy just in this two files:

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 😄

svenevs commented 5 years ago

I have finally learned exactly what we should do for coping with the OLD case and accommodating consuming projects that may or may not be NEW. I will add a PR either tonight or tomorrow, just wanted to give an update that this will be "fixed" for the next release :smile:

SergioRAgostinho commented 5 years ago

Ping @svenevs. Did the world swallow you in unexpected affairs?

SergioRAgostinho commented 5 years ago

Fixed by #2454