Address review comments.

pkgs/_macro_builder/lib/macro_builder.dart

@@ -6,6 +6,8 @@ import 'dart:io';
 
 import 'package:dart_model/dart_model.dart';
 
+import 'src/bootstrap.dart';
+
 /// Builds macros.
 ///
 /// TODO(davidmorgan): add a way to clean up generated files and built output.
@@ -16,40 +18,20 @@ class MacroBuilder {
   /// implements `Macro` from `package:macro`.
   ///
   /// The [packageConfig] must include the macros and all their deps.
+  ///
+  /// TODO(davidmorgan): figure out builder lifecycle: is it one builder per
+  /// host, one per workspace, one per build?
+  /// TODO(davidmorgan): replace `File` packageConfig with a concept of version
+  /// solve and workspace.
+  /// TODO(davidmorgan): support for multi-root workspaces.
+  /// TODO(davidmorgan): support (or decide not to support) in-memory overlay
+  /// filesystems.
   Future<BuiltMacroBundle> build(
-      File packageConfig, Iterable<QualifiedName> macroImplementations) async {
+      Uri packageConfig, Iterable<QualifiedName> macroImplementations) async {
     final script = createBootstrap(macroImplementations.toList());
 
     return await MacroBuild(packageConfig, script).build();
   }
-
-  /// Creates the entrypoint script for [macros].
-  static String createBootstrap(List<QualifiedName> macros) {
-    final script = StringBuffer();
-    for (var i = 0; i != macros.length; ++i) {
-      final macro = macros[i];
-      // TODO(davidmorgan): pick non-clashing prefixes.
-      script.writeln("import '${macro.uri}' as m$i;");
-    }
-    script.write('''
-import 'dart:convert';
-
-import 'package:_macro_client/macro_client.dart';
-import 'package:macro_service/macro_service.dart';
-
-void main(List<String> arguments) {
-   MacroClient.run(
-      endpoint: HostEndpoint.fromJson(json.decode(arguments[0])),
-      macros: [''');
-    for (var i = 0; i != macros.length; ++i) {
-      final macro = macros[i];
-      script.write('m$i.${macro.name}()');
-      if (i != macros.length - 1) script.write(', ');
-    }
-    script.writeln(']);');
-    script.writeln('}');
-    return script.toString();
-  }
 }
 
 /// A bundle of one or more macros that's ready to execute.
@@ -61,8 +43,11 @@ class BuiltMacroBundle {
 }
 
 /// A single build.
+///
+/// TODO(davidmorgan): split to interface+implementations as we add different
+/// ways to build.
 class MacroBuild {
-  final File packageConfig;
+  final Uri packageConfig;
   final String script;
   final Directory workspace =
       Directory.systemTemp.createTempSync('macro_builder');
@@ -76,8 +61,8 @@ class MacroBuild {
   /// Throws on failure to build.
   Future<BuiltMacroBundle> build() async {
     final scriptFile = File.fromUri(workspace.uri.resolve('bin/main.dart'));
-    scriptFile.parent.createSync(recursive: true);
-    scriptFile.writeAsStringSync(script.toString());
+    await scriptFile.create(recursive: true);
+    await scriptFile.writeAsString(script.toString());
 
     final targetPackageConfig =
         File.fromUri(workspace.uri.resolve('.dart_tool/package_config.json'));
@@ -90,7 +75,11 @@ class MacroBuild {
     //
     // For now just use the command line.
 
-    final result = Process.runSync('dart', ['compile', 'exe', 'bin/main.dart'],
+    final result = Process.runSync(
+        // TODO(davidmorgan): this is wrong if run from an AOT-compiled
+        // executable.
+        Platform.resolvedExecutable,
+        ['compile', 'exe', 'bin/main.dart', '--output=bin/main.exe'],
         workingDirectory: workspace.path);
     if (result.exitCode != 0) {
       throw StateError('Compile failed: ${result.stderr}');
@@ -100,11 +89,12 @@ class MacroBuild {
         File.fromUri(scriptFile.parent.uri.resolve('main.exe')).path);
   }
 
-  /// Returns the contents of [pubspec] with relative paths replaced to
+  /// Returns the contents of [packageConfig] with relative paths replaced to
   /// absolute paths, so the pubspec will work from any location.
-  String _makePackageConfigAbsolute(File pubspec) {
-    final root = pubspec.parent.parent.absolute.uri;
-    return pubspec
+  String _makePackageConfigAbsolute(Uri packageConfig) {
+    final file = File.fromUri(packageConfig);
+    final root = file.parent.parent.absolute.uri;
+    return file
         .readAsStringSync()
         .replaceAll('"rootUri": "../', '"rootUri": "$root');
   }

pkgs/_macro_builder/lib/src/bootstrap.dart

@@ -0,0 +1,34 @@
+// Copyright (c) 2024, the Dart project authors. Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+import 'package:dart_model/dart_model.dart';
+
+/// Creates the entrypoint script for [macros].
+String createBootstrap(List<QualifiedName> macros) {
+  final script = StringBuffer();
+  for (var i = 0; i != macros.length; ++i) {
+    final macro = macros[i];
+    // TODO(davidmorgan): pick non-clashing prefixes.
+    script.writeln("import '${macro.uri}' as m$i;");
+  }
+  script.write('''
+import 'dart:convert' as convert;
+
+import 'package:_macro_client/macro_client.dart' as macro_client;
+import 'package:macro_service/macro_service.dart' as macro_service;
+
+void main(List<String> arguments) {
+   macro_client.MacroClient.run(
+      endpoint: macro_service.HostEndpoint.fromJson(
+        convert.json.decode(arguments[0])),
+      macros: [''');
+  for (var i = 0; i != macros.length; ++i) {
+    final macro = macros[i];
+    script.write('m$i.${macro.name}()');
+    if (i != macros.length - 1) script.write(', ');
+  }
+  script.writeln(']);');
+  script.writeln('}');
+  return script.toString();
+}

pkgs/_macro_builder/test/macro_builder_test.dart

@@ -6,13 +6,14 @@ import 'dart:io';
 import 'dart:isolate';
 
 import 'package:_macro_builder/macro_builder.dart';
+import 'package:_macro_builder/src/bootstrap.dart';
 import 'package:dart_model/dart_model.dart';
 import 'package:test/test.dart';
 
 void main() {
   group(MacroBuilder, () {
     test('bootstrap matches golden', () async {
-      final script = MacroBuilder.createBootstrap([
+      final script = createBootstrap([
         QualifiedName('package:_test_macros/declare_x_macro.dart#DeclareX'),
         QualifiedName('package:_test_macros/declare_y_macro.dart#DeclareY'),
         QualifiedName(
@@ -23,14 +24,15 @@ void main() {
 import 'package:_test_macros/declare_x_macro.dart' as m0;
 import 'package:_test_macros/declare_y_macro.dart' as m1;
 import 'package:_more_macros/other_macro.dart' as m2;
-import 'dart:convert';
+import 'dart:convert' as convert;
 
-import 'package:_macro_client/macro_client.dart';
-import 'package:macro_service/macro_service.dart';
+import 'package:_macro_client/macro_client.dart' as macro_client;
+import 'package:macro_service/macro_service.dart' as macro_service;
 
 void main(List<String> arguments) {
-   MacroClient.run(
-      endpoint: HostEndpoint.fromJson(json.decode(arguments[0])),
+   macro_client.MacroClient.run(
+      endpoint: macro_service.HostEndpoint.fromJson(
+        convert.json.decode(arguments[0])),
       macros: [m0.DeclareX(), m1.DeclareY(), m2.OtherMacroImplementation()]);
 }
 ''');
@@ -39,8 +41,7 @@ void main(List<String> arguments) {
     test('builds macros', () async {
       final builder = MacroBuilder();
 
-      final bundle =
-          await builder.build(File.fromUri(Isolate.packageConfigSync!), [
+      final bundle = await builder.build(Isolate.packageConfigSync!, [
         QualifiedName(
             'package:_test_macros/declare_x_macro.dart#DeclareXImplementation')
       ]);

pkgs/_macro_client/lib/macro_client.dart

@@ -9,16 +9,20 @@ import 'package:macro/macro.dart';
 import 'package:macro_service/macro_service.dart';
 
 /// Runs macros, connecting them to a macro host.
+///
+/// TODO(davidmorgan): handle shutdown and dispose.
+/// TODO(davidmorgan): split to multpile implementations depending on
+/// transport used to connect to host.
 class MacroClient {
-  final Iterable<Macro> macros;
-  final Socket socket;
-
-  MacroClient._(this.macros, this.socket) {
+  MacroClient._(Iterable<Macro> macros, Socket socket) {
     // TODO(davidmorgan): negotiation about protocol version goes here.
 
     // Tell the host which macros are in this bundle.
     for (final macro in macros) {
-      _send(MacroStartedRequest(macroDescription: macro.description).node);
+      final request = MacroStartedRequest(macroDescription: macro.description);
+      // TODO(davidmorgan): currently this is JSON with one request per line,
+      // switch to binary.
+      socket.writeln(json.encode(request.node));
     }
   }
 
@@ -28,10 +32,4 @@ class MacroClient {
     final socket = await Socket.connect('localhost', endpoint.port);
     return MacroClient._(macros, socket);
   }
-
-  void _send(Map<String, Object?> node) {
-    // TODO(davidmorgan): currently this is JSON with one request per line,
-    // switch to binary.
-    socket.writeln(json.encode(node));
-  }
 }

pkgs/_macro_client/test/macro_client_test.dart

@@ -14,6 +14,7 @@ void main() {
   group(MacroClient, () {
     test('connects to service', () async {
       final serverSocket = await ServerSocket.bind('localhost', 0);
+      addTearDown(serverSocket.close);
 
       unawaited(MacroClient.run(
           endpoint: HostEndpoint(port: serverSocket.port),

pkgs/_macro_host/lib/macro_host.dart

@@ -2,6 +2,7 @@
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
+import 'dart:async';
 import 'dart:io';
 
 import 'package:_macro_builder/macro_builder.dart';
@@ -22,7 +23,7 @@ class MacroHost implements MacroService {
 
   // TODO(davidmorgan): this should be per macro, as part of tracking per-macro
   // lifecycle state.
-  Set<int>? _macroPhases;
+  Completer<Set<int>>? _macroPhases;
 
   MacroHost._(this.macroServer, this.services) {
     services.services.insert(0, this);
@@ -43,22 +44,22 @@ class MacroHost implements MacroService {
   }
 
   /// Whether [name] is a macro according to that package's `pubspec.yaml`.
-  bool isMacro(File packageConfig, QualifiedName name) {
+  bool isMacro(Uri packageConfig, QualifiedName name) {
     // TODO(language/3728): this is a placeholder, use package config when
     // available.
     return true;
   }
 
   /// Determines which phases the macro implemented at [name] runs in.
   Future<Set<int>> queryMacroPhases(
-      File packageConfig, QualifiedName name) async {
-    if (_macroPhases != null) return _macroPhases!;
+      Uri packageConfig, QualifiedName name) async {
+    // TODO(davidmorgan): track macro lifecycle, correctly run once per macro
+    // code change including if queried multiple times before response returns.
+    if (_macroPhases != null) return _macroPhases!.future;
+    _macroPhases = Completer();
     final macroBundle = await macroBuilder.build(packageConfig, [name]);
-    macroRunner.run(macroBundle: macroBundle, endpoint: macroServer.endpoint);
-    // TODO(davidmorgan): wait explicitly for the MacroStartedRequest to
-    // arrive, remove this hard-coded wait.
-    await Future<void>.delayed(const Duration(seconds: 2));
-    return _macroPhases!;
+    macroRunner.start(macroBundle: macroBundle, endpoint: macroServer.endpoint);
+    return _macroPhases!.future;
   }
 
   /// Handle requests that are for the host.
@@ -68,7 +69,8 @@ class MacroHost implements MacroService {
     // that should be passed through to the service that was passed in.
     final macroStartedRequest =
         MacroStartedRequest.fromJson(request as Map<String, Object?>);
-    _macroPhases = macroStartedRequest.macroDescription.runsInPhases.toSet();
+    _macroPhases!
+        .complete(macroStartedRequest.macroDescription.runsInPhases.toSet());
     return MacroStartedResponse();
   }
 

pkgs/_macro_host/test/macro_host_test.dart

@@ -18,7 +18,7 @@ void main() {
 
       final macroName = QualifiedName(
           'package:_test_macros/declare_x_macro.dart#DeclareXImplementation');
-      final packageConfig = File.fromUri(Isolate.packageConfigSync!);
+      final packageConfig = Isolate.packageConfigSync!;
 
       expect(host.isMacro(packageConfig, macroName), true);
       expect(await host.queryMacroPhases(packageConfig, macroName), {2});

pkgs/_macro_runner/lib/macro_runner.dart

@@ -12,8 +12,8 @@ import 'package:macro_service/macro_service.dart';
 ///
 /// TODO(davidmorgan): support shutdown/cleanup.
 class MacroRunner {
-  /// Runs [macroBundle] connected to [endpoint].
-  void run(
+  /// Starts [macroBundle] connected to [endpoint].
+  void start(
       {required BuiltMacroBundle macroBundle, required HostEndpoint endpoint}) {
     Process.run(macroBundle.executablePath, [json.encode(endpoint)]);
   }

pkgs/_macro_runner/test/macro_runner_test.dart

@@ -15,16 +15,16 @@ void main() {
   group(MacroRunner, () {
     test('runs macros', () async {
       final builder = MacroBuilder();
-      final bundle =
-          await builder.build(File.fromUri(Isolate.packageConfigSync!), [
+      final bundle = await builder.build(Isolate.packageConfigSync!, [
         QualifiedName(
             'package:_test_macros/declare_x_macro.dart#DeclareXImplementation')
       ]);
 
       final serverSocket = await ServerSocket.bind('localhost', 0);
+      addTearDown(serverSocket.close);
 
       final runner = MacroRunner();
-      runner.run(
+      runner.start(
           macroBundle: bundle, endpoint: HostEndpoint(port: serverSocket.port));
 
       expect(

pkgs/_macro_server/lib/macro_server.dart

@@ -14,7 +14,7 @@ class MacroServer {
   final ServerSocket serverSocket;
 
   MacroServer._(this.service, this.endpoint, this.serverSocket) {
-    _handleConnections();
+    serverSocket.forEach(_handleConnection);
   }
 
   /// Serves [service].
@@ -26,10 +26,6 @@ class MacroServer {
         service, HostEndpoint(port: serverSocket.port), serverSocket);
   }
 
-  void _handleConnections() {
-    serverSocket.forEach(_handleConnection);
-  }
-
   void _handleConnection(Socket socket) {
     // TODO(davidmorgan): currently this is JSON with one request per line,
     // switch to binary.

pkgs/macro/lib/macro.dart

@@ -8,6 +8,11 @@ import 'package:macro_service/macro_service.dart';
 /// code.
 abstract interface class Macro {
   /// Description of the macro.
+  ///
+  /// TODO(davidmorgan): where possible the macro information should be
+  /// determined by `macro_builder` and injected in the bootstrap script rather
+  /// than relying on the user-written macro code to return it.
+  ///
   MacroDescription get description;
 
   /// Generate augmentatations and diagnostics for [request].