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

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

Merged
merged 3 commits into from
Apr 7, 2024
Merged

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

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.

Sign in to view
@NobodyXu NobodyXu mentioned this pull request Mar 6, 2024
Merged
weihanglo
weihanglo reviewed Mar 25, 2024
View reviewed changes
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 •
edited
Loading

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.

the8472
the8472 reviewed Apr 1, 2024
View reviewed changes
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
the8472
the8472 approved these changes Apr 6, 2024
View reviewed changes
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
weihanglo
weihanglo reviewed Apr 6, 2024
View reviewed changes
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
Refactor CI: Extract .github/actions/compile-make
bb1cb04 
Signed-off-by: Jiahao XU <[email protected]>
@NobodyXu
Add optimization for linux: Reopen pipe as fifo
89432f4 
so that we could set `O_NONBLOCK` on it.

Signed-off-by: Jiahao XU <[email protected]>
@NobodyXu
Impl Client::try_acquire
81195a4 
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
weihanglo
weihanglo approved these changes Apr 7, 2024
View reviewed changes
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
Reviewers

@the8472 the8472 the8472 approved these changes

@weihanglo weihanglo weihanglo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
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
@NobodyXu @weihanglo @the8472