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 cae6c942e..e1e9f31e5 100644 --- a/lib/src/package.dart +++ b/lib/src/package.dart @@ -396,9 +396,12 @@ See $workspacesDocUrl for more information. /// * 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]; @@ -461,6 +464,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/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 {