From 54a861c51bef239d422ed2b2e01022d6bc4555a2 Mon Sep 17 00:00:00 2001 From: David Morgan Date: Mon, 5 Aug 2024 14:14:38 +0200 Subject: [PATCH] Address review comments. --- pkgs/_macro_builder/lib/macro_builder.dart | 64 ++++++++----------- pkgs/_macro_builder/lib/src/bootstrap.dart | 34 ++++++++++ .../test/macro_builder_test.dart | 17 ++--- pkgs/_macro_client/lib/macro_client.dart | 20 +++--- .../_macro_client/test/macro_client_test.dart | 1 + pkgs/_macro_host/lib/macro_host.dart | 22 ++++--- pkgs/_macro_host/test/macro_host_test.dart | 2 +- pkgs/_macro_runner/lib/macro_runner.dart | 4 +- .../_macro_runner/test/macro_runner_test.dart | 6 +- pkgs/_macro_server/lib/macro_server.dart | 6 +- pkgs/macro/lib/macro.dart | 5 ++ 11 files changed, 104 insertions(+), 77 deletions(-) create mode 100644 pkgs/_macro_builder/lib/src/bootstrap.dart diff --git a/pkgs/_macro_builder/lib/macro_builder.dart b/pkgs/_macro_builder/lib/macro_builder.dart index 03d834be..0c10da0e 100644 --- a/pkgs/_macro_builder/lib/macro_builder.dart +++ b/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 build( - File packageConfig, Iterable macroImplementations) async { + Uri packageConfig, Iterable macroImplementations) async { final script = createBootstrap(macroImplementations.toList()); return await MacroBuild(packageConfig, script).build(); } - - /// Creates the entrypoint script for [macros]. - static String createBootstrap(List 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 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 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'); } diff --git a/pkgs/_macro_builder/lib/src/bootstrap.dart b/pkgs/_macro_builder/lib/src/bootstrap.dart new file mode 100644 index 00000000..9bcc6722 --- /dev/null +++ b/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 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 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(); +} diff --git a/pkgs/_macro_builder/test/macro_builder_test.dart b/pkgs/_macro_builder/test/macro_builder_test.dart index f52df4d9..1929737f 100644 --- a/pkgs/_macro_builder/test/macro_builder_test.dart +++ b/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 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 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') ]); diff --git a/pkgs/_macro_client/lib/macro_client.dart b/pkgs/_macro_client/lib/macro_client.dart index a150aeff..b89ff426 100644 --- a/pkgs/_macro_client/lib/macro_client.dart +++ b/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 macros; - final Socket socket; - - MacroClient._(this.macros, this.socket) { + MacroClient._(Iterable 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 node) { - // TODO(davidmorgan): currently this is JSON with one request per line, - // switch to binary. - socket.writeln(json.encode(node)); - } } diff --git a/pkgs/_macro_client/test/macro_client_test.dart b/pkgs/_macro_client/test/macro_client_test.dart index 481eb75a..f36c4979 100644 --- a/pkgs/_macro_client/test/macro_client_test.dart +++ b/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), diff --git a/pkgs/_macro_host/lib/macro_host.dart b/pkgs/_macro_host/lib/macro_host.dart index 50357e36..fa0812ce 100644 --- a/pkgs/_macro_host/lib/macro_host.dart +++ b/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? _macroPhases; + Completer>? _macroPhases; MacroHost._(this.macroServer, this.services) { services.services.insert(0, this); @@ -43,7 +44,7 @@ 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; @@ -51,14 +52,14 @@ class MacroHost implements MacroService { /// Determines which phases the macro implemented at [name] runs in. Future> 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.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); - _macroPhases = macroStartedRequest.macroDescription.runsInPhases.toSet(); + _macroPhases! + .complete(macroStartedRequest.macroDescription.runsInPhases.toSet()); return MacroStartedResponse(); } diff --git a/pkgs/_macro_host/test/macro_host_test.dart b/pkgs/_macro_host/test/macro_host_test.dart index cf9abf84..21701775 100644 --- a/pkgs/_macro_host/test/macro_host_test.dart +++ b/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}); diff --git a/pkgs/_macro_runner/lib/macro_runner.dart b/pkgs/_macro_runner/lib/macro_runner.dart index 9377aab4..e2a6d8bd 100644 --- a/pkgs/_macro_runner/lib/macro_runner.dart +++ b/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)]); } diff --git a/pkgs/_macro_runner/test/macro_runner_test.dart b/pkgs/_macro_runner/test/macro_runner_test.dart index 5af9a140..20d052ef 100644 --- a/pkgs/_macro_runner/test/macro_runner_test.dart +++ b/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( diff --git a/pkgs/_macro_server/lib/macro_server.dart b/pkgs/_macro_server/lib/macro_server.dart index 7e171f54..746a6529 100644 --- a/pkgs/_macro_server/lib/macro_server.dart +++ b/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. diff --git a/pkgs/macro/lib/macro.dart b/pkgs/macro/lib/macro.dart index f467b76b..6e0b0e83 100644 --- a/pkgs/macro/lib/macro.dart +++ b/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].