CMake: support quotes in CFLAGS (use configue_file to avoid quote errors w/ buildinfo)
AbandonedPublic
Actions

Authored by Campbell Barton (campbellbarton) on Nov 5 2020, 12:09 AM.

Details

Reviewers

Ray Molenkamp (LazyDodo)
Ankit Meel (ankitm)
Sergey Sharybin (sergey)

Summary

Recently a change to CFLAGS was committed that broke the build when buildinfo was enabled.

This patch avoids this happening again by using configure_file which handles escaping C-strings from CMake variables, without having to pass them as defines.

See the error from rBf2c7b4a1c54e: Re-enable WITH_COMPILER_SHORT_FILE_MACRO, fix build error..

We could escape quotes by doing:

string(REPLACE "\"" "\\\"" BUILDINFO_CFLAGS "${BUILDINFO_CFLAGS}").

However I rather let CMake escape the string so we don't run into other corner cases where escaping fails later on.
Note that I don't think this is a very controversial change, I'd to be sure this works on other platforms besides Linux.

Diff Detail

Repository

rB Blender

Branch

TEMP-CMAKE-FIX-QUOTES (branched from master)

Build Status

Buildable 11132
Build 11132: arc lint + arc unit

Event Timeline

Campbell Barton (campbellbarton) requested review of this revision.Nov 5 2020, 12:09 AM

Campbell Barton (campbellbarton) created this revision.

Harbormaster completed remote builds in B11130: Diff 30752.Nov 5 2020, 12:10 AM

Improve comment in configure header

Harbormaster completed remote builds in B11131: Diff 30753.Nov 5 2020, 12:12 AM

Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)Nov 5 2020, 12:24 AM

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

Campbell Barton (campbellbarton) retitled this revision from CMake: use configue_file to avoid quote errors w/ CFLAGS & buildinfo to CMake: support quotes in CFLAGS (use configue_file to avoid quote errors w/ buildinfo).Nov 5 2020, 12:29 AM

Use regular defines, no advantage in 'cmakedefine' in this case.

Harbormaster completed remote builds in B11132: Diff 30754.Nov 5 2020, 12:41 AM

Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)Nov 5 2020, 12:43 AM

Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)

Campbell Barton (campbellbarton) added a reviewer: Sergey Sharybin (sergey).Nov 5 2020, 12:46 AM

Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)

I can't seem to get master nor this patch to misbehave with msvc, but i can validate that when i have quotes they display properly and this patch doesn't break anything.

This revision is now accepted and ready to land.Nov 5 2020, 2:20 AM

Functionality works fine. Added minor comments.

build_files/cmake/buildinfo_static.h.in
1	Typo `wont`.
source/creator/CMakeLists.txt
156	Typo `stings`.
159	Multi-config generators don't have `CMAKE_BUILD_TYPE` defined. use generator expression `CONFIG` maybe ? https://developer.blender.org/diffusion/B/browse/master/CMakeLists.txt;7bc7b7da2d2f$132-137
175–179	how about adding the .h file to `SRC`, so that it shows up in IDEs ? list(APPEND SRC "${CMAKE_CURRENT_BINARY_DIR}/buildinfo_static.h" ) It's static, unlike `buildinfo.h`, so safe from unnecessary rebuilds I guess.

This revision now requires changes to proceed.Nov 5 2020, 9:29 AM

Committed rB9762a0992b3b: CMake: configue_file() to pass strings for build-info.

Some of the other changes can be done in separate patches.

source/creator/CMakeLists.txt
159	+1 to do this, however this can be a separate patch, as this limitation exists already. On Linux I don't think there is a good way to test multi-target build systems either.
175–179	I'm not sure of the logic here, `buildinfo.h` uses `buildinfo_static.h`, so adding only `buildinfo_static.h` doesn't make so much sense. If we add one we might as well add both.