feat: add support for Dart/Flutter SDK at custom sdkPath
#288
Conversation
|
Something seems off - it seems to be doing |
|
Yeah, and some tests are failing too. I'll take look what's going on here. |
|
I have one failed test which seems to be from flutter 2.13 where there are several breaking changes, in this case in HttpClient where they added new overrides. That package needs to release a new major version to support flutter 2.13 and onward. Otherwise i fixed the other failing test with b006a68 and that resolved my issue. |
|
Must be something that changed in master but even if i set |
|
This is the assumption breaking the flutter logic: Not all flutter apps have to use main.dart and in fact it can be whatever the developer wants and can pass to the flutter command via the target parameter. |
|
How strange, even though I set this in my melos.yaml: I still had the race condition issues. The only way I could make it work again for me was to override it to return false from |
|
Doh! I did not have my setting yaml structured correctly. Turning off parallelism fixed mine, and ad3e80e fixes the flutter package identification issue. |
|
Another edge case found: if the sdkPath is invalid in the melos yaml, but is valid via what's passed from the CLI command flag ( |
…ation the global command sdk path and instead always tried to validate the workspace sdk first
|
Fixed that in 9635af4 including a test. I had to modify the workspace validate function to take into account the command sdk path as well because commandSdkPath (passed at command line) has precedence over the workspace sdk setting. |
|
There seems to be something wrong with how the global option for sdkPath was implemented, digging into it now. |
The assumption that a Flutter app package always has a
I changed the value that signals that the user wants to use the SDK on the PATH from the empty string to "auto". That's probably causing some confusion. Using the empty string to convey special meaning is an antipattern, IMHO. A meaningful value makes the command line invocation more readable. I don't think 9635af4 and b006a68 are necessary if you use |
|
The issue seems to be in how globalOptions works when we use the melos commands in the melos.yaml. If I run this command: Then If I put the same I ended up finding that i have to add the |
That's actually a good point. There is a good case for adding the If an SDK path is specified, we should also add it to the path when running scripts, so that when |
|
Good idea on the environment variable and i agree, sounds like a good way to make it cascade down. |
…kage" This reverts commit ad3e80e.
|
@acoutts I've added the |
|
Looks awesome! Thanks a lot for your help.
Do you think it's alright that the command line will override the environment variable, or should it be the other way around? That way if people utilize the command line flag, they can set the flag in CI and they won't have to modify their commands. To test this,
|
|
My gut feeling is that command line arguments should generally have the highest precedence because they are the most granular way for specifying an option. It's what most tools do, so it's also what most devs expect. It might make sense in this case, but it's not obvious to me in what situation. Do you have a concrete use case? |
|
No other concrete example. You’re right here as well, if someone wants to use the environment variable they could pass the command like argument and check the environment variable in a shell command. |
|
@Salakar This PR is ready to review, when you get a chance. |
Sorry I'm a bit busy with Google I/O things at the moment 😫 I'm happy for this to go without my review unless you explicitly want me to review? |
|
👍 No problem. I'm fine with merging it without another review. |
|
@blaugold after bumping to melos 2.2.0, my commands that worked on my fork with The commit that works for me: To test:
If i try the exact same thing on 2.2.0/2.3.0, it instead tries to use the |
|
When I set an invalid |
|
Aha you're right, so to further explain,
melos.yaml command: analyze-ci: flutter pub run melos exec --sdk-path auto -- "dart analyze"Running this will fail: But going into the terminal and pasting that command will work. So the regression between my last commit on my fork and what got merged, is that it's broken for commands put in the melos.yaml file. |
|
You're right, there was a change in Do you have to work around a limitation where the analyze: melos exec -- "dart analyze"Or run the script in CI like so: melos run analyze --sdk-path autoEdit: Actually, |
|
I see - that's a good rationale there I agree. 🤔 Hard coding the path via the environment variable does work, but now i wonder when does the auto setting work and when does it not? |
|
#296 fixes the issue with the |
|
works!! thank you so much for your help. |
Description
This PR adds support for specifying the path to a Dart/Flutter SDK, instead of relying on what's in the path.
This makes melos compatible with flutter version manager (fvm).
The SDK path can be specified though the following mechanisms, in order of highest to lowest precedence:
--sdk-pathcommand line optionMELOS_SDK_PATHenvironment variablesdkPathinmelos.yamlCloses #81
Type of Change
feat-- New feature (non-breaking change which adds functionality)fix-- Bug fix (non-breaking change which fixes an issue)!-- Breaking change (fix or feature that would cause existing functionality to change)refactor-- Code refactorci-- Build configuration changedocs-- Documentationchore-- Chore