Page MenuHome

Cmake: Add option for precompiled options
Needs ReviewPublic

Authored by Aaron Carlisle (Blendify) on Jan 17 2022, 7:55 PM.

Details

Summary

This option was requested in D13797 to make it
easier turn precompiled headers on and off.

This was still possible before with CMAKE_DISABLE_PRECOMPILE_HEADERS
however this change makes it easier to toggle this option.

Diff Detail

Repository
rB Blender
Branch
cmake-pch-option (branched from master)
Build Status
Buildable 20047
Build 20047: arc lint + arc unit

Event Timeline

Aaron Carlisle (Blendify) requested review of this revision.Jan 17 2022, 7:55 PM
Aaron Carlisle (Blendify) created this revision.
This revision is now accepted and ready to land.Jan 17 2022, 8:09 PM
CMakeLists.txt
203

On -> ON for consistency

Ray Molenkamp (LazyDodo) requested changes to this revision.Jan 17 2022, 8:17 PM

Although both of @Jacques Lucke (JacquesLucke) 's usecases are invalid (both are doable without this switch), there is something to be said for discoverability of an option, the option to turn it off is seemingly well hidden in the cmake docs.

I went back and forth in D13797 and ultimately decided against having an explicit option, CMake natively had the option, so why roll our own, seem silly, however given one of our smarter devs failed to find it, what chance do lesser mortals have, I'd rather have 5 extra lines to maintain, and have this explicitly listed in CMakeCache.txt/cmake-gui than have more devs lose time over this.

I would however apply the change consistent with its unity counterpart and use if(WITH_PRECOMPILED_HEADERS) throughout the libraries, rather than the if(COMMAND target_precompile_headers) test that is there now

This revision now requires changes to proceed.Jan 17 2022, 8:17 PM

The if(COMMAND target_precompile_headers) is only needed to protect against old CMake versions, when we update the minimum CMake version this check will be removed.

Simplify setup by adding CMAKE_DISABLE_PRECOMPILE_HEADERS as an option

Aaron Carlisle (Blendify) marked an inline comment as done.Jan 17 2022, 8:52 PM

Yeah no, i was thinking more along the line of P2743