Bad practice to set rule variables?

[real-or-random]

bitcoin/CMakeLists.txt

Lines 168 to 174 in 4778d28

	set(APPEND_CPPFLAGS "" CACHE STRING "Preprocessor flags that are appended to the flags added by the build system.")
	set(APPEND_CFLAGS "" CACHE STRING "C compiler flags that are appended to the flags added by the build system.")
	set(APPEND_CXXFLAGS "" CACHE STRING "(Objective) C++ compiler flags that are appended to the flags added by the build system.")
	set(APPEND_LDFLAGS "" CACHE STRING "Linker flags that are appended to the flags added by the build system.")
	string(APPEND CMAKE_CXX_COMPILE_OBJECT " ${APPEND_CPPFLAGS} ${APPEND_CXXFLAGS}")
	string(APPEND CMAKE_CXX_CREATE_SHARED_LIBRARY " ${APPEND_LDFLAGS}")
	string(APPEND CMAKE_CXX_LINK_EXECUTABLE " ${APPEND_LDFLAGS}")

(added in 84b83b3)

https://cmake.org/pipermail/cmake/2011-September/046090.html says that we are not supposed to set rule variables such as CMAKE_CXX_COMPILE_OBJECT, or, if we really need to, set use CMAKE_USER_MAKE_RULES_OVERRIDE.

Is this code block supposed to achieve the same as our SECP256K1_LATE_CFLAGS (implemented by a user-defined function all_targets_add_compile_options) in libsecp256k1?

[hebasto]

Is this code block supposed to achieve the same as our SECP256K1_LATE_CFLAGS (implemented by a user-defined function all_targets_add_compile_options) in libsecp256k1?

It is. But this approach guaranties to override even options that are abstracted by CMake, for instance #157 (comment).

[real-or-random]

Okay, sorry, I wasn't aware of the history of this. I agree that #240 is not better.

Perhaps it's a good idea to add a comment above the string(APPEND instructions that explains these considerations.
For example:

Overriding rule variables such as those below is discouraged, but it's the only way to guarantee that the contents of the APPEND_*FLAGS variables appear at the end of the command line.

We could also add something like "This is a global override intended for debugging and special builds." to the descriptions of the variables.

---

For consistency with libsecp256k1:

Is this code block supposed to achieve the same as our SECP256K1_LATE_CFLAGS (implemented by a user-defined function all_targets_add_compile_options) in libsecp256k1?

It is. But this approach guaranties to override even options that are abstracted by CMake, for instance #157 (comment).

• If we agree that appending to rule variables is superior, should we also do this in libsecp256k1?
• And/or should we rename the SECP256K1_LATE_CFLAGS variable to APPEND_CFLAGS?

[hebasto]

For consistency with libsecp256k1:

Is this code block supposed to achieve the same as our SECP256K1_LATE_CFLAGS (implemented by a user-defined function all_targets_add_compile_options) in libsecp256k1?

It is. But this approach guaranties to override even options that are abstracted by CMake, for instance #157 (comment).

• If we agree that appending to rule variables is superior, should we also do this in libsecp256k1?

• And/or should we rename the SECP256K1_LATE_CFLAGS variable to APPEND_CFLAGS?

That was/is the plan :)

(after setting everything in Bitcoin Core)

[hebasto]

Perhaps it's a good idea to add a comment above the string(APPEND instructions that explains these considerations. For example:

Overriding rule variables such as those below is discouraged, but it's the only way to guarantee that the contents of the APPEND_*FLAGS variables appear at the end of the command line.

I agree to add an explanatory comment, but considering this technique as discouraged basing on a post that predates CMake 3.0, seems a bit exaggerated.

UPD. The CMAKE_<LANG>_COMPILE_OBJECT variable docs have no restrictions.

[real-or-random]

I agree to add an explanatory comment, but considering this technique as discouraged basing on a post that predates CMake 3.0, seems a bit exaggerated.

Agreed.

Appending to these low-level rule variables is the only way to guarantee that the flags appear at the end of the command line.

[hebasto]

See bitcoin-core/secp256k1#1546.

[hebasto]

@real-or-random

Can this issue be closed or I missed something and left it unaddressed?

[real-or-random]

@real-or-random

Can this issue be closed or I missed something and left it unaddressed?

It's a nit anyway, but perhaps change the description and add the comment also in this repo for consistency. I guess these things will diverge at some point because they live in different repos, but we could at least try to keep it consistent.

[hebasto]

See bitcoin-core/secp256k1#1546.

Did this merged PR resolve this issue?

[real-or-random]

Strictly speaking, no, due to what I said in the previous comment: ;)

It's a nit anyway, but perhaps change the description and add the comment also in this repo for consistency. I guess these things will diverge at some point because they live in different repos, but we could at least try to keep it consistent.