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.

Sign up for GitHub

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

Jump to bottom

ndk-build: Support NDK r22 #94

Merged
merged 3 commits into from
Nov 12, 2020
Merged

ndk-build: Support NDK r22 #94

merged 3 commits into from
Nov 12, 2020

Conversation

MarijnS95
Copy link
Member

$ndk/platforms is deprecated and finally removed in r22; android-ndk-rs was never using this so it's safe to remove.

If NDK platform compatibility checking is still desired, we should check for $ndk/toolchains/${toolchain:-llvm}/prebuilt/${host:-linux-x86_64}/sysroot/usr/lib/$triple/$api/ (Linux ndk, might differ on other platforms) instead.

At the same time perform some minor code cleanup. How are the maintainers feeling about enabling clippy in the CI and fixing few remaining issues?

@MarijnS95
ndk-build: Replace starts_with and manual slice with strip_prefix
2a0d830 
Clippy attained a new lint, manual_strip [1]. It won't find these nested
cases, but we can still introduce .strip_prefix() regardless and make
the code ever so slightly more readable :)

[1]: https://rust-lang.github.io/rust-clippy/master/#manual_strip
@MarijnS95
ndk-build: Also search in ANDROID_NDK_{PATH,HOME}
f90df21 
Apparently these are valid as well:
https://github.com/actions/virtual-environments/blob/0216e5ce2b6ac64435ae244b1e15b957d54a333e/images/win/scripts/Installers/Update-AndroidSDK.ps1#L82-L83
@MarijnS95
ndk-build: Don't search for "platforms" in NDK path (r22 compat)
c189c58 
This folder has been deprecated since r19 and finally disappeared in
r22:

r21 changelog, https://github.com/android/ndk/wiki/Changelog-r22:

    The legacy toolchain install paths will be removed over the coming
    releases. These paths have been obsolete since NDK r19 and take up a
    considerable amount of space in the NDK. The paths being removed
    are:
    - platforms
    - [...]

And removed in r22, https://github.com/android/ndk/wiki/Changelog-r22:

    The deprecated <NDK>/platforms and <NDK>/sysroot directories have
    been removed. These directories were merged and relocated into the
    toolchain during r19. [...]

Since this path was never used in android-ndk-rs (`fn platform_dir` only
ever returns "platforms" inside `sdk_path`) it is safe to remove.
@msiglreith
Copy link
Contributor

At the same time perform some minor code cleanup. How are the maintainers feeling about enabling clippy in the CI and fixing few remaining issues?

I'm personally fine with it, as I'm using a copy of embarks github action 😄

@msiglreith msiglreith merged commit b5844d8 into rust-mobile:master Nov 12, 2020
@MarijnS95
Copy link
Member Author

How are the maintainers feeling about enabling clippy in the CI and fixing few remaining issues?

I'm personally fine with it, as I'm using a copy of embarks github action

@msiglreith You mean their cargo-deny action? That serves a different purpose than a simple clippy job that performs code linting.

@MarijnS95 MarijnS95 deleted the ndk-r22 branch November 12, 2020 11:42
@msiglreith
Copy link
Contributor

I was referring to the linting parts: https://github.com/EmbarkStudios/opensource-template/blob/main/.github/workflows/rust-ci.yml#L8-L35

@MarijnS95
Copy link
Member Author

@msiglreith Right, I expected that after posting. That's the one I'm suggesting to add, but actions-rs/toolchain can install components nowadays with components: rustfmt, clippy, superseding rustup component add. Perhaps worth submitting a PR for that over at Embark as well 😃

@Jasper-Bekkers Jasper-Bekkers mentioned this pull request Dec 10, 2020
Open
38 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@msiglreith msiglreith msiglreith approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@MarijnS95 @msiglreith