-
Notifications
You must be signed in to change notification settings - Fork 17
Invoke dart/flutter in a more robust way #143
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
base: main
Are you sure you want to change the base?
Conversation
PR HealthChangelog Entry ✔️
Changes to files need to be accounted for in their respective changelogs. |
'DART_SDK environment variable.', | ||
) | ||
..addOption( | ||
flutterSdkOption, |
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.
if the Flutter SDK is provided, do we want to always use the Dart SDK included with the Flutter SDK? I could see danger of using a Dart SDK provided by the CLI arg that is incompatible with the Flutter SDK provided by the Flutter CLI arg.
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.
Hmm we could do that, make these flags mutually exclusive. But, it might make some valid use cases also not work.... (for instance some new language feature is supported in a Dart SDK you have but not in your Flutter SDK, and you wouldn't be able to analyze that dart project).
dartSdkPath ??= p.dirname(p.dirname(Platform.resolvedExecutable)); | ||
|
||
final versionFile = dartSdkPath.child('version'); | ||
if (!File(versionFile).existsSync()) { |
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.
is it possible this file doesn't exist even for a valid dart sdk? For example if dart --version
or flutter --version
hasn't been ran yet?
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.
That might be possible with flutter, not sure, but for dart it is a part of the SDK. I am only checking for the version file for the Dart SDK right now.
|
||
final versionFile = dartSdkPath.child('version'); | ||
if (!File(versionFile).existsSync()) { | ||
throw ArgumentError('Invalid Dart SDK path: $dartSdkPath'); |
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.
If dartSdkPath
that was passed in as a parameter fails this check, perhaps we should fail more gracefully by trying again with dartSdkPath = p.dirname(p.dirname(Platform.resolvedExecutable));
?
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 if one is explicitly configured we should always use that one and not silently fall back on another one (most people won't see MCP server output).
Closes #33
Adds --dart-sdk and --flutter-sdk arguments to the CLI, falling back on DART_SDK and FLUTTER_SDK environment variables, and finally
Platform.resolvedExecutable
.The flutter SDK will also be looked up relative to the Dart SDK, if a location is not given by command line arg or environment.