-
Notifications
You must be signed in to change notification settings - Fork 29.8k
Fix gen_snapshot name, path for Windows. #13759
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
Executables have '.exe' extension on Windows. We do have to specify extension for gen_snapshot since when running with local engine, we are looking for that exact file before launching it.
|
Instead of checking if the file exists can we use package:process’ canRun? That one checks if the file exists without requiring file extensions. |
| return 'gen_snapshot'; | ||
| return platform.isWindows ? 'gen_snapshot.exe' : 'gen_snapshot'; | ||
| case Artifact.flutterTester: | ||
| return 'flutter_tester'; |
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.
Why doesn't flutter_tester need it?
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.
Why doesn't flutter_tester need it?
- I don't see flutter_tester allowed on Windows(line 281 below).
- Only gen_snapshot has file existence check(line 269 below).
|
@goderbauer wrote:
Thanks for the suggestion! Changed. |
goderbauer
left a comment
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.
LGTM
Use
canRuninstead of check for file existence because it is guaranteed to work on Windows(where executable file have '.exe' extension) on Windows.Also, gen_snapshot on Windows is in build folder, rather than in
clang_*subfolder.