-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Fix CMake policy warnings/changes introduced in 3.0.0 and higher
- Loading branch information
Showing
5 changed files
with
23 additions
and
2 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
a4aaff5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not the right way to handle policy warnings. The correct fix is to actually use the new behavior (the old behavior is there for backwards compatibility, not as a choice to keep using the old behavior). Policies may be removed in future versions of CMake, leaving only the new behavior.
a4aaff5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, @mathstuf, I agree, except for one point:
It is a clear choice to keep the old behavior until one has found the time to actually update the CMake code to reflect newer CMake changes, which often only affect a small percentage of QGIS developers who are using a newer CMake, e.g on macOS. We do have to keep the codebase compiling, but can't be waylaid rewriting CMake code every time someone decides to use the latest CMake.
Due to the wide array of platforms and OS versions supported, the minimum CMake for the project is still currently at 2.8.6! I'm guessing that would be a large amount of redundant if/then conditional CMake code.
Granted, with QGIS 3.0, it is an opportune time to fix these issues appropriately and decide upon a newer CMake minimum version. No one has yet taken on the task of refactoring large portions of working CMake code to update to a more modern approach. Developers usually work only on the CMake parts that affect their current work. Such a refactoring would take considerable effort. I intend to do so, albeit only for the macOS bundling routines, which were mostly written before the BundleUtilities were introduced into CMake (2.6.2).
Any guidance on even a partial refactoring would be greatly appreciated, like what bad practices (beyond policy handling) might we focus on updating to avoid deprecated CMake support in the near term?
a4aaff5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
CMP0053
errors are easy: just replace@var@
with${var}
.CMP0042
just sets the default, so setting it yourself (should be) sufficient.CMP0048
does indeed require an if/then conditional (since older versions won't set the version variables properly).CMP0040
needs fixed since custom commands are ignored if the target doesn't exist (the policy is to raise an error instead of ignoring the command).