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

better build locks for bootstrap #118535

Closed
wants to merge 2 commits into from
Closed

better build locks for bootstrap #118535

wants to merge 2 commits into from

Conversation

onur-ozkan
Copy link
Member

@onur-ozkan onur-ozkan commented Dec 2, 2023 •
edited

This PR improves the way bootstrap handles the build locks by supporting Solaris and removing many dependencies coming with fd-lock.

cc @psumbera

@onur-ozkan
bootstrap: implement a new utility file_lock
27b5629 
`file_lock` util provides `FileLock` type which is a wrapper around
`fs2` and `std::fs::File`. This is useful as `fs2` supports Solaris and has
fewer dependencies than `fd-lock`.

Signed-off-by: onur-ozkan <work@onurozkan.dev>
@rustbot
Copy link
Collaborator

rustbot commented Dec 2, 2023

r? @Mark-Simulacrum

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Dec 2, 2023
@onur-ozkan
bootstrap: replace fd-lock with util::file_lock
fb59860 
Signed-off-by: onur-ozkan <work@onurozkan.dev>
@psumbera
Copy link
Contributor

psumbera commented Dec 4, 2023

This PR improves the way bootstrap handles the build locks by supporting Solaris and removing many dependencies coming with fd-lock.

Thank you for doing this! I can confirm it builds on Solaris.

@Mark-Simulacrum
Copy link
Member

fs2 and fd-lock look basically the same to me dependency wise. (fd-lock actually looks more modern, e.g., windows-sys vs. winapi). fd-lock is also actively maintained, while fs2 isn't (last release 6 years ago).

Can we stick with fd-lock rather than fs2 and submit a PR for fd-lock support for Solaris? That seems like the better solution here, rather than moving to a mostly unmaintained crate.

@onur-ozkan onur-ozkan added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 9, 2023
@bors
Copy link
Contributor

bors commented Dec 10, 2023

☔ The latest upstream changes (presumably #118069) made this pull request unmergeable. Please resolve the merge conflicts.

@psumbera
Copy link
Contributor

Can we stick with fd-lock rather than fs2 and submit a PR for fd-lock support for Solaris? That seems like the better solution here, rather than moving to a mostly unmaintained crate.

fd-lock support for Solaris is problematic. Solaris doesn't support flock syscall. And emulation via fcntl isn't easy where there is different semantic (flock works per file descriptor and fcntl per process).

@onur-ozkan
Copy link
Member Author

fd-lock support for Solaris is problematic. Solaris doesn't support flock syscall. And emulation via fcntl isn't easy where there is different semantic (flock works per file descriptor and fcntl per process).

I was planning to apply this workaround https://github.com/danburkert/fs2-rs/blob/9a340454a8292df025de368fc4b310bb736f382f/src/unix.rs#L56-L92 on fd-lock. It shouldn't be problem I think as they are sharing almost the same implementation.

@onur-ozkan
Copy link
Member Author

onur-ozkan commented Dec 13, 2023 •
edited

@psumbera Can you test this https://github.com/onur-ozkan/rust/tree/demo-for-build-locks branch (to see if locking works as expected or not on solaris) by any chance?

@psumbera
Copy link
Contributor

@psumbera Can you test this https://github.com/onur-ozkan/rust/tree/demo-for-build-locks branch (to see if locking works as expected or not on solaris) by any chance?

It builds just fine. But still prints:

WARNING: file locking not supported for target, not locking build directory

I think it's now printed always:

onur-ozkan@0c2b090#diff-5624b915604aca92c82fe2af07e555adfd958a3247eada8300061e6fe6c3dd6eR55

@onur-ozkan
Copy link
Member Author

@psumbera Can you test this https://github.com/onur-ozkan/rust/tree/demo-for-build-locks branch (to see if locking works as expected or not on solaris) by any chance?

It builds just fine. But still prints:

WARNING: file locking not supported for target, not locking build directory

I think it's now printed always:

onur-ozkan@0c2b090#diff-5624b915604aca92c82fe2af07e555adfd958a3247eada8300061e6fe6c3dd6eR55

Oh, I forgot to remove that log (not really important anyway, this is just for debugging). Does the locking work? What happens when you run x concurrently?

@psumbera
Copy link
Contributor

Oh, I forgot to remove that log (not really important anyway, this is just for debugging). Does the locking work? What happens when you run x concurrently?

Yes, seems to work:

WARNING: build directory locked by process 2647, waiting for lock

@onur-ozkan onur-ozkan mentioned this pull request Dec 20, 2023
Merged
@onur-ozkan
Copy link
Member Author

Created #119413 PR and closing this.

@onur-ozkan onur-ozkan closed this Dec 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Mark-Simulacrum Mark-Simulacrum

Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@onur-ozkan @rustbot @psumbera @Mark-Simulacrum @bors