cmake: reuse interface lib for compile options if it already exists

[tejlmand]

Fixes: #43835

In zephyr_library_compile_options() the existence of the compile
options interface library is checked and function returns if it already
exists. This results in #43835 meaning two libraries cannot add the
same option.

This commit fixes this by re-using the already created unique interface
library and link this to the Zephyr library.

Signed-off-by: Torsten Rasmussen [email protected]

[marc-hb]

Was "ignore just like CMake" not valid? Just curious.

[tejlmand]

no, cause code was not ignoring, it was preventing two libraries from using the same compile option.

If you have two libraries, both of them should be allowed to add the same flag.
Please read #43835.

If two libs wants to compile with same option, they should be allowed to do so.
For example:

zephyr_library_named(foo)
zephyr_library_compile_options(-Wall)

zephyr_library_named(bar)
zephyr_library_compile_options(-Wall)

existing code would not allow bar to add -Wall because the return statement was preventing target_link_libraries() call from being executed on bar:

zephyr/cmake/modules/extensions.cmake

Lines 503 to 511 in da3ec7d

	if (TARGET ${lib_name})
	# ${item} already added, ignoring duplicate just like CMake does
	return()
	endif()
	add_library( ${lib_name} INTERFACE)
	target_compile_options(${lib_name} INTERFACE ${item} ${ARGN})
	target_link_libraries(${ZEPHYR_CURRENT_LIBRARY} PRIVATE ${lib_name})

Note: CMake still ensures de-duplication on each lib, so this:

zephyr_library_named(bar)
zephyr_library_compile_options(-Wall)
zephyr_library_compile_options(-Wall)

is handled correctly, there will be only a single -Wall.

[marc-hb]

Thanks! While I'm not familiar with it the code change makes sense to me, I'm only wondering about the comments. This area is tricky: fake libraries, checksums, (de-)duplication,... not so surprising a bug was found there. So maybe some good comments could prevent another bug in the future or help solve one that has not been found yet.

Note: CMake still ensures de-duplication on each lib,

Maybe that's worth mentioning. Reading the new code, someone could wonder why the same lib_name target gets added multiple times = what the previous (and buggy) code was trying to avoid?

[tejlmand]

Maybe that's worth mentioning.

Not convinced, should we really make a comment every time there is a risk of someone misunderstanding how a built-in feature of CMake works ?

Doesn't we just risk creating more confusion for others than the two of us.
Remember that anyone else in future looking at this code and the comment won't have the background knowledge we have.
Thus they may start to wonder, why are they talking about de-duplication .... ?

If I look at #15093, the error message there is:

 add_library cannot create target
 "options_interface_lib_e689fdb7eb15d801c2f95dd61301fc5e" because
 another target with the same name already exists.

which clearly states this is not about de-duplication, but that the same library is being created twice.
Which the new comment exactly informs:

# Create the unique target only if it doesn't exist.

[marc-hb]

Thus they may start to wonder, why are they talking about de-duplication .... ?

Because it's not any CMake feature. It's a subtle, surprising and controversial anti-feature with a "design bug" as acknowledged somewhere buried in CMake's documentation itself: "while beneficial for individual options, the de-duplication step can break up option groups. For example, -option A -option B becomes -option A B. ". I think that anti-feature cost me about a day when I tried to use -imacros and as a coincidence it looks like it's just going to be a pain again: thesofproject/sof#5324

Note you're arguing against a one-line comment about CMake de-duplication next to code implementing another de-duplication. The confusion potential seems pretty obvious to me, pretty sure this code would make me pause if I had to read it again (hopefully never) and I'm considered the "CMake expert" in my team (cause in the land of the blinds, the one-eyed is king).

I think your CMake expertise level has somewhat disconnected you from the "CMake real world". Please take that as a compliment :-)

[tejlmand]

think your CMake expertise level has somewhat disconnected you from the "CMake real world".

I don't hope so, but I see enough confusion among CMake users and I see adding proposed comment can result in to things here:

1. risk adding confusion to readers who would otherwise not be confused
2. trying to clear up misunderstandings from the past

I think majority of readers of this code now will be 1., whereas the number of people in 2. may just be a couple.
Therefore my preference is simply to avoid 1., as I don't like to confuse people, and then the few people of 2. must traceback the origin of the commit (that is this PR).

Please take that as a compliment :-)

Thanks, I will.

[tejlmand]

as a coincidence it looks like it's just going to be a pain again: thesofproject/sof#5324

thanks for the pointer.
Added a comment pain killer in that PR.

[marc-hb]

the code in question has nothing to do with de-duplication, it has to do with not creating two identical targets.

Looks like we don't have the same understanding of the English word "duplication". No big deal.

[tejlmand]

English word "duplication"

I thought we were discussing de-duplication ?
You even referenced CMake's own description of de-duplication, and this is not what the code is solving.

We honor target name uniqueness by avoiding duplication CMP002 in the first place.

avoiding duplication != de-duplication (as de-duplication requires that something was duplicated in the first place)

[marc-hb]

avoiding duplication != de-duplication (as de-duplication requires that something was duplicated in the first place)

Yeah it's totally different, there's absolutely no confusion possible. Also, every developer reads and writes perfect English - especially in CMake code.

So it's important to avoid any spurious one-line comment alluding to a CMake anti-feature that everyone should know about.