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: use signal backend on android #80

Merged
merged 4 commits into from
Jun 1, 2024

Conversation

kod-kristoff
Copy link
Contributor

Rustix does not expose pidfd functions for android, so this
commit changes so that the signal backend is used on android.
The pidfd functions were introduced in Linux kernel 5.1, and
epoll integration were introduced in 5.3.

Android version 13, is the first version where all released
version of the Android common kernel is based on Linux kernel
5.4 or higher.
https://source.android.com/docs/core/architecture/kernel/android-common

Resolves Issue #79

Rustix does not expose pidfd functions for android, so this
commit changes so that the signal backend is used on android.
The pidfd functions were introduced in Linux kernel 5.1, and
epoll integration were introduced in 5.3.

Android version 13, is the first version where all released
version of the Android common kernel is based on Linux kernel
5.4 or higher.
<https://source.android.com/docs/core/architecture/kernel/android-common>

Resolves Issue smol-rs#79
Copy link
Member

@notgull notgull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Can you add a test for building on Android to the CI?

@kod-kristoff
Copy link
Contributor Author

Thank you! Can you add a test for building on Android to the CI?

yes, i'll add a test

@kod-kristoff
Copy link
Contributor Author

I am working on tests, but currently all tests pass except (all in tests/std.rs):

  • test_add_to_env
  • test_capture_env_at_spawn
  • test_override_env

All 3 tests fails because they can't find /system/bin/sh when runned through qemu.
All tests passes on my device.

I will give the qemu another look later.

@notgull
Copy link
Member

notgull commented May 27, 2024

A full test is overkill and can be done in another PR. For now just make sure it builds.

@notgull notgull mentioned this pull request Jun 1, 2024
@taiki-e
Copy link
Collaborator

taiki-e commented Jun 1, 2024

All 3 tests fails because they can't find /system/bin/sh when runned through qemu.

I think we can support this case by copying more things here.

@kod-kristoff kod-kristoff force-pushed the use-signal-reaper-on-android branch from 5668511 to 2ed6dae Compare June 1, 2024 14:43
@kod-kristoff
Copy link
Contributor Author

A full test is overkill and can be done in another PR. For now just make sure it builds.

I have change so that the crate only is built for android

Copy link
Member

@notgull notgull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM aside from one nit

.github/workflows/ci.yml Outdated Show resolved Hide resolved
@kod-kristoff kod-kristoff requested a review from notgull June 1, 2024 15:01
Copy link
Member

@notgull notgull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@notgull notgull merged commit dd41201 into smol-rs:master Jun 1, 2024
15 checks passed
@kod-kristoff kod-kristoff deleted the use-signal-reaper-on-android branch June 1, 2024 15:04
@taiki-e taiki-e mentioned this pull request Jun 1, 2024
@taiki-e
Copy link
Collaborator

taiki-e commented Jun 1, 2024

All 3 tests fails because they can't find /system/bin/sh when runned through qemu.

I think we can support this case by copying more things here.

Implemented in setup-cross-toolchain-action 1.22.0: #84

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

Successfully merging this pull request may close these issues.

3 participants