-
Notifications
You must be signed in to change notification settings - Fork 391
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 CMake Support and Enhance Newlib Targets #1112
Conversation
Note that this isn't close to complete yet. This just adds the CMake unittests in its current form (to test which targets are failing). Fixed/Changed:
I need to add a Changelog entry. We don't actually need to care about supporting Meson, since the only Meson build system for Rust as a crate is from 2017, and Meson itself tries to entirely recreate the Rust build system which is absolutely cursed. We would need to provide our own Meson cross-compilation config files for each target we'd want to cross-compile to since the meson crate does not support any cross-compilation automatically. This is currently being blocked on a few other PRs:
Once all merge, I'll rebase, fix the THUMBv8 targets, and we should be ready. |
tryBuild failed: |
bors try --target android |
tryBuild failed: |
bors try --target android |
tryBuild succeeded: |
This currently uses a |
I've fixed CMake support for Android and newlib here, as well as added C++ support for newlib. bors try --target android --target none |
@@ -71,8 +71,6 @@ main() { | |||
popd | |||
|
|||
rm -rf "${td}" | |||
|
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.
We now want to run the CMake tests as well.
@@ -166,7 +164,7 @@ main() { | |||
fi | |||
|
|||
# Test C++ support | |||
if (( ${CPP:-0} )); then | |||
if (( ${STD:-0} )) && (( ${CPP:-0} )); then |
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.
This assumes we have C++ and std support, so we have to make that explicit.
ci/test.sh
Outdated
|
||
pushd "${td}" | ||
retry cargo fetch | ||
if (( ${STD:-0} )) && (( ${RUN:-0} )); then |
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 rust-cmake-hello-world
has both a no_std
lib and a std
binary, so we can check support for either/or.
libnewlib-arm-none-eabi && \ | ||
/qemu.sh arm | ||
libnewlib-arm-none-eabi \ | ||
libstdc++-arm-none-eabi-newlib |
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 targets actually support C++, and gcc-arm-none-eabi
actually contains the C++ compiler, just not libstdc++v3
. If we install that, these targets support C++ just fine. This applies for all newlib targets.
|
||
ENV QEMU_CPU=cortex-m3 \ | ||
ENV QEMU_CPU=cortex-m1 \ |
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.
cortex-m3
is an ARMv7-M target, cortex-m1
is the proper CPU here for ARMv6-M (see ARM Cortex-M).
docker/Dockerfile.thumbv6m-none-eabi
Outdated
|
||
COPY newlib.cmake /opt/newlib.cmake | ||
ENV CMAKE_TOOLCHAIN_FILE=/opt/newlib.cmake | ||
ENV CROSS_NEWLIB_OBJECT_FLAGS="-ffunction-sections -fdata-sections -mthumb -march=armv6s-m" |
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.
See newlib.cmake
below for more info on why these defaults are provided. These flags are identical to the ones provided by cmake-rs
, so they're used as the defaults to ensure anyone using cmake
externally will have these same default flags. This applies for all newlib targets.
docker/android.cmake
Outdated
# toolchain file for android targets, see #1110 | ||
|
||
set(CMAKE_SYSTEM_NAME Android) | ||
set(CMAKE_SYSTEM_PROCESSOR "$ENV{CROSS_ANDROID_ARCH}") |
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.
This is not the same as the arch elsewhere: it must be i686
, aarch64
, x86_64
, armv7-a
, or armv5te
. Just setting arm
will not work.
set(CMAKE_SYSTEM_NAME Android) | ||
set(CMAKE_SYSTEM_PROCESSOR "$ENV{CROSS_ANDROID_ARCH}") | ||
set(CMAKE_ANDROID_STANDALONE_TOOLCHAIN /android-ndk) | ||
set(CMAKE_ANDROID_API "$ENV{CROSS_ANDROID_SDK}") |
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.
We can't auto-detect the Android API, so we manually set it. CMake will still warn about this, but it's irrelevant since it's manually set.
|
||
set(CMAKE_SYSTEM_NAME Android) | ||
set(CMAKE_SYSTEM_PROCESSOR "$ENV{CROSS_ANDROID_ARCH}") | ||
set(CMAKE_ANDROID_STANDALONE_TOOLCHAIN /android-ndk) |
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.
Since we use a standalone Android NDK with the /android-ndk/sysroot
with no version prefix, we need to use CMAKE_ANDROID_STANDALONE_TOOLCHAIN
and not CMAKE_SYSROOT
nor CMAKE_ANDROID_NDK
.
See the docs here.
docker/android.cmake
Outdated
# not all of these are standard, however, they're common enough | ||
# that it's good practice to define them. | ||
set(prefix "$ENV{CROSS_ANDROID_TARGET}") | ||
set(CMAKE_C_COMPILER "${prefix}-gcc") |
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.
Only CMAKE_AR
, CMAKE_RANLIB
, CMAKE_C_COMPILER
, CMAKE_ASM_COMPILER
, and CMAKE_CXX_COMPILER
are standard, however, other tools will often use values provided by those variables even though they're not standard. Many toolchain files will provide them.
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 turns out setting these manually for the CMAKE_ASM_COMPILER
, etc., fails badly with downstream configurations. See #1110 (comment).
CMAKE_FORCE_C_COMPILER("${CMAKE_C_COMPILER}" GNU) | ||
CMAKE_FORCE_CXX_COMPILER("${CMAKE_CXX_COMPILER}" GNU) | ||
|
||
set(CMAKE_FIND_ROOT_PATH_MODE_PROGRAM NEVER) |
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.
Standard for cross-compiling.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
3002069
to
833b6ae
Compare
CROSS_CMAKE_SYSTEM_NAME=Android \ | ||
CROSS_CMAKE_SYSTEM_PROCESSOR=armv7-a \ | ||
CROSS_CMAKE_CRT=android \ | ||
CROSS_CMAKE_OBJECT_FLAGS="--target=arm-linux-androideabi -DANDROID -ffunction-sections -fdata-sections -fPIC --target=armv7-linux-androideabi" |
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 believe this change is correct, with --target
provided twice, since that's what cmake-rs
does as of 0.1.49
and cc-rs
as of 1.0.77
(this is using Rust 1.64.0).
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
bors try |
tryBuild succeeded: |
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.
lgtm
Fixes CMake support for all targets, ensuring support for C11, C++11, `try_run`, and more features. To do so, we've included tests with a CMake test suite that compiles for a bare-metal target, one that compiles against [re2](https://github.com/google/re2), and one using CMake's `try_run` (checking [CMAKE_CROSSCOMPILING_EMULATOR](https://cmake.org/cmake/help/latest/variable/CMAKE_CROSSCOMPILING_EMULATOR.html) is set for any cross-compiled targets). CMake support for Android targets was fixed by providing a toolchain file that specifies the API level and the standalone toolchain path, and ensuring that compiler detection is forcibly disabled. For other targets, a default toolchain file is provided that ensures the correct system name, processor, cross-compiling emulator, binutils, and cross-compilers are used. For bare-metal (newlib) targets, this ensures that `try_compile` uses a static library rather than an executable to avoid requiring a linker script to detect if the compiler works. This also adds a few minor changes for newlib targets: - Adds C++ support to bare-metal newlib targets by installing `libstdc++-arm-none-eabi-newlib`. - Updates `targets.toml` to specify that newlib targets do not have `std` support. - Use the Cortex-M1 CPU for `thumbv6m-none-eabi` rather than the Cortex-M3 (an ARMv7-M CPU) for the Qemu runner.
bors r=Emilgardis |
Build succeeded: |
Fix CMake Support and Enhance Newlib Targets
Fix CMake support for Android targets by providing a toolchain file that specifies the API level and the standalone toolchain path, and ensuring that compiler detection is forcibly disabled. Fix CMake support for newlib targets by providing a toolchain file that signals to CMake it is compiling for an embedded target, providing correct C, C++, and ASM flags even when invoked externally, and ensuring CMake skips testing if the C and C++ compilers work by running a binary file. Also add C++ support to bare-metal newlib targets, by also installing
libstdc++-arm-none-eabi-newlib
. Finally,targets.toml
was updated to ensure newlib targets do not havestd
support.Other minor changes including using the Cortex-M1 CPU for
thumbv6m-none-eabi
rather than the Cortex-M3 (an ARMv7-M CPU) for the Qemu runner.To ensure these work, test suites for CMake using C+11 features, CMake's
try_run
feature (checking CMAKE_CROSSCOMPILING_EMULATOR is set), and ano_std
package with a C++ dependency have been added.Closes #1110.
Closes #1113.