Skip to content

Keep posix_memalign() around for older Android APIs #811

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
Merged
merged 1 commit into from
Apr 25, 2024
Merged

Keep posix_memalign() around for older Android APIs #811

merged 1 commit into from
Apr 25, 2024

Conversation

finagolfin
Copy link
Member

This simply brings back #805 for platforms that need it.

@finagolfin finagolfin mentioned this pull request Feb 1, 2024
Merged
@finagolfin
Copy link
Member Author

@compnerd, this adds the fallback you asked for, please review.

@finagolfin
Copy link
Member Author

@rauhul, please run the CI.

@MaxDesiatov MaxDesiatov requested a review from etcwilde February 7, 2024 12:24
compnerd
compnerd reviewed Feb 9, 2024
View reviewed changes
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.

Personally, I would prefer that we check the support for aligned_alloc in CMake and then fallback to posix_memalign rather than just hardcode the specific platform.

@finagolfin
Copy link
Member Author

Alright, I will look into that.

triplef added a commit to gnustep/tools-android that referenced this pull request Feb 9, 2024
@triplef
libdispatch: added patch to fix builds with API level < 28
19439bc 
To be removed when this is merged upstream:
swiftlang/swift-corelibs-libdispatch#811

Fixes #19.
@triplef triplef mentioned this pull request Feb 9, 2024
Closed
@finagolfin
Copy link
Member Author

I will finish this up this month and submit it.

etcwilde
etcwilde reviewed Mar 22, 2024
View reviewed changes
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.

Seems reasonable to me. I agree with @compnerd that we can let CMake select it so it's not platform specific, but bassed on what the sysroot itself has. I tried doing that in parts of corelibs-Foundation here, but ran into issues with xcodebuild: swiftlang/swift-corelibs-foundation#4860
That shouldn't be a problem here because we don't build this for macOS at all.

@finagolfin
Check for aligned_alloc() and use posix_memalign() instead on pla…
4a832dc 
…tforms, like Android before API 28, that didn't have it
@finagolfin
Copy link
Member Author

Sorry about the delay on this, @compnerd, ready for testing and review.

@finagolfin
Copy link
Member Author

@kateinoigakukun, please run the CI on this.

@finagolfin
Copy link
Member Author

@rauhul, need a CI run here.

@kateinoigakukun
Copy link
Member

@swift-ci test

@finagolfin
Copy link
Member Author

Ping @compnerd, passed CI, just needs review and merge.

@compnerd compnerd merged commit d35e7a3 into swiftlang:main Apr 25, 2024
2 checks passed
@finagolfin finagolfin deleted the droid branch April 25, 2024 20:29
@finagolfin finagolfin mentioned this pull request Apr 25, 2024
Closed
triplef added a commit to gnustep/tools-android that referenced this pull request Apr 26, 2024
@triplef
libdispatch: removed patch fixed upstream
1c0378a 
swiftlang/swift-corelibs-libdispatch#811
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@etcwilde etcwilde etcwilde left review comments

@compnerd compnerd compnerd 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.
4 participants
@finagolfin @kateinoigakukun @compnerd @etcwilde