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.

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

Fix recent build regression on Android #883

Merged
merged 1 commit into from Jul 11, 2023
Merged

Conversation

finagolfin
Copy link
Contributor

@finagolfin finagolfin commented Jul 7, 2023

Bionic added execinfo.h last year, but only made the backtrace APIs available
for Android API 33 or later
.

@3405691582, #881 also removed your OpenBSD check, but I see that OpenBSD recently added this header, mesonbuild/meson#11151, so the header check should suffice there?

@al45tair, you suggested this change, please review.

Here is the error message showing this failing on my Android CI, and the subsequent pull adding this change to my Android CI that fixed it, finagolfin/swift-android-sdk#112.

@3405691582
Copy link
Contributor

#881 is probably fine for OpenBSD, since we're just changing headers at this point. (I really should do a build soon to confirm, but I'm a little behind on Swift stuff.)

@al45tair
Copy link
Contributor

al45tair commented Jul 7, 2023

@swift-ci Please smoke test

@al45tair
Copy link
Contributor

al45tair commented Jul 7, 2023

@swift-ci Please smoke test Windows platform

@finagolfin
Copy link
Contributor Author

The Windows CI got stuck?

@finagolfin
Copy link
Contributor Author

Wait a second, I screwed up my logic in the condition, will fix it.

@finagolfin
Copy link
Contributor Author

@al45tair, your suggestion in the comment above was right in the first place. I was trying to make it less convoluted, but my reformulation was not correct.

@al45tair
Copy link
Contributor

al45tair commented Jul 8, 2023

@swift-ci Please smoke test

@al45tair
Copy link
Contributor

al45tair commented Jul 8, 2023

@swift-ci Please smoke test Windows platform

@finagolfin
Copy link
Contributor Author

Windows CI failed at checkout, all others passed.

@al45tair
Copy link
Contributor

al45tair commented Jul 8, 2023

@swift-ci Please smoke test windows platform

@finagolfin
Copy link
Contributor Author

Gah, should have written this as if __has_include(<execinfo.h>) && !(defined(__ANDROID__) && __ANDROID_API__ < 33), which reads better, and is what I was trying to come up with a couple days ago but screwed up.

@al45tair, wdyt, worth another CI run to rewrite this?

@al45tair
Copy link
Contributor

Up to you. I don't mind either way. Add another commit to get it how you want it and I'll trigger CI again for you.

Bionic added execinfo.h last year, but only made the backtrace APIs available
for Android API 33 or later.
@finagolfin
Copy link
Contributor Author

Alright, rewrote it, one last CI run and this can go in.

@al45tair
Copy link
Contributor

@swift-ci Please smoke test

@al45tair
Copy link
Contributor

@swift-ci Please smoke test Windows platform

@finagolfin
Copy link
Contributor Author

@al45tair, this one and apple/swift#66745 are ready for merge.

@al45tair al45tair merged commit 8c724a2 into apple:main Jul 11, 2023
3 checks passed
@finagolfin finagolfin deleted the droid branch July 11, 2023 10:41
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.

None yet

3 participants