Lock bootstrap (x.py) build directory #88310

worldeva · 2021-08-25T00:01:55Z

Closes #76661, closes #80849,
x.py creates a lock file at project_root/lock.db

r? @jyn514 , because he was one that told me about this~

rust-highfive · 2021-08-25T00:01:58Z

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

Please see the contribution instructions for more information.

jyn514 · 2021-08-25T00:19:02Z

Happy to be responsible for this, but @Mark-Simulacrum will want to take a look too I'm sure.

jyn514

I found that this doesn't lock in bootstrap.py, but maybe that's ok - certainly I haven't seen any bugs reported about it.

// terminal 1
$ x check
downloading https://ci-artifacts.rust-lang.org/rustc-builds/db002a06ae9154a35d410550bc5132df883d7baa/rust-dev-nightly-x86_64-unknown-linux-gnu.tar.xz
############################################################################### 100.0%
extracting /home/joshua/src/rust/rust/build/cache/llvm-db002a06ae9154a35d410550bc5132df883d7baa-False/rust-dev-nightly-x86_64-unknown-linux-gnu.tar.xz

// terminal 2
$ x build
Updating only changed submodules
Submodules updated in 0.02 seconds
downloading https://ci-artifacts.rust-lang.org/rustc-builds/db002a06ae9154a35d410550bc5132df883d7baa/rust-dev-nightly-x86_64-unknown-linux-gnu.tar.xz
###                                                                               4.9%^C

I don't feel qualified to review the flock code, but I also think I may not have to - I left a comment below.

I'm very happy with how this fixes the parallel builds problem :) thank you for tackling it!

jyn514 · 2021-08-25T00:22:32Z

src/bootstrap/flock.rs

@@ -0,0 +1,206 @@
+//! Handles creating a file lock to avoid race conditions[1].


This is now the third flock implementation in-tree, there's also one for rustdoc ... maybe we should separate this out into a tiny crate or something? It would be hard to get cargo to use it since it's a submodule, but we could at least share it with rustdoc.

cc @rust-lang/cargo, do you have opinions on this? Would you be willing to separate the flock implementation into a different crate, and maybe put it on rust-lang/flock? I don't think there's any need to publish to crates.io but it would be nice to share at least between different tools.
cc @kinnison @rbtcollins, I'm sure rustup has its own implementation of flock too.

rust-lang/rustup#988 (comment) and the top level summary are probably relevant.

tl;dr: don't want the cargo implementation, it is buggy; but also haven't gotten around to fixing the issue in rustup, which is complected with rust-lang/rustup#2441 making this a little gnarly to think through.

Ok. x.py itself probably doesn't need an airtight flock implementation, I wouldn't expect people to be using NFS for builds. I totally get that rustup does have to deal with those scenarios though.

Does rustup's flock implementation have any downsides compared to cargo's? Could we start from that as the shared codebase instead?

After working a bit on standalone flock implementation for a bit, I wonder why we don't just use fcntl(2)? There is a issue with the lock being able to be acquired by two threads of the same process, but we don't seem to be doing that, and it works over nfs.

There is a issue with the lock being able to be acquired by two threads of the same process

That would be an issue for the flock implementation in rustc itself though as multiple rustc sessions can run inside the same process when using custom drivers. For example rls used to do this.

That would be an issue for the flock implementation in rustc itself though as multiple rustc sessions can run inside the same process when using custom drivers. For example rls used to do this.

We could avoid that with a mutex, right? So any thread has to unlock a mutex just to call flock? Then you get both per-thread and per-process synchronization.

If I understand it correctly that flock will always succeed if a file has already been locked elsewhere inside the same process, that would require serializing all rustc sessions such that each session completely finishes until the next session locks any file, even if it isn't the same file.

We don't have one for rustup yet; we just have quite clear understanding of the problem space we're dealing with :/

There's also fd-lock, which worked relatively well for me in the past (admittedly it was a small project). https://crates.io/crates/fd-lock/

That said, perhaps you need more control than it gives.

Mark-Simulacrum · 2021-09-01T02:38:43Z

So... this might be a little "out there", but it feels simpler than the solution this suggests and avoids duplicating code from Cargo. Both Python 2.7.x and Python 3 natively have sqlite built-in, as far as I can tell, via the sqlite3 module.

That means that we could likely implement this by "just" leveraging sqlite transactions to block. This is sort of overkill, but I'd expect it to work pretty well across any platform or filesystem, since sqlite itself is pretty battle tested.

It's also true that the python script currently manages a bit of state via pretty adhoc files (e.g., for the download stamps). I wouldn't change those today, there's not much point, but in the future it might make sense to leverage a more structured format that sqlite trivially gives us for this storage and without creating a bunch of files all over the place.

I'm curious to hear what folks think of this idea. I haven't actually tried a sample implementation, either, but I'd naively expect it to work out to at most a couple dozen lines of Python -- not too complex. Since sqlite comes pre-compiled for us through python, this seems a nice solution that avoids some of the hassle we'd otherwise run into. If and/or when we migrate away from the python wrapper, we can explore alternatives (such as flock proposed by this PR), but for now this would seem better. It also avoids any parallelism issues in the python script's downloads and such, which also seems good -- I'm not sold those are bug free today.

I do have mixed feelings about this so I could be convinced the impl in this PR is something we should just go with, but if the sqlite-based solution actually works and isn't too hard, then it seems preferable to me personally on the grounds of not having to maintain pretty low-level platform-specific code in bootstrap -- which is a place I'd really prefer to avoid that.

jyn514 · 2021-09-25T17:07:49Z

Hi @worldeva - do you know when you'll get a chance to look at this again?

worldeva · 2021-12-05T03:32:13Z

Sorry for the long delay for the tiny commit; life came up >,<
As per suggestion, I've removed all the stuff in bootstrap-proper and moved it over to a sqlite3 based bit in x.py, it hopefully should be good to go now!~

src/bootstrap/bootstrap.py

jyn514

This looks good to me with the tidy lint fixed :)

src/bootstrap/bootstrap.py

Mark-Simulacrum · 2021-12-07T02:39:30Z

src/bootstrap/bootstrap.py


 from time import time

+def acquire_lock():
+    try:
+        con = sqlite3.Connection("xlock.db", timeout=0)


Is this creating the database in the working directory? I think we probably want it in build/ -- build.build_dir probably?

I changed it to creating it in build.rust_dir: The lock locks a lot more than just the stuff in build (submodules, etc), and the folder may not exist at the time of lock creation.

Mark-Simulacrum · 2021-12-07T02:39:51Z

Mostly just some documentation, but I expect we can move ahead once that's added. It'd be good to do some spot checking locally to confirm this mostly works, and make a note in the documentation (and PR description) the location of the database file, so if there's some bug with this it's easy to delete it.

…Simulacrum Lock bootstrap (x.py) build directory Closes rust-lang#76661, closes rust-lang#80849, `x.py` creates a lock file at `project_root/lock.db` r? ``@jyn514`` , because he was one that told me about this~

matthiaskrgr · 2021-12-08T15:03:30Z

@bors r-

#91664 (comment)
Failed in a rollup.

src/bootstrap/bootstrap.py

worldeva · 2021-12-09T02:49:45Z

Should be fixed!~

Mark-Simulacrum · 2021-12-09T03:22:08Z

src/bootstrap/bootstrap.py

+                    pass
+            return curs
+    except ImportError:
+        print("warning: sqlite3 not available, skipping lock")


I think it's worth asking users to file an issue if they see this (on rust-lang/rust) and making it clear that the lock isn't necessary for correctness of the results (something like "build directory lock", maybe)?

worldeva · 2021-12-25T01:35:39Z

Done-
The one thing that I'm mildly worried about is "consider filing a issue" makes it seem more important than it is- should I perhaps make it even more explicit with a "this lock is only to prevent multiple concurrent bootstraps from running" or such?
(...Though we're straying into the bikeshedding territory now :p )

Prevent spurious build failures and other bugs caused by parallel runs of x.py. We back the lock with sqlite, so that we have a cross-platform locking strategy, and which can be done nearly first in the build process (from Python), which helps move the lock as early as possible.

Mark-Simulacrum · 2021-12-31T15:31:45Z

Slightly adjusted wording, but seems good to me otherwise. Thanks!

@bors r+

bors · 2021-12-31T15:31:47Z

📌 Commit 1326bd6 has been approved by Mark-Simulacrum

…askrgr Rollup of 7 pull requests Successful merges: - rust-lang#88310 (Lock bootstrap (x.py) build directory) - rust-lang#92097 (Implement split_at_spare_mut without Deref to a slice so that the spare slice is valid) - rust-lang#92412 (Fix double space in pretty printed TryBlock) - rust-lang#92420 (Fix whitespace in pretty printed PatKind::Range) - rust-lang#92457 (Sync rustc_codegen_gcc) - rust-lang#92460 ([rustc_builtin_macros] add indices to format_foreign::printf::Substitution::Escape) - rust-lang#92469 (Make tidy check for magic numbers that spell things) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

rust-highfive assigned jyn514 Aug 25, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 25, 2021

worldeva changed the title ~~Lock for Bootstrap build directory~~ Lock bootstrap (x.py) build directory Aug 25, 2021

jyn514 added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Aug 25, 2021

jyn514 reviewed Aug 25, 2021

View reviewed changes

jyn514 assigned Mark-Simulacrum Aug 25, 2021

Mark-Simulacrum 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 Sep 1, 2021

JohnCSimon 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 20, 2021

apiraino added the T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. label Oct 14, 2021

worldeva force-pushed the bootstrap-locking branch from a458940 to 94a3a8b Compare November 20, 2021 00:47

This comment has been minimized.

Sign in to view

worldeva force-pushed the bootstrap-locking branch from 94a3a8b to b229c54 Compare December 5, 2021 03:29

jyn514 reviewed Dec 5, 2021

View reviewed changes

src/bootstrap/bootstrap.py Outdated Show resolved Hide resolved

src/bootstrap/bootstrap.py Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

jyn514 removed their assignment Dec 5, 2021

jyn514 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 5, 2021

jyn514 approved these changes Dec 5, 2021

View reviewed changes

Mark-Simulacrum reviewed Dec 7, 2021

View reviewed changes

Mark-Simulacrum 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 7, 2021

matthiaskrgr mentioned this pull request Dec 8, 2021

Rollup of 8 pull requests #91664

Closed

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 8, 2021

matthiaskrgr reviewed Dec 8, 2021

View reviewed changes

src/bootstrap/bootstrap.py Outdated Show resolved Hide resolved

worldeva force-pushed the bootstrap-locking branch from 0a84930 to c22cdbf Compare December 9, 2021 02:35

Mark-Simulacrum reviewed Dec 9, 2021

View reviewed changes

worldeva force-pushed the bootstrap-locking branch 2 times, most recently from 2742b0d to f25e4bf Compare December 24, 2021 23:43

Mark-Simulacrum added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 25, 2021

Mark-Simulacrum force-pushed the bootstrap-locking branch from f25e4bf to 1326bd6 Compare December 31, 2021 15:31

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 31, 2021

Milo123459 mentioned this pull request Dec 31, 2021

python x.py check always rebuilds rustc_llvm on Windows #92369

Closed

matthiaskrgr mentioned this pull request Jan 1, 2022

Rollup of 7 pull requests #92471

Merged

bors merged commit 5a5c928 into rust-lang:master Jan 1, 2022

rustbot added this to the 1.59.0 milestone Jan 1, 2022

jyn514 mentioned this pull request Mar 20, 2022

Tracking Issue for removing python as the main bootstrap entry point #94829

Open

15 tasks

worldeva deleted the bootstrap-locking branch March 25, 2022 23:23

worldeva restored the bootstrap-locking branch March 25, 2022 23:24

jyn514 mentioned this pull request Jan 19, 2023

"build directory locked" should say which process is holding the lock #107077

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lock bootstrap (x.py) build directory #88310

Lock bootstrap (x.py) build directory #88310

worldeva commented Aug 25, 2021 •

edited

Loading

rust-highfive commented Aug 25, 2021

jyn514 commented Aug 25, 2021

jyn514 left a comment •

edited

Loading

jyn514 Aug 25, 2021

rbtcollins Aug 25, 2021

jyn514 Aug 25, 2021

worldeva Aug 25, 2021

bjorn3 Aug 25, 2021

jyn514 Aug 25, 2021

bjorn3 Aug 25, 2021

rbtcollins Aug 26, 2021

thomcc Oct 31, 2021

Mark-Simulacrum commented Sep 1, 2021

jyn514 commented Sep 25, 2021

This comment has been minimized.

worldeva commented Dec 5, 2021 •

edited

Loading

This comment has been minimized.

jyn514 left a comment

Mark-Simulacrum Dec 7, 2021

worldeva Dec 7, 2021

Mark-Simulacrum commented Dec 7, 2021

matthiaskrgr commented Dec 8, 2021

worldeva commented Dec 9, 2021

Mark-Simulacrum Dec 9, 2021

worldeva commented Dec 25, 2021

Mark-Simulacrum commented Dec 31, 2021

bors commented Dec 31, 2021

		@@ -0,0 +1,206 @@
		//! Handles creating a file lock to avoid race conditions[1].

Lock bootstrap (x.py) build directory #88310

Lock bootstrap (x.py) build directory #88310

Conversation

worldeva commented Aug 25, 2021 • edited Loading

rust-highfive commented Aug 25, 2021

jyn514 commented Aug 25, 2021

jyn514 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mark-Simulacrum commented Sep 1, 2021

jyn514 commented Sep 25, 2021

This comment has been minimized.

worldeva commented Dec 5, 2021 • edited Loading

This comment has been minimized.

jyn514 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mark-Simulacrum commented Dec 7, 2021

matthiaskrgr commented Dec 8, 2021

worldeva commented Dec 9, 2021

Choose a reason for hiding this comment

worldeva commented Dec 25, 2021

Mark-Simulacrum commented Dec 31, 2021

bors commented Dec 31, 2021

worldeva commented Aug 25, 2021 •

edited

Loading

jyn514 left a comment •

edited

Loading

worldeva commented Dec 5, 2021 •

edited

Loading