Skip to content

Conversation

@yyoon
Copy link
Contributor

@yyoon yyoon commented Nov 7, 2017

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.

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.
@yyoon
Copy link
Contributor Author

yyoon commented Nov 8, 2017

@DanTup BTW, the location might change again it seems, so please don't merge this yet.

@DanTup
Copy link
Member

DanTup commented Nov 8, 2017

Ok, just let me know when it's good to go (and whether it's best to release immediately after merging).

@yyoon
Copy link
Contributor Author

yyoon commented Nov 9, 2017

Thanks for waiting :)

So the Dart location ended up remaining the same as before. I reverted that portion from the PR.
Still, this PR makes the dart.sdkPath to take priority, because it would be more natural. Our team members tried to temporarily override the sdk path by setting that property and then surprised by the fact that it's only used after the auto detection is failed.

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.

@DanTup
Copy link
Member

DanTup commented Nov 12, 2017

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)

@devoncarew
Copy link
Contributor

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 dart: namespace via a special analyzer mechanism: https://github.com/flutter/engine/blob/6c5315c3a1d72ee2aae520e05c1ebf0f6794ae0e/sky/packages/sky_engine/lib/_embedder.yaml.

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 -

@yyoon
Copy link
Contributor Author

yyoon commented Nov 14, 2017

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?

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 dart.sdkPath would override the auto-detection and were surprised when we realized that it's not the case. Because there was no way to force override the dart sdk path with the setting, we needed to build a patched version of Dart-Code to fix it temporarily, which is not ideal.

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?

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.

@DanTup
Copy link
Member

DanTup commented Nov 14, 2017

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

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.

@yyoon
Copy link
Contributor Author

yyoon commented Nov 14, 2017

Both approaches sound okay to me. I see the benefit of having separate overrides when you're using the user settings as you mentioned.

@DanTup
Copy link
Member

DanTup commented Nov 16, 2017

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)
#441 to extend sdkContainer and Flutter + Fuchsia equivs to support pointing at a single folder - this will allow you to quickly switch between a "normal" and a single dev SDK without opening the settings file and pasting paths in (I think this will support the use case mentioned above pretty well)

@yyoon
Copy link
Contributor Author

yyoon commented Nov 16, 2017

Thanks!

@yyoon yyoon deleted the fuchsia-dart-update branch November 16, 2017 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants