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

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

Closed
sfan5 opened this issue Dec 18, 2021 · 6 comments
Closed

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

sfan5 opened this issue Dec 18, 2021 · 6 comments
Assignees
Labels

Comments

@sfan5
Copy link

sfan5 commented Dec 18, 2021

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

  • NDK Version: 23.1.7779620
  • Build system: ndk-build
  • Host OS: Arch Linux x86_64
  • ABI: (irrelevant)
  • NDK API level: (irrelevant)
  • Device API level: (irrelevant)
@sfan5 sfan5 added the bug label Dec 18, 2021
@DanAlbert
Copy link
Member

DanAlbert commented Jan 4, 2022

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 DanAlbert added this to Triaged in r23c via automation Jan 4, 2022
@DanAlbert DanAlbert added this to Triaged in r24 via automation Jan 4, 2022
@DanAlbert
Copy link
Member

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
Copy link
Author

sfan5 commented Jan 5, 2022

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
Copy link
Member

Great, thanks!

@DanAlbert
Copy link
Member

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 DanAlbert self-assigned this Jan 11, 2022
@DanAlbert DanAlbert removed this from Triaged in r24 Jan 14, 2022
@DanAlbert DanAlbert added this to Triaged in r24b via automation Jan 14, 2022
@DanAlbert DanAlbert moved this from Triaged to Needs cherry-pick in r23c May 10, 2022
@DanAlbert DanAlbert moved this from Needs cherry-pick to Merged in r23c May 11, 2022
@DanAlbert
Copy link
Member

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.

@DanAlbert DanAlbert added this to Triaged in r25 via automation Jun 27, 2022
@DanAlbert DanAlbert moved this from Triaged to Needs cherry-pick in r25 Jun 27, 2022
@DanAlbert DanAlbert moved this from Needs cherry-pick to Merged in r25 Jun 27, 2022
osspop pushed a commit to osspop/android-ndk that referenced this issue Jan 17, 2023
Bug: android/ndk#1634
Test: N/A
Change-Id: Iae728c946cda17e6afb3457038aac60443b6fbf1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
r23c
  
Merged
r24b
  
Triaged
r25
  
Merged
Development

No branches or pull requests

2 participants