Skip to content

Commit

Permalink
Fix CMAKE_SYSTEM_PROCESSOR CMake use case.
Browse files Browse the repository at this point in the history
This isn't actually the documented interface for CMake without a
toolchain file, but a user reported it as broken for them. It's trivial
to put the data back, so why not.

I've added a test, but it's pretty useless. The built library won't ever
make it to a device because it's a build-only test. Adding full test
coverage for this workflow would be a fair amount more work, and because
we *expect* that workflow to have behavior differences (if it didn't, we
wouldn't need the toolchain file), it'd always be a case where we were
disabling tests as not supported.

Still, we should make sure we can at least configure a build correctly
in this mode. Anything else that goes wrong is almost certainly the
result of avoiding all the other stuff the toolchain file does, so WAI.

The test actually would not have caught the breakage the user reported,
because the break is only present in specifically the
CMAKE_SYSTEM_PROCESSOR case; skipping the toolchain file but using the
documented CMAKE_ANDROID_ARCH_ABI option instead still worked.

Bug: android/ndk#2049
Test: manual edit of the added test case to make sure I fixed this, but
      reverted that change because it's not actually the documented
      CMake interface
(cherry picked from https://android-review.googlesource.com/q/commit:b49fc7f30d84241dba753d952e4531fb503bbf33)
Merged-In: I614dab1c72a9ee3814e050393dac8c9adc677f51
Change-Id: I614dab1c72a9ee3814e050393dac8c9adc677f51
  • Loading branch information
DanAlbert authored and Android Build Coastguard Worker committed Aug 29, 2024
1 parent 9652d01 commit b390f99
Show file tree
Hide file tree
Showing 5 changed files with 99 additions and 0 deletions.
12 changes: 12 additions & 0 deletions docs/changelogs/Changelog-r27.md
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,18 @@ directly, see the [build system maintainers guide].

[Support 16 KB page sizes]: https://developer.android.com/guide/practices/page-sizes

## r27b

* [Issue 2049]: Restored metadata used by CMake in non-toolchain-file use cases
that used `CMAKE_SYSTEM_PROCESSOR` instead of `CMAKE_ANDROID_ARCH_ABI`. If you
ran into this problem, you should switch to using `CMAKE_ANDROID_ARCH_ABI`
because that's what [CMake's docs] say to use. Better still, use the NDK's
[toolchain file].

[Issue 2049]: https://github.com/android/ndk/issues/2049
[CMake's docs]: https://cmake.org/cmake/help/latest/manual/cmake-toolchains.7.html#cross-compiling-for-android-with-the-ndk
[toolchain file]: https://developer.android.com/ndk/guides/cmake

## Changes

* Updated LLVM to clang-r522817. See `clang_source_info.md` in the toolchain
Expand Down
6 changes: 6 additions & 0 deletions ndk/checkbuild.py
Original file line number Diff line number Diff line change
Expand Up @@ -1506,6 +1506,12 @@ def abis_meta_transform(metadata: dict[str, Any]) -> dict[str, Any]:
abi_infos[f"NDK_ABI_{abi}_LLVM_TRIPLE"] = llvm_triple
abi_infos[f"NDK_ABI_{abi}_MIN_OS_VERSION"] = int(abi_data["min_os_version"])

# These will appear to be unused because there is no reference to them anywhere
# in the code base, but they're used by CMake for non-toolchain-file users, so
# they must be preserved.
abi_infos[f"NDK_PROC_{proc}_ABI"] = abi
abi_infos[f"NDK_ARCH_{arch}_ABI"] = abi

meta_vars = {
"NDK_DEFAULT_ABIS": sorted(default_abis),
"NDK_DEPRECATED_ABIS": sorted(deprecated_abis),
Expand Down
4 changes: 4 additions & 0 deletions tests/build/cmake_without_toolchain_file/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
cmake_minimum_required(VERSION 3.22.0)
project(HelloWorld)

add_library(foo SHARED foo.cpp)
2 changes: 2 additions & 0 deletions tests/build/cmake_without_toolchain_file/foo.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
extern "C" void foo() {
}
75 changes: 75 additions & 0 deletions tests/build/cmake_without_toolchain_file/test.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
#
# Copyright (C) 2024 The Android Open Source Project
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
#
"""Tests that using CMake without the toolchain file doesn't fail for trivial cases.
This isn't something we expect to work well, but it is something people use, so we at
least verify that the most trivial of builds succeed.
"""
import subprocess
from pathlib import Path
from tempfile import TemporaryDirectory

from ndk.hosts import Host
from ndk.paths import ANDROID_DIR
from ndk.test.spec import BuildConfiguration


def find_cmake_and_ninja() -> tuple[Path, Path]:
host = Host.current()
if host is Host.Windows64:
tag = "windows-x86"
else:
tag = f"{host.value}-x86"
return (
ANDROID_DIR / f"prebuilts/cmake/{tag}/bin/cmake",
ANDROID_DIR / f"prebuilts/ninja/{tag}/ninja",
)


def run_test(ndk_path: str, config: BuildConfiguration) -> tuple[bool, str | None]:
cmake, ninja = find_cmake_and_ninja()
with TemporaryDirectory() as build_dir:
try:
subprocess.run(
[
cmake,
"-B",
build_dir,
"-S",
".",
f"-DCMAKE_ANDROID_NDK={ndk_path}",
"-DCMAKE_SYSTEM_NAME=Android",
f"-DCMAKE_SYSTEM_VERSION={config.api}",
f"-DCMAKE_ANDROID_ARCH_ABI={config.abi}",
f"-DCMAKE_MAKE_PROGRAM={ninja}",
"-G",
"Ninja",
],
check=True,
stdout=subprocess.PIPE,
stderr=subprocess.STDOUT,
text=True,
)
subprocess.run(
[cmake, "--build", build_dir],
check=True,
stdout=subprocess.PIPE,
stderr=subprocess.STDOUT,
text=True,
)
except subprocess.CalledProcessError as ex:
return False, f"Build failed:\n{ex.output}"
return True, None

0 comments on commit b390f99

Please sign in to comment.