libc icon indicating copy to clipboard operation
libc copied to clipboard

Add missing constant for Android

Open yujincheng08 opened this issue 1 year ago • 1 comments

yujincheng08 avatar Jul 20 '24 09:07 yujincheng08

r? @JohnTitor

rustbot has assigned @JohnTitor. They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

rustbot avatar Jul 20 '24 09:07 rustbot

@maurer would you mind taking a look at this?

tgross35 avatar Aug 12 '24 10:08 tgross35

r? @maurer

yujincheng08 avatar Aug 12 '24 10:08 yujincheng08

Failed to set assignee to maurer: invalid assignee

Note: Only org members with at least the repository "read" role, users with write permissions, or people who have commented on the PR may be assigned.

rustbot avatar Aug 12 '24 10:08 rustbot

This looks fine to me, the reason those constants were 64-bit only before was as a hacky form of version detection. However, now that our min API level is 21, we shouldn't need to worry about that, and making them available to 32-bit should be appropriate. The rest of this appears to be appropriate constant migration.

The only thing I'll note is that since this bumps the NDK to the latest stable (r27), we'll want to bump it in rustc as well before integrating this change there after it lands. (I don't think it'll be as troublesome as last time, but we'll have to do the whole song and dance with updating the mirror again.)

maurer avatar Aug 12 '24 23:08 maurer

thanks @maurer

r? @tgross35 could you check if it's now okay to merge it?

yujincheng08 avatar Aug 13 '24 09:08 yujincheng08

:umbrella: The latest upstream changes (presumably #3777) made this pull request unmergeable. Please resolve the merge conflicts.

bors avatar Aug 14 '24 18:08 bors

@tgross35 hi, would you mind taking a look at this?

yujincheng08 avatar Aug 15 '24 07:08 yujincheng08

Please be patient with reviews, there are a lot of open PRs.

Relevant headers for comparison:

  • https://android.googlesource.com/platform/bionic/+/refs/tags/android-14.0.0_r61/libc/kernel/uapi/linux/auxvec.h
  • https://android.googlesource.com/platform/bionic/+/refs/tags/android-14.0.0_r61/libc/kernel/uapi/asm-arm/asm/auxvec.h
  • https://android.googlesource.com/platform/bionic/+/refs/tags/android-14.0.0_r61/libc/kernel/uapi/asm-arm64/asm/auxvec.h
  • https://android.googlesource.com/platform/bionic/+/refs/tags/android-14.0.0_r61/libc/kernel/uapi/asm-riscv/asm/auxvec.h
  • https://android.googlesource.com/platform/bionic/+/refs/tags/android-14.0.0_r61/libc/kernel/uapi/asm-x86/asm/auxvec.h

A few questions below then LGTM. Thanks Matthew for taking a look too.

@rustbot author

tgross35 avatar Aug 16 '24 07:08 tgross35

@rustbot ready

yujincheng08 avatar Aug 16 '24 07:08 yujincheng08

@rustbot author

yujincheng08 avatar Aug 16 '24 08:08 yujincheng08

@rustbot ready

yujincheng08 avatar Aug 16 '24 14:08 yujincheng08

Oh I only meant to start with the API added here for the risc-v file. But no harm of course :)

Please rebase to drop the merge commit, then looks good to me.

tgross35 avatar Aug 16 '24 16:08 tgross35

Rebased

yujincheng08 avatar Aug 16 '24 17:08 yujincheng08