Skip to content
This repository has been archived by the owner on May 13, 2024. It is now read-only.

Set policies through version 3.9 to allow better IPO support #57

Merged
merged 3 commits into from
Aug 21, 2021

Conversation

JosiahWI
Copy link
Contributor

@JosiahWI JosiahWI commented Aug 21, 2021

The OLD behavior of a CMake policy is deprecated by default, so a version range can be used to set all policies to NEW on the appropriate versions. It is difficult to test this extensively, but it is unlikely to cause a problem, and could fix some bugs for people using newer versions. I picked 3.9 as the top of the version range to minimize the effects, because 3.9 will set CMP0069 to NEW, which is the focus of this commit.

If a version is newer than the top of the range, its policies will be set based on the highest version in the range.

Fixes #54.

The OLD behavior of a CMake policy is deprecated by default, so a version range can be used to set all policies to NEW on the appropriate versions. It is difficult to test this extensively, but it is unlikely to cause a problem, and could fix some bugs for people using newer versions. I picked 3.9 as the top of the version range to minimize the effects, because 3.9 will set CMP0069 to NEW, which is the focus of this commit.
@sfan5
Copy link
Member

sfan5 commented Aug 21, 2021

Copy link
Member

@sfan5 sfan5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The min...max syntax was only added in CMake 3.12 so it'll only work there but fine by me.

@JosiahWI
Copy link
Contributor Author

There's a way to set up policies on older versions (< 3.12) also, but I hadn't considered it. I could add that if necessary; I don't know how many people would need it.

@JosiahWI
Copy link
Contributor Author

JosiahWI commented Aug 21, 2021

@sfan5 fmtlib/fmt#809 Does this apply to us? I'm trying to figure out what's the point of using a version range at all when a conditional block works for all versions to set policies and I noticed this.

@sfan5
Copy link
Member

sfan5 commented Aug 21, 2021

I don't think so.

The version range that was previously introduced has two drawbacks: it has no effect prior to version 3.12, and on older MSVC versions there is a bug that causes it to error. The conditional block solves both of this issues.
@JosiahWI
Copy link
Contributor Author

Changed it anyway because I see only pros.

CMakeLists.txt Show resolved Hide resolved
@JosiahWI JosiahWI changed the title Add version range up to 3.9 to allow better IPO support Set policies through ver 3.9 to allow better IPO support Aug 21, 2021
@JosiahWI JosiahWI changed the title Set policies through ver 3.9 to allow better IPO support Set policies through version 3.9 to allow better IPO support Aug 21, 2021
CMakeLists.txt Outdated Show resolved Hide resolved
Co-authored-by: sfan5 <sfan5@live.de>
Copy link
Member

@sfan5 sfan5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Tested with CMake 3.10.2

@sfan5 sfan5 merged commit 1aab3db into minetest:master Aug 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CMake warnings when LTO is enabled as part of building Minetest
2 participants