From 30bfc439fedba1ee3daadcf542f1483479bc4909 Mon Sep 17 00:00:00 2001 From: Sigurd Meldgaard Date: Mon, 2 Dec 2024 13:21:48 +0100 Subject: [PATCH] Delete directories between workspace package and workspace root (#4446) --- lib/src/entrypoint.dart | 34 +++++++++++- lib/src/package.dart | 37 +++++++++++++ pubspec.lock | 14 ++--- test/workspace_test.dart | 110 ++++++++++++++++++++++++++++++++------- 4 files changed, 168 insertions(+), 27 deletions(-) diff --git a/lib/src/entrypoint.dart b/lib/src/entrypoint.dart index 7c8da54bc..7e7354e66 100644 --- a/lib/src/entrypoint.dart +++ b/lib/src/entrypoint.dart @@ -1335,15 +1335,45 @@ See https://dart.dev/go/sdk-constraint /// Remove any `pubspec.lock` or `.dart_tool/package_config.json` files in /// workspace packages that are not the root package. /// + /// Also remove from directories between the workspace package and the + /// workspace root, to prevent stray package configs from shadowing the shared + /// workspace package config. + /// /// This is to avoid surprises if a package is turned into a workspace member /// but still has an old package config or lockfile. void _removeStrayLockAndConfigFiles() { + final visited = { + // By adding this to visited we will never go above the workspaceRoot.dir. + p.canonicalize(workspaceRoot.dir), + }; + var deletedAny = false; for (final package in workspaceRoot.transitiveWorkspace) { if (package.pubspec.resolution == Resolution.workspace) { - deleteEntry(p.join(package.dir, 'pubspec.lock')); - deleteEntry(p.join(package.dir, '.dart_tool', 'package_config.json')); + for (final dir in parentDirs(package.dir)) { + if (!visited.add(p.canonicalize(dir))) { + // No reason to delete from the same directory twice. + break; + } + void deleteIfPresent(String path, String type) { + fileExists(path); + log.warning('Deleting old $type: `$path`.'); + deleteEntry(path); + deletedAny = true; + } + + deleteIfPresent(p.join(dir, 'pubspec.lock'), 'lock-file'); + deleteIfPresent( + p.join(dir, '.dart_tool', 'package_config.json'), + 'package config', + ); + } } } + if (deletedAny) { + log.warning( + 'See https://dart.dev/go/workspaces-stray-files for details.', + ); + } } /// Returns a list of changes to constraints of workspace pubspecs updated to diff --git a/lib/src/package.dart b/lib/src/package.dart index be6ebaad1..4edf7e524 100644 --- a/lib/src/package.dart +++ b/lib/src/package.dart @@ -435,9 +435,12 @@ $symlinkResolvedDir => ${p.canonicalize(symlinkResolvedParent)} /// * If a package name occurs twice. /// * If two packages in the workspace override the same package name. /// * A workspace package is overridden. +/// * A pubspec not included in the workspace exists in a directory +/// between the root and a workspace package. void validateWorkspace(Package root) { if (root.workspaceChildren.isEmpty) return; + /// Maps the `p.canonicalize`d dir of each workspace-child to its parent. final includedFrom = {}; final stack = [root]; @@ -500,6 +503,40 @@ Consider removing one of the overrides. Cannot override workspace packages. Package `$override` at `${overriddenWorkspacePackage.presentationDir}` is overridden in `${package.pubspecPath}`. +'''); + } + } + } + + // Check for pubspec.yaml files between the root and any workspace package. + final visited = { + // By adding this to visited we will never go above the workspaceRoot.dir. + p.canonicalize(root.dir), + }; + for (final package in root.transitiveWorkspace) { + // Run through all parent directories until we meet another workspace + // package. + for (final dir in parentDirs(package.dir).skip(1)) { + // Stop if we meet another package directory. + if (includedFrom.containsKey(p.canonicalize(dir))) { + break; + } + if (!visited.add(p.canonicalize(dir))) { + // We have been here before. + break; + } + final pubspecCandidate = p.join(dir, 'pubspec.yaml'); + if (fileExists(pubspecCandidate)) { + fail(''' +The file `$pubspecCandidate` is located in a directory between the workspace root at +`${root.dir}` and a workspace package at `${package.dir}`. But is not a member of the +workspace. + +This blocks the resolution of the package at `${package.dir}`. + +Consider removing it. + +See https://dart.dev/go/workspaces-stray-files for details. '''); } } diff --git a/pubspec.lock b/pubspec.lock index 5e5212c0d..9539fb96f 100644 --- a/pubspec.lock +++ b/pubspec.lock @@ -5,23 +5,23 @@ packages: dependency: transitive description: name: _fe_analyzer_shared - sha256: "45cfa8471b89fb6643fe9bf51bd7931a76b8f5ec2d65de4fb176dba8d4f22c77" + sha256: "16e298750b6d0af7ce8a3ba7c18c69c3785d11b15ec83f6dcd0ad2a0009b3cab" url: "https://pub.dev" source: hosted - version: "73.0.0" + version: "76.0.0" _macros: dependency: transitive description: dart source: sdk - version: "0.3.2" + version: "0.3.3" analyzer: dependency: "direct main" description: name: analyzer - sha256: "4959fec185fe70cce007c57e9ab6983101dbe593d2bf8bbfb4453aaec0cf470a" + sha256: "1f14db053a8c23e260789e9b0980fa27f2680dd640932cae5e1137cce0e46e1e" url: "https://pub.dev" source: hosted - version: "6.8.0" + version: "6.11.0" args: dependency: "direct main" description: @@ -194,10 +194,10 @@ packages: dependency: transitive description: name: macros - sha256: "0acaed5d6b7eab89f63350bccd82119e6c602df0f391260d0e32b5e23db79536" + sha256: "1d9e801cd66f7ea3663c45fc708450db1fa57f988142c64289142c9b7ee80656" url: "https://pub.dev" source: hosted - version: "0.1.2-main.4" + version: "0.1.3-main.0" matcher: dependency: transitive description: diff --git a/test/workspace_test.dart b/test/workspace_test.dart index b1336d453..ada3677b1 100644 --- a/test/workspace_test.dart +++ b/test/workspace_test.dart @@ -809,7 +809,40 @@ foo:foomain''', ); }); - test('Removes lock files and package configs from workspace members', + test('Reports error if pubspec inside workspace is not part of the workspace', + () async { + await dir(appPath, [ + libPubspec( + 'myapp', + '1.2.3', + extras: { + 'workspace': ['pkgs/a', 'pkgs/a/example'], + }, + sdk: '^3.5.0', + ), + dir('pkgs', [ + libPubspec('not_in_workspace', '1.0.0'), + dir( + 'a', + [ + libPubspec('a', '1.1.1', resolutionWorkspace: true), + dir('example', [ + libPubspec('example', '0.0.0', resolutionWorkspace: true), + ]), + ], + ), + ]), + ]).create(); + await pubGet( + environment: {'_PUB_TEST_SDK_VERSION': '3.5.0'}, + error: contains( + 'The file `.${s}pkgs${s}pubspec.yaml` ' + 'is located in a directory between the workspace root', + ), + ); + }); + + test('Removes lock files and package configs from inside the workspace', () async { await dir(appPath, [ libPubspec( @@ -825,34 +858,75 @@ foo:foomain''', 'a', [ libPubspec('a', '1.1.1', resolutionWorkspace: true), + dir('test_data', []), ], ), ]), ]).create(); + // Directories outside the workspace should not be affected. + final outideWorkpace = sandbox; + // Directories of worksace packages should be cleaned. final aDir = p.join(sandbox, appPath, 'pkgs', 'a'); + // Directories between workspace root and workspace packages should + // be cleaned. final pkgsDir = p.join(sandbox, appPath, 'pkgs'); - final strayLockFile = File(p.join(aDir, 'pubspec.lock')); - final strayPackageConfig = - File(p.join(aDir, '.dart_tool', 'package_config.json')); + // Directories inside a workspace package should not be cleaned. + final inside = p.join(aDir, 'test_data'); - final unmanagedLockFile = File(p.join(pkgsDir, 'pubspec.lock')); - final unmanagedPackageConfig = - File(p.join(pkgsDir, '.dart_tool', 'package_config.json')); - strayPackageConfig.createSync(recursive: true); - strayLockFile.createSync(recursive: true); + void createLockFileAndPackageConfig(String dir) { + File(p.join(dir, 'pubspec.lock')).createSync(recursive: true); + File(p.join(dir, '.dart_tool', 'package_config.json')) + .createSync(recursive: true); + } - unmanagedPackageConfig.createSync(recursive: true); - unmanagedLockFile.createSync(recursive: true); + void validateLockFileAndPackageConfig( + String dir, + FileSystemEntityType state, + ) { + expect( + File(p.join(dir, 'pubspec.lock')).statSync().type, + state, + ); + expect( + File(p.join(dir, '.dart_tool', 'package_config.json')).statSync().type, + state, + ); + } - await pubGet(environment: {'_PUB_TEST_SDK_VERSION': '3.5.0'}); + createLockFileAndPackageConfig(sandbox); + createLockFileAndPackageConfig(aDir); + createLockFileAndPackageConfig(pkgsDir); + createLockFileAndPackageConfig(inside); - expect(strayLockFile.statSync().type, FileSystemEntityType.notFound); - expect(strayPackageConfig.statSync().type, FileSystemEntityType.notFound); + await pubGet( + environment: {'_PUB_TEST_SDK_VERSION': '3.5.0'}, + warning: allOf( + contains('Deleting old lock-file: `.${s}pkgs/a${s}pubspec.lock'), + contains( + 'Deleting old package config: ' + '`.${s}pkgs/a$s.dart_tool${s}package_config.json`', + ), + contains('Deleting old lock-file: `.${s}pkgs${s}pubspec.lock'), + contains( + 'Deleting old package config: ' + '`.${s}pkgs$s.dart_tool${s}package_config.json`', + ), + contains( + 'See https://dart.dev/go/workspaces-stray-files for details.', + ), + ), + ); - // We only delete stray files from directories that contain an actual - // package. - expect(unmanagedLockFile.statSync().type, FileSystemEntityType.file); - expect(unmanagedPackageConfig.statSync().type, FileSystemEntityType.file); + validateLockFileAndPackageConfig( + outideWorkpace, + FileSystemEntityType.file, + ); + validateLockFileAndPackageConfig(aDir, FileSystemEntityType.notFound); + validateLockFileAndPackageConfig(pkgsDir, FileSystemEntityType.notFound); + validateLockFileAndPackageConfig( + inside, + FileSystemEntityType.file, + ); }); test('Reports error if workspace doesn\'t form a tree.', () async {