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

Remove explicit setting of CM0074 policy to NEW #3962

Closed

Conversation

shrijitsingh99
Copy link
Contributor

@shrijitsingh99 shrijitsingh99 commented Apr 22, 2020

Expanding upon #2671
Couldn't quite understand why this was needed:

# TODO: update *_ROOT variables to be PCL_*_ROOT or equivalent.

@kunaltyagi
Copy link
Member

kunaltyagi commented Apr 22, 2020

There's another PR working on Boost. Can't link it now

@kunaltyagi kunaltyagi added changelog: removal Meta-information for changelog generation module: cmake labels Apr 22, 2020
@shrijitsingh99
Copy link
Contributor Author

There's another PR working on Boost. Can't link it now

I had mistakenly modified the Boost dependency. Removed it.

@kunaltyagi
Copy link
Member

Couldn't quite understand why this was needed:

CMake considers <module>_ROOT variables to be used to find the CMake module called <module>. By hiding them with a PCL_ prefix,

  1. we can prevent name clashes since we can assume nobody but PCL will make a module called PCL_surface
  2. BUT other people can release modules called surface to find which
  3. CMake will search for surface_ROOT and due to our 'new' naming, there will not be any confusions between surface_ROOT and PCL_surface_ROOT

@kunaltyagi kunaltyagi added the needs: code review Specify why not closed/merged yet label Apr 22, 2020
@SergioRAgostinho
Copy link
Member

Couldn't quite understand why this was needed:

Recent versions of cmake we're at some point spewing the warning claiming that policy was not set. This comment clarifies under which situations it is emitted.

If you're using CMake >= 3.12 and no warning related the policy is being emitted, then it might mean that it is no longer required. Try this both for pcl and a downstream project with a cmake_minimum_required < 3.12, while using a CMake version >= 3.12.

@kunaltyagi kunaltyagi added needs: author reply Specify why not closed/merged yet needs: testing Specify why not closed/merged yet and removed needs: code review Specify why not closed/merged yet labels Apr 30, 2020
@shrijitsingh99
Copy link
Contributor Author

shrijitsingh99 commented May 1, 2020

2. BUT other people can release modules called `surface` to find which

3. CMake will search for `surface_ROOT` and due to our 'new' naming, there will not be any confusions between `surface_ROOT` and `PCL_surface_ROOT`

Still slightly confused. We use BOOST_ROOT to help CMake discover the Boost module. in all_inone_installermode. So why create something likePCL_BOOST_ROOT`, what would be its purpose?

If you're using CMake >= 3.12 and no warning related the policy is being emitted, then it might mean that it is no longer required. Try this both for pcl and a downstream project with a cmake_minimum_required < 3.12, while using a CMake version >= 3.12.

Will test and report back.

@shrijitsingh99
Copy link
Contributor Author

I checked it out, didn't notice any CMake warnings on setting minimum version to 3.0

@kunaltyagi kunaltyagi added needs: code review Specify why not closed/merged yet and removed needs: testing Specify why not closed/merged yet needs: author reply Specify why not closed/merged yet labels May 24, 2020
@kunaltyagi
Copy link
Member

@SergioRAgostinho I'm not comfortable merging PR with CMake changes. PTAL

@SergioRAgostinho
Copy link
Member

I checked it out, didn't notice any CMake warnings on setting minimum version to 3.0

We might have removed the previous usage of *_ROOT in most finder scripts. I still see them being set in PCLConfig for PCL_ALL_IN_ONE_INSTALLER code. I would be more comfortable unsetting the policy once all situations in which a *_ROOT variable is set are removed from the code base.

@kunaltyagi kunaltyagi added the skill: cmake Skills/areas of expertise needed to tackle the issue label Sep 6, 2020
@kunaltyagi
Copy link
Member

Maybe @larshg can help review the CMake code so there's more confidence in merging this

@kunaltyagi kunaltyagi added this to the pcl-1.12.0 milestone Sep 6, 2020
@SergioRAgostinho SergioRAgostinho removed their request for review September 7, 2020 07:28
@larshg
Copy link
Contributor

larshg commented Mar 28, 2021

As I can see it, Package_ROOT variables are still used in various find scripts. And the policy is first available from 3.12. I couldn't find any information if it has been removed in later Cmake versions, but I don't get any warnings not setting the policy, running Cmake 3.19.1.
But most dependencies are also found with config files now or with find_package.

However, I don't see any reason not to keep it as it just allow CMake to look in package_ROOT directories if they are defined.
We could however remove all the XX_ROOT variables, but first when CMake 3.12 are required. If we remove the paths now, all old CMake versions that doesn't know the policy, would go with the old behavior and won't automatically look in package_ROOT folders and eventually lead to dependencies not being found.

I would close this one for now.

@kunaltyagi
Copy link
Member

Will reopen if there's interest by original author post 3.12 requirement

@kunaltyagi kunaltyagi closed this Mar 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: removal Meta-information for changelog generation module: cmake needs: code review Specify why not closed/merged yet skill: cmake Skills/areas of expertise needed to tackle the issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants