You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Add separate overrides for Fuchsia and Flutter SDKs (+ tidy search logic) #440
Flutter and Fuchsia require specific versions (or patched) SDKs so using the same override doesn't really make sense (though was added in #437 as a short-term fix).
We should add equiv options to userDefinedSdkPath for these, and also probably userDefinedSdkPaths (and do #441).
@yyoon@devoncarew Ok, I documented all of the current search paths and it turned out that it's rather complicated! I've added new overrides in for all SDKs for all project types and the list is below (marked with NEW). The settings are individual, so the Flutter SDK override is different to the Fuchsia Flutter SDK override - I'm assuming that if you're doing both Flutter and Fuchsia dev (for ex.) and you choose to override the Flutter SDK path for Flutter dev, it might not be appropriate to use that for Fuchsia. For the same reason, we don't currently fall back to searching the other paths (for example, if for Fuchsia we can't find a Dart SDK in the override or the local Fuchsia folder, we will not start searching PATH, etc.; since again, I don't know if it's appropriate to have use the generic SDKs). I don't know if this idea is sound.
Dart project
Dart SDK
sdkPath
[%PATH%]
Flutter project
Flutter SDK
flutterSdkPath (NEW)
workspace root
extract from packages file
%FLUTTER_ROOT%
[%PATH%]
Dart SDK
flutterDartSdkPath (NEW)
(Flutter SDK)/bin/cache/dart-sdk
Fuchsia project
Fuchsia SDK
fuchsiaSdkPath (NEW)
workspace root (walking up looking for .jiri_root)
Does this all sounds like the best way to go (as an aside, if all IDEs are having to do all of this, probably it could be simplified or at least made consistent across the different project types)?
I want to expand the sdkContainer/quick switching feature (which I incorrectly referred to as sdkPaths yesterday) support to handle all of these too to allow for fast switching, though given the number of override options above, it looks like it'd mean 6 of them (so in total, 12 options for SDK paths across 3 project types). Before I embark on this, I want to be sure it makes sense to you two.
Hmm, I don't understand all the above. This is for auto-detection of SDKs, or auto-detection and explicit configuration, or both?
It may be useful to try and simplify the above, if only to minimize bugs and make the sdk used a bit more predictable for the user. Perhaps have arbitrary auto-magic detection if there's no sdk set, but once one is found, automatically set the sdk config setting for the user (and notify them that we did)?
I'm less familiar w/ why there's a separate fuchsiaFlutterSdkPath / fuchsiaDartSdkPath path. In IntelliJ, we do have a separate 'flutter sdk' config setting. That's mostly for easy discoverability for the user though. When you set that setting, it'll automatically set the dart sdk setting (based on the flutter sdk and bin/cache/dart-sdk). We do support per project values for the sdk setting, and remember all the ones globally, in order to have a good set of values to choose from for new projects. We do still have issues w/ the overall sdk setting logic - esp. with new project creation - so improvements and ideas for the process definitely welcome :)
Hmm, I don't understand all the above. This is for auto-detection of SDKs, or auto-detection and explicit configuration, or both?
It's the list of places we'll search for each SDK at startup. If you ignore the ones marked NEW then it's what we already do today. Some of them don't have overrides and that's an issue for this scenario described by@yyoon:
Occasionally, there could be some gap between when a fuchsia-change is made and when it gets shipped in the new version of SDK. That's when we need to temporarily switch to the locally built Dart SDK instead of the prebuilt one, to make the Dart analysis work correctly.
So there needs to be a way to override the Dart SDK for a Fuchsia project. The current fix uses dart.sdkPath but this doesn't seem ideal - if this is a Fuchsia-specific (or even just unstable) SDK you might want to use it for Fuchsia, but not Dart (or Flutter) projects. So, per-project-type overrides seem to make sense. However because the number of SDKs is large (1 for Dart, 2 for Flutter, 3 for Fuchsia) it makes the lists quite long!
It may be useful to try and simplify the above, if only to minimize bugs and make the sdk used a bit more predictable for the user.
That's kinda what I'm trying to do... currently this isn't documented anywhere so I was trying to build a list so it can at least be seen by the user (I also hope to simplify the code so it looks just like a list, rather than a bunch of code with conditionals). A lot of the complexity comes from there not being standard places for the SDKs or ways of detecting them.
Perhaps have arbitrary auto-magic detection if there's no sdk set, but once one is found, automatically set the sdk config setting for the user (and notify them that we did)?
I'm concerned that if we stamp the setting based on the result of the search, we might end up doing the wrong thing (eg. if a Flutter SDK lives inside a project folder, it doesn't make sense to make that projects version the one used for all Flutter projects). It's also something for the user to then maintain - if we can auto-detect it (eg. from PATH) it seems to just make sense to do it every time (for ex. if the user changes PATH to point at a different SDK, it'll still work).
I'm less familiar w/ why there's a separate fuchsiaFlutterSdkPath / fuchsiaDartSdkPath path.
This is for the comment quoted above - Fuchsia sometimes needs to use a locally built Dart SDK (I don't know if Flutter has a similar issue - if the Flutter team were using VS Code for core Flutter dev and needed to make changes to the Dart SDK, how could they get those changes immediately without waiting for a dev Dart SDK build?).
We do still have issues w/ the overall sdk setting logic - esp. with new project creation - so improvements and ideas for the process definitely welcome :)
I don't think what we have listed above is that bad, there are just a few weird things:
Flutter SDK using FLUTTER_HOME and extracting from packages seems strange; however I think these are necessary for new projects unless it becomes common for Flutter in be in PATH (which on the surface seems like a good idea to me, rather than having project-local copies, but I'm sure it's not that simple)
Fuchsia SDK walking up looking for .jiri_root feels like a hack
Dart SDK for Fuchsia has platform in the search path (I'm sure there's a reason, but Flutter doesn't seem to need this and works similarly?)
If those things were eliminated, I think the search paths would actually be pretty simple. There might still be some slight confusion over Fuchsia and Flutter having "their own" versions of the Dart SDK which may need their own overrides, but at least it'd be easy to document and explain the search path to uses if we don't find something.
Sorry for my late reply. Your proposal seems okay and I don't mind it too much, but overall it also feels overly complicated to me, when looking at the entire sdk path configurations outlined here.
The auto-detection is great, because in normal situations you don't even have to worry about setting the sdk path. However, I still think that sdk path override could be done per project, in their Workspace Settings. When overriding the sdk path, you probably know what you're doing, and it's going to be a one-time thing to do.
Another point against adding project-specific overrides is that it might confuse users more than helping them. I could imagine someone on Flutter or Fuchsia project mistakenly setting the dart.sdkPath instead of the project-specific ones, and being confused as to why the setting is not taking effect.
@yyoon I did think about that - overriding the SDK in the workspace would allow the same setting to be used because it'd already be scoped to the Dart/Flutter/Fuchsia project.
The only problem is that if you do set it globally (as I often do, using the quick SDK switcher) then you could get an incorrect one. There's also a question of whether the SDK switcher should then edit the workspace or user setting.
That said; most of my testing of different SDKs is in a simple trivial sample app I use for testing/reproing, so maybe just making the switcher work at the workspace level all the time wouldn't make much different to me, and then all the above would work.
We'd still end up with three overrides, but it would eliminate FuchsiaFlutter, FuchsiaDart and FlutterDart override options.
A downside of doing this though would be that you can't really commit the workspace settings (it has paths in that are likely local to your machine). I suspect this is fairly common (Dart Code has this committed for example).
I'll think about this a little more, but I think maybe you've convinced me.
Ok, made good progress on this and much happier with it now. I've flattened all the old methods (which were somewhat inconsistent - some took paths and only searched there, while others also searched PATH etc.). Code looks much simpler now:
It's not perfect (all the xxx && is to handle where they're not appropriate, without having to write if statements to conditionally push onto the array) but much easier to understand than previously being split across a load of methods (and it not being clear when which options worked).
The Dart SDK override does always still come first (I've compared the results of the new code to the old and ensured the results are all the same) and I will add an override for flutter to handle the remaining case.
For now there won't be a Fuchsia override, since it doesn't look like we actually use the Fuchsia path for anything except finding Flutter/Dart. That means just two overrides, not 6 :-) I'll add a flutter override before closing this case.
Activity
DanTup commentedon Nov 17, 2017
@yyoon @devoncarew Ok, I documented all of the current search paths and it turned out that it's rather complicated! I've added new overrides in for all SDKs for all project types and the list is below (marked with NEW). The settings are individual, so the Flutter SDK override is different to the Fuchsia Flutter SDK override - I'm assuming that if you're doing both Flutter and Fuchsia dev (for ex.) and you choose to override the Flutter SDK path for Flutter dev, it might not be appropriate to use that for Fuchsia. For the same reason, we don't currently fall back to searching the other paths (for example, if for Fuchsia we can't find a Dart SDK in the override or the local Fuchsia folder, we will not start searching PATH, etc.; since again, I don't know if it's appropriate to have use the generic SDKs). I don't know if this idea is sound.
Dart project
sdkPath[%PATH%]Flutter project
flutterSdkPath(NEW)%FLUTTER_ROOT%[%PATH%]flutterDartSdkPath(NEW)(Flutter SDK)/bin/cache/dart-sdkFuchsia project
fuchsiaSdkPath(NEW)fuchsiaFlutterSdkPath(NEW)(Fuchsia SDK)/third_party/dart-pkg/git/flutterfuchsiaDartSdkPath(NEW)(Fuchsia SDK)/third_party/dart/tools/sdks/(platform)/dart-sdkDoes this all sounds like the best way to go (as an aside, if all IDEs are having to do all of this, probably it could be simplified or at least made consistent across the different project types)?
I want to expand the
sdkContainer/quick switching feature (which I incorrectly referred to as sdkPaths yesterday) support to handle all of these too to allow for fast switching, though given the number of override options above, it looks like it'd mean 6 of them (so in total, 12 options for SDK paths across 3 project types). Before I embark on this, I want to be sure it makes sense to you two.Teach Dart-Code about the Fuchsia proto-SDK
devoncarew commentedon Nov 18, 2017
Hmm, I don't understand all the above. This is for auto-detection of SDKs, or auto-detection and explicit configuration, or both?
It may be useful to try and simplify the above, if only to minimize bugs and make the sdk used a bit more predictable for the user. Perhaps have arbitrary auto-magic detection if there's no sdk set, but once one is found, automatically set the sdk config setting for the user (and notify them that we did)?
I'm less familiar w/ why there's a separate fuchsiaFlutterSdkPath / fuchsiaDartSdkPath path. In IntelliJ, we do have a separate 'flutter sdk' config setting. That's mostly for easy discoverability for the user though. When you set that setting, it'll automatically set the dart sdk setting (based on the flutter sdk and bin/cache/dart-sdk). We do support per project values for the sdk setting, and remember all the ones globally, in order to have a good set of values to choose from for new projects. We do still have issues w/ the overall sdk setting logic - esp. with new project creation - so improvements and ideas for the process definitely welcome :)
DanTup commentedon Nov 19, 2017
@devoncarew
It's the list of places we'll search for each SDK at startup. If you ignore the ones marked NEW then it's what we already do today. Some of them don't have overrides and that's an issue for this scenario described by @yyoon:
So there needs to be a way to override the Dart SDK for a Fuchsia project. The current fix uses
dart.sdkPathbut this doesn't seem ideal - if this is a Fuchsia-specific (or even just unstable) SDK you might want to use it for Fuchsia, but not Dart (or Flutter) projects. So, per-project-type overrides seem to make sense. However because the number of SDKs is large (1 for Dart, 2 for Flutter, 3 for Fuchsia) it makes the lists quite long!That's kinda what I'm trying to do... currently this isn't documented anywhere so I was trying to build a list so it can at least be seen by the user (I also hope to simplify the code so it looks just like a list, rather than a bunch of code with conditionals). A lot of the complexity comes from there not being standard places for the SDKs or ways of detecting them.
I'm concerned that if we stamp the setting based on the result of the search, we might end up doing the wrong thing (eg. if a Flutter SDK lives inside a project folder, it doesn't make sense to make that projects version the one used for all Flutter projects). It's also something for the user to then maintain - if we can auto-detect it (eg. from PATH) it seems to just make sense to do it every time (for ex. if the user changes PATH to point at a different SDK, it'll still work).
This is for the comment quoted above - Fuchsia sometimes needs to use a locally built Dart SDK (I don't know if Flutter has a similar issue - if the Flutter team were using VS Code for core Flutter dev and needed to make changes to the Dart SDK, how could they get those changes immediately without waiting for a dev Dart SDK build?).
I don't think what we have listed above is that bad, there are just a few weird things:
.jiri_rootfeels like a hackIf those things were eliminated, I think the search paths would actually be pretty simple. There might still be some slight confusion over Fuchsia and Flutter having "their own" versions of the Dart SDK which may need their own overrides, but at least it'd be easy to document and explain the search path to uses if we don't find something.
yyoon commentedon Nov 20, 2017
Sorry for my late reply. Your proposal seems okay and I don't mind it too much, but overall it also feels overly complicated to me, when looking at the entire sdk path configurations outlined here.
The auto-detection is great, because in normal situations you don't even have to worry about setting the sdk path. However, I still think that sdk path override could be done per project, in their Workspace Settings. When overriding the sdk path, you probably know what you're doing, and it's going to be a one-time thing to do.
Another point against adding project-specific overrides is that it might confuse users more than helping them. I could imagine someone on Flutter or Fuchsia project mistakenly setting the
dart.sdkPathinstead of the project-specific ones, and being confused as to why the setting is not taking effect.DanTup commentedon Nov 21, 2017
@yyoon I did think about that - overriding the SDK in the workspace would allow the same setting to be used because it'd already be scoped to the Dart/Flutter/Fuchsia project.
The only problem is that if you do set it globally (as I often do, using the quick SDK switcher) then you could get an incorrect one. There's also a question of whether the SDK switcher should then edit the workspace or user setting.
That said; most of my testing of different SDKs is in a simple trivial sample app I use for testing/reproing, so maybe just making the switcher work at the workspace level all the time wouldn't make much different to me, and then all the above would work.
We'd still end up with three overrides, but it would eliminate FuchsiaFlutter, FuchsiaDart and FlutterDart override options.
A downside of doing this though would be that you can't really commit the workspace settings (it has paths in that are likely local to your machine). I suspect this is fairly common (Dart Code has this committed for example).
I'll think about this a little more, but I think maybe you've convinced me.
DanTup commentedon Nov 25, 2017
Ok, made good progress on this and much happier with it now. I've flattened all the old methods (which were somewhat inconsistent - some took paths and only searched there, while others also searched PATH etc.). Code looks much simpler now:
It's not perfect (all the
xxx &&is to handle where they're not appropriate, without having to writeifstatements to conditionally push onto the array) but much easier to understand than previously being split across a load of methods (and it not being clear when which options worked).The Dart SDK override does always still come first (I've compared the results of the new code to the old and ensured the results are all the same) and I will add an override for flutter to handle the remaining case.
For now there won't be a Fuchsia override, since it doesn't look like we actually use the Fuchsia path for anything except finding Flutter/Dart. That means just two overrides, not 6 :-) I'll add a flutter override before closing this case.
DanTup commentedon Nov 26, 2017
dart.flutterSdkPathnow exists so the logic is as posted above with the TODO uncommented.yyoon commentedon Nov 27, 2017
Looks good to me! Much nicer than the original proposal.