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.

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

Display pid of process holding lock #108107

Closed
wants to merge 0 commits into from

Conversation

zephaniahong
Copy link
Contributor

@zephaniahong zephaniahong commented Feb 16, 2023

Fixes #107077
The PR works but does not seem the most elegant to me. Also, I'm not quite understanding why creating the build_lock using a std::fs::File::open results in an error.

More details here: https://rust-lang.zulipchat.com/#narrow/stream/326414-t-infra.2Fbootstrap/topic/Writing.20to.20lock.20file

Happy to receive any feedback.
Thank you!

p.s. Seems like the CI is also detecting the issue I'm facing... Someone on Zulip (bjorn) has mentioned the reason behind the error. But just want to make sure I'm on the right track before continuing

@rustbot
Copy link
Collaborator

rustbot commented Feb 16, 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 Feb 16, 2023
@zephaniahong zephaniahong marked this pull request as ready for review February 17, 2023 04:28
@zephaniahong
Copy link
Contributor Author

zephaniahong commented Feb 17, 2023

@ChrisDenton I've made the necessary changes. If all's good, I'll squash my commits.
Thank you!

src/bootstrap/bin/main.rs Outdated Show resolved Hide resolved
src/bootstrap/bin/main.rs Outdated Show resolved Hide resolved
src/bootstrap/bin/main.rs Outdated Show resolved Hide resolved
@Mark-Simulacrum
Copy link
Member

@rustbot author

@rustbot rustbot 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 Feb 25, 2023
@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Mar 14, 2023

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

let path = config.out.join("lock");
let pid = t!(std::fs::read_to_string(&path));
Copy link
Member

Choose a reason for hiding this comment

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

You need to check if this path exists before reading the file. if it doesn't exist, there's no process holding the lock and this will panic because the file won't be found.

(even better than checking if it exists is to handle ErrorKind::NotFound, to avoid TOCTOU issues.)

@JohnCSimon
Copy link
Member

@zephaniahong

ping from triage - can you post your status on this PR? There hasn't been an update in a few months. Thanks!

FYI: when a PR is ready for review, send a message containing
@rustbot ready to switch to S-waiting-on-review so the PR is in the reviewer's backlog.

@zephaniahong
Copy link
Contributor Author

zephaniahong commented Jun 11, 2023

@JohnCSimon hey! Sorry for the delay! The PR is almost done and requires just a few finishing touches. I'll get it out in the coming week😊

edit: I accidentally deleted some important binaries on my computer so do give me a couple more days to do a hard reset.
I'll open another PR for this issue once my computer is fixed

@zephaniahong
Copy link
Contributor Author

I've followed up this issue at #112918

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

"build directory locked" should say which process is holding the lock
8 participants