Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

Implements cargo file locking using fcntl on Solaris. #11439

Merged
merged 1 commit into from
Dec 3, 2022
Merged

Implements cargo file locking using fcntl on Solaris. #11439

merged 1 commit into from
Dec 3, 2022

Conversation

psumbera
Copy link
Contributor

Fixes #11421.

@rustbot
Copy link
Collaborator

rustbot commented Nov 29, 2022

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) soon.

Please see the contribution instructions for more information.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 29, 2022
ehuss
ehuss reviewed Dec 2, 2022
View reviewed changes
src/cargo/util/flock.rs Outdated
Comment on lines 408 to 416
if (flag & libc::LOCK_SH) != 0 {
flock.l_type |= libc::F_RDLCK;
}
if (flag & libc::LOCK_EX) != 0 {
flock.l_type |= libc::F_WRLCK;
}
if (flag & libc::LOCK_UN) != 0 {
flock.l_type |= libc::F_UNLCK;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think l_type is a bitmask. That is, F_RDLCK|F_WRLCK would result in 3 (F_UNLCK). Should this be = instead? That is, something like:

flock.l_type = if flag & libc::LOCK_UN != 0 { libc::F_RDLCK }
    else if flag & libc::LOCK_EX != 0 { libc::F_WRLCK }
    else if flag & libc::LOCK_SH != 0 { libc::F_RDLCK }
    else { panic!("…") };

Although here those flags won't get set simultaneously, I think it looks a little strange to use bitwise-or for something that is not a bitmask.

@ehuss
Copy link
Contributor

ehuss commented Dec 3, 2022

Thanks! I don't have access to a Solaris system handy, so I can test this, but it seems like it should be correct to me.

@bors r+

@bors
Copy link
Contributor

bors commented Dec 3, 2022

📌 Commit acd9aa3 has been approved by ehuss

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 3, 2022
@bors
Copy link
Contributor

bors commented Dec 3, 2022

⌛ Testing commit acd9aa3 with merge 7bdb969...

@bors
Copy link
Contributor

bors commented Dec 3, 2022

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing 7bdb969 to master...

@bors bors merged commit 7bdb969 into rust-lang:master Dec 3, 2022
@weihanglo weihanglo mentioned this pull request Dec 10, 2022
Merged
weihanglo added a commit to weihanglo/rust that referenced this pull request Dec 10, 2022
@weihanglo
Update cargo
ac90c9b 
2 commits in f6e737b1e3386adb89333bf06a01f68a91ac5306..70898e522116f6c23971e2a554b2dc85fd4c84cd
2022-12-02 20:21:24 +0000 to 2022-12-05 19:43:44 +0000
- Rename `generate_units` -> `generate_root_units` (rust-lang/cargo#11458)
- Implements cargo file locking using fcntl on Solaris. (rust-lang/cargo#11439)
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 10, 2022
@matthiaskrgr
Rollup merge of rust-lang#105516 - weihanglo:update-cargo, r=weihanglo
6d7e3df 
Update cargo

2 commits in f6e737b1e3386adb89333bf06a01f68a91ac5306..70898e522116f6c23971e2a554b2dc85fd4c84cd 2022-12-02 20:21:24 +0000 to 2022-12-05 19:43:44 +0000

- Rename `generate_units` -> `generate_root_units` (rust-lang/cargo#11458)
- Implements cargo file locking using fcntl on Solaris. (rust-lang/cargo#11439)

r? `@ghost`
luqmana
luqmana reviewed Dec 11, 2022
View reviewed changes
src/cargo/util/flock.rs
l_pad: [0, 0, 0, 0],
};
flock.l_type = if flag & libc::LOCK_UN != 0 {
libc::F_RDLCK
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be F_UNLCK?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh my, I didn't catch that. @psumbera can you follow up with a PR to update this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will fix it. Sorry I didn't catch that. Since I have tested it this doesn't cause the problem with use in cargo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#11474

@ehuss ehuss added this to the 1.68.0 milestone Dec 14, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@ehuss ehuss ehuss left review comments

@luqmana luqmana luqmana left review comments

Assignees

@ehuss ehuss

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.68.0
Development

Successfully merging this pull request may close these issues.

cargo locking doesn't work on Solaris
5 participants
@psumbera @rustbot @ehuss @bors @luqmana