feat: add support for Dart/Flutter SDK at custom `sdkPath` #284

acoutts · 2022-04-27T16:11:35Z

Description

This PR adds a new melos.yaml configuration setting sdk_path which allows someone to specify the path to their dart/flutter sdk instead of relying on what's in the path.
This makes melos compatible with flutter version manager.

Closes #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 refactor
✅ ci -- Build configuration change
📝 docs -- Documentation
🗑️ chore -- Chore

CLAassistant · 2022-04-27T16:11:44Z

All committers have signed the CLA.

mikehardy · 2022-04-27T16:44:47Z

Just a question - shouldn't a version manager reconfigure the PATH so that invoked scripts are already the fvm-correct invoked scripts?

I ask because version managers come and go, there always ends up being more than one, and in the react-native ecosystem at least, after a while there were too many of them to support, and they finally scrapped supporting any of them and said "use your version manager before invoking our scripts, if the version manager is doing it's job on PATH correctly, our scripts will work with your version-manager's binaries..."

acoutts · 2022-04-27T17:24:13Z

I see your point, it's not great to make melos opinionated like that.
But the point of fvm is not to change your path at all and to work locally within each project.
If you have 3 flutter projects on your computer, all 3 can have their own flutter version independent of the one in your path- the idea being when you run fvm flutter ... in any of those projects it will do so with the correct flutter version that project is intended to use.

It appears that just trying to wrap the melos command with fvm flutter pub global run melos doesn't work because melos will simply run flutter ... from path which is not the one we want to happen.

acoutts · 2022-04-27T17:26:58Z

Perhaps a better approach is like what #81 is asking for - instead of use-fvm it could be an optional path to the flutter executable which would make this compatible with any version manager.

mikehardy · 2022-04-27T17:41:08Z

But the point of fvm is not to change your path at all and to work locally within each project.
If you have 3 flutter projects on your computer, all 3 can have their own flutter version independent of the one in your path

🤔 I see what you mean. It quickly becomes a rabbit-hole, in the node ecosystem it's typically to have 'nvm' to alter PATH, and 'auto-nvm' installed in combination with an .nvmrc or .node_version file in a directory and if auto-nvm sees it it will nvm use <version from config file>.

Not sure there even is a good way to do it - it's kind of all hacks 😆 - I think opening up path config vs having an opinion on a specific tool is better though, personally.

I'm not deeply invested in this though - I just wanted to bring in other-ecosystem experience where other-ecosystem finally just threw up their hands and said "we're not supporting specific version managers" because of proliferation. If I can predict the future here, it's that

acoutts · 2022-04-27T18:20:35Z

Ok i've updated it to just pass an optional sdkPath setting either in your melos.yaml or to the bootstrap command.

This way if you use fvm you can just set this in your melos.yaml:

sdk_path: .fvm/flutter_sdk

blaugold

Currently, we assume that melos runs with the same version of Dart as the dart tool that is invoked by Melos:

melos/packages/melos/lib/src/common/utils.dart

Lines 107 to 109 in 1fdff16

    
           Version get currentDartVersion { 
        
             return Version.parse(currentPlatform.version.split(' ')[0]); 
        
           }

currentDartVersion needs to become a function that takes the sdk path, invokes dart --version and parses its output.

blaugold · 2022-04-27T19:08:57Z

packages/melos/lib/src/command_runner/base.dart

@@ -123,6 +123,12 @@ abstract class MelosCommand extends Command<void> {
          'matches the other filters. The included packages skip --ignore '
          'and --since checks.',
    );
+
+    argParser.addOption(
+      filterOptionSdkPath,


I don't think this option should be a filter option. We could add a new GlobalOptions object for which the argParser is setup in setupGlobalOptionsParser and is parsed in parseGlobalOptions.

Thanks for clarifying. What is a filter option supposed to be used for?

The filter options are used to select a subset of the packages in a workspace, for example to only bootstrap Flutter packages and not pure Dart packages you could use melos bootstrap --flutter.

blaugold · 2022-04-27T19:18:57Z

packages/melos/lib/src/commands/bootstrap.dart

-    final execArgs = package.isFlutterPackage
-        ? ['flutter', ...pubGetArgs]
-        : [if (utils.isPubSubcommand()) 'dart', ...pubGetArgs];
+    late final List<String> execArgs;


Why not just this?

final execArgs = [ if (package.isFlutterApp) '${workspace.sdkPath}flutter' else if (utils.isPubSubcommand(sdkPath: workspace.sdkPath)) '${workspace.sdkPath}dart', 'pub', 'get', ]

Of if workspace.sdkPath is nullable:

final execArgs = [ if (package.isFlutterApp) '${workspace.sdkPath ?? ''}flutter' else if (utils.isPubSubcommand(sdkPath: workspace.sdkPath)) '${workspace.sdkPath ?? ''}dart', 'pub', 'get', ]

Simplified in ca2d3a8 (includes tests)

blaugold · 2022-04-27T19:20:23Z

packages/melos/lib/src/common/utils.dart

@@ -46,6 +46,7 @@ const filterOptionDependsOn = 'depends-on';
 const filterOptionNoDependsOn = 'no-depends-on';
 const filterOptionIncludeDependents = 'include-dependents';
 const filterOptionIncludeDependencies = 'include-dependencies';
+const filterOptionSdkPath = 'sdk-path';


filterOptionSdkPath -> globalOptionSdkPath

Renamed in 7a9a915

blaugold · 2022-04-27T19:23:49Z

packages/melos/lib/src/workspace.dart

@@ -98,6 +128,9 @@ class MelosWorkspace {
  late final bool isFlutterWorkspace =
      allPackages.values.any((package) => package.isFlutterPackage);

+  /// Optionally specify sdk path for dart/flutter
+  late final String sdkPath;


This field does not need to be late.
I would prefere it if this field is nullable to signify that the sdk path is not overriden and not the empty string.

Fixed in 79c9973, 9364e2e and 17b102a

sdkPath is nullable to reflect its optional nature

Added a new function to simplify the logic here assembling the final sdk path

Using the path package now to correctly append the paths together (to work on all platforms)

Added tests

blaugold · 2022-04-27T19:26:26Z

packages/melos/lib/src/workspace.dart

-  Future<Map<String, Set<String>>> getDependencyGraph() async {
+  Future<Map<String, Set<String>>> getDependencyGraph({
+    required String sdkPath,
+  }) async {
    final pubDepsExecArgs = ['--style=list', '--dev'];
    final pubListCommandOutput = await Process.run(
      isFlutterWorkspace
          ? 'flutter'


We should also use the sdkPath here and and wherever else we invoke flutter and dart to be consistent.

blaugold · 2022-04-27T19:27:01Z

packages/melos/lib/src/workspace_configs.dart

@@ -418,10 +419,16 @@ class MelosWorkspaceConfig {
      map: yaml,
    );

+    final sdkPath = assertKeyIsA<String?>(
+      key: 'sdk_path',


sdk_path -> sdkPath

Fixed in ebe5de7

acoutts · 2022-04-27T21:55:32Z

I'm experiencing an issue and I can't tell if it's my branch or the v2 release.
There is some kind of race condition because i have to run bootstrap twice on a fresh repo. The first one fails with an error like:

Could not find a file named "pubspec.yaml" in "/builds/M5hecXxM/3/bottle/pay/app/.pub-cache/git/mockito-aa841a3c3762afab2c31486677015ff0a9abc2cf".
BootstrapException: Failed to install.: common at /builds/M5hecXxM/3/bottle/pay/app/packages/common.

But if I run it again it works.
If I use melos 1.5.x (before 2.x), there are no issues either.

blaugold · 2022-04-27T22:21:28Z

We changed to bootstrapping packages in parallel in 2.0.0, which was disabled because of a race condition in pub, which should be fixed in Dart 2.16.0.

blaugold · 2022-04-27T22:23:15Z

It might be that there is another race condition specific to git dependencies.

acoutts · 2022-04-27T22:26:17Z

Interesting- good context to have. In my case i'm on dart 2.17 from the flutter master channel.
Is there a way to toggle off parallel pub gets?

In my project, 4 of my packages rely on the same external git dependency in that error message (same version)- maybe it's something with shared git dependencies when run in parallel.

blaugold · 2022-04-27T22:38:29Z

Interesting- good context to have. In my case i'm on dart 2.17 from the flutter master channel. Is there a way to toggle off parallel pub gets?

Not as a feature. To validate your code in this PR you could temporarily hard-code to return false from canRunPubGetConcurrently:

melos/packages/melos/lib/src/common/utils.dart

Line 118 in 1fdff16

bool get canRunPubGetConcurrently =>

In my project, 4 of my packages rely on the same external git dependency in that error message (same version)- maybe it's something with shared git dependencies when run in parallel.

Yeah, that might be it. Once the repo directory exists in the pub cache, pub might assume that the package is available. Could you open an issue in the pub repo for this?

acoutts · 2022-04-27T23:30:26Z

Not as a feature. To validate your code in this PR you could temporarily hard-code to return false from canRunPubGetConcurrently:

I can confirm this worked. I filed dart-lang/pub#3404 for it.
I temporarily committed it here just so I can get my build pushed tonight, and tomorrow I will add a flag for it as this will probably be broken for other people as well, and will a future dart sdk fix.

acoutts · 2022-04-28T09:13:41Z

Just pushed some refactoring from your feedback- thanks for that.
The only one i'm not 💯 on how to do is how to convert what i created as a filter setting into a global setting.

While we're in there for that we should make one for doing pub in parallel since that is broken for at least my use case (and currently published like that).
At the moment i just return false as you instructed, which resolved my race condition issue.

blaugold · 2022-04-28T09:40:45Z

Thanks for this PR, BTW! I have created a separate PR for the option to disable running pub get in parallel (#285). I can take care of refactoring to a global setting, if you don't mind.

acoutts · 2022-04-28T09:57:40Z

Cheers! That would be great. Want me to add you as a contributor to my fork or what's easiest for you?

blaugold · 2022-04-28T10:01:37Z

If you have "Allow edits from maintainers" on this PR enabled, I should be able to update the feature branch.

acoutts · 2022-04-28T10:06:07Z

I dont see that option for some reason, but I just invited you as a maintainer to the fork in case that works.

acoutts · 2022-04-28T11:18:17Z

Is there a trick to get around this problem? Every time I run bootstrap, i can't use the flutter pub command:

The pubspec.yaml file has changed since the pubspec.lock file was generated, please run "flutter pub get" again.
pub finished with exit code 65

I can do melos exec flutter pub get but of course that reverts the bootstrap magic.

blaugold · 2022-04-28T11:42:51Z

I think you're running into #117. Until this is fixed, a workaround is to run melos exec flutter pub get to let flutter generate temporary files and then bootstrap with melos.

acoutts · 2022-04-28T12:18:23Z

Weird i guess bootstrap never worked for me then! i was using melos exec flutter pub get always but learned while doing this PR that i was using it wrong before.

This is what my lockfiles look like for modules when doing the melos pub get:

  nisaba:
    dependency: transitive
    description:
      path: "packages/nisaba"
      ref: "nisaba-v1.0.0"
      resolved-ref: "1bcf666152369d89810f2ff340c536131809fae6"
      url: redacted
    source: git
    version: "1.0.0"

And then if i run bootstrap the lock file changes to this:

  nisaba:
    dependency: "direct overridden"
    description:
      path: "../nisaba"
      relative: true
    source: path
    version: "1.0.0"

acoutts · 2022-04-28T12:26:32Z

I guess my workaround for now is to just use path: ../nisaba in the pubspec instead of trying to use the git tags system from melos.
It's ok for now as we don't have any external consumers of our packages so paths works. Still sorting out a good app release flow with melos (currently use tags to initiate CI releases but melos would interfere with that).

blaugold · 2022-04-29T08:30:48Z

I just tried to push to bottlepay/melos/main but I must have made some kind of mistake. 😅 I'm trying to fix it right now, though.

blaugold · 2022-04-29T08:41:15Z

@acoutts OK, the tip of bottlepay/melos/main now has the correct changes. Could open a new PR from that branch with the same title as this one?

acoutts · 2022-04-29T10:26:53Z

Opened #288

acoutts changed the title ~~Added support for Flutter Version Manager (FVM) with new optional yaml config~~ Added support for custom sdkPath setting to point to dart/flutter sdk Apr 27, 2022

blaugold reviewed Apr 27, 2022

View reviewed changes

blaugold mentioned this pull request Apr 28, 2022

feat: add command/bootstrap/runPubGetInParallel melos.yaml option #285

Merged

7 tasks

blaugold changed the title ~~Added support for custom sdkPath setting to point to dart/flutter sdk~~ feat: add support for Dart/Flutter SDK at custom sdkPath Apr 28, 2022

blaugold closed this Apr 29, 2022

blaugold force-pushed the main branch from eac6966 to 9870270 Compare April 29, 2022 08:22

	Version get currentDartVersion {
	return Version.parse(currentPlatform.version.split(' ')[0]);
	}

feat: add support for Dart/Flutter SDK at custom sdkPath #284

feat: add support for Dart/Flutter SDK at custom sdkPath #284

Conversation

acoutts commented Apr 27, 2022 • edited Loading

Description

Type of Change

CLAassistant commented Apr 27, 2022 • edited Loading

mikehardy commented Apr 27, 2022

acoutts commented Apr 27, 2022

acoutts commented Apr 27, 2022

mikehardy commented Apr 27, 2022

acoutts commented Apr 27, 2022

blaugold left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blaugold Apr 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acoutts Apr 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acoutts commented Apr 27, 2022

blaugold commented Apr 27, 2022 • edited Loading

blaugold commented Apr 27, 2022

acoutts commented Apr 27, 2022 • edited Loading

blaugold commented Apr 27, 2022 • edited Loading

acoutts commented Apr 27, 2022

acoutts commented Apr 28, 2022 • edited Loading

blaugold commented Apr 28, 2022

acoutts commented Apr 28, 2022

blaugold commented Apr 28, 2022

acoutts commented Apr 28, 2022

acoutts commented Apr 28, 2022

blaugold commented Apr 28, 2022

acoutts commented Apr 28, 2022

acoutts commented Apr 28, 2022

blaugold commented Apr 29, 2022

blaugold commented Apr 29, 2022

acoutts commented Apr 29, 2022

feat: add support for Dart/Flutter SDK at custom `sdkPath` #284

feat: add support for Dart/Flutter SDK at custom `sdkPath` #284

acoutts commented Apr 27, 2022 •

edited

Loading

CLAassistant commented Apr 27, 2022 •

edited

Loading

blaugold Apr 27, 2022 •

edited

Loading

acoutts Apr 28, 2022 •

edited

Loading

blaugold commented Apr 27, 2022 •

edited

Loading

acoutts commented Apr 27, 2022 •

edited

Loading

blaugold commented Apr 27, 2022 •

edited

Loading

acoutts commented Apr 28, 2022 •

edited

Loading