-
Notifications
You must be signed in to change notification settings - Fork 234
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove indirection from ocloc_lib and ocloc names #456
base: master
Are you sure you want to change the base?
Conversation
zboszor
commented
Sep 18, 2021
@@ -6,18 +6,6 @@ | |||
|
|||
function(level_zero_gen_kernels target platform_name suffix options) | |||
|
|||
if(NOT DEFINED cloc_cmd_prefix) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you pass -Dcloc_cmd_prefix=ocloc
to cmake command in your environment instead of removing this code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that with two or more levels of variable indirection, cmake
doesn't apply the CMAKE_CROSSCOMPILING_EMULATOR
wrapper command and this is the case with cloc_cmd_prefix
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For these add_custom_command
there is only one level of indirection - usage of ${cloc_cmd_prefix}
. The problem I see with using CMAKE_CROSSCOMPILING_EMULATOR
is that cmake adds CMAKE_CROSSCOMPILING_EMULATOR
before command and in our default scenario there will be something like ${CMAKE_CROSSCOMPILING_EMULATOR} LD_LIBRARY_PATH=... ocloc ...
which will cause issue as environment variable cannot be defined in the middle of cmdline. Therefore I hope that if you define cloc_cmd_prefix
as ocloc
then you will get ${CMAKE_CROSSCOMPILING_EMULATOR} ocloc ...
which looks like a valid cmdline. Please double check that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zboszor have you checked if -Dcloc_cmd_prefix=ocloc
helps for the issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I checked. It is this way in meta-intel
and it's good if ocloc
is built for running on the crosscompiling host.
But it is broken if coupled with -DCMAKE_CROSSCOMPILING_EMULATOR=...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you share build logs? what exactly is broken?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you share build logs? what exactly is broken?
I don't have the build logs.
The issue is that the ocloc
command is not prefixed with the crosscompiling wrapper.
If ocloc is cross-built for the target, it can't run properly using the host libraries.
E.g. the Yocto cross-build is not multilib but the host is, the runtime linker path is not good to run the binary without the wrapper that executes the binary in qemu, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zboszor could you verify if it is still needed? cross-build with qemu works fine for us
@@ -4,9 +4,8 @@ | |||
# SPDX-License-Identifier: MIT | |||
# | |||
|
|||
project(${OCLOC_NAME}_lib) | |||
project(ocloc_lib) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this change needed? I see what can be incorrect with our current implementation - project is defined before OCLOC_NAME is defined which evaluates to project(_lib)
. Simply moving set(OCLOC_NAME "ocloc")
to a line before project(${OCLOC_NAME}_lib)
should fix this issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not really needed. It just looked more consistent with the rest of this patch.
fa0fce1
to
7232ef5
Compare
This allows CMAKE to wrap the executable with -DCMAKE_CROSSCOMPILING_EMULATOR=... Signed-off-by: Zoltán Böszörményi <[email protected]>
5cfb59c
to
6a6ab80
Compare
fa44cc1
to
7aef244
Compare
3c72253
to
a4888b3
Compare
7a09c51
to
93e941f
Compare