React when Bazel/Blaze prefix changes

Previously if no Bazel/Blaze symlinks were present, we defaulted to the
`bazel-*` ones and never checked if that assumption was accurate. This
change starts watching the symlinks and recreates the analysis contexts
if a symlink gets added. This way we'll run the `BazelWorkspace` code
again and use the correct paths.

This is quite important since `bazel/blaze clean` does remove the
symlinks so if the analysis server starts at such a point, we would
never find any generated files if the workspaced turned out to be a
Blaze one.

Bug: http://b/175087705
Change-Id: If7bb21c7d69a3092832c18004691d56949e5af54
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/197540
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Michal Terepeta <michalt@google.com>

pkg/analysis_server/lib/src/context_manager.dart

@@ -460,7 +460,9 @@ class ContextManagerImpl implements ContextManager {
       for (var path in watched.paths) {
         bazelWatcherService!.stopWatching(watched.workspace, path);
       }
+      _stopWatchingBazelBinPaths(watched);
     }
+    bazelSearchSubscriptions.remove(rootFolder)?.cancel();
     driverMap.remove(rootFolder);
   }
 
@@ -474,6 +476,11 @@ class ContextManagerImpl implements ContextManager {
     }
   }
 
+  List<String> _getPossibelBazelBinPaths(_BazelWatchedFiles watched) => [
+        pathContext.join(watched.workspace, 'bazel-bin'),
+        pathContext.join(watched.workspace, 'blaze-bin'),
+      ];
+
   /// Establishes watch(es) for the Bazel generated files provided in
   /// [notification].
   ///
@@ -493,10 +500,23 @@ class ContextManagerImpl implements ContextManager {
   }
 
   /// Notifies the drivers that a generated Bazel file has changed.
-  void _handleBazelWatchEvents(List<WatchEvent> events) {
+  void _handleBazelWatchEvents(List<WatchEvent> allEvents) {
+    // First check if we have any changes to the bazel-*/blaze-* paths.  If
+    // we do, we'll simply recreate all contexts to make sure that we follow the
+    // correct paths.
+    var bazelSymlinkPaths = bazelWatchedPathsPerFolder.values
+        .expand((watched) => _getPossibelBazelBinPaths(watched))
+        .toSet();
+    if (allEvents.any((event) => bazelSymlinkPaths.contains(event.path))) {
+      refresh();
+      return;
+    }
+
+    var fileEvents =
+        allEvents.where((event) => !bazelSymlinkPaths.contains(event.path));
     for (var driver in driverMap.values) {
       var needsUriReset = false;
-      for (var event in events) {
+      for (var event in fileEvents) {
         if (event.type == ChangeType.ADD) {
           driver.addFile(event.path);
           needsUriReset = true;
@@ -599,6 +619,28 @@ class ContextManagerImpl implements ContextManager {
         existingExcludedSet.containsAll(excludedPaths);
   }
 
+  /// Starts watching for the `bazel-bin` and `blaze-bin` symlinks.
+  ///
+  /// This is important since these symlinks might not be present when the
+  /// server starts up, in which case `BazelWorkspace` assumes by default the
+  /// Bazel ones.  So we want to detect if the symlinks get created to reset
+  /// everything and repeat the search for the folders.
+  void _startWatchingBazelBinPaths(_BazelWatchedFiles watched) {
+    var watcherService = bazelWatcherService;
+    if (watcherService == null) return;
+    var paths = _getPossibelBazelBinPaths(watched);
+    watcherService.startWatching(
+        watched.workspace, BazelSearchInfo(paths[0], paths));
+  }
+
+  /// Stops watching for the `bazel-bin` and `blaze-bin` symlinks.
+  void _stopWatchingBazelBinPaths(_BazelWatchedFiles watched) {
+    var watcherService = bazelWatcherService;
+    if (watcherService == null) return;
+    var paths = _getPossibelBazelBinPaths(watched);
+    watcherService.stopWatching(watched.workspace, paths[0]);
+  }
+
   /// Listens to files generated by Bazel that were found or searched for.
   ///
   /// This is handled specially because the files are outside the package
@@ -607,13 +649,19 @@ class ContextManagerImpl implements ContextManager {
   /// Does nothing if the [driver] is not in a Bazel workspace.
   void _watchBazelFilesIfNeeded(Folder folder, AnalysisDriver analysisDriver) {
     if (!experimentalEnableBazelWatching) return;
+    var watcherService = bazelWatcherService;
+    if (watcherService == null) return;
+
     var workspace = analysisDriver.analysisContext?.contextRoot.workspace;
     if (workspace is BazelWorkspace &&
         !bazelSearchSubscriptions.containsKey(folder)) {
-      var searchSubscription = workspace.bazelCandidateFiles.listen(
+      bazelSearchSubscriptions[folder] = workspace.bazelCandidateFiles.listen(
           (notification) =>
               _handleBazelSearchInfo(folder, workspace.root, notification));
-      bazelSearchSubscriptions[folder] = searchSubscription;
+
+      var watched = _BazelWatchedFiles(workspace.root);
+      bazelWatchedPathsPerFolder[folder] = watched;
+      _startWatchingBazelBinPaths(watched);
     }
   }
 }

pkg/analysis_server/test/integration/server/bazel_changes_test.dart

@@ -35,9 +35,9 @@ class BazelChangesTest extends AbstractAnalysisServerIntegrationTest {
   late String bazelRoot;
   late String tmpPath;
   late String workspacePath;
-  late String bazelOutPath;
-  late String bazelBinPath;
-  late String bazelGenfilesPath;
+  late String bazelOrBlazeOutPath;
+  late String bazelOrBlazeBinPath;
+  late String bazelOrBlazeGenfilesPath;
   late Directory oldSourceDirectory;
 
   String inTmpDir(String relative) =>
@@ -61,21 +61,19 @@ class BazelChangesTest extends AbstractAnalysisServerIntegrationTest {
     sourceDirectory = Directory(inWorkspace('third_party/dart/project'));
     sourceDirectory.createSync(recursive: true);
 
-    bazelRoot = inTmpDir('bazel_root');
+    bazelRoot = inTmpDir('bazel_or_blaze_root');
     Directory(bazelRoot).createSync(recursive: true);
 
-    bazelOutPath = '$bazelRoot/execroot/bazel_workspace/bazel-out';
-    bazelBinPath = '$bazelRoot/execroot/bazel_workspace/bazel-out/bin';
-    bazelGenfilesPath =
-        '$bazelRoot/execroot/bazel_workspace/bazel-out/genfiles';
+    bazelOrBlazeOutPath =
+        '$bazelRoot/execroot/bazel_or_blaze_workspace/bazel_or_blaze-out';
+    bazelOrBlazeBinPath =
+        '$bazelRoot/execroot/bazel_or_blaze_workspace/bazel_or_blaze-out/bin';
+    bazelOrBlazeGenfilesPath =
+        '$bazelRoot/execroot/bazel_or_blaze_workspace/bazel_or_blaze-out/genfiles';
 
-    Directory(inTmpDir(bazelOutPath)).createSync(recursive: true);
-    Directory(inTmpDir(bazelBinPath)).createSync(recursive: true);
-    Directory(inTmpDir(bazelGenfilesPath)).createSync(recursive: true);
-
-    Link(inWorkspace('bazel-out')).createSync(bazelOutPath);
-    Link(inWorkspace('bazel-bin')).createSync(bazelBinPath);
-    Link(inWorkspace('bazel-genfiles')).createSync(bazelGenfilesPath);
+    Directory(inTmpDir(bazelOrBlazeOutPath)).createSync(recursive: true);
+    Directory(inTmpDir(bazelOrBlazeBinPath)).createSync(recursive: true);
+    Directory(inTmpDir(bazelOrBlazeGenfilesPath)).createSync(recursive: true);
 
     commandLogPath = inTmpDir('$bazelRoot/command.log');
   }
@@ -91,6 +89,17 @@ class BazelChangesTest extends AbstractAnalysisServerIntegrationTest {
   // not run from a snapshot.
   @TestTimeout(Timeout.factor(2))
   Future<void> test_bazelChanges() async {
+    await testChangesImpl('bazel');
+  }
+
+  // Add a bit more time -- the isolate take a while to start when the test is
+  // not run from a snapshot.
+  @TestTimeout(Timeout.factor(2))
+  Future<void> test_blazeChanges() async {
+    await testChangesImpl('blaze');
+  }
+
+  Future<void> testChangesImpl(String prefix) async {
     var testFile = inWorkspace('${sourceDirectory.path}/lib/test.dart');
 
     var errors = <AnalysisError>[];
@@ -126,8 +135,9 @@ void main() { my_fun(); }
 
     await resetCompleterAndErrors();
     var generatedFilePath = inWorkspace(
-        '$bazelGenfilesPath/third_party/dart/project/lib/generated.dart');
+        '$bazelOrBlazeGenfilesPath/third_party/dart/project/lib/generated.dart');
     writeFile(generatedFilePath, 'my_fun() {}');
+    _createSymlinks(prefix);
     writeFile(commandLogPath, 'Build completed successfully');
 
     await processedNotification.future;
@@ -145,6 +155,7 @@ void main() { my_fun(); }
     // Now delete the file completely.
     await resetCompleterAndErrors();
     File(generatedFilePath).deleteSync();
+    _deleteSymlinks(prefix);
     writeFile(commandLogPath, 'Build did NOT complete successfully');
 
     await processedNotification.future;
@@ -153,9 +164,22 @@ void main() { my_fun(); }
     // And finally re-add the correct file -- errors should go away once again.
     await resetCompleterAndErrors();
     writeFile(generatedFilePath, 'my_fun() {}');
+    _createSymlinks(prefix);
     writeFile(commandLogPath, 'Build completed successfully');
 
     await processedNotification.future;
     expect(errors, isEmpty);
   }
+
+  void _createSymlinks(String prefix) {
+    Link(inWorkspace('$prefix-out')).createSync(bazelOrBlazeOutPath);
+    Link(inWorkspace('$prefix-bin')).createSync(bazelOrBlazeBinPath);
+    Link(inWorkspace('$prefix-genfiles')).createSync(bazelOrBlazeGenfilesPath);
+  }
+
+  void _deleteSymlinks(String prefix) {
+    Link(inWorkspace('$prefix-out')).deleteSync();
+    Link(inWorkspace('$prefix-bin')).deleteSync();
+    Link(inWorkspace('$prefix-genfiles')).deleteSync();
+  }
 }

pkg/analyzer/lib/src/workspace/bazel_watcher.dart

@@ -110,10 +110,21 @@ class BazelFilePoller {
   /// exist.
   _TimestampAndLength? _pollOne(String path) {
     try {
-      var file = _provider.getFile(path);
-      var timestamp = file.modificationStamp;
-      var length = file.lengthSync;
-      return _TimestampAndLength(timestamp, length);
+      // This might seem a bit convoluted but is necessary to deal with a
+      // symlink to a directory (e.g., `bazel-bin`).
+      var resource = _provider.getResource(
+          _provider.getResource(path).resolveSymbolicLinksSync().path);
+      if (resource is File) {
+        var timestamp = resource.modificationStamp;
+        var length = resource.lengthSync;
+        return _TimestampAndLength(timestamp, length);
+      } else if (resource is Folder) {
+        // `ResourceProvider` doesn't currently support getting timestamps of a
+        // folder, so we use a dummy value here. But it's still useful: this
+        // will correctly generate `ADD` or `REMOVE` events (we'll be just
+        // unable to generate any `CHANGE` events).
+        return _TimestampAndLength(0, 0);
+      }
     } on FileSystemException catch (_) {
       // File doesn't exist, so return null.
       return null;

pkg/analyzer/test/src/workspace/bazel_watcher_test.dart

@@ -233,6 +233,47 @@ class BazelWatcherTest with ResourceProviderMixin {
     recPort.close();
   }
 
+  void test_bazelFileWatcherWithFolder() async {
+    _addResources([
+      '/workspace/WORKSPACE',
+    ]);
+
+    // The `_addResources`/`_deleteResources` functions recognize a folder by a
+    // trailing `/`, but everywhere else we need to use normalized paths.
+    var addFolder = (path) => _addResources(['$path/']);
+    var deleteFolder = (path) => _deleteResources(['$path/']);
+
+    var candidates = [
+      convertPath('/workspace/bazel-out'),
+      convertPath('/workspace/blaze-out'),
+    ];
+    var watcher = BazelFilePoller(resourceProvider, candidates);
+
+    // First do some tests with the first candidate path.
+    addFolder(candidates[0]);
+    var event = watcher.poll()!;
+
+    expect(event.type, ChangeType.ADD);
+    expect(event.path, candidates[0]);
+
+    deleteFolder(candidates[0]);
+    event = watcher.poll()!;
+
+    expect(event.type, ChangeType.REMOVE);
+    expect(event.path, candidates[0]);
+
+    // Now check that if we add the *second* candidate, we'll get the
+    // notification for it.
+    addFolder(candidates[1]);
+    event = watcher.poll()!;
+
+    expect(event.type, ChangeType.ADD);
+    expect(event.path, candidates[1]);
+
+    // Next poll should be `null` since there were no changes.
+    expect(watcher.poll(), isNull);
+  }
+
   /// Create new files and directories from [paths].
   void _addResources(List<String> paths) {
     for (String path in paths) {