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

Added support for vita target #1721

Merged
merged 2 commits into from
Oct 20, 2023
Merged

Added support for vita target #1721

merged 2 commits into from
Oct 20, 2023

Conversation

nikarh
Copy link
Contributor

@nikarh nikarh commented Oct 18, 2023

With these changes mio compiles and (mostly) works with armv7-sony-vita-newlibeabihf target with --cfg mio_unsupported_force_poll_poll --cfg mio_unsupported_force_waker_pipe rust flags.

armv7-sony-vita-newlibeabihf is a Tier 3 target with working std via newlib.

The target has no epoll or kqueue, only poll and select and async pipes, so cfg flags are required for compilation.
This is not an issue for devs using the target, since most if not all who work with it use cargo-vita tool for building, and it automatically adds these flags to RUSTFLAGS.

  • Like espidf it has no process API and thus no CLOEXEC
  • sockaddr_in is mildly different on this target, and has an additional field
  • no ioctl, so set_nonblocking for vita follows illumos
  • async pipes are implemented with pipe2 call for this target

Copy link
Collaborator

@Thomasdezeeuw Thomasdezeeuw left a comment

Choose a reason for hiding this comment

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

Hello again :)

With these changes mio compiles and (mostly) works with armv7-sony-vita-newlibeabihf target with --cfg mio_unsupported_force_poll_poll --cfg mio_unsupported_force_waker_pipe rust flags.

armv7-sony-vita-newlibeabihf is a Tier 3 target with working std via newlib.

The target has no epoll or kqueue, only poll and select and async pipes, so cfg flags are required for compilation. This is not an issue for devs using the target, since most if not all who work with it use cargo-vita tool for building, and it automatically adds these flags to RUSTFLAGS.

To be clear --cfg mio_unsupported_force_poll_poll/mio_unsupported_force_waker_pipe is not meant to be used for normal development. It's only to overwrite the preferred polling/waking implementation.

Since vita doesn't have epoll or kqueue the preferred polling implementation is the poll(2) one. It's a bit messy at the moment with cfg are round this place (also see #1715), but once merged no develop should have to use a RUSTFLAG to run Mio on vita.

* Like `espidf` it has no process API and thus no `CLOEXEC`

* `sockaddr_in` is mildly different on this target, and has an additional field

* no `ioctl`, so `set_nonblocking` for vita follows illumos

* async pipes are implemented with `pipe2` syscall for this target

👍 LGTM.

Can we add a cargo check to the CI here as well? I think the socket2 one can be copied.

@nikarh
Copy link
Contributor Author

nikarh commented Oct 18, 2023

Hi!

Can we add a cargo check to the CI here as well? I think the socket2 one can be copied.

I can, but it won't compile without these cfg flags right now, and I'm not sure they should be added to ci checks.
Should I change #[cfg()] in code and add vita target so that it compiles without RUSTFLAGS? Or wait for #1715? Or add CI but with the flags?

@Thomasdezeeuw
Copy link
Collaborator

I can, but it won't compile without these cfg flags right now, and I'm not sure they should be added to ci checks.

They were actually added to test the poll(2) implementation on the CI, but see below.

Should I change #[cfg()] in code and add vita target so that it compiles without RUSTFLAGS? Or wait for #1715? Or add CI but with the flags?

If you can change them in this pr that would be great. I wanted to this myself in #1715, but I without rustup support for Solaris isn't quite a pain in the but to test.

@nikarh
Copy link
Contributor Author

nikarh commented Oct 18, 2023

Updated PR - added target_os = "vita" to cfg where necessary, and a CI cargo check step.

Although for tokio we still need to set mio_unsupported_force_poll_poll - https://github.com/tokio-rs/tokio/blob/881b510a072f5acd773a10d3be0debf74113404e/tokio/src/io/poll_evented.rs#L171

Maybe this implementation detail can somehow be exposed? (not in the scope of this PR of course)

@Thomasdezeeuw
Copy link
Collaborator

Updated PR - added target_os = "vita" to cfg where necessary, and a CI cargo check step.

👍

Although for tokio we still need to set mio_unsupported_force_poll_poll - https://github.com/tokio-rs/tokio/blob/881b510a072f5acd773a10d3be0debf74113404e/tokio/src/io/poll_evented.rs#L171

Maybe this implementation detail can somehow be exposed? (not in the scope of this PR of course)

This is technically Tokio depending on unspecific behaviour (see tokio-rs/tokio#5866 (comment)). Let's try resolve that in tokio-rs/tokio#5866.

Copy link
Collaborator

@Thomasdezeeuw Thomasdezeeuw left a comment

Choose a reason for hiding this comment

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

LGTM, can't test, but the CI seems happy.

@Thomasdezeeuw Thomasdezeeuw merged commit 4034872 into tokio-rs:master Oct 20, 2023
21 checks passed
@Thomasdezeeuw
Copy link
Collaborator

Thanks @nikarh

@nikarh
Copy link
Contributor Author

nikarh commented Oct 22, 2023

Hey, sorry to bother you @Thomasdezeeuw, would it be possible to make a release with these changes?
This would help with tokio-rs/tokio/pull/6094

Thanks!

@zetanumbers zetanumbers deleted the vita branch October 22, 2023 18:28
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.

2 participants