From 899eb599a885bd3249da5b578af5c0df042d57d1 Mon Sep 17 00:00:00 2001 From: Sigurd Meldgaard Date: Tue, 1 Oct 2024 15:47:16 +0200 Subject: [PATCH] Find .dart_tool/package_config.json by looking in parent directories (#8347) --- .../release_notes/NEXT_RELEASE_NOTES.md | 3 + .../lib/src/service/service_manager.dart | 2 + .../lib/src/extensions/extension_manager.dart | 52 +++++--------- packages/devtools_shared/pubspec.yaml | 2 +- .../test/helpers/extension_test_manager.dart | 70 +++++++++++++++++-- .../helpers/extension_test_manager_test.dart | 16 ++--- .../server/devtools_extensions_api_test.dart | 40 +++++++---- 7 files changed, 125 insertions(+), 60 deletions(-) diff --git a/packages/devtools_app/release_notes/NEXT_RELEASE_NOTES.md b/packages/devtools_app/release_notes/NEXT_RELEASE_NOTES.md index c4958bfa367..53f59c93396 100644 --- a/packages/devtools_app/release_notes/NEXT_RELEASE_NOTES.md +++ b/packages/devtools_app/release_notes/NEXT_RELEASE_NOTES.md @@ -18,6 +18,9 @@ with WebAssembly. - [#8270](https://github.com/flutter/devtools/pull/8270) ![Wasm opt-in setting](images/wasm_setting.png "DevTools setting to opt into wasm.") +* Added support for loading extensions in pub workspaces + [8347](https://github.com/flutter/devtools/pull/8347). + ## Inspector updates - Added a setting to the Flutter Inspector controls that allows users to opt-in to the newly redesigned Flutter Inspector. - [#8342](https://github.com/flutter/devtools/pull/8342) diff --git a/packages/devtools_app_shared/lib/src/service/service_manager.dart b/packages/devtools_app_shared/lib/src/service/service_manager.dart index 0963a6f5bf0..b1dbd285dfd 100644 --- a/packages/devtools_app_shared/lib/src/service/service_manager.dart +++ b/packages/devtools_app_shared/lib/src/service/service_manager.dart @@ -652,6 +652,8 @@ class ServiceManager { // TODO(kenz): consider caching this value for the duration of the VM service // connection. + // TODO(https://github.com/dart-lang/sdk/issues/56784) there should be a + // vm-service call doing this. /// Returns the root package directory for the main isolate. /// /// If a non-null value is returned, the value will be a file URI String and diff --git a/packages/devtools_shared/lib/src/extensions/extension_manager.dart b/packages/devtools_shared/lib/src/extensions/extension_manager.dart index ecc9a5f4528..b2c2f1196a0 100644 --- a/packages/devtools_shared/lib/src/extensions/extension_manager.dart +++ b/packages/devtools_shared/lib/src/extensions/extension_manager.dart @@ -105,13 +105,6 @@ class ExtensionsManager { if (root.toString() == rootFileUriString) continue; await _addExtensionsForRoot( - // TODO(https://github.com/flutter/devtools/issues/7944): this logic - // assumes that the .dart_tool folder containing the - // package_config.json file is in the same directory as the - // pubspec.yaml file (since `dartToolingDaemon.getProjectRoots` - // returns all directories within the IDE workspace roots that have - // a pubspec.yaml file). This may be an incorrect assumption for - // pub workspaces. root.toString(), logs: logs, parsingErrors: parsingErrors, @@ -142,27 +135,16 @@ class ExtensionsManager { }) async { _assertUriFormat(rootFileUriString); final List extensions; - try { - // TODO(https://github.com/flutter/devtools/issues/7944): this assumes - // that the .dart_tool/package_config.json file is in the package root, - // which may be an incorrect assumption for pub workspaces. - final packageConfigPath = path.posix.join( - rootFileUriString, - '.dart_tool', - 'package_config.json', - ); - extensions = await findExtensions( - 'devtools', - packageConfig: Uri.parse(packageConfigPath), - ); - logs.add( - 'ExtensionsManager._addExtensionsForRoot find extensions for ' - 'config: $packageConfigPath, result: ' - '${extensions.map((e) => e.package).toList()}', - ); - } catch (e) { - rethrow; - } + final packageConfigPath = findPackageConfig(Uri.parse(rootFileUriString)); + extensions = await findExtensions( + 'devtools', + packageConfig: packageConfigPath, + ); + logs.add( + 'ExtensionsManager._addExtensionsForRoot find extensions for ' + 'config: $packageConfigPath, result: ' + '${extensions.map((e) => e.package).toList()}', + ); for (final extension in extensions) { final config = extension.config; @@ -188,12 +170,14 @@ class ExtensionsManager { final extensionConfig = DevToolsExtensionConfig.parse({ ...config, DevToolsExtensionConfig.extensionAssetsPathKey: location, - // TODO(https://github.com/flutter/devtools/issues/7944): for pub - // workspaces, we may want to store the devtools_options.yaml at the - // same location as the workspace's .dart_tool/package_config.json - // file. - DevToolsExtensionConfig.devtoolsOptionsUriKey: - path.join(rootFileUriString, devtoolsOptionsFileName), + // The [packageConfigPath] will look like + // 'pkg/.dart_tool/package_config.json' so we will store the + // devtools_options.yaml file at the 'pkg' root: + // 'pkg/devtools_options.yaml'. + DevToolsExtensionConfig.devtoolsOptionsUriKey: path.url.join( + path.url.dirname(path.url.dirname(packageConfigPath.toString())), + devtoolsOptionsFileName, + ), DevToolsExtensionConfig.isPubliclyHostedKey: isPubliclyHosted, DevToolsExtensionConfig.detectedFromStaticContextKey: staticContext.toString(), diff --git a/packages/devtools_shared/pubspec.yaml b/packages/devtools_shared/pubspec.yaml index d69b0b58bea..c2ed4905845 100644 --- a/packages/devtools_shared/pubspec.yaml +++ b/packages/devtools_shared/pubspec.yaml @@ -12,7 +12,7 @@ dependencies: args: ^2.4.2 collection: ^1.15.0 dtd: ^2.2.0 - extension_discovery: ^2.0.0 + extension_discovery: ^2.1.0 meta: ^1.9.1 path: ^1.8.0 shelf: ^1.1.0 diff --git a/packages/devtools_shared/test/helpers/extension_test_manager.dart b/packages/devtools_shared/test/helpers/extension_test_manager.dart index 31271d5a03c..ede07c296bd 100644 --- a/packages/devtools_shared/test/helpers/extension_test_manager.dart +++ b/packages/devtools_shared/test/helpers/extension_test_manager.dart @@ -47,6 +47,12 @@ class ExtensionTestManager { /// .dart_tool/ # Generated from 'pub get' /// package_config.json # Generated from 'pub get' /// pubspec.yaml + /// workspace_root/ + /// .dart_tool/ # Generated from 'pub get' + /// package_config.json # Generated from 'pub get' + /// pubspec.yaml # Defines workspace + /// wokspace_member + /// pubspec.yaml /// extensions/ /// static_extension_1/ /// extension/ @@ -90,7 +96,7 @@ class ExtensionTestManager { expect(testDirectoryContents.length, 2); final packageRoots = Directory.fromUri(packagesRootUri).listSync().whereType(); - expect(packageRoots.length, 3); + expect(packageRoots.length, 4); for (final packageRoot in packageRoots) { expect(File(p.join(packageRoot.path, 'pubspec.yaml')).existsSync(), true); @@ -129,6 +135,12 @@ class ExtensionTestManager { /// .dart_tool/ # Generated from 'pub get' /// package_config.json # Generated from 'pub get' /// pubspec.yaml + /// workspace_root/ + /// .dart_tool/ # Generated from 'pub get' + /// package_config.json # Generated from 'pub get' + /// pubspec.yaml # Defines workspace + /// workspace_member/ + /// pubspec.yaml void _setupPackages({ required bool includeDependenciesWithExtensions, required bool includeBadExtension, @@ -152,6 +164,45 @@ class ExtensionTestManager { includeDependenciesWithExtensions: includeDependenciesWithExtensions, ), ); + setupWorkspace( + createTestPackageFrom( + workspaceMember, + includeDependenciesWithExtensions: includeDependenciesWithExtensions, + ), + ); + } + + void setupWorkspace(TestPackage package) { + File( + p.join( + testDirectory.path, + 'packages', + 'workspace_root', + 'pubspec.yaml', + ), + ) + ..createSync(recursive: true) + ..writeAsStringSync(''' +name: _ +environment: + sdk: "^3.5.0" +workspace: + - ${package.name} +'''); + File( + p.join( + testDirectory.path, + 'packages', + 'workspace_root', + package.name, + 'pubspec.yaml', + ), + ) + ..createSync(recursive: true) + ..writeAsStringSync(''' +${package.pubspecContent} +resolution: workspace +'''); } /// extensions/ @@ -239,6 +290,7 @@ TestPackage createTestPackageFrom( name: originalPackage.name, dependencies: includeDependenciesWithExtensions ? originalPackage.dependencies : [], + pathToExtensions: originalPackage.pathToExtensions, ); } @@ -258,6 +310,11 @@ final otherRoot2Package = TestPackage( name: 'other_root_2', dependencies: [newerStaticExtension1Package], ); +final workspaceMember = TestPackage( + name: 'workspace_member', + dependencies: [staticExtension1Package], + pathToExtensions: '../../../extensions', +); final driftPackage = TestPackageWithExtension( name: 'drift', @@ -355,15 +412,20 @@ environment: } class TestPackage { - const TestPackage({required this.name, required this.dependencies}); + const TestPackage({ + required this.name, + required this.dependencies, + this.pathToExtensions = '../../extensions', + }); final String name; final List dependencies; + final String pathToExtensions; String get pubspecContent => ''' name: $name environment: - sdk: ">=3.4.0-282.1.beta <4.0.0" + sdk: "^3.5.0" dependencies: ${_dependenciesAsString()} '''; @@ -378,7 +440,7 @@ ${_dependenciesAsString()} sb ..writeln() // Add a new line for the path dependency. ..writeln( - ' path: ../../extensions/${dep.relativePathFromExtensions}', + ' path: $pathToExtensions/${dep.relativePathFromExtensions}', ); } } diff --git a/packages/devtools_shared/test/helpers/extension_test_manager_test.dart b/packages/devtools_shared/test/helpers/extension_test_manager_test.dart index d6ef4ba62de..bef5499bedb 100644 --- a/packages/devtools_shared/test/helpers/extension_test_manager_test.dart +++ b/packages/devtools_shared/test/helpers/extension_test_manager_test.dart @@ -179,7 +179,7 @@ materialIconCodePoint: 58634 ''' name: my_app environment: - sdk: ">=3.4.0-282.1.beta <4.0.0" + sdk: "^3.5.0" dependencies: drift: 2.16.0 provider: 6.1.2 @@ -198,7 +198,7 @@ dependencies: ''' name: my_app environment: - sdk: ">=3.4.0-282.1.beta <4.0.0" + sdk: "^3.5.0" dependencies: drift: 2.16.0 provider: 6.1.2 @@ -219,7 +219,7 @@ dependencies: ''' name: other_root_1 environment: - sdk: ">=3.4.0-282.1.beta <4.0.0" + sdk: "^3.5.0" dependencies: static_extension_1: path: ../../extensions/static_extension_1 @@ -238,7 +238,7 @@ dependencies: ''' name: other_root_2 environment: - sdk: ">=3.4.0-282.1.beta <4.0.0" + sdk: "^3.5.0" dependencies: static_extension_1: path: ../../extensions/newer/static_extension_1 @@ -257,7 +257,7 @@ dependencies: ''' name: my_app environment: - sdk: ">=3.4.0-282.1.beta <4.0.0" + sdk: "^3.5.0" dependencies: ''', @@ -271,7 +271,7 @@ dependencies: ''' name: my_app environment: - sdk: ">=3.4.0-282.1.beta <4.0.0" + sdk: "^3.5.0" dependencies: ''', @@ -285,7 +285,7 @@ dependencies: ''' name: other_root_1 environment: - sdk: ">=3.4.0-282.1.beta <4.0.0" + sdk: "^3.5.0" dependencies: ''', @@ -299,7 +299,7 @@ dependencies: ''' name: other_root_2 environment: - sdk: ">=3.4.0-282.1.beta <4.0.0" + sdk: "^3.5.0" dependencies: ''', diff --git a/packages/devtools_shared/test/server/devtools_extensions_api_test.dart b/packages/devtools_shared/test/server/devtools_extensions_api_test.dart index 7e14ae77d08..c88743b62e1 100644 --- a/packages/devtools_shared/test/server/devtools_extensions_api_test.dart +++ b/packages/devtools_shared/test/server/devtools_extensions_api_test.dart @@ -264,7 +264,7 @@ void _verifyAllExtensions( bool includeRuntime = true, }) { if (includeRuntime) { - expect(extensionsManager.devtoolsExtensions.length, 9); + expect(extensionsManager.devtoolsExtensions.length, 11); final runtimeExtensions = extensionsManager.devtoolsExtensions .where((ext) => !ext.detectedFromStaticContext) .toList(); @@ -285,60 +285,74 @@ void _verifyExpectedRuntimeExtensions( _verifyExtension( extensions[0], extensionPackage: driftPackage, - detectedFromPackage: 'my_app', + detectedFromPath: 'my_app', fromStaticContext: false, ); _verifyExtension( extensions[1], extensionPackage: providerPackage, - detectedFromPackage: 'my_app', + detectedFromPath: 'my_app', fromStaticContext: false, ); _verifyExtension( extensions[2], extensionPackage: staticExtension1Package, - detectedFromPackage: 'my_app', + detectedFromPath: 'my_app', fromStaticContext: false, ); } void _verifyExpectedStaticExtensions(List extensions) { - expect(extensions.length, 6); + expect(extensions.length, 8); extensions.sort(); _verifyExtension( extensions[0], extensionPackage: driftPackage, - detectedFromPackage: 'my_app', + detectedFromPath: 'my_app', fromStaticContext: true, ); _verifyExtension( extensions[1], extensionPackage: providerPackage, - detectedFromPackage: 'my_app', + detectedFromPath: 'my_app', fromStaticContext: true, ); _verifyExtension( extensions[2], extensionPackage: newerStaticExtension1Package, - detectedFromPackage: 'other_root_2', + detectedFromPath: 'other_root_2', fromStaticContext: true, ); _verifyExtension( extensions[3], extensionPackage: staticExtension1Package, - detectedFromPackage: 'my_app', + detectedFromPath: 'my_app', fromStaticContext: true, ); _verifyExtension( extensions[4], extensionPackage: staticExtension1Package, - detectedFromPackage: 'other_root_1', + detectedFromPath: 'other_root_1', fromStaticContext: true, ); + // This extension gets added once by the workspace root pubspec.yaml, and once + // by the workspace member pubspec.yaml. _verifyExtension( extensions[5], + extensionPackage: staticExtension1Package, + detectedFromPath: 'workspace_root', + fromStaticContext: true, + ); + _verifyExtension( + extensions[6], + extensionPackage: staticExtension1Package, + detectedFromPath: 'workspace_root', + fromStaticContext: true, + ); + _verifyExtension( + extensions[7], extensionPackage: staticExtension2Package, - detectedFromPackage: 'other_root_1', + detectedFromPath: 'other_root_1', fromStaticContext: true, ); } @@ -346,7 +360,7 @@ void _verifyExpectedStaticExtensions(List extensions) { void _verifyExtension( DevToolsExtensionConfig ext, { required TestPackageWithExtension extensionPackage, - required String detectedFromPackage, + required String detectedFromPath, required bool fromStaticContext, }) { expect(ext.name, extensionPackage.name); @@ -387,7 +401,7 @@ void _verifyExtension( expect( ext.devtoolsOptionsUri, endsWith( - p.join('packages', detectedFromPackage, devtoolsOptionsFileName), + p.join('packages', detectedFromPath, devtoolsOptionsFileName), ), ); expect(ext.detectedFromStaticContext, fromStaticContext);