Details

Reviewers

Ray Molenkamp (LazyDodo)
Campbell Barton (campbellbarton)
Brecht Van Lommel (brecht)

Commits

rBcd0acba330eb: CMake: Support external callback in configuration

Summary

Use case: I needed to modify compile flags (or any property for that matter) for a target.
Constraints:

Without modifying the source code so that I don't accidentally check it in/ have to maintain a patch to be applied after every git pull

cmake -C ../my_script.cmake -B ../build doesn't work since targets don't exist at the beginning.

This new option is similar to the postinstall script
Option: https://developer.blender.org/diffusion/B/browse/master/CMakeLists.txt;743b9c5e1d9e9d23b8f51fd6778db8292de204ae$705-706
Usage: https://developer.blender.org/diffusion/B/browse/master/source/creator/CMakeLists.txt;743b9c5e1d9e9d23b8f51fd6778db8292de204ae$1269-1271

Not checking if the file exists is intentional: better for automation instead of masking the error.

Diff Detail

Event Timeline

Ankit Meel (ankitm) requested review of this revision.Jan 13 2022, 6:40 PM

Ankit Meel (ankitm) created this revision.

Ankit Meel (ankitm) edited the summary of this revision. (Show Details)

Ankit Meel (ankitm) edited the summary of this revision. (Show Details)Jan 13 2022, 6:46 PM

Ankit Meel (ankitm) updated this revision to Diff 47036.Jan 13 2022, 7:06 PM

Ankit Meel (ankitm) edited the summary of this revision. (Show Details)

Ankit Meel (ankitm) updated this revision to Diff 47037.Jan 13 2022, 7:11 PM

Ankit Meel (ankitm) edited the summary of this revision. (Show Details)

Ankit Meel (ankitm) added reviewers: Ray Molenkamp (LazyDodo), Campbell Barton (campbellbarton).

I'm honestly not sure what that POSTINSTALL_SCRIPT does, adding it as an option as is done there just makes a on/off checkbox in the cmake-gui
force feeding it a text value will likely be seen as true, but it's a little bit of codesmelly setup honestly.

I'm mostly neutral about this change, if you get some use out of it and @Campbell Barton (campbellbarton) signs off , go for it.

Ankit Meel (ankitm) retitled this revision from Optionally include external cmake config at the end to Support running external cmake script.Jan 14 2022, 9:04 PM

Ankit Meel (ankitm) edited the summary of this revision. (Show Details)

Ankit Meel (ankitm) added a reviewer: Brecht Van Lommel (brecht). Ankit Meel (ankitm) removed 1 blocking reviewer(s): Campbell Barton (campbellbarton).Jan 18 2022, 10:02 AM

Neither should be a boolean, these should be paths similar to e.g.:

set(PYTHON_NUMPY_PATH            "" CACHE PATH "Path to python site-packages or dist-packages containing 'numpy' module")
mark_as_advanced(PYTHON_NUMPY_PATH)

I'm not sure why you need to change compile flags of specific targets though.

I see the use of it, I mean quite a few of our patches in our deps builder are adding a lib here or fudging an include folder there... having callback like this would have worked (for some of those) if they had it available, as for the specifics of what @Ankit Meel (ankitm) is doing no idea, don't think it matters all that much.

The question really is, do we mind offering a callback or not, I'm leaning towards why not, but if someone has a strong opinion for not doing it, I'm equally fine with that decision

Some targets have lots of warnings, probably due to my own actions (LLVM toolchain). Silencing by using -Wno-blah for the whole project means a full rebuild.

Ankit Meel (ankitm) removed a subscriber: Ankit jain (Ankit).Jan 19 2022, 6:54 PM

Ankit Meel (ankitm) mentioned this in rB596ce115567e: CMake Cleanup: use set for filepath instead of option.Jan 19 2022, 7:13 PM

Ok, I don't mind the functionality.

Brecht Van Lommel (brecht) requested changes to this revision.Jan 19 2022, 7:14 PM

This revision now requires changes to proceed.Jan 19 2022, 7:14 PM

option -> set

the other option handled in master

Brecht Van Lommel (brecht) accepted this revision.Jan 19 2022, 7:27 PM

This revision is now accepted and ready to land.Jan 19 2022, 7:27 PM

Closed by commit rBcd0acba330eb: CMake: Support external callback in configuration (authored by Ankit Meel (ankitm)). · Explain WhyJan 19 2022, 7:46 PM

This revision was automatically updated to reflect the committed changes.

Ankit Meel (ankitm) added a commit: rBcd0acba330eb: CMake: Support external callback in configuration.