android / ndk

The Android Native Development Kit
1.99k stars 257 forks source link

[BUG] Shaderc: libshaderc_combined make target doesn't work #1634

Closed sfan5 closed 2 years ago

sfan5 commented 2 years ago

Description

Attempting to build a combined shaderc using sources/third_party/shaderc/Android.mk fails with the following error:

[arm64-v8a] Combine: libshaderc_combined.a <= libglslang.a libOGLCompiler.a libOSDependent.a libshaderc.a libshaderc_util.a libSPIRV.a libHLSL.a libSPIRV-Tools.a libSPIRV-Tools-opt.a
/bin/sh: line 1: [...]/android-ndk-r23b/toolchains/llvm/prebuilt/linux-x86_64/bin/aarch64-linux-android-ar: No such file or directory
make: *** [Android.mk:77: [...]/local/arm64-v8a/libshaderc_combined.a] Error 127

Binutils were removed from the NDK in r23 so someone just forgot to update the build script.

Fixing this is a two line change:

--- sources/third_party/shaderc/Android.mk.old  2021-12-18 15:31:11 +0100
+++ sources/third_party/shaderc/Android.mk  2021-12-18 15:31:14 +0100
@@ -52,8 +52,8 @@

 $(1)/libshaderc_combined.a: $(addprefix $(1)/, $(ALL_LIBS)) $(1)/combine.ar
    @echo "[$(TARGET_ARCH_ABI)] Combine: libshaderc_combined.a <= $(ALL_LIBS)"
-   @cd $(1) && $(2)ar -M < combine.ar && cd $(ROOT_SHADERC_PATH)
-   @$(2)objcopy --strip-debug $(1)/libshaderc_combined.a
+   @cd $(1) && llvm-ar -M < combine.ar && cd $(ROOT_SHADERC_PATH)
+   @llvm-objcopy --strip-debug $(1)/libshaderc_combined.a

 $(NDK_APP_LIBS_OUT)/$(APP_STL)/$(TARGET_ARCH_ABI)/libshaderc.a: \
        $(1)/libshaderc_combined.a

Environment Details

DanAlbert commented 2 years ago

Hmm, I guess that configuration isn't covered by our tests? What's the build command here? I'm not familiar with shaderc (CC @dneto0 )

Your fix seems obviously correct (thanks!) but I want to make sure we get a regression test added.

DanAlbert commented 2 years ago

If we get this sorted in the next week we can pick this up for r24 RC 1. Sorry, I know I'm responding 17 days after filing asking for a quick response, but we were all out on vacation :)

sfan5 commented 2 years ago
cd android-ndk-r23b/sources/third_party/shaderc
../../../ndk-build NDK_PROJECT_PATH=. APP_BUILD_SCRIPT=Android.mk APP_STL=c++_shared APP_ABI=arm64-v8a libshaderc_combined

should reproduce it

DanAlbert commented 2 years ago

Great, thanks!

DanAlbert commented 2 years ago

I fixed it slightly differently: https://android-review.googlesource.com/c/platform/external/shaderc/shaderc/+/1946087

I think the project owner will want to send that upstream and merge it back down so it might not make it into the initial r24 release, but we can include it in r24b (I see you're using r23b, and it's planned for r23c either way). I'm waiting for him to respond though, so it might be something that makes it for r24 RC 1 after all.

DanAlbert commented 2 years ago

r24 is not an LTS so it will lose support when r25 ships in early July. Due to a lack of bandwidth in our dependencies, it won't be possible to ship r24b before the end of the support window. Closing since this is already fixed in r25 and r23c.