-
Notifications
You must be signed in to change notification settings - Fork 340
Update the Dart SDK path finding logic for fuchsia #437
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
Conversation
This changes the logic for finding the correct Dart SDK location for Fuchsia. Also, now the user-defined "dart.sdkPath" setting takes precedence, in case the auto-detected Dart SDK is not the desired one to use.
|
@DanTup BTW, the location might change again it seems, so please don't merge this yet. |
|
Ok, just let me know when it's good to go (and whether it's best to release immediately after merging). |
|
Thanks for waiting :) So the Dart location ended up remaining the same as before. I reverted that portion from the PR. Also, it updates the flutter location under fuhsia. Because the sdk location is reverted back, the analysis is all working again and we're unblocked. This doesn't need to be released immediately, but would be nice to be included in the next release. |
|
Is it valid to use a standard Dart SDK for Fuchsia dev? I was under the impression (but this may be incorrect) that the Dart SDK shipping in Flutter was patched in someway, so a standard one wouldn't work. If this is true and the same for Fuchsia, then I wonder if using userDefinedSdkPath might be incorrect? Eg., I use userDefinedSdkPath to jump between Stable/Dev SDKs when testing (#222 (comment)), would this interfere with Fuchsia/Flutter dev? If so, would I be better having separate overrides for them? (cc @devoncarew - maybe you know the details about whether Flutter's Dart SDK is actually custom or not) |
|
Flutter's is using a mostly stock version of the Dart SDK. They pin it at a specific version - it's generally slightly older than the last dev sdk. A few new libraries are added into the For regular Flutter development, you are better off using the analysis server from flutter's version of the dart sdk, but that above mechanism let's most dart sdks work with flutter code. Not sure if that's true for fuchsia development or not - |
In Fuchsia, the situation is similar to what @devoncarew said, but we generally need more recent version of Dart SDK than the one Flutter uses, as we make fuchsia-specific changes to the sdk relatively frequently. Therefore, we now use the latest prebuilt Dart SDK, instead of a pinned version, which gets automatically downloaded when we update our source code. 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. Also, many of us thought the
Do you use set the sdk paths in the user settings or the workspace settings? I would think that's what the workspace settings in VSCode are for, and setting the right set of sdks under the fuchsia workspace should generally work. Even when setting it in the user settings, I would imagine that adding the fuchsia's Dart SDK locations as the options would just work. |
I set it in the user settings, since I want it set globally (and to keep out of source control since it contains paths). I'm slightly conflicted on making the same override work for them all - if I switch to an older Dart SDK for testing Dart stuff, probably doesn't make sense for it to just break my Flutter stuff because it's a standard SDK and Flutter needs a patched one. So I think it'd make sense for flutter to have its own override. But then it feels weird if Fuchsia doesn't have its own? I'm wondering it it'd make most sense to have three separate overrides (Dart, Flutter, Fuchsia) so the user is in complete control. That way the Dart SDK override can point at a stock SDK, the Flutter one can point at a Flutter-specific one and for consistency, Fuchsia gets its own (and probably there will be a runtime override to support WSL too, but that's for another day!). I can merge and ship this PR for now (will likely be Thursday - currently I'm not set up for publishing from my MacBook and the PC is disconnected - need to fix one of these soon though!). I think some of this SDK detection code could do with tidying up (it's grown over time, and I think it's become a bit cluttered and hard to follow) so trying to decide on the best way for this to work to avoid re-working it again afterwards. Unless anyone has any strong opinions for why a shared override setting is better, probably I'll keep them separate. |
|
Both approaches sound okay to me. I see the benefit of having separate overrides when you're using the user settings as you mentioned. |
|
This change was pushed out in v2.4.4. I've also raised two new cases: #440 to support separate Flutter + Fuchsia overrides (since the main override might just point at a normal Dart SDK) |
|
Thanks! |
This changes the logic for finding the correct Dart SDK location for
Fuchsia.
Also, now the user-defined "dart.sdkPath" setting takes precedence, in
case the auto-detected Dart SDK is not the desired one to use.