Build macros, run them, serve a (test) service, host, connect to it #20

davidmorgan · 2024-07-22T09:38:23Z

Flesh out much of the end to end functionality:

Build macros (with just "dart compile exe")
Run macros (with Process)
Serve them (TCP/IP)
Connect to them
First message passed macro->host: which phases to run in

Next after this: add first introspection and augmentation RPCs/capability; then it will be an actual e2e macro.

jakemac53 · 2024-07-25T17:59:05Z

pkgs/_macro_builder/lib/macro_builder.dart

@@ -2,22 +2,110 @@
 // 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:io';


Should we separate out an interface here, and then have an implementation which imports dart:io? It might anyways make sense to have an abstraction here, for kernel versions or a version which uses the front end as a library (this might live in the SDK, under pkg/front_end).

For sure this will end up with multiple implementations; added a TODO.

Some of them may come from code already in the analyzer and/or CFE.

jakemac53 · 2024-07-25T18:00:45Z

pkgs/_macro_builder/lib/macro_builder.dart

-    // to by `macroImplementations` then pass them to `MacroClient.run` in
-    // `package:_macro_client`.
-    return BuiltMacroBundle();
+      File packageConfig, Iterable<QualifiedName> macroImplementations) async {


I think the interface probably shouldn't expose any dart:io types - I would make the package config be something provided up front and stored in a field (also not part of the interface probably, just an implementation specific thing).

Otherwise, there is no need for a class at all here.

I think the key question here is about the scope of MacroBuilder: is it one builder for the host, one for the workspace, one for the workspace at a particular time ... wider scope allows more optimization, by which I mean reuse of (resident?) compilers and compile artifacts across builds.

I agree that File does not seem like the right concept, what we are really talking about here is the version solve and workspace that we need to do a build: the builder is going to have to care about both and about changes to both. There are questions of file overlays when using the analyzer and multiroot file systems.

So a lot to unpack here :) which is why I have it at the simplest+wrong thing currently. I'll add a pile of TODOs for all the above.

Fwiw I don't think the version solve or workspace do matter - those are both encapsulated by the package config. And the package config is the only thing the compiler or analyzer should care about. It is the only unifying concept across all our environments.

I just don't think we want File (or any dart:io type) in the public API. Even if this was just a URI I probably would be happy :).

I think we probably do have to end up worrying about version solve + workspace in order to handle finding prebuilt macros and changes to macros; it's not needed for now though.

Sure, Uri works; done. Thanks.

jakemac53 · 2024-07-25T18:02:26Z

pkgs/_macro_builder/lib/macro_builder.dart

+  }
+
+  /// Creates the entrypoint script for [macros].
+  static String createBootstrap(List<QualifiedName> macros) {


Does this need to be public?

If we want to unit test it, I would put it in a different library under lib/src.

For the "private" packages, starting with an underscore, I put implementation directly under lib instead of lib/src; my expectation is that we end up folding them entirely into some package's src so adding lib/src now just seemed like noise.

So I'm thinking of this entire file as private.

Happy to do something else though, as you like.

I do think there is a difference here worth encapsulating.

If I understand correctly, MacroBuilder will be public (ie: accessible to the CFE/Analyzer). But this function is not intended to be used by either, and should ideally not be exposed.

That makes sense, moved out to lib/src as you originally suggested. Thanks :)

jakemac53 · 2024-07-25T18:02:43Z

pkgs/_macro_builder/lib/macro_builder.dart

+      script.writeln("import '${macro.uri}' as m$i;");
+    }
+    script.write('''
+import 'dart:convert';


nit: you might want to prefix all these, to be safe

Thanks, done.

jakemac53 · 2024-07-25T19:37:48Z

pkgs/_macro_builder/lib/macro_builder.dart

+  Future<BuiltMacroBundle> build() async {
+    final scriptFile = File.fromUri(workspace.uri.resolve('bin/main.dart'));
+    scriptFile.parent.createSync(recursive: true);
+    scriptFile.writeAsStringSync(script.toString());


nit: it is a bit weird to be using sync I/O within an async function like this, I would just use async

jakemac53 · 2024-07-25T20:48:30Z

pkgs/_macro_runner/lib/macro_runner.dart

 class MacroRunner {
-  void run(BuiltMacroBundle macro, HostEndpoint endpoint) {}
+  /// Runs [macroBundle] connected to [endpoint].
+  void run(


It seems a bit odd to not have this return a Future - maybe just call it start? It also seems like it should just be a top level method.

Rename done.

I expect it needs to be a class for the shutdown/cleanup TODO :)

pkgs/_macro_runner/test/macro_runner_test.dart

jakemac53 · 2024-07-25T20:53:35Z

pkgs/_macro_server/lib/macro_server.dart

+  }
+
+  void _handleConnections() {
+    serverSocket.forEach(_handleConnection);


This could probably be inlined above

jakemac53 · 2024-07-25T20:54:10Z

pkgs/_macro_server/test/macro_server_test.dart

+  @override
+  Future<Object> handle(Object request) async {
+    _macroStartedRequestsController.add(request as MacroStartedRequest);
+    return MacroStartedResponse() as Object;


Why is this cast necessary?

Extension types don't implement Object unless it's explicitly stated :)

This gets resolved properly when we add union types for requests+responses in PR #22, so we don't use Object and use the union type.

jakemac53 · 2024-07-25T20:56:24Z

pkgs/_test_macros/lib/declare_x_macro.dart

@@ -12,7 +12,7 @@ class DeclareX {
 // TODO(davidmorgan): this is a placeholder; make it do something, test it.
 class DeclareXImplementation implements Macro {
  @override
-  MacroDescription get description => MacroDescription(runsInPhases: [3]);
+  MacroDescription get description => MacroDescription(runsInPhases: [2]);


It seems wrong for users to be creating this description, with explicit phase numbers like this

Yes, added a TODO on the Macro interface that MacroDescription should be populated (as far as possible) by macro_builder which can figure out the information and inject it in the bootstrap script, rather than asking the user macro code to do it.

davidmorgan

Please see my general reply to your email about these PRs, as to why there are a lot of TODOs open instead of addressed: tl;dr so we can figure out where to reuse code instead of writing new.

Thanks, PTAL.

davidmorgan · 2024-08-05T10:09:23Z

pkgs/_macro_builder/lib/macro_builder.dart

+      script.writeln("import '${macro.uri}' as m$i;");
+    }
+    script.write('''
+import 'dart:convert';


Thanks, done.

davidmorgan · 2024-08-05T10:10:45Z

pkgs/_macro_builder/lib/macro_builder.dart

+  /// Throws on failure to build.
+  Future<BuiltMacroBundle> build() async {
+    final scriptFile = File.fromUri(workspace.uri.resolve('bin/main.dart'));
+    scriptFile.parent.createSync(recursive: true);


davidmorgan · 2024-08-05T10:11:21Z

pkgs/_macro_builder/lib/macro_builder.dart

+  Future<BuiltMacroBundle> build() async {
+    final scriptFile = File.fromUri(workspace.uri.resolve('bin/main.dart'));
+    scriptFile.parent.createSync(recursive: true);
+    scriptFile.writeAsStringSync(script.toString());


davidmorgan · 2024-08-05T10:13:57Z

pkgs/_macro_builder/lib/macro_builder.dart

+    //
+    // For now just use the command line.
+
+    final result = Process.runSync('dart', ['compile', 'exe', 'bin/main.dart'],


Done; although the resolved executable is also not always correct, if this is used from an AOT-compiled binary. Added a TODO as well :)

davidmorgan · 2024-08-05T10:15:26Z

pkgs/_macro_builder/lib/macro_builder.dart

+    //
+    // For now just use the command line.
+
+    final result = Process.runSync('dart', ['compile', 'exe', 'bin/main.dart'],


davidmorgan · 2024-08-05T11:06:55Z

pkgs/_macro_runner/lib/macro_runner.dart

 class MacroRunner {
-  void run(BuiltMacroBundle macro, HostEndpoint endpoint) {}
+  /// Runs [macroBundle] connected to [endpoint].
+  void run(


Rename done.

I expect it needs to be a class for the shutdown/cleanup TODO :)

pkgs/_macro_runner/test/macro_runner_test.dart

davidmorgan · 2024-08-05T11:08:27Z

pkgs/_macro_server/lib/macro_server.dart

+  }
+
+  void _handleConnections() {
+    serverSocket.forEach(_handleConnection);


davidmorgan · 2024-08-05T11:10:59Z

pkgs/_macro_server/test/macro_server_test.dart

+  @override
+  Future<Object> handle(Object request) async {
+    _macroStartedRequestsController.add(request as MacroStartedRequest);
+    return MacroStartedResponse() as Object;


Extension types don't implement Object unless it's explicitly stated :)

This gets resolved properly when we add union types for requests+responses in PR #22, so we don't use Object and use the union type.

davidmorgan · 2024-08-05T12:13:29Z

pkgs/_test_macros/lib/declare_x_macro.dart

@@ -12,7 +12,7 @@ class DeclareX {
 // TODO(davidmorgan): this is a placeholder; make it do something, test it.
 class DeclareXImplementation implements Macro {
  @override
-  MacroDescription get description => MacroDescription(runsInPhases: [3]);
+  MacroDescription get description => MacroDescription(runsInPhases: [2]);


Yes, added a TODO on the Macro interface that MacroDescription should be populated (as far as possible) by macro_builder which can figure out the information and inject it in the bootstrap script, rather than asking the user macro code to do it.

jakemac53 · 2024-08-05T17:13:00Z

pkgs/_macro_builder/lib/macro_builder.dart

+  }
+
+  /// Creates the entrypoint script for [macros].
+  static String createBootstrap(List<QualifiedName> macros) {


I do think there is a difference here worth encapsulating.

If I understand correctly, MacroBuilder will be public (ie: accessible to the CFE/Analyzer). But this function is not intended to be used by either, and should ideally not be exposed.

jakemac53 · 2024-08-05T17:16:08Z

pkgs/_macro_host/lib/macro_host.dart

  final MacroServer macroServer;
+  final ListOfServices services;


👍 Fine to follow up in later prs

davidmorgan marked this pull request as draft July 22, 2024 09:38

davidmorgan force-pushed the macro-builder branch 7 times, most recently from 0501a0f to 3c4a927 Compare July 23, 2024 07:50

davidmorgan changed the title ~~Build macros.~~ Build macros, run them, serve a (test) service, host, connect to it Jul 23, 2024

davidmorgan force-pushed the macro-builder branch 2 times, most recently from a5287c5 to 81605eb Compare July 23, 2024 12:37

davidmorgan marked this pull request as ready for review July 24, 2024 14:04

davidmorgan requested a review from jakemac53 July 24, 2024 14:04

jakemac53 reviewed Jul 25, 2024

View reviewed changes

davidmorgan commented Aug 5, 2024

View reviewed changes

jakemac53 approved these changes Aug 5, 2024

View reviewed changes

Build macros, run them, serve a (test) service, host.

17e4220

davidmorgan force-pushed the macro-builder branch from 5425428 to 54a861c Compare August 6, 2024 07:12

Address review comments.

be9d3d7

davidmorgan force-pushed the macro-builder branch from 54a861c to be9d3d7 Compare August 6, 2024 07:16

davidmorgan merged commit b3f4c73 into dart-lang:main Aug 6, 2024
39 checks passed

davidmorgan deleted the macro-builder branch August 6, 2024 07:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Build macros, run them, serve a (test) service, host, connect to it #20

Build macros, run them, serve a (test) service, host, connect to it #20

davidmorgan commented Jul 22, 2024 •

edited

Loading

jakemac53 Jul 25, 2024

davidmorgan Aug 5, 2024

jakemac53 Jul 25, 2024

davidmorgan Aug 5, 2024

jakemac53 Aug 5, 2024 •

edited

Loading

davidmorgan Aug 6, 2024

jakemac53 Jul 25, 2024

davidmorgan Aug 5, 2024

jakemac53 Aug 5, 2024

davidmorgan Aug 6, 2024

jakemac53 Jul 25, 2024

davidmorgan Aug 5, 2024

jakemac53 Jul 25, 2024

davidmorgan Aug 5, 2024

jakemac53 Jul 25, 2024

davidmorgan Aug 5, 2024

jakemac53 Jul 25, 2024

davidmorgan Aug 5, 2024

jakemac53 Jul 25, 2024

davidmorgan Aug 5, 2024

jakemac53 Jul 25, 2024

davidmorgan Aug 5, 2024

davidmorgan left a comment

davidmorgan Aug 5, 2024

davidmorgan Aug 5, 2024

davidmorgan Aug 5, 2024

davidmorgan Aug 5, 2024

davidmorgan Aug 5, 2024

davidmorgan Aug 5, 2024

davidmorgan Aug 5, 2024

davidmorgan Aug 5, 2024

davidmorgan Aug 5, 2024

jakemac53 Aug 5, 2024

jakemac53 Aug 5, 2024

		final MacroServer macroServer;
		final ListOfServices services;

Build macros, run them, serve a (test) service, host, connect to it #20

Build macros, run them, serve a (test) service, host, connect to it #20

Conversation

davidmorgan commented Jul 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakemac53 Aug 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidmorgan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidmorgan commented Jul 22, 2024 •

edited

Loading

jakemac53 Aug 5, 2024 •

edited

Loading