Skip to content

Fix gen_snapshot name, path for Windows.#13759

Merged
aam merged 2 commits into
flutter:masterfrom
aam:fix-build-aot-windows-local-engine
Dec 23, 2017
Merged

Fix gen_snapshot name, path for Windows.#13759
aam merged 2 commits into
flutter:masterfrom
aam:fix-build-aot-windows-local-engine

Conversation

@aam

@aam aam commented Dec 22, 2017

Copy link
Copy Markdown
Member

Use canRun instead 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.

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.
@aam
aam requested a review from zanderso December 22, 2017 22:41
@goderbauer

Copy link
Copy Markdown
Member

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';

Copy link
Copy Markdown
Member

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?

Copy link
Copy Markdown
Member Author

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?

  1. I don't see flutter_tester allowed on Windows(line 281 below).
  2. Only gen_snapshot has file existence check(line 269 below).

@aam

aam commented Dec 22, 2017

Copy link
Copy Markdown
Member Author

@goderbauer wrote:

Instead of checking if the file exists can we use package:process’ canRun? That one checks if the file exists without requiring file extensions.

Thanks for the suggestion! Changed.

@goderbauer goderbauer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@aam
aam merged commit 70b32e8 into flutter:master Dec 23, 2017
@aam
aam deleted the fix-build-aot-windows-local-engine branch December 23, 2017 01:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants