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

Unified all Linux sockopts definitions by arch #2135

Merged
merged 1 commit into from
Apr 6, 2021
Merged

Unified all Linux sockopts definitions by arch #2135

merged 1 commit into from
Apr 6, 2021

Conversation

zonyitoo
Copy link
Contributor

@zonyitoo zonyitoo commented Mar 31, 2021

@rust-highfive
Copy link

r? @JohnTitor

(rust-highfive has picked a reviewer for you, use r? to override)

Copy link
Member

@JohnTitor JohnTitor left a comment

Choose a reason for hiding this comment

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

I think we can declare SO_INCOMING_CPU as 49 on musl and uclibc regardless of the arch, cannot we? I'd like to declare this on linux/mod.rs with a cfg for the sparc targets.

src/unix/linux_like/linux/gnu/b32/sparc/mod.rs Outdated Show resolved Hide resolved
@zonyitoo
Copy link
Contributor Author

zonyitoo commented Mar 31, 2021

I think we can declare SO_INCOMING_CPU as 49 on musl and uclibc regardless of the arch, cannot we? I'd like to declare this on linux/mod.rs with a cfg for the sparc targets.

I wouldn't suggest that. Because there still have other targets, like parisc, have different values of SO_INCOMING_CPU.

@JohnTitor
Copy link
Member

What the target depends on parisc? AFAIK sparc and parisc have a different value but other arch/env has 49 as a value so it shouldn't be difficult to gate with cfg.

@zonyitoo
Copy link
Contributor Author

I have no idea. I don't know if there are other arch/env has different values right now or in the future.

Anyway, I am not familiar with uClibc. I just downloaded its latest release and couldn't find any related definition about SO_INCOMING_CPU. Could we make an accumption that this value is only defined in Linux Kernal headers?

@zonyitoo zonyitoo changed the title Add define SO_INCOMING_CPU for linux-like gnu & musl Add define SO_INCOMING_CPU for linux-like gnu & musl & uclibc Mar 31, 2021
@JohnTitor
Copy link
Member

Creating PRs one by one to fix builds is not reasonable, and not gating it with a cfg or defining it for all linux_like/linux targets at once increases review/maintenance costs. And I'm not sure why you put it to each uclibc file instead of mod.rs as it should be the same value for all the uclibc from what I see https://doc.rust-lang.org/nightly/rustc/platform-support.html.

@zonyitoo
Copy link
Contributor Author

zonyitoo commented Mar 31, 2021

cfg_if! {
    if #[cfg(target_arch = "parisc")] {
        // arch/parisc/include/uapi/asm/socket.h
        pub const SO_INCOMING_CPU = 0x402A;
    } else if #[cfg(target_arch = "sparc")] {
        // arch/sparc/include/uapi/asm/socket.h
        pub const SO_INCOMING_CPU = 0x0033;
    } else {
        // include/uapi/asm-generic/socket.h
        // arch/alpha/include/uapi/asm/socket.h
        // tools/include/uapi/asm-generic/socket.h
        // arch/mips/include/uapi/asm/socket.h
        pub const SO_INCOMING_CPU = 49;
    }
}

Should I put this block in linux/mod.rs and delete all SO_INCOMING_CPU in subdirectories?

@JohnTitor
Copy link
Member

AFAIK we don't have any parisc arch targets so the first cfg is meaningless, right? And the second cfg doesn't catch sparc64 but it should, I think. Once it's done, yeah, we can clean-up it from subdirs. If something is wrong, CI will catch it (if the target is tested) so let's give it a try.

@zonyitoo zonyitoo changed the title Add define SO_INCOMING_CPU for linux-like gnu & musl & uclibc Unified all Linux sockopts definitions by arch Apr 1, 2021
@zonyitoo
Copy link
Contributor Author

zonyitoo commented Apr 1, 2021

This commit:

  1. Moved all SO_* and SOL_SOCKET from folders separated by env to arch
  2. Copy all latest definitions from socket.h on Linux's master branch

@JohnTitor
Copy link
Member

Oh, I just mean we clean-up SO_INCOMING_CPU only, but it's now great clean-up. I really need #2109 to get merged first, to ensure all the things aren't broken...
Meanwhile, let's @bors try to see if the build is fine.

bors added a commit that referenced this pull request Apr 1, 2021
Unified all Linux sockopts definitions by arch

- ref rust-lang/socket2#213
@bors
Copy link
Contributor

bors commented Apr 1, 2021

⌛ Trying commit 9733ded with merge 2ccc2f4...

@bors
Copy link
Contributor

bors commented Apr 1, 2021

💔 Test failed - checks-actions

@zonyitoo
Copy link
Contributor Author

zonyitoo commented Apr 1, 2021

Ah... Ok. Well, you can just merge #2109 and I will rebase my branch.

This is quite a big change. I have checked several times with my eyes but ... you should double check again.

@zonyitoo
Copy link
Contributor Author

zonyitoo commented Apr 1, 2021

What does this build fail mean?

@JohnTitor
Copy link
Member

What does this build fail mean?

That's because Cirrus CI re-scheduled the jobs, @bors try

@bors
Copy link
Contributor

bors commented Apr 1, 2021

⌛ Trying commit 9733ded with merge 214ce87...

bors added a commit that referenced this pull request Apr 1, 2021
Unified all Linux sockopts definitions by arch

- ref rust-lang/socket2#213
@bors
Copy link
Contributor

bors commented Apr 1, 2021

💔 Test failed - checks-actions

@JohnTitor
Copy link
Member

JohnTitor commented Apr 2, 2021

The failure is:

error: use of deprecated type alias `unix::linux_like::linux::musl::time_t`: This type is changed to 64-bit in musl 1.2.0, we'll follow that change in the future release. See #1848 for more info.
  --> src/unix/linux_like/linux/arch/mips/b32.rs:5:18
   |
5  |     if size_of::<::time_t>() == size_of::<::c_long>() {
   |                  ^^^^^^^^
   |
note: the lint level is defined here
  --> src/lib.rs:18:38
   |
18 | #![cfg_attr(libc_deny_warnings, deny(warnings))]
   |                                      ^^^^^^^^
   = note: `#[deny(deprecated)]` implied by `#[deny(warnings)]`

error: use of deprecated type alias `unix::linux_like::linux::musl::time_t`: This type is changed to 64-bit in musl 1.2.0, we'll follow that change in the future release. See #1848 for more info.
  --> src/unix/linux_like/linux/arch/mips/b32.rs:11:18
   |
11 |     if size_of::<::time_t>() == size_of::<::c_long>() {
   |                  ^^^^^^^^

error: use of deprecated type alias `unix::linux_like::linux::musl::time_t`: This type is changed to 64-bit in musl 1.2.0, we'll follow that change in the future release. See #1848 for more info.
  --> src/unix/linux_like/linux/arch/mips/b32.rs:17:18
   |
17 |     if size_of::<::time_t>() == size_of::<::c_long>() {
   |                  ^^^^^^^^

error: use of deprecated type alias `unix::linux_like::linux::musl::time_t`: This type is changed to 64-bit in musl 1.2.0, we'll follow that change in the future release. See #1848 for more info.
  --> src/unix/linux_like/linux/arch/mips/b32.rs:24:18
   |
24 |     if size_of::<::time_t>() == size_of::<::c_long>() {
   |                  ^^^^^^^^

error: use of deprecated type alias `unix::linux_like::linux::musl::time_t`: This type is changed to 64-bit in musl 1.2.0, we'll follow that change in the future release. See #1848 for more info.
  --> src/unix/linux_like/linux/arch/mips/b32.rs:30:18
   |
30 |     if size_of::<::time_t>() == size_of::<::c_long>() {
   |                  ^^^^^^^^

error: aborting due to 5 previous errors

error: could not compile `libc`

We should allow it.

@zonyitoo
Copy link
Contributor Author

zonyitoo commented Apr 2, 2021

Oh, you have merged that regression-test PR. Shoudl I rebase it?

@JohnTitor
Copy link
Member

It's unnecessary; bors automatically rebases to master.

@zonyitoo
Copy link
Contributor Author

zonyitoo commented Apr 2, 2021

I tried to rebase locally without any conflicts.

@JohnTitor
Copy link
Member

It shouldn't have any conflicts, I mean, there's no need to rebase but you can if you like (or if you want to run it on your local).

@JohnTitor
Copy link
Member

@bors try -- let's check it anyway.

@zonyitoo
Copy link
Contributor Author

zonyitoo commented Apr 2, 2021

@bors try

bors added a commit that referenced this pull request Apr 2, 2021
Unified all Linux sockopts definitions by arch

- ref rust-lang/socket2#213
@bors
Copy link
Contributor

bors commented Apr 2, 2021

⌛ Trying commit 7ed1544 with merge e786d3a...

@bors
Copy link
Contributor

bors commented Apr 2, 2021

☀️ Try build successful - checks-actions, checks-cirrus-freebsd-11, checks-cirrus-freebsd-12, checks-cirrus-freebsd-13
Build commit: e786d3a (e786d3ae11117f04ee013060a8af48bfc5180393)

@zonyitoo
Copy link
Contributor Author

zonyitoo commented Apr 5, 2021

@JohnTitor I think it is ready to be merged.

- fixed other missing SO_INCOMING_CPU definition in rust-lang/socket2#213
@zonyitoo
Copy link
Contributor Author

zonyitoo commented Apr 5, 2021

@bors try

@bors
Copy link
Contributor

bors commented Apr 5, 2021

⌛ Trying commit 682eba6 with merge bf574cf...

bors added a commit that referenced this pull request Apr 5, 2021
Unified all Linux sockopts definitions by arch

- ref rust-lang/socket2#213

Fixes #2133
@bors
Copy link
Contributor

bors commented Apr 5, 2021

💥 Test timed out

@JohnTitor JohnTitor closed this Apr 5, 2021
@JohnTitor JohnTitor reopened this Apr 5, 2021
@JohnTitor
Copy link
Member

JohnTitor commented Apr 5, 2021

Seems GHA won't start their jobs.
Related Zulip topic: https://rust-lang.zulipchat.com/#narrow/stream/242791-t-infra/topic/Homu.20Stuck.3F
I'm holding off this until it's resolved.

@JohnTitor
Copy link
Member

@bors try

@bors
Copy link
Contributor

bors commented Apr 5, 2021

⌛ Trying commit 682eba6 with merge 5f8c790...

bors added a commit that referenced this pull request Apr 5, 2021
Unified all Linux sockopts definitions by arch

- ref rust-lang/socket2#213

Fixes #2133
@bors
Copy link
Contributor

bors commented Apr 5, 2021

☀️ Try build successful - checks-actions, checks-cirrus-freebsd-11, checks-cirrus-freebsd-12, checks-cirrus-freebsd-13
Build commit: 5f8c790 (5f8c790892a77fe623b78f128840a23cd7d78a0a)

@JohnTitor
Copy link
Member

Awesome, thanks for the clean-up!
@bors r+

@bors
Copy link
Contributor

bors commented Apr 5, 2021

📌 Commit 682eba6 has been approved by JohnTitor

@bors
Copy link
Contributor

bors commented Apr 5, 2021

⌛ Testing commit 682eba6 with merge ac494b1...

@bors
Copy link
Contributor

bors commented Apr 6, 2021

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

@bors bors merged commit ac494b1 into rust-lang:master Apr 6, 2021
@Thomasdezeeuw
Copy link
Contributor

Thanks for doing this @zonyitoo.

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.

SO_INCOMING_CPU not defined for s390x-unknown-linux-gnu
5 participants