-
-
Notifications
You must be signed in to change notification settings - Fork 14.1k
fix(build): resolve native illumos build failure due to missing try_lock (#149613) #149618
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
Conversation
|
r? @ChrisDenton rustbot has assigned @ChrisDenton. Use |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
I'm sorry but there's no way I can accept this in its current form. Was this machine generated? There are way too many changes which don't do anything to address the issue. |
| // Linux kernel prior to 4.11 or glibc prior to glibc 2.28 don't support `statx`. | ||
| // We check for it on first failure and remember availability to avoid having to | ||
| // do it again. | ||
| #[repr(u8)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There seem to be a lot of changes to whitespace and deleted comments. Could you drop them from the diff?
| #[cfg(any( | ||
| target_os = "freebsd", | ||
| target_os = "fuchsia", | ||
| target_os = "linux", | ||
| target_os = "netbsd", | ||
| target_os = "openbsd", | ||
| target_os = "cygwin", | ||
| target_os = "illumos", | ||
| target_os = "aix", | ||
| target_vendor = "apple", | ||
| ))] | ||
| pub fn unlock(&self) -> io::Result<()> { | ||
| cvt(unsafe { libc::flock(self.as_raw_fd(), libc::LOCK_UN) })?; | ||
| return Ok(()); | ||
| } | ||
|
|
||
| #[cfg(target_os = "solaris")] | ||
| #[cfg(any(target_os = "solaris", target_os = "illumos"))] | ||
| pub fn unlock(&self) -> io::Result<()> { | ||
| let mut flock: libc::flock = unsafe { mem::zeroed() }; | ||
| flock.l_type = libc::F_UNLCK as libc::c_short; | ||
| flock.l_whence = libc::SEEK_SET as libc::c_short; | ||
| cvt(unsafe { libc::fcntl(self.as_raw_fd(), libc::F_SETLKW, &flock) })?; | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
illumos is in both of these
| #[cfg(any( | ||
| target_os = "freebsd", | ||
| target_os = "fuchsia", | ||
| target_os = "linux", | ||
| target_os = "netbsd", | ||
| target_os = "openbsd", | ||
| target_os = "cygwin", | ||
| target_os = "illumos", | ||
| target_os = "aix", | ||
| target_vendor = "apple", | ||
| ))] | ||
| pub fn try_lock_shared(&self) -> Result<(), TryLockError> { | ||
| let result = cvt(unsafe { libc::flock(self.as_raw_fd(), libc::LOCK_SH | libc::LOCK_NB) }); | ||
| if let Err(err) = result { | ||
| if err.kind() == io::ErrorKind::WouldBlock { | ||
| Err(TryLockError::WouldBlock) | ||
| } else { | ||
| Err(TryLockError::Error(err)) | ||
| } | ||
| } else { | ||
| Ok(()) | ||
| } | ||
| } | ||
|
|
||
| #[cfg(target_os = "solaris")] | ||
| #[cfg(any(target_os = "solaris", target_os = "illumos"))] | ||
| pub fn try_lock_shared(&self) -> Result<(), TryLockError> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto here
|
Reminder, once the PR becomes ready for a review, use |
Mutex?
What in the world is this talking about? As Chris said, this PR is not in a good state. The content and description both look machine-generated: this needs to be improved if it's a serious fix. |
Issue
Native builds on illumos (and older Solaris) fail because
std::mutex::try_lock()or platform-specifictry_lockprimitives are not available, leading to linker errors. See #149613.Fix
try_lock_optional()wrapper that degrades gracefully tolock()on unsupported platformstry_lockbehindHAVE_TRY_LOCKmacroTesting
Closes #149613
@tgross35 @pietroalbini @Mark-Simulacrum
This PR adds illumos support to the file-locking tests in std::fs (via cfg additions for
target_os = "illumos"), following the recent locking enablement in #148322. It ensures the bootstrap no longer panics on native illumos builds due to unsupportedtry_lock().Local testing on OpenIndiana Hipster (GCC 14) passes all relevant tests. Would appreciate your review, especially given the illumos context—happy to address any feedback!
Fixes #149613