Skip to content

replace legacy impl with Poll - #329

Merged
ihciah merged 3 commits into
monoio-rs:masterfrom
loongs-zhang:dev-refactor-poll
Mar 11, 2025
Merged

replace legacy impl with Poll#329
ihciah merged 3 commits into
monoio-rs:masterfrom
loongs-zhang:dev-refactor-poll

Conversation

@loongs-zhang

Copy link
Copy Markdown
Member

No description provided.

@ihciah

ihciah commented Feb 5, 2025

Copy link
Copy Markdown
Member

Thank you very much! Please allow me some time to review it!

@ihciah

ihciah commented Feb 26, 2025

Copy link
Copy Markdown
Member

Could you re-push it and trigger the CI?
CI was broken before because the company org misconfigured the Action.

@loongs-zhang

Copy link
Copy Markdown
Member Author

You can also saw CI results in https://github.com/loongs-zhang/monoio/commits/dev-refactor-poll
image

@loongs-zhang

Copy link
Copy Markdown
Member Author

I think the CI fails due to cross.

@ihciah ihciah left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's a good change which I wanted long time before(#269).
The only thing I concern is the legacy feature. I think it cannot be treated as poll-io, which is a supplement to polling ability under iouring.

Comment thread monoio/Cargo.toml Outdated
Comment thread monoio/src/builder.rs Outdated
@ihciah

ihciah commented Feb 27, 2025

Copy link
Copy Markdown
Member

And another unrelated issue I think about when reading my old PR draft, the latest monoio-macro's test macro seems not work in user code due to some #cfg you introduced 13 months ago. They are not published so the problem haven't affected users. For this issue, I can think about these cases:

  1. #[monoio::test] in user code: use iouring by default
  2. #[monoio::test(driver=legacy)] in user code: use legacy driver
  3. #[monoio::test_all] in user code: generate tests for both drivers
  4. #[monoio::test_all] in monoio code: generate tests for both drivers with cfg.

Maybe we can come about a way to fix and do it in another PR.

@loongs-zhang
loongs-zhang force-pushed the dev-refactor-poll branch 4 times, most recently from 1d38188 to 2e26222 Compare February 28, 2025 16:17

@ihciah ihciah left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's great! Could you by the way fix the clippy warnings to pass CI?

This will bump the MSRV to 1.74, but since 1.74 was published at Nov. 2023, I think it is fine.
https://rust-lang.github.io/rust-clippy/master/index.html#io_other_error

@loongs-zhang

Copy link
Copy Markdown
Member Author

It's great! Could you by the way fix the clippy warnings to pass CI?

This will bump the MSRV to 1.74, but since 1.74 was published at Nov. 2023, I think it is fine. https://rust-lang.github.io/rust-clippy/master/index.html#io_other_error

Done, but the loongarch64-unknown-linux-gnu nightly fails, track on cross-rs/cross#1640

@ihciah ihciah left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@ihciah
ihciah merged commit f1d46d8 into monoio-rs:master Mar 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants