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

Warning generated by policy CMP0054 with CMake >= 3.1 #189

Closed
bo0ts opened this issue Jul 22, 2015 · 13 comments · Fixed by #251
Closed

Warning generated by policy CMP0054 with CMake >= 3.1 #189

bo0ts opened this issue Jul 22, 2015 · 13 comments · Fixed by #251
Assignees
Labels
Bug CMake scripts Warnings For an issue about warnings, or a pull-request that fixes warnings
Milestone

Comments

@bo0ts
Copy link
Member

bo0ts commented Jul 22, 2015

Users have reported that CMake >= 3.1 triggers the following warning:

CMake Warning (dev) at CMakeLists.txt:425 (if):
Policy CMP0054 is not set: Only interpret if() arguments as variables or
keywords when unquoted. Run "cmake --help-policy CMP0054" for policy
details. Use the cmake_policy command to set the policy and suppress this
warning.

Quoted variables like "MSVC" will no longer be dereferenced when the policy
is set to NEW. Since the policy is not set the OLD behavior will be used.
This warning is for project developers. Use -Wno-dev to suppress it.

@bo0ts
Copy link
Member Author

bo0ts commented Jul 22, 2015

I understand what is happening, but we cannot set the policy to new without checking all places that trigger the warning. Unfortunately this can be quite difficult because it is only triggered when a code path is evaluated and our cmake code has platform depend branches in many different places.

The safe choice for this release would be to make sure this warning is never triggered by any of the installed user-side cmake components (Find*.cmake, CGALConfig.cmake, and so on) and then set it to old.

@cjamin
Copy link
Member

cjamin commented Jul 22, 2015

Or maybe could we just rename those MSVC variables to another name?

@bo0ts
Copy link
Member Author

bo0ts commented Jul 22, 2015

We cannot rename anything in that case because we don't own those variables. As far as I understand what happens in Line 425 it is this:

 if( "${CMAKE_CXX_COMPILER_ID}" MATCHES SunPro )
 # explicit expansion on visual studio leads to:
 if( "MSVC" MATCHES SunPro )
 # notice that the quotes have not been stripped! incidentally 
 # MSVC is also a predefined variable of CMake itself

 # with the policy on OLD and on Visual Studio we get implicit expansion:
 if("1" STREQUAL "SunPro) # this is of course not really what we wanted, but it will still work!

Here setting the policy to new will not change anything and the code will actually behave closer to our expectation. I don't know about the other cases.

@cjamin
Copy link
Member

cjamin commented Jul 22, 2015

I agree with your "what happens". It's a non-problematic bug, but only because we're lucky. And it has always been there. I didn't know that MSVC was a CMake variable.
In the old policy, is there a way to prevent CMake from implicitly expand the variable locally? In other words: how should we have done to fix that bug, if we have found it two years ago?

@bo0ts
Copy link
Member Author

bo0ts commented Jul 22, 2015

I'm not sure how quotes affect implicit expansion. The post http://public.kitware.com/pipermail/cmake/2014-March/057200.html suggests it doesn't. However adding explicit extra quotes might help, but it would look really obscure. Not to mention that we would have to do it in all if() statements where a variable could possibly expand to the name of another variable.

Could you please try both approaches (removing the quotes and adding extra quotes). I can't have a thorough look at this before next week.

@cjamin
Copy link
Member

cjamin commented Jul 22, 2015

Just tested it.
Adding extra quotes seems to be the way to go.

if( "${CMAKE_CXX_COMPILER_ID}" MATCHES MSVC )
is what we have now (I changed SunPro to MSVC for testing) => it triggers a warning and returns false => bug

if( ${CMAKE_CXX_COMPILER_ID} MATCHES MSVC )
does not trigger any warning, but returns false => visibly expanded to 1 MATCHES MSVC

if( "\"${CMAKE_CXX_COMPILER_ID}\"" MATCHES MSVC )
does not produce any warning and returns true => visibly expanded to "MSVC" MATCHES MSVC

Note that I'm getting another warning at line 671:

Quoted variables like "CGAL_CFG_BOOST_VARIANT_SWAP_BUG" will no longer be dereferenced when the policy is set to NEW. Since the policy is not set the OLD behavior will be used.`

@lrineau lrineau added Bug help wanted CMake scripts Warnings For an issue about warnings, or a pull-request that fixes warnings labels Jul 27, 2015
@lrineau
Copy link
Member

lrineau commented Jul 27, 2015

Hi. I have read your discussion. Is there a chance we can find a solution before the end of the week?

@bo0ts
Copy link
Member Author

bo0ts commented Jul 27, 2015

I think the first step would be to apply what @cjamin suggested. After that we can have a look at the warnings generated by the configuration.

@lrineau
Copy link
Member

lrineau commented Jul 27, 2015

Is not it simpler to set the policy to OLD for CGAL-4.7, and work on the issue for CGAL-4.8?

@bo0ts
Copy link
Member Author

bo0ts commented Jul 27, 2015

On 27 July 2015 at 13:01, Laurent Rineau notifications@github.com wrote:

Is not it simpler to set the policy to OLD for CGAL-4.7, and work on the
issue for CGAL-4.8?

Sure, that would be an appropriate stop-gap.

@lrineau
Copy link
Member

lrineau commented Jul 29, 2015

I cannot reproduce with CMake 3.3.20150729-g700d5. What are the steps to reproduce that issue?

@lrineau
Copy link
Member

lrineau commented Jul 29, 2015

... I found out. The following command triggers the warning:

cmake -DMSVC:BOOL=FALSE

@lrineau lrineau added this to the 4.8-beta milestone Jul 30, 2015
@lrineau
Copy link
Member

lrineau commented Jul 30, 2015

I have merged the PR #212. Now we need to solve the real issue. I postpone it to 4.8.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug CMake scripts Warnings For an issue about warnings, or a pull-request that fixes warnings
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants