-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[android] Update to LTS NDK 27 #76460
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
Conversation
Rebased, @al45tair, would you run the CI on this and review? |
@swift-ci test |
Rebased, updated to latest NDK 27c, and added some runtime libraries that were being listed over and over again when linking test runners for Android to the list of libraries to be de-duplicated. @etcwilde, would you review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the changes look fine. I'd like to get approval from @compnerd once he's back though. Is this needed immediately, or can it wait a couple more weeks?
It can wait. The BC guys like Saleem haven't been too active on github lately, something going on? |
@hyp, if you're active again, would you review? |
Add a new bits/ header to the Android overlay, include runtime libraries that are auto-extracted and listed many times to the list of libraries to be de-duplicated, enable a C++ interop test that's working again, and update the doc with new libraries that need to be available to run a simple executable.
Rebased and enabled another C++ Interop test that now passes on Android. Ping @etcwilde, it's been three weeks with no response, it would be good to get this in before the branch. If the BC people have any complaint later, which is extremely unlikely, we can always modify the single module map change then. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is unclear if the test/modularization changes would result in issues for r26 or not. In essence, does this prevent the use of r26 or is it simply updating the documentation to reference r27? r26 also is a LTS release, so I'm not sure if we should be upgrading so aggressively. However, updating to also support r27 is preferable.
The modularization change was explicitly tested with NDK 26, as I noted months ago above, "it still worked fine with NDK 26 too." The test change I've only been running with NDK 27 lately, but I believe it works for both.
Neither, both NDKs should be fine and it is clearly updating more than the docs.
NDK 27 is the current LTS release, which means NDK 26 is no longer officially supported. Nevertheless, this pull should work fine with both.
Yes, that is what this pull does. |
Ping @compnerd, answered all your questions, would be good to get this in. |
@egorzhdan, need a CI run here. |
@bnbarham, please run the CI here. |
@swift-ci please test |
I validated this on a windows host with NDK 26 , this looks good to me |
Alright, passed CI, @ktoso, would you merge? I figure everybody else in this thread is in the US and probably sleeping now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, happy to since seems others approved already
Add a new
bits/
header to the Android overlay, include runtime libraries that areauto-extracted and listed many times to the list of libraries to be de-duplicated,
enable a C++ interop test that's working again, and update the doc with new
libraries that need to be available to run a simple executable.
@compnerd and @hyp, I switched over to the latest NDK 27 with this patch a couple months ago, no problem, plus it still worked fine with NDK 26 too. Let me know if you're using NDK 27 yet and please review.
@etcwilde, let me know what you think.