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

Don't use on Solaris libc::LOCK_* which were removed from libc in ver… #15143

Merged
merged 1 commit into from
Feb 7, 2025

Conversation

psumbera
Copy link
Contributor

@psumbera psumbera commented Feb 4, 2025

Relevant libc change was: rust-lang/libc@251e8e8

@rustbot
Copy link
Collaborator

rustbot commented Feb 4, 2025

r? @ehuss

rustbot has assigned @ehuss.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-filesystem Area: issues with filesystems S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 4, 2025
@epage
Copy link
Contributor

epage commented Feb 4, 2025

Wouldn't that be a breaking change? I'm not seeing that discussed in rust-lang/libc#4017

@psumbera
Copy link
Contributor Author

psumbera commented Feb 5, 2025

Wouldn't that be a breaking change? I'm not seeing that discussed in rust-lang/libc#4017

Solaris never had flock() and corresponding LOCK_* definition. Only Illumos (Solaris fork) added this. And since Rust libc definitions are partially shared between these two it was there defined for both. While I was fixing Rust libc tests for Solaris I had to remove it from there.

Comment on lines 475 to 479
flock(file, libc::LOCK_EX | libc::LOCK_NB)
}
#[cfg(target_os = "solaris")]
{
flock(file, 2 | 4)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we make our own constants so we limit all solaris-specific code to just the solaris implementation of flock

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'm open for any suggestion. But not sure what do you exactly mean here...

Copy link
Member

Choose a reason for hiding this comment

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

Guess something like?

#[cfg(all(unix, not(target_os = "solaris")))]
const LOCK_UN = libc::LOCK_UN;
#[cfg(all(unix, target_os = "solaris"))]
const LOCK_UN = 8;

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 good with this. Thanks!

Comment on lines 475 to 479
flock(file, libc::LOCK_EX | libc::LOCK_NB)
}
#[cfg(target_os = "solaris")]
{
flock(file, 2 | 4)
Copy link
Member

Choose a reason for hiding this comment

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

Guess something like?

#[cfg(all(unix, not(target_os = "solaris")))]
const LOCK_UN = libc::LOCK_UN;
#[cfg(all(unix, target_os = "solaris"))]
const LOCK_UN = 8;

@@ -430,30 +430,48 @@ fn is_on_nfs_mount(_path: &Path) -> bool {
false
}

#[cfg(all(unix, not(target_os = "solaris")))]
Copy link
Member

Choose a reason for hiding this comment

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

Sorry for being picky. Feel like this can move one level down into the #[cfg(unix)] mod sys so we can remove all(unix, part from cfg?

@psumbera psumbera force-pushed the solaris-libc branch 2 times, most recently from 3d94759 to 1cc87c9 Compare February 7, 2025 07:04
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 added this pull request to the merge queue Feb 7, 2025
Merged via the queue into rust-lang:master with commit 49e5d24 Feb 7, 2025
21 checks passed
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 8, 2025
Update cargo

14 commits in 0e3d73849ab8cbbab3ec5c65cbd555586cb21339..2928e32734b04925ee51e1ae88bea9a83d2fd451
2025-02-01 20:14:40 +0000 to 2025-02-07 16:50:22 +0000
- Simplify backtrack (rust-lang/cargo#15150)
- Don't use on Solaris libc::LOCK_* which were removed from libc in ver… (rust-lang/cargo#15143)
- feat: emit error if package not found within workspace (rust-lang/cargo#15071)
- Make cache tracking resilient to unexpected files (rust-lang/cargo#15147)
- Small resolver cleanups (rust-lang/cargo#15040)
- feat: add `cargo pkgid` support for cargo-script (rust-lang/cargo#14961)
- Suggest similar feature names on CLI (rust-lang/cargo#15133)
- fix: Don't use "did you mean" in errors (rust-lang/cargo#15138)
- Fix changelog link (rust-lang/cargo#15142)
- chore(deps): update rust crate rand to 0.9.0 (rust-lang/cargo#15129)
- Remove the original changelog (rust-lang/cargo#15123)
- chore(deps): update rust crate gix to 0.70.0 (rust-lang/cargo#15128)
- allow windows reserved names in CI (rust-lang/cargo#15135)
- removed a word that was repeated (rust-lang/cargo#15136)
@rustbot rustbot added this to the 1.86.0 milestone Feb 8, 2025
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
A-filesystem Area: issues with filesystems S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants