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.

Sign up for GitHub

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

Jump to bottom

implement minimal epoll_create1 shim #2357

Merged
merged 1 commit into from
Dec 14, 2022
Merged

implement minimal epoll_create1 shim #2357

merged 1 commit into from
Dec 14, 2022

Conversation

DebugSteven
Copy link
Contributor

Implements minimal shim for #602

@oli-obk oli-obk self-assigned this Jul 11, 2022
oli-obk
oli-obk reviewed Jul 18, 2022
View reviewed changes
src/shims/unix/fs.rs Outdated Show resolved Hide resolved
@RalfJung
Copy link
Member

Thanks a lot for working on this. :)

I am a bit unclear on the status of this PR. It contains several TODOs and no tests, and CI is failing. Is this something you would like a review on, or more of a draft?

@oli-obk
Copy link
Contributor

oli-obk commented Jul 19, 2022

I'm mentoring the work on this. We'll ping you once it's reviewable

@RalfJung
Copy link
Member

Ah, I see. Reflecting this in the GH status then.
@rustbot author

@rustbot rustbot added the S-waiting-on-author Status: Waiting for the PR author to address review comments label Jul 19, 2022
@RalfJung RalfJung marked this pull request as draft July 19, 2022 11:16
@bors
Copy link
Contributor

bors commented Jul 20, 2022

☔ The latest upstream changes (presumably #2403) made this pull request unmergeable. Please resolve the merge conflicts.

@LegNeato
Copy link
Contributor

As epoll is linux-specific, the implementation on FileDescriptor should probably be generalized and use something like https://github.com/smol-rs/polling for cross-compiling. And then in the Linux-specific shims you would implement the epoll stuff as a translation to the generic polling APIs.

I'm not a maintainer so feel free to ignore me.

@oli-obk
Copy link
Contributor

oli-obk commented Jul 27, 2022

I'm not a maintainer so feel free to ignore me.

I am, and I have almost no idea about how all of these linux APIs work. Without informed community members telling us where things can be better, we can't do any of this :)

As epoll is linux-specific, the implementation on FileDescriptor should probably be generalized and use something like https://github.com/smol-rs/polling for cross-compiling

will check this out. Until we reach actual polling operations, we're just implementing what the APIs dictate (thus the Vec<i32> of file descriptors registered on the Epoll type via epoll_ctl)

@DebugSteven DebugSteven force-pushed the epoll_create1-shim branch 3 times, most recently from 3ad58b1 to 51dcc27 Compare September 1, 2022 22:39
@DebugSteven DebugSteven force-pushed the epoll_create1-shim branch 3 times, most recently from a293897 to 1ae2a84 Compare September 19, 2022 17:04
oli-obk
oli-obk reviewed Oct 11, 2022
View reviewed changes
src/shims/unix/fs.rs Outdated Show resolved Hide resolved
@oli-obk
Copy link
Contributor

oli-obk commented Oct 25, 2022

As epoll is linux-specific, the implementation on FileDescriptor should probably be generalized and use something like https://github.com/smol-rs/polling for cross-compiling. And then in the Linux-specific shims you would implement the epoll stuff as a translation to the generic polling APIs.

This PR does not yet actually handle polling (it just immediately marks everything as ready). In follow up PRs (with tests that actually do something interesting) we'll address this.

@DebugSteven DebugSteven force-pushed the epoll_create1-shim branch 2 times, most recently from 8b531e9 to 94d41f0 Compare October 27, 2022 21:55
@bors
Copy link
Contributor

bors commented Dec 12, 2022

☔ The latest upstream changes (presumably #2727) made this pull request unmergeable. Please resolve the merge conflicts.

@RalfJung
Copy link
Member

Sorry for the fallout, that's due to some parallel refactor. I pushed a commit that should make it build again.

@DebugSteven
Copy link
Contributor Author

Thank you! I was wondering what I missed. :)

@RalfJung
Copy link
Member

All right, looks green. :) Can you squash the commits?

@oli-obk
Copy link
Contributor

oli-obk commented Dec 13, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Dec 13, 2022

📌 Commit 0dfa31b has been approved by oli-obk

It is now in the queue for this repository.

@oli-obk oli-obk closed this Dec 14, 2022
@oli-obk oli-obk reopened this Dec 14, 2022
@oli-obk
Copy link
Contributor

oli-obk commented Dec 14, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Dec 14, 2022

💡 This pull request was already approved, no need to approve it again.

@bors
Copy link
Contributor

bors commented Dec 14, 2022

📌 Commit 0dfa31b has been approved by oli-obk

It is now in the queue for this repository.

@oli-obk
Copy link
Contributor

oli-obk commented Dec 14, 2022

@bors retry

@oli-obk
Copy link
Contributor

oli-obk commented Dec 14, 2022

@bors r-

@oli-obk oli-obk closed this Dec 14, 2022
@oli-obk oli-obk reopened this Dec 14, 2022
@oli-obk
Copy link
Contributor

oli-obk commented Dec 14, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Dec 14, 2022

📌 Commit 0dfa31b has been approved by oli-obk

It is now in the queue for this repository.

@oli-obk oli-obk removed the S-waiting-on-author Status: Waiting for the PR author to address review comments label Dec 14, 2022
@RalfJung
Copy link
Member

See https://rust-lang.zulipchat.com/#narrow/stream/242791-t-infra/topic/bors.20stuck

@bors
Copy link
Contributor

bors commented Dec 14, 2022

⌛ Testing commit 0dfa31b with merge 205ab4f...

@bors
Copy link
Contributor

bors commented Dec 14, 2022

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 205ab4f to master...

@bors bors merged commit 205ab4f into rust-lang:master Dec 14, 2022
@RalfJung RalfJung mentioned this pull request Dec 24, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 25, 2022
@bors
Auto merge of rust-lang#106121 - RalfJung:miri, r=RalfJung
298d763 
update Miri

Noteworthy PRs:
- rust-lang/miri#2357
- rust-lang/miri#2646
- rust-lang/miri#2718
- rust-lang/miri#2721
- rust-lang/miri#2725
@taiki-e taiki-e mentioned this pull request Dec 27, 2022
Closed
RalfJung pushed a commit to RalfJung/miri that referenced this pull request Dec 28, 2022
@bors
Auto merge of #106121 - RalfJung:miri, r=RalfJung
6f83431 
update Miri

Noteworthy PRs:
- rust-lang#2357
- rust-lang#2646
- rust-lang#2718
- rust-lang#2721
- rust-lang#2725
spikespaz pushed a commit to spikespaz/dotwalk-rs that referenced this pull request Aug 29, 2024
@bors
Auto merge of #106121 - RalfJung:miri, r=RalfJung
9de478e 
update Miri

Noteworthy PRs:
- rust-lang/miri#2357
- rust-lang/miri#2646
- rust-lang/miri#2718
- rust-lang/miri#2721
- rust-lang/miri#2725
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LegNeato LegNeato LegNeato left review comments

@oli-obk oli-obk oli-obk left review comments

@RalfJung RalfJung RalfJung left review comments

Assignees

@oli-obk oli-obk

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@DebugSteven @RalfJung @oli-obk @bors @LegNeato @rustbot