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

Add support for vita #465

Merged
merged 3 commits into from
Oct 9, 2023
Merged

Add support for vita #465

merged 3 commits into from
Oct 9, 2023

Conversation

pheki
Copy link
Contributor

@pheki pheki commented Sep 6, 2023

Needs rust-lang/libc#3284 to compile

@nikarh
Copy link
Contributor

nikarh commented Sep 6, 2023

I think socketaddr_in is also a bit different on Vita and we need to add sin_len and sin_vport fields under a cfg flag
NVM, I think it's related to 0.4.9 and hyper using it

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.

I'm fine with merging this, but I don't think we can officially support it at this time.

Any way we can set up CI for this? Running cargo test or cargo check?

@pheki
Copy link
Contributor Author

pheki commented Sep 7, 2023

No official support is fine for me, it's a tier 3 platform after all :)

Any way we can set up CI for this? Running cargo test or cargo check?

AFAIK there's currently no way to run tests right now. Check works using:

cargo +nightly check -Z build-std=std --target armv7-sony-vita-newlibeabihf

So we could add check to Github CI using nightly and -Z build-std.

@Thomasdezeeuw
Copy link
Collaborator

No official support is fine for me, it's a tier 3 platform after all :)

Any way we can set up CI for this? Running cargo test or cargo check?

AFAIK there's currently no way to run tests right now. Check works using:

cargo +nightly check -Z build-std=std --target armv7-sony-vita-newlibeabihf

So we could add check to Github CI using nightly and -Z build-std.

That sounds good. You can copy what I removed in 14be1bc

@nikarh
Copy link
Contributor

nikarh commented Sep 8, 2023

Some tests are failing, because on vita there's no fcntl for non-blocking sockets, need to do it via setsockopt like in std:

setsockopt(fd, libc::SOL_SOCKET, libc::SO_NONBLOCK, nonblocking as libc::c_int)

@pheki pheki marked this pull request as draft September 9, 2023 15:12
@pheki
Copy link
Contributor Author

pheki commented Sep 9, 2023

Converted this PR to draft as we (vita-rust community) are still figuring some things out

@nikarh
Copy link
Contributor

nikarh commented Sep 23, 2023

At this point PR can be undrafted when rust-lang/libc#3284 is merged and released (we will need to bump libc version here for a green pipeline).

bors added a commit to rust-lang/libc that referenced this pull request Oct 6, 2023
Update crate version to 0.2.149

Contains fixes for net structs/calls for Vita target - #3284 and #3366. Required for rust-lang/socket2/pull/465
@nikarh
Copy link
Contributor

nikarh commented Oct 7, 2023

So, we're finally ready 😇

  • Added check and docs for VIta to CI pipeline
  • Nonblocking flag is set in via setsockopt and not fcntl on Vita (fcntl doesn't exist in Vita newlib, though at some point we probably will implement it least for that particular operation)
  • There is no ipv6 stack on Vita, so all tests using that are disabled for the target
  • All tests that are enabled for Vita are passing

Don't run tests for properties not available

Added CI, fixed tests
@pheki pheki marked this pull request as ready for review October 7, 2023 16:07
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.

One change has to be reverted as it's unsound. The other comments are small points.

Other than that I think this on the way to get merged.

src/lib.rs Outdated Show resolved Hide resolved
src/sys/unix.rs Outdated Show resolved Hide resolved
tests/socket.rs Show resolved Hide resolved
tests/socket.rs Outdated Show resolved Hide resolved
tests/socket.rs Show resolved Hide resolved
tests/socket.rs Outdated Show resolved Hide resolved
tests/socket.rs Outdated Show resolved Hide resolved
@nikarh
Copy link
Contributor

nikarh commented Oct 9, 2023

Fixed all the comments, please check. With the API change I shouldn't have trusted clippy --fix, reverted and added clippy allow there.

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.

Nice, LGTM!

Also forgot to say that I like the CI setup.

@Thomasdezeeuw Thomasdezeeuw merged commit f61a788 into rust-lang:master Oct 9, 2023
39 checks passed
@Thomasdezeeuw
Copy link
Collaborator

Thanks @nikarh

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.

3 participants