/coreclr

Synchronous toolchain.cmake files with CoreFX #9409

Merged
merged 1 commit into from Feb 17, 2017
+15 −33

Reviewers

@hqueue hqueue

@qmfrederik qmfrederik

@janvorli janvorli

Assignees

No one assigned

Labels

cla-already-signed

Projects

None yet

Milestone

No milestone

5 participants

@jyoungyun @qmfrederik @hqueue @janvorli @dnfclas
@jyoungyun
Contributor
jyoungyun commented Feb 8, 2017

Remove unnecessary TOOLCHAIN_PREFIX define and synchronous toolchain.cmake files with CoreFX

Related issue: dotnet/corefx#15882
@jyoungyun jyoungyun Synchronous toolchain.cmake files with CoreFX 
Signed-off-by: Jiyoung Yun <[email protected]>

All checks have passed

13 successful checks
CentOS7.1 x64 Debug Build and Test — Build finished.
Details
FreeBSD x64 Checked Build — Build finished.
Details
Linux ARM Emulator Cross Debug Build — Build finished.
Details
Linux ARM Emulator Cross Release Build — Build finished.
Details
OSX x64 Checked Build and Test — Build finished.
Details
Ubuntu x64 Checked Build and Test — Build finished.
Details
Ubuntu x64 Formatting — Build finished.
Details
Windows_NT arm Cross Debug Build — Build finished.
Details
Windows_NT arm Cross Release Build — Build finished.
Details
Windows_NT x64 Debug Build and Test — Build finished.
Details
Windows_NT x64 Formatting — Build finished.
Details
Windows_NT x64 Release Priority 1 Build and Test — Build finished.
Details
Windows_NT x86 Checked Build and Test — Build finished.
Details
5779a0c
@jyoungyun
Contributor
jyoungyun commented Feb 8, 2017

/cc @stephentoub @qmfrederik @leemgs @hqueue @chunseoklee @hseok-oh
cross/arm/toolchain.cmake
set(TOOLCHAIN "arm-linux-gnueabihf")
-set(TOOLCHAIN_PREFIX ${TOOLCHAIN}-)
-#set(CMAKE_C_COMPILER ${TOOLCHAIN_PREFIX}gcc)
@hqueue
hqueue Feb 8, 2017 Contributor

Cool! I wanted it be removed for a long time. Thanks :)

CMakeLists.txt
@@ -103,7 +103,7 @@ else (WIN32)
# Ensure that objcopy is present
if (DEFINED ENV{CROSSCOMPILE} AND NOT DEFINED CLR_CROSS_COMPONENTS_BUILD)
if (CMAKE_SYSTEM_PROCESSOR STREQUAL armv7l OR CMAKE_SYSTEM_PROCESSOR STREQUAL aarch64)
- find_program(OBJCOPY ${TOOLCHAIN_PREFIX}objcopy)
+ find_program(OBJCOPY ${TOOLCHAIN}-objcopy)
@qmfrederik
qmfrederik Feb 8, 2017 Contributor

To cross-compile for Android, we need to use objcopy from the Android toolchain, not the host. On Android, that would be ${CROSS_NDK_TOOLCHAIN}/bin/aarch64-linux-android-objcopy; previously we set ${TOOLCHAIN_PREFIX} to ${CROSS_NDK_TOOLCHAIN}/bin/aarch64-linux-android- so it worked.

If you set it to ${TOOLCHAIN} this will evaluate to aarch64-linux-android-objcopy and from what I get, CMake will look for this file in the bin folders on the host, which doesn't work.

Another option would be to set ${TOOLCHAIN_PREFIX} to ${CROSS_NDK_TOOLCHAIN}/bin/ on Android, leave it empty for Linux, and change this line to:

 find_program(OBJCOPY ${TOOLCHAIN_PREFIX}${TOOLCHAIN}-objcopy)
@jyoungyun
jyoungyun Feb 8, 2017 Contributor

What about replace ${TOOLCHAIN_PREFIX} to ${TOOLCHAIN} and redefine other macro for the previous ${TOOLCHAIN} variable? I can revise the find objcopy logic as you said. But I think this issue can be resolved by modifiying cross/android/arm64/toolchain.cmake file. What do you think?

cross/arm/toolchain.cmake
add_compile_options(-target armv7-linux-gnueabihf)
add_compile_options(-mthumb)
add_compile_options(-mfpu=vfpv3)
add_compile_options(--sysroot=${CROSS_ROOTFS})
set(CROSS_LINK_FLAGS "${CROSS_LINK_FLAGS} -target ${TOOLCHAIN}")
-set(CROSS_LINK_FLAGS "${CROSS_LINK_FLAGS} -B ${CROSS_ROOTFS}/usr/lib/gcc/${TOOLCHAIN}")
+set(CROSS_LINK_FLAGS "${CROSS_LINK_FLAGS} -B${CROSS_ROOTFS}/usr/lib/gcc/${TOOLCHAIN}")
@qmfrederik
qmfrederik Feb 8, 2017 Contributor

We do the same on Android (to keep the files consistent), hence ${TOOLCHAIN} must be an absolute path, so we can't prefix ${TOOLCHAIN} like we could do with ${TOOLCHAIN_PREFIX}

@jyoungyun
jyoungyun Feb 8, 2017 Contributor

As I suggest, you can define other macro for the previous ${TOOLCHAIN} and use it here.

@qmfrederik
Contributor
qmfrederik commented Feb 8, 2017 • edited

@jyoungyun I left some comments; the gist of it is that for Android, we need to set a full path for objcopy. We did that in the past by setting ${TOOLCHAIN_PREFIX} to a full path, but if you remove it, it doesn't work anymore.

Another option may be to agree that ${TOOLCHAIN_PREFIX} should contain the path in which the toolchain is installed (where "prefix" means "path" like in ./configure --prefix=...) and change the find command for objcopy to find_program(OBJCOPY ${TOOLCHAIN_PREFIX}${TOOLCHAIN}-objcopy).

I think you could then leave TOOLCHAIN_PREFIX empty for Linux cross-compile and we would only need to set it for Android.

/cc @janvorli
@jyoungyun
Contributor
jyoungyun commented Feb 8, 2017

@qmfrederik I think we can support android build without revising CMakeLists.txt. Here is my example.

set(ANDROID_TOOLCHAIN "aarch64-linux-android")
set(TOOLCHAIN ${CROSS_NDK_TOOLCHAIN}/bin/${ANDROID_TOOLCHAIN})
set(CMAKE_C_COMPILER ${TOOLCHAIN}-clang)
...
set(CROSS_LINK_FLAGS "${CROSS_LINK_FLAGS} -B ${CROSS_ROOTFS}/usr/lib/gcc/${ANDROID_TOOLCHAIN}") 
set(CROSS_LINK_FLAGS "${CROSS_LINK_FLAGS} -L${CROSS_ROOTFS}/lib/${ANDROID_TOOLCHAIN}")

I prefer not to create unnecessary define. So, if possible, I hope it will be implemented in existing structure. Let me know if something is wrong with me. If we need ${TOOLCHAIN_PREFIX}, I will patch it. :)
@jyoungyun jyoungyun referenced this pull request in dotnet/corefx Feb 8, 2017
Merged

Android: Removed unused WITH_LLDB_* defines #15944

@qmfrederik
Contributor
qmfrederik commented Feb 8, 2017

@jyoungyun Yup, we've done something similar in #9174 to support cross compiling for Android.

When we did that, we hit a problem with objcopy, though. Like I described above, the checks you have now would cause the script to either pick up the objcopy from the host OS or set it to OBJCOPY-NOTFOUND - see cydhaselton#2 (comment) for the discussion around that.

Looking at the documentation for find_program, it looks like there are some alternatives we can try:

  • Include the Android toolchain in the CMAKE_PREFIX_PATH varible
  • Provide an explicit hint for Android

I can check the CMAKE_PREFIX_PATH option later today my time zone, and I'll get back to you. Would that be OK?
@qmfrederik
Contributor
qmfrederik commented Feb 8, 2017

@jyoungyun Yup, setting CMAKE_PREFIX_PATH to the Android toolchain folder works and has the nice side-effect of simplifying the scripts.

I've updated #9174

LGTM!
@qmfrederik qmfrederik referenced this pull request in dotnet/corefx Feb 8, 2017
Merged

Add cmake files which support cross-compiling for Android arm64 #15882

@jyoungyun
Contributor
jyoungyun commented Feb 14, 2017

@gkhanna79 @janvorli PTAL
@janvorli
janvorli approved these changes Feb 14, 2017 View changes

LGTM, thank you!

@janvorli janvorli merged commit 849ad71 into dotnet:master Feb 17, 2017
13 checks passed

13 checks passed

CentOS7.1 x64 Debug Build and Test Build finished.
Details
FreeBSD x64 Checked Build Build finished.
Details
Linux ARM Emulator Cross Debug Build Build finished.
Details
Linux ARM Emulator Cross Release Build Build finished.
Details
OSX x64 Checked Build and Test Build finished.
Details
Ubuntu x64 Checked Build and Test Build finished.
Details
Ubuntu x64 Formatting Build finished.
Details
Windows_NT arm Cross Debug Build Build finished.
Details
Windows_NT arm Cross Release Build Build finished.
Details
Windows_NT x64 Debug Build and Test Build finished.
Details
Windows_NT x64 Formatting Build finished.
Details
Windows_NT x64 Release Priority 1 Build and Test Build finished.
Details
Windows_NT x86 Checked Build and Test Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment