Delete directories between workspace package and workspace root (#4446)

Author: sigurdm

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 = <String>{
+      // 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

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 = <String, String>{};
   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 = <String>{
+    // 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.
 ''');
       }
     }

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:

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 {