-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Fix armel cross build of native part of libraries #32127
Fix armel cross build of native part of libraries #32127
Conversation
if (CLR_CMAKE_HOST_ARCH_ARMV7L) | ||
set(CLR_CMAKE_TARGET_ARCH_ARMV7L 1) | ||
set(ARM_SOFTFP 1) | ||
endif() |
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.
These lines shouldn't be needed. Line 190 below should be sufficient. Any reason this is duplicated here?
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.
I've updated my commit, this would be a better solution. Problem was that we ended up with
CLR_CMAKE_HOST_UNIX_ARM 1
CLR_CMAKE_HOST_UNIX_ARMV7L 1
CLR_CMAKE_HOST_ARCH_ARM 1
CLR_CMAKE_HOST_ARCH "arm"
CLR_CMAKE_HOST_ARCH_ARMV7L 1
which lead to
CLR_CMAKE_TARGET_ARCH "arm"
CLR_CMAKE_TARGET_ARCH_ARM 1
Now, CLR_CMAKE_HOST_ARCH "armel"
and target will be set correctly.
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.
Do you need set(ARM_SOFTFP 1)
here to? From your previous discussion, I assume Line 191 below will not be reached.
Or
set(CLR_CMAKE_TARGET_ARCH "armel")
And let the code below set the variables.
OR
set(CLR_CMAKE_HOST_ARCH "armel")
in 141 above. If this would work it seems the cleanest.
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.
ARM_SOFTFP=1 is passed from outside for this case (see ./eng/native/gen-buildsys.sh
), so it is not needed here.
We can't set neither CLR_CMAKE_TARGET_ARCH to "armel", nor CLR_CMAKE_HOST_ARCH, because we actually can't distinguish armel from arm here (CLR_CMAKE_HOST_ARCH_ARMV7L=1, CLR_CMAKE_HOST_UNIX_ARMV7L=1 for both).
0937ecd
to
5de9bd3
Compare
The patch looks good to me. However the arm build is currently failing with [ 0%] Building CXX object src/pal/src/CMakeFiles/tracepointprovider.dir/misc/tracepointprovider.cpp.o
In file included from /__w/1/s/src/coreclr/src/pal/src/misc/tracepointprovider.cpp:19:
In file included from /__w/1/s/src/coreclr/src/pal/src/include/pal/palinternal.h:159:
In file included from /crossrootfs/arm/usr/lib/gcc/arm-linux-gnueabihf/4.8/../../../../include/c++/4.8/type_traits:38:
In file included from In file included from /__w/1/s/src/libraries/Native/Unix/System.Globalization.Native/pal_calendarData.c:5:
In file included from /crossrootfs/arm/usr/include/assert.h:35:
In file included from /crossrootfs/arm/usr/include/features.h:398:
/crossrootfs/arm/usr/include/arm-linux-gnueabihf/gnu/stubs.h:7:11: fatal error: 'gnu/stubs-soft.h' file not found
# include <gnu/stubs-soft.h>
^~~~~~~~~~~~~~~~~~
/crossrootfs/arm/usr/lib/gcc/arm-linux-gnueabihf/4.8/../../../../include/arm-linux-gnueabihf/c++/4.8/bits/c++config.h:426:
In file included from /crossrootfs/arm/usr/lib/gcc/arm-linux-gnueabihf/4.8/../../../../include/arm-linux-gnueabihf/c++/4.8/bits/os_defines.h:39:
In file included from /crossrootfs/arm/usr/include/features.h:398:
/crossrootfs/arm/usr/include/arm-linux-gnueabihf/gnu/stubs.h:7:11: fatal error: 'gnu/stubs-soft.h' file not found
# include <gnu/stubs-soft.h>
^~~~~~~~~~~~~~~~~~
Scanning dependencies of target libunwind Perhaps we need an updated crossrootfs? |
5de9bd3
to
4b0bc52
Compare
@sdmaclea where is the rootfs created in azure? This patch shouldn't affect arm builds, this seems strange |
The Linux arm builds are built in a docker container that has a rootfs. See eng/pipelines/common/platform-matrix.yml. Linux arm is currently using the It is a bit strange. I assume when the |
With current setup in master there are some problems. For
However, it seems that there is a typo and next vars should be setup as well:
But So, for HOST we have two different states:
But these two become indistinguishable for TARGET when we check just So, either all target flags should be setup when CLR_CMAKE_TARGET_ARCH is copied from host, or additional What do you think? |
We seem to be missing a mechanism to set Therefore I am OK with setting this in the HOST section if needed. A comment explaining why would be nice. Either approach would be acceptable. I am not certain where |
4b0bc52
to
2f6bbd7
Compare
I've made the changes the same way as in the first version of this PR. armel and arm builds should work now |
This fixes build breaks after #31659
cc @alpencolt