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

feature: Impl Client::{supports_try_acquire, try_acquire} #73

Merged
merged 3 commits into from
Apr 7, 2024

Conversation

NobodyXu
Copy link
Contributor

@NobodyXu NobodyXu commented Mar 2, 2024

Resolve #72

@NobodyXu

This comment was marked as outdated.

Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

I am not super familiar with the compatibility between these changes. It's better to have another pair of eyes. @petrochenkov do you have time to review?

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/unix.rs Show resolved Hide resolved
src/unix.rs Outdated Show resolved Hide resolved
src/unix.rs Outdated Show resolved Hide resolved
src/unix.rs Outdated Show resolved Hide resolved
@NobodyXu
Copy link
Contributor Author

NobodyXu commented Mar 26, 2024

P.S. I was trying to run cargo +nightly update -Zminimal-versions and got 😂:

thread 'main' panicked at src/cargo/util/context/mod.rs:412:20:
already borrowed: BorrowMutError
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

It seems to pretty persistent when using Cargo.lock from 845870a

Edit:

Issue is fixed after removing Cargo.lock

@NobodyXu NobodyXu force-pushed the feat/try_acquire branch 2 times, most recently from 0e3082b to ff9cdb4 Compare March 26, 2024 12:21
@weihanglo
Copy link
Member

P.S. I was trying to run cargo +nightly update -Zminimal-versions and got 😂:

thread 'main' panicked at src/cargo/util/context/mod.rs:412:20:
already borrowed: BorrowMutError
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

It seems to pretty persistent when using Cargo.lock from 845870a

Edit:

Issue is fixed after removing Cargo.lock

This was a bug in Cargo rust-lang/cargo#13647. A new nightly with that fix will be out today.

@NobodyXu NobodyXu requested review from weihanglo and the8472 March 31, 2024 05:09
Copy link
Member

@the8472 the8472 left a comment

Choose a reason for hiding this comment

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

Can you clean up the branch's git history a bit? The "fix ..." commits likely won't help future readers through the history

src/unix.rs Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
src/unix.rs Show resolved Hide resolved
src/unix.rs Outdated Show resolved Hide resolved
@NobodyXu NobodyXu requested a review from the8472 April 6, 2024 01:20
Copy link
Member

@the8472 the8472 left a comment

Choose a reason for hiding this comment

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

Other than the workflow comment it LGTM now.

.github/actions/compile-make/action.yml Outdated Show resolved Hide resolved
Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks!

BTW, could you clean up the commit history again? Intermediate commits don't help much for future contributors (or future us 😆).

For me, I might break them into one for CI update, One for try_acquire support, one for FIFO conversion optimization, and probably tests.

src/unix.rs Show resolved Hide resolved
NobodyXu added 2 commits April 8, 2024 03:19
so that we could set `O_NONBLOCK` on it.

Signed-off-by: Jiahao XU <[email protected]>
With suggestions from @weihanglo

Co-authored-by: Weihang Lo <[email protected]>

Signed-off-by: Jiahao XU <[email protected]>
@NobodyXu NobodyXu requested a review from weihanglo April 7, 2024 17:38
Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

Thanks!

@weihanglo weihanglo merged commit 507eb38 into rust-lang:main Apr 7, 2024
14 checks passed
@NobodyXu NobodyXu deleted the feat/try_acquire branch April 8, 2024 01:01
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.

feature: Add new API Client::{try_acquire, support_try_acquire} for non-blocking operation
3 participants