cmake: lookup strip tool and set CMAKE_STRIP for host-gnu target

[tejlmand]

Fixes: #51821

Set CMAKE_STRIP using find_program(CMAKE_STRIP strip) to support strip when building on native posix.

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

[marc-hb]

Thanks @tejlmand for the very prompt fix.

Any idea why running cmake twice worked around this problem?

Could there be other differences between running cmake once versus twice?

[marc-hb]

Trying to answer myself after some cmake --trace-expand.

I think the design issue here is: CMAKE_STRIP (and CMAKE_OBJCOPY etc.) are built-in / INTERNAL CMake variables; they're not supposed to be defined by find_program() in any CMakeLists.txt because CMake itself already owns them, defines them and generally manages them.

I suspect the reason why running CMake twice avoided bug #51821 is because of some "race" to between target_bintools.cmake trying to use CMAKE_STRIP and CMake itself defining it.
On the first cmake run, zephyr/cmake/bintools/gnu/target_bintools.cmake was running "too early" and setting strip_command = ${CMAKE_STRIP} before CMake itself had time to define ${CMAKE_STRIP}.
On the second CMake run, target_bintools.cmake found the ${CMAKE_STRIP} left in CMakeCache.txt by the first cmake run.

I think some CMAKE_STRIP -> ZTOOLCHAIN_STRIP mass rename would likely avoid subtle "races" like the above and avoid any "undeterministic" behavior like this for any future CMAKE_FOO / ZTOOLCHAIN_FOO binary. It would make sure ZTOOLCHAIN_FOO is either always found or never found and it would never drive anyone crazy. Problems mysteriously coming and going are really the worst kind.

Messing with CMake's INTERNAL variables feels like a really bad idea.

[tejlmand]

I think the design issue here is: CMAKE_STRIP (and CMAKE_OBJCOPY etc.) are built-in / INTERNAL CMake variables; they're not supposed to be defined by find_program() in any CMakeLists.txt because CMake itself already owns them, defines them and generally manages them.

Not correct, those variables are perfectly valid to set and are often required to be set in a cross compilation environment.
That is usually done in a toolchain file, but in Zephyr we have our own infrastructure for this.

But it is correct, that if you don't specify them, then CMake will try to look up a host tool.

I suspect the reason why running CMake twice avoided bug #51821 is because of some "race" to between target_bintools.cmake trying to use CMAKE_STRIP and CMake itself defining it.

No, the reason it worked on a second run is because CMake will lookup the tool and set it in the CMake cache if it has not been set already by the system. However, CMake did this after we had used it for the external command call.

Therefore, on a second CMake run, the Zephyr build system would be using the value that is now present in the CMake cache, but on the first run it was empty.

[marc-hb]

Thanks @tejlmand !

Not correct, those variables are perfectly valid to set

Could you share the corresponding CMake documentation? What I found seemed different.

In https://cmake.org/cmake/help/latest/manual/cmake-variables.7.html

CMake reserves identifiers that: 1. begin with CMAKE_

CMAKE_AR is in the section "Variables that Provide Information". I (mis?)understand "provide" as "read-only".

CMAKE_STRIP is not on that page.

In the CMakeCache.txt, CMAKE_STRIP seems marked as "Internal" CMAKE_STRIP-ADVANCED:INTERNAL=1
In https://cmake.org/cmake/help/latest/manual/cmake-variables.7.html#internal-variables

CMake has many internal variables. Most of them are undocumented. Some of them, however, were at some point described as normal variables, and therefore may be encountered in legacy code. They are subject to change, and not recommended for use in project code.

---

No, the reason it worked on a second run is because CMake will lookup the tool and set it in the CMake cache if it has not been set already by the system. However, CMake did this after we had used it for the external command call.

Yes, that's exactly what I was trying to say. A behavior depending on the number of time cmake is run is a very unexpected hence incredibly confusing cmake debug experience and I suspect it will happen again the next time someone forgets something in a new toolchain or wants to add some new CMAKE_TOOL and misses one spot.

[marc-hb]

Not correct, those variables are perfectly valid to set

Could you share the corresponding CMake documentation? What I found seemed different.

@tejlmand, ping?

cc: @romkell (from #51212)

[tejlmand]

Could you share the corresponding CMake documentation? What I found seemed different.

@tejlmand, ping?

because your looking at general CMake documentation.

What we are doing in Zephyr is to implement our own toolchain infrastructure, this means the corresponding toolchain handling we do is more aligned with how CMake's built-in toolchain support is working.
So this means when Zephyr has it's own infrastructure then Zephyr must ensure we correct defines such settings, just like FindBinUtilTools.cmake would have been doing if you are using CMake's built-in support.

So for a normal user, this variable is surely informational, but when implementing complete toolchain support (which users regularly don't do), then such variables are valid to set, just as such variables are set by modules shipped as part of CMake releases.

In the CMakeCache.txt, CMAKE_STRIP seems marked as "Internal" CMAKE_STRIP-ADVANCED:INTERNAL=1

And so is the CMake C compiler, yet it's a variable users are expected to set when cross compiling:

$ grep -w CMAKE_C_COMPILER build/CMakeCache.txt
CMAKE_C_COMPILER:STRING=/opt/zephyr-sdk-0.15.1/arm-zephyr-eabi/bin/arm-zephyr-eabi-gcc
//ADVANCED property for variable: CMAKE_C_COMPILER
CMAKE_C_COMPILER-ADVANCED:INTERNAL=1

A behavior depending on the number of time cmake is run is a very unexpected hence incredibly confusing cmake debug experience

I agree, but this is actually not specific to toolchain handling, but a general problem that if we have any CMake code which relies on a cache variable not set on first invocation then we are in trouble.

Also one reason among many that I introduced #41301 because it provides us a better possibility to ensure depending modules has been loaded before current module.

and I suspect it will happen again the next time someone forgets something in a new toolchain or wants to add some new CMAKE_TOOL and misses one spot.

Which is why I would like if we could disable the FindBinUtilsTools from executing, similar to how we request CMake to not test the compiler, as we do here:

zephyr/cmake/modules/FindHostTools.cmake

Lines 75 to 77 in 5d05911

	# Prevent CMake from testing the toolchain
	set(CMAKE_C_COMPILER_FORCED 1)
	set(CMAKE_CXX_COMPILER_FORCED 1)

(just noticed that the above code should probably be moved to another location).

That way, something like your issue would probably have been easier to identify if the regular CMake infrastructure would not kick in at a later unexpected stage, which resulted in a second CMake apparently fixing the problem.