-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Comments
We have 19 finder scripts, so this will take some effort to review. Definitely not before the 1.9.0 release. |
Yes, this is very low priority! I doubt many people are even using 3.12 yet 😉 |
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. |
This warning, and possibly other like this one, but for different targets, are generated by the By my understanding, the new policy basically claims that Having a quick look at For the time being, to remove the warning one could just enforce the To completely address this issue, we need to check and refactor the variables with suffix |
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. |
This is a really good point. I asked about this on the cpplang slack and one of the CMake devs told me setting
I will stew on this. At the very least, the currently cached values should probably just get a |
I don't get the point behind this advice. I absolutely want
Could you explain this expression for non-native speakers here? :) |
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).
|
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 ;) |
I'm open to enforcing this for now. |
👍
Exactly, if we want to "minimize" refactoring and maintain the same logic and notation, prefixing
Yes, and it will be available to still do that 😄 The policy just says that
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.
Ok, I'll try to think about this. I need to understand whether it is better to enforce the policy project wise, or just |
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 I don't know whether we are saying the same thing or not actually ahahah 🤣 |
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. |
The implication of the advice was entirely removing the 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. |
Update: no bites (maybe I asked poorly). How imminent is the imminent release? Could we just commit to doing |
I think we should commit to |
My ideas was to go with |
My two cents here: go for |
Any specific reason to use push/pop? We typically have: if(POLICY xyz)
cmake_policy(SET xyz OLD)
endif() |
It is just a convenient way to isolate the code that needs a temporary |
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. |
@claudiofantacci I'm not sure that From an old but relevant mailing list post
The problem is that in our case this means finding / checking every Does this make sense / do people agree? This one is kind of confusing because (as stated)
So um. Do we set it in |
@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 Here is the output of
As you can see all the relevant
thus we should add the policy also in this file. Since both files will be (most likely) calld by |
I have finally learned exactly what we should do for coping with the |
Ping @svenevs. Did the world swallow you in unexpected affairs? |
Fixed by #2454 |
This affects all of your
find*
modules. SeeCMP0074
documentation, this policy is introduced in CMake 3.12+.Naively, the fix would just be
I looked specifically at the
FindFLANN.cmake
file, and it does appear to respectFLANN_ROOT
. Here's the relevant CMake (v3.12.1) output:For reference:
I did a little snooping on Flann specifically, and that module appears to be respecting
FLANN_ROOT
, but it's also worth mentioning that myflann.pc
also shows up in$PKG_CONFIG_PATH
, so while thefind_{path,library}
are seemingly respectingFLANN_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
toNEW
.The text was updated successfully, but these errors were encountered: