Skip to content
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

[question] How to robustly access tools like strip from cmake #1478

Closed
baldurk opened this issue Mar 26, 2021 · 13 comments
Closed

[question] How to robustly access tools like strip from cmake #1478

baldurk opened this issue Mar 26, 2021 · 13 comments

Comments

@baldurk
Copy link

baldurk commented Mar 26, 2021

I use the android cmake toolchain file to build my project, and previously I was using CMAKE_STRIP to strip libraries in release builds. I recently built a new system and as part of that updated to the latest ndk, and found that CMAKE_STRIP is no longer exported by the android toolchain file.

This seems fine but I'm not 100% sure I have the right way of getting the proper way of getting the path to the strip utility. This is what I've updated to do:

    set(ANDROID_STRIP_TOOL "${ANDROID_TOOLCHAIN_ROOT}/bin/llvm-strip${ANDROID_TOOLCHAIN_SUFFIX}")
    if(NOT EXISTS "${ANDROID_STRIP_TOOL}")
        set(ANDROID_STRIP_TOOL "${CMAKE_STRIP}")
    endif()

    message(STATUS "libraries will be stripped with ${ANDROID_STRIP_TOOL}")

Which should work on windows/linux hosts for the new NDK, and fall back to CMAKE_STRIP on older NDKs (I support back to 14b for building).

Mainly I'm wondering if there's a better intended way to get the android strip tool, in case that hardcoded path changes in future, or if llvm-strip doesn't work for all ABIs (I'm unclear if I need a per-ABI strip tool, it seems to work for arm 32-bit and 64-bit).

NDK version: 23.0.7196353
ABI: armeabi-v7a & arm64-v8a
Build system: cmake (I'm using 3.20 but I support back to 2.8.12 in principle)

@hhb
Copy link
Collaborator

hhb commented Mar 27, 2021

You can use CMAKE_STRIP directly. It will be set to the correct strip tool here.

@baldurk
Copy link
Author

baldurk commented Mar 27, 2021

That is only true if you enable the legacy toolchain file, which I assume won't be available forever. By default that file is not invoked so CMAKE_STRIP is not set, otherwise I would never have noticed this problem in the first place.

@hhb
Copy link
Collaborator

hhb commented Mar 29, 2021

For CMake >= 3.20, CMake should detect and set CMAKE_STRIP.

If this is really a problem we should revert this: https://android.googlesource.com/platform/ndk/+/e854ac5e0060b7c0061a06c1d32e39fcce9fbad2

@baldurk
Copy link
Author

baldurk commented Mar 29, 2021

That doesn't happen for me with cmake 3.20. I made this little repro case (test.c is empty):

cmake_minimum_required(VERSION 2.8.12)

set(CMAKE_TOOLCHAIN_FILE "$ENV{ANDROID_NDK}/build/cmake/android.toolchain.cmake")
set(ANDROID_PLATFORM "android-21")
set(CMAKE_ANDROID_NDK_TOOLCHAIN_VERSION "clang")
set(ANDROID_ABI "armeabi-v7a")

project(test CXX C)
add_library(test SHARED test.c)

message(STATUS "Stripping with ${CMAKE_STRIP}")
add_custom_command(TARGET test POST_BUILD COMMAND ${CMAKE_STRIP} --strip-unneeded $<TARGET_FILE:test>)

In an msys shell it uses the host strip:

$ cmake -G "MSYS Makefiles" ..
-- Android: Targeting API '21' with architecture 'arm', ABI 'armeabi-v7a', and processor 'armv7-a'
-- Android: Selected unified Clang toolchain
-- The CXX compiler identification is Clang 11.0.5
-- The C compiler identification is Clang 11.0.5
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: T:/android/studiosdk/ndk/23.0.7196353/toolchains/llvm/prebuilt/windows-x86_64/bin/clang++.exe - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: T:/android/studiosdk/ndk/23.0.7196353/toolchains/llvm/prebuilt/windows-x86_64/bin/clang.exe - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Stripping with C:/msys/usr/bin/strip.exe
-- Configuring done
-- Generating done
-- Build files have been written to: T:/tmp/droid_cmake_test/build
$ cmake --build .
[ 50%] Building C object CMakeFiles/test.dir/test.c.o
[100%] Linking C shared library libtest.so
/C/msys/usr/bin/strip: Unable to recognise the format of the input file `T:/tmp/droid_cmake_test/build/libtest.so'
make[2]: *** [CMakeFiles/test.dir/build.make:97: libtest.so] Error 1
make[2]: *** Deleting file 'libtest.so'
make[1]: *** [CMakeFiles/Makefile2:83: CMakeFiles/test.dir/all] Error 2
make: *** [Makefile:91: all] Error 2
$ cmake --version
cmake version 3.20.0

CMake suite maintained and supported by Kitware (kitware.com/cmake).

And in regular cmd it can't find strip at all:

>cmake -G Ninja ..
-- Android: Targeting API '21' with architecture 'arm', ABI 'armeabi-v7a', and processor 'armv7-a'
-- Android: Selected unified Clang toolchain
-- The CXX compiler identification is Clang 11.0.5
-- The C compiler identification is Clang 11.0.5
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: T:/android/studiosdk/ndk/23.0.7196353/toolchains/llvm/prebuilt/windows-x86_64/bin/clang++.exe - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: T:/android/studiosdk/ndk/23.0.7196353/toolchains/llvm/prebuilt/windows-x86_64/bin/clang.exe - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Stripping with CMAKE_STRIP-NOTFOUND
-- Configuring done
-- Generating done
-- Build files have been written to: T:/tmp/droid_cmake_test/build
>cmake --build .
[2/2] Linking C shared library libtest.so
FAILED: libtest.so
cmd.exe /C "cd . && T:\android\studiosdk\ndk\23.0.7196353\toolchains\llvm\prebuilt\windows-x86_64\bin\clang.exe --target=armv7-none-linux-androideabi21 --sysroot=T:/android/studiosdk/ndk/23.0.7196353/toolchains/llvm/prebuilt/windows-x86_64/sysroot -fPIC -DANDROID -fdata-sections -ffunction-sections -funwind-tables -fstack-protector-strong -no-canonical-prefixes -D_FORTIFY_SOURCE=2 -march=armv7-a -mthumb -Wformat -Werror=format-security -fexceptions -O2 -g -DNDEBUG  -Wl,--exclude-libs,libgcc.a -Wl,--exclude-libs,libgcc_real.a -Wl,--exclude-libs,libatomic.a -Wl,--build-id=sha1 -Wl,--no-rosegment -Wl,--fatal-warnings -Qunused-arguments -Wl,--no-undefined -shared -Wl,-soname,libtest.so -o libtest.so CMakeFiles/test.dir/test.c.o  -latomic -lm && cmd.exe /C "cd /D T:\tmp\droid_cmake_test\build && CMAKE_STRIP-NOTFOUND --strip-unneeded T:/tmp/droid_cmake_test/build/libtest.so""
'CMAKE_STRIP-NOTFOUND' is not recognized as an internal or external command,
operable program or batch file.
ninja: build stopped: subcommand failed.
>cmake --version
cmake version 3.20.0

CMake suite maintained and supported by Kitware (kitware.com/cmake).

@hhb
Copy link
Collaborator

hhb commented Mar 30, 2021

Sent a fix upstream here.

Once that is in we can figure out what should we do in NDK side.

@hhb hhb added the bug label Apr 1, 2021
@hhb
Copy link
Collaborator

hhb commented Apr 1, 2021

@DanAlbert Can you help change this to 3.20.1 and cherry-pick it to r23? I have some mysterious permission issue to upload a change...

@DanAlbert
Copy link
Member

Done (and thanks!)

I'm a little unsure how the testing works here. I see we have https://cs.android.com/android/platform/superproject/+/master-ndk:ndk/ndk/test/types.py;l=421-424, but prebuilts/cmake only has CMake 3.18. Is the case here that our new toolchain file more or less works with 3.18 but 3.20 (and now 3.21) is the better default since some features weren't ready?

@hhb
Copy link
Collaborator

hhb commented Apr 2, 2021

Done (and thanks!)

The fix will be in CMake 3.20.1. Not 3.21. But surely you can set it to 3.21 to be safe.

I'm a little unsure how the testing works here.

I cherry-picked necessary changes into that Cmake before 3.20 is available. 😀

@DanAlbert
Copy link
Member

The fix will be in CMake 3.20.1. Not 3.21. But surely you can set it to 3.21 to be safe.

I had misread, but I think I'm going to stick with it. Potential behavior changes shouldn't be introduced because the user took a bugfix release of CMake IMO. Sound reasonable to you?

@enh-google
Copy link
Collaborator

(i know that question wasn't addressed to me, but from hhb's comment, your comment, and my usual cowardice about such things, it looks like we're all in agreement that 3.21 is the safe choice :-) )

@hhb
Copy link
Collaborator

hhb commented Apr 3, 2021

Sounds good. The alternative is to revert this for 3.20.0. But I guess that's too much trouble.

@hhb hhb closed this as completed Apr 3, 2021
@baldurk
Copy link
Author

baldurk commented Apr 3, 2021

Just to clarify then it sounds like my original approach should be fine, if that executable moves location in a future NDK then the $CMAKE_STRIP variable will be pointing to the new right place?

@DanAlbert
Copy link
Member

I believe that's the intent, yes. @hhb is the expert here so he'll correct me if I'm wrong :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants