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 definitions for s390x musl targets #2071

Merged
merged 8 commits into from
Feb 28, 2021
Merged

Conversation

kaniini
Copy link
Contributor

@kaniini kaniini commented Feb 15, 2021

Add support for s390x musl targets to libc.

I haven't added CI because I am not familiar with the pipelines, but would be glad to do so if somebody outlines what needs to be done.

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Amanieu (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@kaniini
Copy link
Contributor Author

kaniini commented Feb 16, 2021

CI is mostly blocking on rust-lang/rust#82166 being merged, which adds the s390x-unknown-linux-musl target.

@Amanieu
Copy link
Member

Amanieu commented Feb 16, 2021

LGTM. Let's wait on rust-lang/rust#82166 so we can have CI set up.

I'm not too familiar with the libc CI, but basically just copy what we're already doing for other architectures on musl (e.g. mips, arm). The relevant files are all in ci/.

@kaniini
Copy link
Contributor Author

kaniini commented Feb 16, 2021

Yeah that's why I decided to go with #82166 first. Then handle the libc bits and any std bits afterward.

Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

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

These all are destined to go into fn ioctl which is expecting a c_int.

src/unix/linux_like/linux/musl/b64/s390x.rs Outdated Show resolved Hide resolved
src/unix/linux_like/linux/musl/b64/s390x.rs Outdated Show resolved Hide resolved
@kaniini
Copy link
Contributor Author

kaniini commented Feb 26, 2021

Based on conversation in Zulip, I wonder if it makes sense to move a lot of these constants to the common part of the musl support code.

@workingjubilee
Copy link
Member

Yes, I think so, that's kind of how the libc crate is "supposed" to be designed (according to https://github.com/rust-lang/libc/blob/master/CONTRIBUTING.md anyways!). Probably using a similar structure as the "generic bits" vs. "arch-specific bits" pattern that musl uses for its own organization.

kaniini and others added 3 commits February 27, 2021 17:35
Co-authored-by: Jubilee <46493976+workingjubilee@users.noreply.github.com>
…constants

Co-authored-by: Jubilee <46493976+workingjubilee@users.noreply.github.com>
@kaniini
Copy link
Contributor Author

kaniini commented Feb 28, 2021

@Amanieu this should be good to go now. rust#82166 has landed.

@Amanieu
Copy link
Member

Amanieu commented Feb 28, 2021

Is ci/s390x-linux-musl.json still needed?

@kaniini
Copy link
Contributor Author

kaniini commented Feb 28, 2021

git, how does it even work?

@Amanieu
Copy link
Member

Amanieu commented Feb 28, 2021

You need to add the target to .github/workflows/bors.yml so it gets picked up by CI.

@Amanieu
Copy link
Member

Amanieu commented Feb 28, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Feb 28, 2021

📌 Commit f230862 has been approved by Amanieu

bors added a commit that referenced this pull request Feb 28, 2021
add definitions for s390x musl targets

Add support for s390x musl targets to libc.

I haven't added CI because I am not familiar with the pipelines, but would be glad to do so if somebody outlines what needs to be done.
@bors
Copy link
Contributor

bors commented Feb 28, 2021

⌛ Testing commit f230862 with merge 3c2e292...

@bors
Copy link
Contributor

bors commented Feb 28, 2021

💔 Test failed - checks-actions

@kaniini
Copy link
Contributor Author

kaniini commented Feb 28, 2021

oh, technically the s390x-unknown-linux-musl target is tier 3 atm. can we just skip the CI? or how do you want to tackle it?

@Amanieu
Copy link
Member

Amanieu commented Feb 28, 2021

Ah good point, let's just skip it for now.

@JohnTitor
Copy link
Member

@bors r=Amanieu

@bors
Copy link
Contributor

bors commented Feb 28, 2021

📌 Commit 41cda2a has been approved by Amanieu

@bors
Copy link
Contributor

bors commented Feb 28, 2021

⌛ Testing commit 41cda2a with merge 5fd6540...

@bors
Copy link
Contributor

bors commented Feb 28, 2021

☀️ Test successful - checks-actions, checks-cirrus-freebsd-11, checks-cirrus-freebsd-12, checks-cirrus-freebsd-13
Approved by: Amanieu
Pushing 5fd6540 to master...

@bors bors merged commit 5fd6540 into rust-lang:master Feb 28, 2021
@Amanieu
Copy link
Member

Amanieu commented Feb 28, 2021

@bors r+

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

Successfully merging this pull request may close these issues.

6 participants