zephyr_library_compile_options(): de-duplicate #15094
Conversation
Codecov Report
@@ Coverage Diff @@
## master #15094 +/- ##
=======================================
Coverage 52.87% 52.87%
=======================================
Files 309 309
Lines 45267 45267
Branches 10451 10451
=======================================
Hits 23934 23934
Misses 16558 16558
Partials 4775 4775Continue to review full report at Codecov.
|
cmake/extensions.cmake
Outdated
| @@ -400,6 +400,11 @@ function(zephyr_library_compile_options item) | |||
| string(MD5 uniqueness ${item}) | |||
| set(lib_name options_interface_lib_${uniqueness}) | |||
|
|
|||
| if (TARGET ${lib_name}) | |||
| message(WARNING "${item} already added, ignoring duplicate") | |||
There was a problem hiding this comment.
We should drop the warning to align with CMake's silent de-duplication.
There was a problem hiding this comment.
Are you sure? I thought the silent de-duplication was a bug you just discovered. Is it really a feature?
There was a problem hiding this comment.
I am sure.
It is CMake behaviour that we want to align with to be consistent.
There was a problem hiding this comment.
Generally we want to keep the logs quiet. It's best to prefer errors and avoid warnings. In this case we can't use an error as that would be inconsistent with CMake.
There was a problem hiding this comment.
@marc-hb can you please address the above?
There was a problem hiding this comment.
I will remove the warning.
The entire and unique purpose of this PR is to restore the same behaviour than CMake, so no question about that. I would still appreciate more clarity on that behaviour though; I mean I would still like to know whether that behaviour is a CMake feature or a CMake bug. * If * adding the same option twice is considered a usage error, then users would rather be warned about their error than enjoy the consistency of wasting as much time as they would have with CMake. Now I don't work in any Zephyr support role right now so this is admittedly just for my personal education.
There was a problem hiding this comment.
please submit the change ASAP, this PR is resolved a P1 bug, we need to clear the P1 bug list ASAP, otherwise we might resort to reverting the change introducing this issue.
There was a problem hiding this comment.
I will submit the change today.
Sorry I had no idea issue #15093 was now high priority, this is complete news to me. Github doesn't notify label changes and #15093 is not even assigned to me, it's still assigned to @SebastianBoe . I asked in #15093 about its priority but no one gave any assessment.
Please make some github noise next time.
There was a problem hiding this comment.
It's a CMake feature.
They have explicitly written code to de-duplicate the flags.
PR zephyrproject-rtos#14776 / commit 915a353 changed function zephyr_library_compile_options() to use MD5 instead of RANDOM for build reproduction reasons. As later predicted by Sebastian Bøe in the PR (thx), the names generated for these pseudo-libraries won't be unique anymore if the exact same flag gets added more than once. To reproduce the issue, simply add the following two lines in some CMakeLists.txt file like samples/hello_world/CMakeLists.txt: zephyr_library_compile_options("-Dfubar=barfu") zephyr_library_compile_options("-Dfubar=barfu") cmake will then fail like this: CMake Error at zephyr/cmake/extensions.cmake:403 (add_library): add_library cannot create target "options_interface_lib_e689fdb7eb15d801c2f95dd61301fc5e" because another target with the same name already exists. The existing target is an interface library created in source directory "zephyr/samples/hello_world". See documentation for policy CMP0002 for more details. Call Stack (most recent call first): CMakeLists.txt:9 (zephyr_library_compile_options) As requested by Sebastian, silently discard duplicate options just like CMake does. Fixes zephyrproject-rtos#15093 Signed-off-by: Marc Herbert <[email protected]>
marc-hb
force-pushed
the
deduplicate-library-options
branch
from
3417d54 to
c9ba025
Compare
PR #14776 / commit 915a353 changed function
zephyr_library_compile_options() to use MD5 instead of RANDOM for build
reproduction reasons. As later predicted by Sebastian Bøe in the PR
(thx), the names generated for these pseudo-libraries won't be unique
anymore if the exact same flag gets added more than once.
To reproduce the issue, simply add the following two lines in some
CMakeLists.txt file like samples/hello_world/CMakeLists.txt:
zephyr_library_compile_options("-Dfubar=barfu")
zephyr_library_compile_options("-Dfubar=barfu")
cmake will then fail like this:
CMake Error at zephyr/cmake/extensions.cmake:403 (add_library):
add_library cannot create target
"options_interface_lib_e689fdb7eb15d801c2f95dd61301fc5e" because
another target with the same name already exists. The existing
target is an interface library created in source directory
"zephyr/samples/hello_world". See documentation for
policy CMP0002 for more details.
Call Stack (most recent call first):
CMakeLists.txt:9 (zephyr_library_compile_options)
As suggested by Sebastian, discard duplicate options. After this fix
cmake succeeds with this warning:
CMake Warning at zephyr/cmake/extensions.cmake:404 (message):
-Dfubar=barfu already added, ignoring duplicate
Call Stack (most recent call first):
CMakeLists.txt:9 (zephyr_library_compile_options)
Fixes #15093
Signed-off-by: Marc Herbert [email protected]