Update the flutter script's locking mechanism and follow_links #57590
Conversation
70a2143 to
4249008
Compare
8e54354 to
45a4013
Compare
|
I did consider using the mkdir method for all platforms, simply because then we would be able to not be limited to locking for a single host if the flutter tree is on a file share, but the downside is that if someone kills a running script with This is a tradeoff, but I think a file-shared repo is a less common situation than killing a running script on a single host, so I preferred flock where it exists. I've switched the flock call to be in a loop though, so that we can immediately print the "Waiting for another flutter command..." prompt: as it is now, it doesn't print that until the Dart snapshot is built and runs, so if you don't have it built, it takes a while to say why it's waiting (like 10-20 seconds). |
73d2170 to
c35bbeb
Compare
|
After looking into it some more, I think I'm just going to use the |
|
The advantage of using I did find after fixing this, though, that the locking that the flutter tools use internally also doesn't work over a network share (at least on an AFP-over-TCP share mounted on a macOS machine), so I'm not sure that this is a real fix to a problem anyone has right now: they already couldn't use Flutter from that kind of share, even if the bash script worked perfectly. |
|
Even more data! :-) Actually, if I So I guess I'm going to leave it using |
We need to link to this PR in the code, lest someone end up going through the whole thing again in a few years :P |
b4d5e5e to
54792b6
Compare
|
Well, the PR is implicitly linked, since it's in the history for the file. |
|
OK, I've updated this to HEAD, and I think this is ready for review "for reals". |
54792b6 to
473c5fe
Compare
jonahwilliams
left a comment
There was a problem hiding this comment.
Changes and comments look good for the most part.
I did not exhaustively verify the new lock logic, I think that if it works on CI and locally thats really the best we can do with shells
bin/shared.sh
Outdated
There was a problem hiding this comment.
nit. should we print a newline here? when I issued dart -h while another process had a lock, it looked like this:
~/git/flutter$ bin/dart -h
A command-line utility for Dart development.se the startup lock...
There was a problem hiding this comment.
The reason this prints with a return is that the Dart code itself will start up and lock, printing that same message.
So, with the newline, it can look like this:
~/git/flutter$ bin/dart -h
Waiting for another flutter command to release the startup lock...
Waiting for another flutter command to release the startup lock...
A command-line utility for Dart development.
Maybe printing it twice is better than printing it once and sometimes overwriting it, but...
I had an idea: the code now prints
printf " \r"when this lock is released, and then if the Dart code locks, it'll just flicker once, and then there's no confusion. Does that work?
christopherfujino
left a comment
There was a problem hiding this comment.
LGTM with a nit.
|
Thanks so much for doing this @gspencergoog! |
…on to be more robust and support more platforms.
473c5fe to
5fc41fb
Compare
Description
Update the flutter and dart scripts' locking mechanism and follow_links function to be more robust and support more platforms.
This adds support for using mkdir as a fallback if the system doesn't have flock instead of using shlock, since shlock doesn't work on shared filesystems.
It also fixes a problem in the follow_links function where it failed when the link resolved to the root directory.
Related Issues
Tests
Breaking Change