-
Notifications
You must be signed in to change notification settings - Fork 29.7k
[flutter_tools] fix missing cmake #103761
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
[flutter_tools] fix missing cmake #103761
Conversation
Jasguerrero
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
| && targetPlatform == TargetPlatform.linux_arm64; | ||
| int result; | ||
| if (!globals.processManager.canRun('cmake')) { | ||
| throwToolExit(globals.userMessages.cmakeMissing); |
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 want to remove the on ArgumentError block? Or at least make it throw the same thing?
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.
good thinking
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 removed the try-catch, as I'm assuming if we get an ArgumentError that means it's a tool bug.
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.
You can tell I wrote this code when I was less familiar with Dart. Otherwise the fact that I was catching an Error type in the first place would have been a big red flag that the code was wrong...
|
This pull request is not suitable for automatic merging in its current state.
|
Fixes #88897
Previously we were calling cmake and catching
ArgumentErrorfor the case where cmake was not installed. However, per #88897, it was actually throwing aProcessException. Instead, explicitly checkprocessManager.canRun('cmake')(rather than catching allProcessExceptions), and if it is not runnable give the user the sameUserMessagethe doctor validator would.