Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11170003:
Pub fix for Windows (Closed)
|
Created:
8 years, 2 months ago by Johnni Winther Modified:
8 years, 2 months ago CC:
reviews_dartlang.org, Bob Nystrom Visibility:
Public. |
DescriptionPub fix for Windows
BUG=5859
Committed: https://code.google.com/p/dart/source/detail?r=13676
Patch Set 1 #
Total comments: 5
Patch Set 2 : Updated cf. comments #
Total comments: 8
Messages
Total messages: 14 (0 generated)
LGTM! https://codereview.chromium.org/11170003/diff/1/utils/pub/hosted_source.dart File utils/pub/hosted_source.dart (right): https://codereview.chromium.org/11170003/diff/1/utils/pub/hosted_source.dart#... utils/pub/hosted_source.dart:90: // TODO(rnystrom): Awful hack. On Windows, we see cases where the extract Move inside if (windows).
LGTM with comment: https://codereview.chromium.org/11170003/diff/1/utils/pub/io.dart File utils/pub/io.dart (right): https://codereview.chromium.org/11170003/diff/1/utils/pub/io.dart#newcode639 utils/pub/io.dart:639: var pathTo7zip = '7zip/7za.exe'; Have you double checked that this works with create_sdk.py?
https://codereview.chromium.org/11170003/diff/1/utils/pub/io.dart File utils/pub/io.dart (right): https://codereview.chromium.org/11170003/diff/1/utils/pub/io.dart#newcode639 utils/pub/io.dart:639: var pathTo7zip = '7zip/7za.exe'; On 2012/10/16 08:01:27, kasperl wrote: > Have you double checked that this works with create_sdk.py? I didn't see this change. Is this intentional?
https://codereview.chromium.org/11170003/diff/1/utils/pub/hosted_source.dart File utils/pub/hosted_source.dart (right): https://codereview.chromium.org/11170003/diff/1/utils/pub/hosted_source.dart#... utils/pub/hosted_source.dart:90: // TODO(rnystrom): Awful hack. On Windows, we see cases where the extract On 2012/10/16 07:59:47, ajohnsen wrote: > Move inside if (windows). Done. https://codereview.chromium.org/11170003/diff/1/utils/pub/io.dart File utils/pub/io.dart (right): https://codereview.chromium.org/11170003/diff/1/utils/pub/io.dart#newcode639 utils/pub/io.dart:639: var pathTo7zip = '7zip/7za.exe'; On 2012/10/16 08:03:19, ajohnsen wrote: > On 2012/10/16 08:01:27, kasperl wrote: > > Have you double checked that this works with create_sdk.py? > > I didn't see this change. Is this intentional? Removed.
LGTM.
LGTM
Thanks for looking at this! https://codereview.chromium.org/11170003/diff/5/utils/pub/hosted_source.dart File utils/pub/hosted_source.dart (right): https://codereview.chromium.org/11170003/diff/5/utils/pub/hosted_source.dart#... utils/pub/hosted_source.dart:97: return renameDir(tempDir, destPath); Maybe I just haven't had my coffee yet, but I don't understand what's different here. I structured it the way I did to avoid a duplicating "renameDir" in the code, but as far as I can tell, the before and after have the same semantics here. What am I missing? https://codereview.chromium.org/11170003/diff/5/utils/pub/io.dart File utils/pub/io.dart (right): https://codereview.chromium.org/11170003/diff/5/utils/pub/io.dart#newcode505 utils/pub/io.dart:505: }); Well this is a distinct improvement. :) This code in pub is older than Process.run, and this is much better. At the same time, I would like to know more about where the issue was in the original code. Since that API is still available, other users could likely misuse it the same way I was. Any idea what was wrong with the old code?
I hope this helps explaining the changes we did :) https://codereview.chromium.org/11170003/diff/5/utils/pub/hosted_source.dart File utils/pub/hosted_source.dart (right): https://codereview.chromium.org/11170003/diff/5/utils/pub/hosted_source.dart#... utils/pub/hosted_source.dart:97: return renameDir(tempDir, destPath); On 2012/10/16 14:07:29, Bob Nystrom wrote: > Maybe I just haven't had my coffee yet, but I don't understand what's different > here. I structured it the way I did to avoid a duplicating "renameDir" in the > code, but as far as I can tell, the before and after have the same semantics > here. > > What am I missing? The problem is that before, you started the renameDir as the first thing. That means that _before_ you sleep, you start renaming. Then, when you are done sleeping, you chain your renameDir (which is then already completed/broken). We changed this so it will not start the renameDir until it's done sleeping. https://codereview.chromium.org/11170003/diff/5/utils/pub/io.dart File utils/pub/io.dart (right): https://codereview.chromium.org/11170003/diff/5/utils/pub/io.dart#newcode505 utils/pub/io.dart:505: }); On 2012/10/16 14:07:29, Bob Nystrom wrote: > Well this is a distinct improvement. :) > > This code in pub is older than Process.run, and this is much better. > > At the same time, I would like to know more about where the issue was in the > original code. Since that API is still available, other users could likely > misuse it the same way I was. Any idea what was wrong with the old code? There is nothing wrong with the old code, other then the same issues that lead to us introducing Process.run: it's very easy to do wrong. You have to wait for several events to occur, and make sure all the underlying pipes are closed. I saw this when we looked for the issue, and thought that the error might be originated here. It turned up it was't, but I though we should keep the change in anyway :)
https://codereview.chromium.org/11170003/diff/5/utils/pub/hosted_source.dart File utils/pub/hosted_source.dart (right): https://codereview.chromium.org/11170003/diff/5/utils/pub/hosted_source.dart#... utils/pub/hosted_source.dart:97: return renameDir(tempDir, destPath); On 2012/10/16 14:29:57, ajohnsen wrote: > On 2012/10/16 14:07:29, Bob Nystrom wrote: > > Maybe I just haven't had my coffee yet, but I don't understand what's > different > > here. I structured it the way I did to avoid a duplicating "renameDir" in the > > code, but as far as I can tell, the before and after have the same semantics > > here. > > > > What am I missing? > > The problem is that before, you started the renameDir as the first thing. That > means that _before_ you sleep, you start renaming. Then, when you are done > sleeping, you chain your renameDir (which is then already completed/broken). We > changed this so it will not start the renameDir until it's done sleeping. /me smacks forehead. Right you are. Thanks.
https://codereview.chromium.org/11170003/diff/5/utils/pub/io.dart File utils/pub/io.dart (right): https://codereview.chromium.org/11170003/diff/5/utils/pub/io.dart#newcode505 utils/pub/io.dart:505: }); On 2012/10/16 14:29:57, ajohnsen wrote: > On 2012/10/16 14:07:29, Bob Nystrom wrote: > > Well this is a distinct improvement. :) > > > > This code in pub is older than Process.run, and this is much better. > > > > At the same time, I would like to know more about where the issue was in the > > original code. Since that API is still available, other users could likely > > misuse it the same way I was. Any idea what was wrong with the old code? > > There is nothing wrong with the old code, other then the same issues that lead > to us introducing Process.run: it's very easy to do wrong. You have to wait for > several events to occur, and make sure all the underlying pipes are closed. I > saw this when we looked for the issue, and thought that the error might be > originated here. It turned up it was't, but I though we should keep the change > in anyway :) This doesn't support pipeStdout/pipeStderr. Either those arguments should be removed or we should support them.
https://codereview.chromium.org/11170003/diff/5/utils/pub/io.dart File utils/pub/io.dart (right): https://codereview.chromium.org/11170003/diff/5/utils/pub/io.dart#newcode505 utils/pub/io.dart:505: }); On 2012/10/16 17:20:15, nweiz wrote: > On 2012/10/16 14:29:57, ajohnsen wrote: > > On 2012/10/16 14:07:29, Bob Nystrom wrote: > > > Well this is a distinct improvement. :) > > > > > > This code in pub is older than Process.run, and this is much better. > > > > > > At the same time, I would like to know more about where the issue was in the > > > original code. Since that API is still available, other users could likely > > > misuse it the same way I was. Any idea what was wrong with the old code? > > > > There is nothing wrong with the old code, other then the same issues that lead > > to us introducing Process.run: it's very easy to do wrong. You have to wait > for > > several events to occur, and make sure all the underlying pipes are closed. I > > saw this when we looked for the issue, and thought that the error might be > > originated here. It turned up it was't, but I though we should keep the change > > in anyway :) > > This doesn't support pipeStdout/pipeStderr. Either those arguments should be > removed or we should support them. Ahh, good point. Bob, is the piping needed in Pub? Otherwise, feel free to revert this part of the patch.
https://codereview.chromium.org/11170003/diff/5/utils/pub/io.dart File utils/pub/io.dart (right): https://codereview.chromium.org/11170003/diff/5/utils/pub/io.dart#newcode505 utils/pub/io.dart:505: }); On 2012/10/17 07:05:25, ajohnsen wrote: > On 2012/10/16 17:20:15, nweiz wrote: > > On 2012/10/16 14:29:57, ajohnsen wrote: > > > On 2012/10/16 14:07:29, Bob Nystrom wrote: > > > > Well this is a distinct improvement. :) > > > > > > > > This code in pub is older than Process.run, and this is much better. > > > > > > > > At the same time, I would like to know more about where the issue was in > the > > > > original code. Since that API is still available, other users could likely > > > > misuse it the same way I was. Any idea what was wrong with the old code? > > > > > > There is nothing wrong with the old code, other then the same issues that > lead > > > to us introducing Process.run: it's very easy to do wrong. You have to wait > > for > > > several events to occur, and make sure all the underlying pipes are closed. > I > > > saw this when we looked for the issue, and thought that the error might be > > > originated here. It turned up it was't, but I though we should keep the > change > > > in anyway :) > > > > This doesn't support pipeStdout/pipeStderr. Either those arguments should be > > removed or we should support them. > > Ahh, good point. Bob, is the piping needed in Pub? Otherwise, feel free to > revert this part of the patch. It doesn't look like we're using that right now, so I think we're OK. We'll be adding "--verbose" support to pub relatively soon and we can revisit this then. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
