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

CMake Policy CMP0074 #2425

Closed
svenevs opened this issue Sep 10, 2018 · 27 comments
Closed

CMake Policy CMP0074 #2425

svenevs opened this issue Sep 10, 2018 · 27 comments

Comments

@svenevs
Copy link
Contributor

svenevs commented Sep 10, 2018

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.

@SergioRAgostinho SergioRAgostinho added module: cmake needs: code review Specify why not closed/merged yet labels Sep 10, 2018
@taketwo
Copy link
Member

taketwo commented Sep 10, 2018

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

@svenevs
Copy link
Contributor Author

svenevs commented Sep 10, 2018

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

@SergioRAgostinho
Copy link
Member

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
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor Author

svenevs commented Sep 11, 2018

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
Copy link
Member

taketwo commented Sep 11, 2018

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
Copy link
Member

SergioRAgostinho commented Sep 11, 2018 via email

@svenevs
Copy link
Contributor Author

svenevs commented Sep 11, 2018

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
Copy link
Member

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
Copy link
Contributor

claudiofantacci commented Sep 12, 2018

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
Copy link
Member

taketwo commented Sep 12, 2018

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
Copy link
Contributor

claudiofantacci commented Sep 12, 2018

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
Copy link
Member

taketwo commented Sep 12, 2018

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

@svenevs
Copy link
Contributor Author

svenevs commented Sep 12, 2018

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
Copy link
Contributor Author

svenevs commented Sep 13, 2018

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
Copy link
Member

taketwo commented Sep 13, 2018

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 SergioRAgostinho added this to the pcl-1.9.0 milestone Sep 13, 2018
@SergioRAgostinho
Copy link
Member

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
Copy link
Contributor

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
Copy link
Member

taketwo commented Sep 13, 2018

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

if(POLICY xyz)
  cmake_policy(SET xyz OLD)
endif()

@claudiofantacci
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor Author

svenevs commented Sep 14, 2018

@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 😢

@claudiofantacci
Copy link
Contributor

claudiofantacci commented Sep 14, 2018

@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:

  • by my understanding PCLConfig.cmake.in is not used at PCL build time, but only by external projects targeting PCL. This will remove the warning for external projects.
  • I'm no expert of pcl_all_in_one_installer.cmake, but I see it is include()d in
git grep pcl_all_in_one_installer
CMakeLists.txt:include("${PCL_SOURCE_DIR}/cmake/pcl_all_in_one_installer.cmake")

thus we should add the policy also in this file.

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
Copy link
Contributor Author

svenevs commented Sep 15, 2018

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 😄

@SergioRAgostinho
Copy link
Member

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

@SergioRAgostinho
Copy link
Member

Fixed by #2454

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants