Actual smol rs update

[TTWNO]

Slight modification of #494 , fixes #479

Depends on getting rust_uds_windows/#9 merged upstream.

For now I have added my person Github entry as the location for uds_windows. This should let us test of the tests pass now.

Once I get this merged upstream, I'll commit, squash, add emojis, etc. here.

[zeenix]

Depends on getting rust_uds_windows/#9 merged upstream.

Kudos for going the extra mile and fixing the issue in another crate. 👍

[zeenix]

@TTWNO I converted to a Draft to avoid accidental merge.

[zeenix]

I'm a bit puzzled by the needed change in uds-windows crate.. zbus doesn't make use of AsSocket and async-io doesn't seem to require it either. 🤔

[TTWNO]

asynx-io on a -windows- platform does use it: https://docs.rs/async-io/latest/x86_64-pc-windows-msvc/async_io/struct.Async.html#method.new

That's what was causing the compile etror previously, since Async::new(...) requires an argument that implements AsSocket

[zeenix]

asynx-io on a -windows- platform does use it: https://docs.rs/async-io/latest/x86_64-pc-windows-msvc/async_io/struct.Async.html#method.new

That's very strange. I saw that it's a dev dep only: https://github.com/smol-rs/async-io/blob/master/Cargo.toml#L53

That's what was causing the compile etror previously, since Async::new(...) requires an argument that implements AsSocket

hmm.. so the latest async-io is broken on windows, essentially?

[TTWNO]

hmm.. so the latest async-io is broken on windows, essentially?

No. But uds_windows::UnixStream doesn't implement AsSocket, which is now required by async-io (as of 2.x).

That's the problem I'm trying to fix upstream. Looks like the only tests failing now are for Linux.

[TTWNO]

I can't reproduce the Linux build error. Based on the "OS error 22", it's an "invalid parameter" problem; now to recreate and fix it....

[zeenix]

I can't reproduce the Linux build error. Based on the "OS error 22", it's an "invalid parameter" problem; now to recreate and fix it....

yeah, it's very strange. My not-so-educated guess here is that we end-up using an FD after it's been closed. It's possible that it's a zbus issue entirely and new version of async-io somehow exposes it. Of course, I could be completely wrong here.

[TTWNO]

I understand what you mean, but that seems extremely weird since these tests work on my local machine. I'm wondering if it's an "Ubuntu is too old" problem.

[TTWNO]

I see the branch I based this off of is quite old, and didn't include the safe-io2 branch. I tried merging that in to see if that fixes the issue. It looks like we were playing with RawFds in zbus/src/connection.rs, even though async-io now expects to deal with Borrowed/OwnedFds now. Perhaps that was the mix up?

🤞

[TTWNO]

Just confirmed with act, which can run Github CI locally using Docker... No bueno. Looks like whatever broke it is still there.

[zeenix]

I see the branch I based this off of is quite old, and didn't include the safe-io2 branch.

Oh, I didn't realize that.

It looks like we were playing with RawFds in zbus/src/connection.rs, even though async-io now expects to deal with Borrowed/OwnedFds now. Perhaps that was the mix up?

While that's unlikely to be the source of the problem, it would be good to base it on current main branch anyway and ensure we don't revert to RawFd use anywhere.

Just confirmed with act, which can run Github CI locally using Docker...

TIL about act, thanks. :)

No bueno. Looks like whatever broke it is still there.

😞 @notgull any chance you can do us a favor and take a look?

[notgull]

I can't reproduce this in act. When I try I get this error when it tries to reinstall Rust.

[Lint, Build and Test/lint]   🐳  docker exec cmd=[bash --noprofile --norc -e -o pipefail /var/run/act/workflow/1-composite-3.sh] user= workdir=
| info: syncing channel updates for 'stable-x86_64-unknown-linux-gnu'
| info: latest update on 2023-11-16, rust version 1.74.0 (79e9716c9 2023-11-13)
| info: downloading component 'clippy'
| info: downloading component 'rustfmt'
| info: downloading component 'cargo'
| info: downloading component 'rust-std'
| info: downloading component 'rustc'
| info: removing previous version of component 'clippy'
| info: rolling back changes
| error: could not rename component file from '/home/runner/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/share/doc/clippy' to '/home/runner/.rustup/tmp/ikls7w8pmy1b2clj_dir/bk': Invalid cross-device link (os error 18)

I would guess that the error is in the connect() function, since that underwent some changes in async-io v2.

[notgull]

Probably related: smol-rs/async-io#146

[notgull]

This problem seems to be dbus-specific: I was able to use Async::connect() to connect to seatd just fine. See here: https://github.com/smol-rs/async-io/actions/runs/7073336022/job/19253127954#step:9:24

[notgull]

I ran the tests with strace and I didn't see EINVAL anywhere, which makes me skeptical that this is a failed system call. Could this be bytecodealliance/rustix#869 ?

[zeenix]

@notgull Many thanks for looking into this! 👍

I ran the tests with strace and I didn't see EINVAL anywhere, which makes me skeptical that this is a failed system call. Could this be bytecodealliance/rustix#869 ?

Oh? That's very much possible. Does this mean rustix doesn't support/has problems with abstract unix sockets? I wonder if #329 would help with this. 🤔

[zeenix]

@TTWNO async-io 2.2.2 was just released and it has the fix for the issue. I restarted the failing Linux job and hopeful that it'll now pass because it should pick up the latest release. However, it'd be best to change async-io version in the PR to ensure we only build against the fixed version.

[zeenix]

🍾 🥳

[TTWNO]

Wicked! Looking forward to getting any cleanups that are necessary :)

[TTWNO]

Should async-io have a 2.2.0 minimum version, then?

[zeenix]

Should async-io have a 2.2.0 minimum version, then?

As I wrote, let's require 2.2.2.

[TTWNO]

Alright, got those dependencies updated and have removed the link to my git release. As long as the tests pass, I think this should be ready to rock!

[zeenix]

Alright, got those dependencies updated and have removed the link to my git release. As long as the tests pass, I think this should be ready to rock!

Awesome! However, there is still one concern though: It looks like event-listener might need another API break to effectively remove some footguns/unsoundness issues. Need to figure how to solve that.

In the meantime, could you please clean up the commits?

[TTWNO]

Updated to remove specific issue listed, should this wait until event-listener #105?

[notgull]

It might take a while to make sure everything works. So I'd say to go ahead with this first

[zeenix]

It might take a while to make sure everything works. So I'd say to go ahead with this first

Hmm.. but the event-listener update would then be a breaking change since we return Pin<Box<EventListener>> to the user and I was hoping not to make any more breaks any time soon.

Thinking more about this, I guess we can add our own abstraction and expose that or just return impl Future<Output = ()> since all you can do with returned listener is wait on its Future impl.

[zeenix]

Thinking more about this, I guess we can add our own abstraction and expose that or just return impl Future<Output = ()> since all you can do with returned listener is wait on its Future impl.

Yeah, actually let's do that but I'll do it on a separate PR. This one has waited long enough.