Skip to content

Conversation

@aam
Copy link
Member

@aam aam commented Dec 22, 2017

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
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
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
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
Copy link
Member Author

aam commented Dec 22, 2017

@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.

Copy link
Member

@goderbauer goderbauer left a comment

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
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 12, 2021
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