Skip to content

[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

Merged
merged 1 commit into from
Nov 15, 2024
Merged

[android] Update to LTS NDK 27 #76460

merged 1 commit into from
Nov 15, 2024

Conversation

finagolfin
Copy link
Member

@finagolfin finagolfin commented Sep 13, 2024

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.

@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.

@finagolfin
Copy link
Member Author

Rebased, @al45tair, would you run the CI on this and review?

@shahmishal
Copy link
Member

@swift-ci test

@finagolfin finagolfin changed the title [android] Update to LTS NDK 27b [android] Update to LTS NDK 27 Oct 21, 2024
@finagolfin
Copy link
Member Author

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?

Copy link
Contributor

@etcwilde etcwilde left a 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?

@finagolfin
Copy link
Member Author

It can wait.

The BC guys like Saleem haven't been too active on github lately, something going on?

@finagolfin
Copy link
Member Author

@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.
@finagolfin
Copy link
Member Author

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.

Copy link
Member

@compnerd compnerd left a 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.

@finagolfin
Copy link
Member Author

It is unclear if the test/modularization changes would result in issues for r26 or not.

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.

In essence, does this prevent the use of r26 or is it simply updating the documentation to reference r27?

Neither, both NDKs should be fine and it is clearly updating more than the docs.

r26 also is a LTS release, so I'm not sure if we should be upgrading so aggressively

NDK 27 is the current LTS release, which means NDK 26 is no longer officially supported. Nevertheless, this pull should work fine with both.

However, updating to also support r27 is preferable.

Yes, that is what this pull does.

@finagolfin
Copy link
Member Author

Ping @compnerd, answered all your questions, would be good to get this in.

@finagolfin
Copy link
Member Author

@egorzhdan, need a CI run here.

@finagolfin
Copy link
Member Author

@bnbarham, please run the CI here.

@bnbarham
Copy link
Contributor

@swift-ci please test

@hyp
Copy link
Contributor

hyp commented Nov 14, 2024

I validated this on a windows host with NDK 26 , this looks good to me

@finagolfin
Copy link
Member Author

Alright, passed CI, @ktoso, would you merge? I figure everybody else in this thread is in the US and probably sleeping now.

Copy link
Contributor

@ktoso ktoso left a 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

@ktoso ktoso merged commit 45d15d1 into swiftlang:main Nov 15, 2024
5 checks passed
@finagolfin
Copy link
Member Author

Thanks, @etcwilde, @compnerd, @hyp, and @ktoso, wanted to get this in before the upcoming branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants