-
Notifications
You must be signed in to change notification settings - Fork 29.4k
Fix download Dart DK step to work for paths with apostrophes #15137
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
Conversation
bin/flutter.bat
Outdated
| :do_sdk_update_and_snapshot | ||
| ECHO Checking Dart SDK version... | ||
| CALL PowerShell.exe -ExecutionPolicy Bypass -Command "& '%FLUTTER_ROOT%/bin/internal/update_dart_sdk.ps1'" | ||
| SET update_dart_bin="%FLUTTER_ROOT%/bin/internal/update_dart_sdk.ps1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you really need double quotes on RHS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so in case there are spaces in the path. I cribbed this code off of SO...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so in case there are spaces in the path. I cribbed this code off of SO...
Generally speaking when setting variable to a value with spaces you don't need quotes. When you do use quotes in set they will become part of the variable's value. That could have been your intent. Or not :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering whether the difference is substantive - I tried the fix in my Windows 10 instance, and it worked. Do you think it'd preferable to remove the quotes? If so, I'll try that and verify that it still works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My preference is to not to use quotes unless there is a reason to use them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
Merging on red because the ChromeBot failure is red on a rerun, but the dashboard doesn't reflect that. |
Fixes #15136