Re-land SDK detection changes with support for running from PATH (#6824)

* Revert "Revert "Always use tool/flutter-sdk for devtools_tool + remote FLUTTER_ROOT (#6776)" (#6820)" This reverts commit 79cde66. * Add an env var to force devtools_tool to use SDKs from PATH
flutter · Nov 29, 2023 · 456ff0f · 456ff0f
1 parent b145193
commit 456ff0f
Show file tree

Hide file tree

Showing 14 changed files with 68 additions and 93 deletions.
diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml
@@ -32,7 +32,7 @@ jobs:
         uses: actions/cache@704facf57e6136b1bc63b828d79edcd491f0ee84
         with:
           path: |
-            ./flutter-sdk
+            ./tool/flutter-sdk
           key: flutter-sdk-${{ runner.os }}-${{ needs.flutter-prep.outputs.latest_flutter_candidate }}
 
       - name: tool/ci/bots.sh
@@ -55,7 +55,7 @@ jobs:
         uses: actions/cache@704facf57e6136b1bc63b828d79edcd491f0ee84
         with:
           path: |
-            ./flutter-sdk
+            ./tool/flutter-sdk
           key: flutter-sdk-${{ runner.os }}-${{ needs.flutter-prep.outputs.latest_flutter_candidate }}
       - name: Run tool/ci/bots.sh
         run: ./tool/ci/bots.sh
@@ -65,7 +65,7 @@ jobs:
           wget -qO- https://dcm.dev/pgp-key.public | sudo gpg --dearmor -o /usr/share/keyrings/dcm.gpg
           echo 'deb [signed-by=/usr/share/keyrings/dcm.gpg arch=amd64] https://dcm.dev/debian stable main' | sudo tee /etc/apt/sources.list.d/dart_stable.list
           sudo apt-get update
-          sudo apt-get install dcm=1.11.0-1 
+          sudo apt-get install dcm=1.11.0-1
           sudo chmod +x /usr/bin/dcm
           echo "$(dcm --version)"
       - name: Setup Dart SDK
@@ -92,7 +92,7 @@ jobs:
         uses: actions/cache@704facf57e6136b1bc63b828d79edcd491f0ee84
         with:
           path: |
-            ./flutter-sdk
+            ./tool/flutter-sdk
           key: flutter-sdk-${{ runner.os }}-${{ needs.flutter-prep.outputs.latest_flutter_candidate }}
       - name: tool/ci/package_tests.sh
         env:
@@ -118,7 +118,7 @@ jobs:
         uses: actions/cache@704facf57e6136b1bc63b828d79edcd491f0ee84
         with:
           path: |
-            ./flutter-sdk
+            ./tool/flutter-sdk
           key: flutter-sdk-${{ runner.os }}-${{ needs.flutter-prep.outputs.latest_flutter_candidate }}
       - name: tool/ci/bots.sh
         env:
@@ -145,7 +145,7 @@ jobs:
         uses: actions/cache@704facf57e6136b1bc63b828d79edcd491f0ee84
         with:
           path: |
-            ./flutter-sdk
+            ./tool/flutter-sdk
           key: flutter-sdk-${{ runner.os }}-${{ needs.flutter-prep.outputs.latest_flutter_candidate }}
       - name: tool/ci/bots.sh
         env:
@@ -215,7 +215,7 @@ jobs:
         uses: actions/cache@704facf57e6136b1bc63b828d79edcd491f0ee84
         with:
           path: |
-            ./flutter-sdk
+            ./tool/flutter-sdk
           key: flutter-sdk-${{ runner.os }}-${{ needs.flutter-prep.outputs.latest_flutter_candidate }}
       - name: tool/ci/bots.sh
         env:
@@ -248,7 +248,7 @@ jobs:
         uses: actions/cache@704facf57e6136b1bc63b828d79edcd491f0ee84
         with:
           path: |
-            ./flutter-sdk
+            ./tool/flutter-sdk
           key: flutter-sdk-${{ runner.os }}-${{ needs.flutter-prep.outputs.latest_flutter_candidate }}
       - name: tool/ci/bots.sh
         env:
@@ -269,7 +269,7 @@ jobs:
         uses: actions/cache@704facf57e6136b1bc63b828d79edcd491f0ee84
         with:
           path: |
-            ./flutter-sdk
+            ./tool/flutter-sdk
           key: flutter-sdk-${{ runner.os }}-${{ needs.flutter-prep.outputs.latest_flutter_candidate }}
       - name: tool/ci/benchmark_tests.sh
         run: ./tool/ci/benchmark_tests.sh

diff --git a/.github/workflows/flutter-prep.yaml b/.github/workflows/flutter-prep.yaml
@@ -42,21 +42,21 @@ jobs:
         uses: actions/cache@704facf57e6136b1bc63b828d79edcd491f0ee84
         with:
           path: |
-            ./flutter-sdk
+            ./tool/flutter-sdk
           key: flutter-sdk-${{ runner.os }}-${{ steps.flutter-candidate.outputs.FLUTTER_CANDIDATE }}
 
       - if: ${{ steps.cache-flutter.outputs.cache-hit != 'true' }}
         name: Clone Flutter SDK if none cached
         run: |
-          git clone https://github.com/flutter/flutter.git ./flutter-sdk
-          cd flutter-sdk
+          git clone https://github.com/flutter/flutter.git ./tool/flutter-sdk
+          cd tool/flutter-sdk
           git checkout $LATEST_FLUTTER_CANDIDATE
         env:
           LATEST_FLUTTER_CANDIDATE: ${{ steps.flutter-candidate.outputs.FLUTTER_CANDIDATE }}
 
       - name: Assert that the Latest Flutter Candidate is checked out
         run: |
-          cd flutter-sdk
+          cd tool/flutter-sdk
           HEAD_SHA=$(git rev-parse HEAD)
           LATEST_FLUTTER_CANDIDATE_SHA=$(git rev-list -1 "$LATEST_FLUTTER_CANDIDATE")
           if [ "$HEAD_SHA" != "$LATEST_FLUTTER_CANDIDATE_SHA" ]; then
@@ -68,6 +68,6 @@ jobs:
 
       - name: Setup Flutter SDK
         run: |
-          ./flutter-sdk/bin/flutter config --no-analytics
-          ./flutter-sdk/bin/flutter doctor
-          ./flutter-sdk/bin/cache/dart-sdk/bin/dart --disable-analytics
+          ./tool/flutter-sdk/bin/flutter config --no-analytics
+          ./tool/flutter-sdk/bin/flutter doctor
+          ./tool/flutter-sdk/bin/cache/dart-sdk/bin/dart --disable-analytics
diff --git a/tool/bin/devtools_tool b/tool/bin/devtools_tool
@@ -1,3 +1,11 @@
 #!/bin/bash -e
+
 SCRIPT_DIR=$( cd -- "$( dirname -- "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )
-dart run "$SCRIPT_DIR/devtools_tool.dart" "$@"
+
+if [ ! -z "$DEVTOOLS_TOOL_FLUTTER_FROM_PATH" ]
+then
+	echo Running devtools_tool using Dart/Flutter from PATH because DEVTOOLS_TOOL_FLUTTER_FROM_PATH is set
+	dart run "$SCRIPT_DIR/devtools_tool.dart" "$@"
+else
+	"$SCRIPT_DIR/../flutter-sdk/bin/dart" run "$SCRIPT_DIR/devtools_tool.dart" "$@"
+fi
diff --git a/tool/bin/devtools_tool.bat b/tool/bin/devtools_tool.bat
@@ -1,14 +1,10 @@
 @echo off
 
-pushd "%~dp0"
-dart run ./devtools_tool.dart %*
+IF DEFINED DEVTOOLS_TOOL_FLUTTER_FROM_PATH (
+	echo Running devtools_tool using Dart/Flutter from PATH because DEVTOOLS_TOOL_FLUTTER_FROM_PATH is set
+	dart run %~dp0/devtools_tool.dart %*
+) ELSE (
+	%~dp0/../flutter-sdk/bin/dart run %~dp0/devtools_tool.dart %*
+)
 
-IF %errorlevel% NEQ 0 GOTO :error
-
-popd
-EXIT /B 0
-
-:error
-popd
 EXIT /B %errorlevel%
-
diff --git a/tool/ci/setup.sh b/tool/ci/setup.sh
@@ -31,16 +31,16 @@ if [[ $RUNNER_OS == "Windows" ]]; then
 fi
 
 # Make sure Flutter sdk has been provided
-if [ ! -d "./flutter-sdk" ]; then
-    echo "Expected ./flutter-sdk to exist"
+if [ ! -d "./tool/flutter-sdk" ]; then
+    echo "Expected ./tool/flutter-sdk to exist"
     exit 1;
 fi
 
 # Look in the dart bin dir first, then the flutter one, then the one for the
 # devtools repo. We don't use the dart script from flutter/bin as that script
 # can and does print 'Waiting for another flutter command...' at inopportune
 # times.
-export PATH=`pwd`/flutter-sdk/bin/cache/dart-sdk/bin:`pwd`/flutter-sdk/bin:`pwd`/bin:$PATH
+export PATH=`pwd`/tool/flutter-sdk/bin/cache/dart-sdk/bin:`pwd`/tool/flutter-sdk/bin:`pwd`/bin:$PATH
 
 # Look up the latest flutter candidate (this is the latest flutter version in g3)
 # TODO(https://github.com/flutter/devtools/issues/4591): re-write this script as a

diff --git a/tool/flutter_customer_tests/setup.sh b/tool/flutter_customer_tests/setup.sh
@@ -6,6 +6,8 @@
 root_dir=$(pwd)
 tool_dir="$root_dir/tool/bin"
 export PATH=$PATH:$tool_dir
+# Force devtools_tool to use the current Flutter (which is available on PATH).
+export DEVTOOLS_TOOL_FLUTTER_FROM_PATH=true
 cd tool
 flutter pub get
 devtools_tool pub-get

diff --git a/tool/lib/commands/analyze.dart b/tool/lib/commands/analyze.dart
@@ -21,11 +21,6 @@ class AnalyzeCommand extends Command {
   @override
   Future run() async {
     final sdk = FlutterSdk.current;
-    if (sdk == null) {
-      print('Unable to locate a Flutter sdk.');
-      return 1;
-    }
-
     final log = Logger.standard();
     final repo = DevToolsRepo.getInstance();
     final processManager = ProcessManager();

diff --git a/tool/lib/commands/build.dart b/tool/lib/commands/build.dart
@@ -37,7 +37,7 @@ import '../utils.dart';
 class BuildCommand extends Command {
   BuildCommand() {
     argParser
-      ..addUseFlutterFromPathFlag()
+      ..addUpdateFlutterFlag()
       ..addUpdatePerfettoFlag()
       ..addPubGetFlag()
       ..addBulidModeOption();
@@ -54,8 +54,7 @@ class BuildCommand extends Command {
     final repo = DevToolsRepo.getInstance();
     final processManager = ProcessManager();
 
-    final useLocalFlutter =
-        argResults![BuildCommandArgs.useFlutterFromPath.flagName];
+    final updateFlutter = argResults![BuildCommandArgs.updateFlutter.flagName];
     final updatePerfetto =
         argResults![BuildCommandArgs.updatePerfetto.flagName];
     final runPubGet = argResults![BuildCommandArgs.pubGet.flagName];
@@ -64,7 +63,7 @@ class BuildCommand extends Command {
     final webBuildDir =
         Directory(path.join(repo.devtoolsAppDirectoryPath, 'build', 'web'));
 
-    if (!useLocalFlutter) {
+    if (updateFlutter) {
       logStatus('updating tool/flutter-sdk to the latest flutter candidate');
       await processManager.runProcess(CliCommand.tool('update-flutter-sdk'));
     }

diff --git a/tool/lib/commands/pub_get.dart b/tool/lib/commands/pub_get.dart
@@ -35,12 +35,6 @@ class PubGetCommand extends Command {
 
   @override
   Future run() async {
-    final sdk = FlutterSdk.current;
-    if (sdk == null) {
-      print('Unable to locate a Flutter sdk.');
-      return 1;
-    }
-
     final log = Logger.standard();
     final repo = DevToolsRepo.getInstance();
     final processManager = ProcessManager();

diff --git a/tool/lib/commands/serve.dart b/tool/lib/commands/serve.dart
@@ -57,7 +57,7 @@ class ServeCommand extends Command {
             'Whether to build the DevTools web app before starting the DevTools'
             ' server.',
       )
-      ..addUseFlutterFromPathFlag()
+      ..addUpdateFlutterFlag()
       ..addUpdatePerfettoFlag()
       ..addPubGetFlag()
       ..addBulidModeOption()
@@ -87,16 +87,16 @@ class ServeCommand extends Command {
     final processManager = ProcessManager();
 
     final buildApp = argResults![_buildAppFlag];
-    final useFlutterFromPath =
-        argResults![BuildCommandArgs.useFlutterFromPath.flagName];
+    final updateFlutter = argResults![BuildCommandArgs.updateFlutter.flagName];
     final updatePerfetto =
         argResults![BuildCommandArgs.updatePerfetto.flagName];
     final runPubGet = argResults![BuildCommandArgs.pubGet.flagName];
     final devToolsAppBuildMode =
         argResults![BuildCommandArgs.buildMode.flagName];
 
     final remainingArguments = List.of(argResults!.arguments)
-      ..remove(BuildCommandArgs.useFlutterFromPath.asArg())
+      ..remove(BuildCommandArgs.updateFlutter.asArg())
+      ..remove(BuildCommandArgs.updateFlutter.asArg(negated: true))
       ..remove(BuildCommandArgs.updatePerfetto.asArg())
       ..remove(valueAsArg(_buildAppFlag))
       ..remove(valueAsArg(_buildAppFlag, negated: true))
@@ -125,11 +125,12 @@ class ServeCommand extends Command {
 
     final devToolsBuildLocation =
         path.join(repo.devtoolsAppDirectoryPath, 'build', 'web');
+
     if (buildApp) {
       final process = await processManager.runProcess(
         CliCommand.tool(
           'build'
-          '${useFlutterFromPath ? ' ${BuildCommandArgs.useFlutterFromPath.asArg()}' : ''}'
+          ' ${BuildCommandArgs.updateFlutter.asArg(negated: !updateFlutter)}'
           '${updatePerfetto ? ' ${BuildCommandArgs.updatePerfetto.asArg()}' : ''}'
           ' ${BuildCommandArgs.buildMode.asArg()}=$devToolsAppBuildMode'
           ' ${BuildCommandArgs.pubGet.asArg(negated: !runPubGet)}',

diff --git a/tool/lib/commands/shared.dart b/tool/lib/commands/shared.dart
@@ -41,21 +41,21 @@ extension BuildCommandArgsExtension on ArgParser {
     );
   }
 
-  void addUseFlutterFromPathFlag() {
+  void addUpdateFlutterFlag() {
     addFlag(
-      BuildCommandArgs.useFlutterFromPath.flagName,
-      negatable: false,
-      defaultsTo: false,
-      help: 'Whether to use the Flutter SDK on PATH instead of the Flutter SDK '
-          'contained in the "tool/flutter-sdk" directory.',
+      BuildCommandArgs.updateFlutter.flagName,
+      negatable: true,
+      defaultsTo: true,
+      help: 'Whether to update the Flutter SDK contained in the '
+          '"tool/flutter-sdk" directory.',
     );
   }
 }
 
 enum BuildCommandArgs {
   buildMode('build-mode'),
   pubGet('pub-get'),
-  useFlutterFromPath('use-flutter-from-path'),
+  updateFlutter('update-flutter'),
   updatePerfetto('update-perfetto');
 
   const BuildCommandArgs(this.flagName);

diff --git a/tool/lib/commands/update_flutter_sdk.dart b/tool/lib/commands/update_flutter_sdk.dart
@@ -102,10 +102,6 @@ class UpdateFlutterSdkCommand extends Command {
 
     if (updateFlutterFromPath) {
       final sdk = FlutterSdk.current;
-      if (sdk == null) {
-        print('Unable to locate a Flutter sdk.');
-        return 1;
-      }
 
       log.stdout('Updating local flutter/flutter repository...');
 

diff --git a/tool/lib/model.dart b/tool/lib/model.dart
@@ -99,18 +99,21 @@ class FlutterSdk {
 
   /// The current located Flutter SDK (or `null` if one could not be found).
   ///
-  /// Tries to locate from the running Dart VM, FLUTTER_ROOT or searching PATH.
+  /// Tries to locate from the running Dart VM. If not found, will print a
+  /// warning and use Flutter from PATH.
   static final current = _findSdk();
 
-  /// Return the Flutter SDK.
+  /// Finds the Flutter SDK that contains the Dart VM being used to run this
+  /// script.
   ///
-  /// This can return null if the Flutter SDK can't be found.
-  static FlutterSdk? _findSdk() {
-    // TODO(dantup): Everywhere that calls this just prints and exits - should
-    //  we just make this throw like DevToolsRepo.getInstance();
+  /// Throws if the current VM is not inside a Flutter SDK.
+  static FlutterSdk _findSdk() {
     // Look for it relative to the current Dart process.
     final dartVmPath = Platform.resolvedExecutable;
     final pathSegments = path.split(dartVmPath);
+    // TODO(dantup): Should we add tool/flutter-sdk to the front here, to
+    // ensure we _only_ ever use this one, to avoid potentially updating a
+    // different Flutter if the user runs explicitly with another Flutter?
     final expectedSegments = path.posix.split('bin/cache/dart-sdk/bin/dart');
 
     if (pathSegments.length >= expectedSegments.length) {
@@ -128,31 +131,16 @@ class FlutterSdk {
       }
 
       if (expectedSegments.isEmpty) {
-        return FlutterSdk._(path.joinAll(pathSegments));
+        final flutterSdkRoot = path.joinAll(pathSegments);
+        print('Using Flutter SDK from $flutterSdkRoot');
+        return FlutterSdk._(flutterSdkRoot);
       }
     }
 
-    // Next try FLUTTER_ROOT to allow using a custom flutter (eg. from
-    // `tool/flutter-sdk`) when invoking this without needing to override `PATH`
-    // (it's easier to set override an entire env variable than prepend to one
-    // in some places like the `dart.customDevTools` setting in VS Code).
-    final flutterRootEnv = Platform.environment['FLUTTER_ROOT'];
-    if (flutterRootEnv != null && flutterRootEnv.isNotEmpty) {
-      return FlutterSdk._(flutterRootEnv);
-    }
-
-    // Look to see if we can find the 'flutter' command in the PATH.
-    final whichCommand = Platform.isWindows ? 'where.exe' : 'which';
-    final result = Process.runSync(whichCommand, ['flutter']);
-    if (result.exitCode == 0) {
-      final sdkPath = result.stdout.toString().split('\n').first.trim();
-      // 'flutter/bin'
-      if (path.basename(path.dirname(sdkPath)) == 'bin') {
-        return FlutterSdk._(path.dirname(path.dirname(sdkPath)));
-      }
-    }
-
-    return null;
+    throw Exception(
+      'Unable to locate the Flutter SDK from the current running Dart VM:\n'
+      '${Platform.resolvedExecutable}',
+    );
   }
 
   final String sdkPath;

diff --git a/tool/lib/utils.dart b/tool/lib/utils.dart
@@ -76,10 +76,6 @@ class CliCommand {
     bool throwOnException = true,
   }) {
     final sdk = FlutterSdk.current;
-    if (sdk == null) {
-      throw Exception('Unable to locate a Flutter sdk.');
-    }
-
     return CliCommand._(
       exe: sdk.flutterToolPath,
       args: args.split(' '),