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

[FR] improve fallback behavior of ndk-which when triple-prefixed tools are not found #1593

Closed
Hsilgos opened this issue Sep 29, 2021 · 5 comments
Assignees
Projects

Comments

@Hsilgos
Copy link

Hsilgos commented Sep 29, 2021

Description

Currently we switch from 21.4.7075529 to 23.0.7599858 and found an issue.
In our build script we use ndk-which utility to find readelf.
In NDK 23 it stopped to show any result.

Step to reproduce:
Run ~/AndroidSDK/ndk/23.0.7599858/ndk-which --abi x86_64 readelf
Expected result: Full path to readelf utility.
Actual result: empty output.

Details:
After small debugging it's clear that ndk-which is trying path ~/AndroidSDK/ndk/23.0.7599858/toolchains/llvm/prebuilt/darwin-x86_64/bin/x86_64-linux-android-readelf which doesn't exist in NDK anymore.
Either this binary should be returned back or ndk-which must try path ~/AndroidSDK/ndk/23.0.7599858/toolchains/llvm/prebuilt/darwin-x86_64/bin/llvm-readelf instead.

Environment Details

  • NDK Version: 23.0.7599858
  • Host OS: Mac (may be other OSes)
  • ABI: Any
@Hsilgos Hsilgos added the bug label Sep 29, 2021
@enh-google
Copy link
Collaborator

yeah, the GNU binutils have been removed in r23. you'll want to use llvm-readelf instead. (no need to choose a different binary for each architecture --- one benefit of the LLVM tools is that they "just work" regardless of architecture.)

so this is working as intended from that perspective, but... this makes me realize that when the release notes say "GNU binutils ... has been removed", it's not necessarily the case that everyone knows what GNU binutils is. maybe we should call out the individual tools explicitly in the changelog, and list the replacement next to each one? (for anyone reading this before we do that --- just remove any existing architecture specific prefix like x86_64-linux-android- and replace it with llvm- instead.)

@Hsilgos
Copy link
Author

Hsilgos commented Sep 30, 2021

yeah, the GNU binutils have been removed in r23. you'll want to use llvm-readelf instead. (no need to choose a different binary for each architecture --- one benefit of the LLVM tools is that they "just work" regardless of architecture.)

so this is working as intended from that perspective, but... this makes me realize that when the release notes say "GNU binutils ... has been removed", it's not necessarily the case that everyone knows what GNU binutils is. maybe we should call out the individual tools explicitly in the changelog, and list the replacement next to each one? (for anyone reading this before we do that --- just remove any existing architecture specific prefix like x86_64-linux-android- and replace it with llvm- instead.)

I see your point. But to my understanding ndk-which is more than tool which combines ABI + architecture + platform + utility-name, but it's more or less cross platform entry point to find necessary tool in NDK's directory hierarchy. All I need is environment variable ANDROID_NDK_HOME and that's it, I was able to find necessary tool like readelf on any platform with simple call to ndk-which. It's convenient and would be nice to have.

@enh-google
Copy link
Collaborator

yeah, we actually talked about this in yesterday's team meeting and @DanAlbert suggested that what we should probably do is change it so that if ndk-which foo fails, it tries to look for llvm-foo too as an automatic fallback. there are a few special cases where there's a tool that isn't equivalent, but we should probably catch those too and output an error message telling you what your choices are.

@DanAlbert DanAlbert added this to Triaged in r24 via automation Sep 30, 2021
@DanAlbert DanAlbert added this to Triaged in r23c via automation Sep 30, 2021
@DanAlbert
Copy link
Member

(we generally don't backport features but the work is so non-invasive I think it makes sense to backport once done)

@Hsilgos Hsilgos changed the title [BUG] NDK 23, ndk-which doesn't can't find most of utilities. [BUG] NDK 23, ndk-which can't find most of utilities. Oct 13, 2021
@DanAlbert DanAlbert moved this from Triaged to Needs prebuilt update in r23c Nov 9, 2021
@DanAlbert DanAlbert moved this from Needs prebuilt update to Needs cherry-pick in r23c Nov 9, 2021
@DanAlbert DanAlbert moved this from Needs cherry-pick to Triaged in r23c Nov 9, 2021
@DanAlbert DanAlbert added enhancement and removed bug labels Dec 7, 2021
@DanAlbert DanAlbert changed the title [BUG] NDK 23, ndk-which can't find most of utilities. [FR] improve fallback behavior of ndk-which when triple-prefixed tools are not found Dec 7, 2021
@DanAlbert DanAlbert moved this from Triaged to Needs cherry-pick in r24 Jan 11, 2022
r24 automation moved this from Needs cherry-pick to Merged Jan 14, 2022
r23c automation moved this from Triaged to Merged Jan 14, 2022
@DanAlbert DanAlbert moved this from Merged to Needs cherry-pick in r23c Jan 14, 2022
@DanAlbert
Copy link
Member

Should be fixed in r23 build 8486889.

@DanAlbert DanAlbert moved this from Needs cherry-pick to Merged in r23c Apr 22, 2022
MaoHan001 pushed a commit to riscv-android-src/platform-ndk that referenced this issue Jun 22, 2022
This is already in the r24 changelog, but I'm cherry-picking the fix for
r23c too.

Bug: android/ndk#1593
Test: None
Change-Id: I15a8b4e3e4c4e03b60ebd1318f946447212570b6
(cherry picked from commit c8bb28397646674d388202d0dad1e7025f251fa4)
Merged-In: I15a8b4e3e4c4e03b60ebd1318f946447212570b6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
r23c
  
Merged
r24
  
Merged
Development

No branches or pull requests

4 participants