Closed ivmarkov closed 1 year ago
OK. Will schedule some time over the weekend to do that.
@ivmarkov, this is a related: https://github.com/smol-rs/polling/pull/129
I rant into this while using your branch for embedded development and building tests for my MacOS host. The Rustix dependency update breaks some targets.
I merged #123 because I don't want to build up changes that are going to get stale. Just a heads up. This also takes care of the rustix v0.38 port.
Ah, thanks, did not see that it was already pending!
@notgull Two additional follow up questions:
Notify
implementation in the kqueue
module, it is pretty much identical in behavior to the pipe-based notification mechanism in the poll
module, except based on unix domain sockets. Shall I re-base the one in poll
that would be #[cfg(not(target_os = "espidf"))]
to be also based on unix domain sockets (not sure these are available everywhere else?) or shall I keep it based on pipes, as it is now?kqueue
and poll
(not sure that's easily possible but I can try), or do you want two separate implementations - one in kqueue
and one in poll
?I'm in favor of a minimal variant where poll
still has its own, private implementation based on pipe
(and eventfd
for the ESP IDF) as this stuff is difficult to test with so many variants of polling mechanisms and operating systems, but also would like to understand where you want to drive this in the end, so that I don't have to redo the PR multiple times.
UnixStream
does not work on Fuchsia. As Fuchsia isn't technically Unix, libstd
does not expose it. So pipes are necessary for this use case.Notifier
type in this module. The goal is to have each module be as self contained as possible. Not to mention, the code can't really be shared between Unix and Windows.
- Unfortunately
UnixStream
does not work on Fuchsia. As Fuchsia isn't technically Unix,libstd
does not expose it. So pipes are necessary for this use case.- I would prefer to keep the
Notifier
type in this module. The goal is to have each module be as self contained as possible. Not to mention, the code can't really be shared between Unix and Windows.
OK it should be ready for another review.
pipe
based approach as well as the eventfd
based one on Linux as it supports both approaches.PollFlags::IN
- as documented actually, while ESP IDF doesn't care.Overall this looks good to me. Can you add a test to CI to make sure this code builds when targeting ESP-IDF? Even a simple
cargo check
would be enough.Ignore the Wine and Cross failures in CI, they aren't your fault.
cargo check
should be doable, yes, but with the following two caveats:
nightly
-only exercise. The problem is, ESP IDF - being a Tier 3 target - does not get a pre-built STD by default. So we do need a command line like rustup component add rust-src --toolchain nightly; cargo +nightly check --target riscv32imc-esp-espidf -Zbuild-std=std,panic_abort
, where the -Zbuild-std
part is unfortunately a nightly-only feature of Cargo. (UPDATE: rustix
already does that for the ESP IDF and a bunch of others, so yeah, why not?)eventfd
binding in rustix
for ESP IDF specifically, just got merged the other day, and thus missed the 0.38.6 release, so it is only in rustix
's mainline. Let me actually open a PR against their repo for issuing another rustix
point release. (UPDATE: rustix
release PR here).So if you are OK with "Caveat 1", yes, I'll work on that.
OK, CI is ready. For now (and until rustix
0.38.7 is released), cargo check
for ESP IDF runs with the rustix
dependency patched against its mainline (and there is a weird warning that the patched dependency is not used, but surely it is or else the build would've failed).
(This has been long supported in a fork of the
polling
crate, but the fork was against an ancient version. So I finally jumped the gun to rebase it against the latestpolling
and try to upstream now.)ESP IDF already does support the
poll
syscall, so this PR is only necessary because - ESP IDF being an embedded OS (or toolkit?) - does not support anything process-related, including thepipe
syscall.However it does support a subset of the
eventfd
syscall API, so what this PR does is to augment thepoll
module - specifically for ESP IDF - with an alternativeeventfd
codepath to wherepipe
is usually utilized. The same idea (useeventfd
for the ESP IDF) was I believe re-used by @jasta in the recently upstreamed support for ESP IDF in tokio's reactor.In terms of changes, two main items:
poll
module is augmented with alternative code-path based oneventfd
rustix
dependency from0.37.X
to0.38.X
as the support for ESP IDF inrustix
(including theeventfd
syscall) lands in the0.38.X
version. I hope you don't mind and I noticed other crates in thesmol
ecosystem already userustix
0.38.X
so I guess it was just a matter of time and somebody taking the effort to update it for this crate as wellrustix
version are really mainly addressing latestrustix
shuffling code around (the newevent
module). Two changes worth noting are this one and this one where I believe in the latter, the cast totime_t
was incorrect in the first place.