Add a `--print-dtd` flag to print the DTD address served by DevTools server by kenzieschmoll · Pull Request #144272 · flutter/flutter

kenzieschmoll · 2024-02-27T23:08:03Z

Now that https://dart-review.googlesource.com/c/sdk/+/354460 has landed, DevTools server (or DDS depending on how DDS is started) will spawn the Dart Tooling Daemon (DTD). We need a way to access this URI from both the Dart CLI (dart-lang/sdk#55034) and Flutter CLI (#144270)

This PR

adds a command runner flag --print-dtd that is passed along to the DevToolsLauncher class. This value is then read when printing the DevTools output to determine whether we should also print a DTD uri.
[cleanup for testing] merges multiple FakeDevtoolsLauncher classes into a single fake defined in fakes.dart

Fixes #144270.

@bkonyi @christopherfujino

…ols server

flutter-dashboard · 2024-02-27T23:08:05Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

christopherfujino · 2024-02-27T23:43:49Z

packages/flutter_tools/lib/src/devtools_launcher.dart

+        final Match? dtdMatch = _serveDtdPattern.firstMatch(line);
+        if (dtdMatch != null) {
+          final String uri = dtdMatch[1]!;
+          dtdUri = Uri.parse(uri);


nit, we can return here and not bother checking for _serveDevToolsPattern, right?

No these are independent of one another. The _serveDevToolsPattern should always be in the stdout. The _serveDtdPattern might be there depending on whether the --print-dtd flag was passed to the dart devtools command above.

Would they be on the same line though?

No these will be on separate lines.

christopherfujino · 2024-02-27T23:45:13Z

packages/flutter_tools/lib/src/devtools_launcher.dart


-      devToolsUrl = await completer.future;
+      // We do not need to wait for a [Completer] holding the DTD URI because
+      // the DTD URI will be output to stdout before the DevTools URI. Awaiting


Is this an implementation detail that could be inadvertently broken in a future refactor?

Could also be a reason to use the nifty new FutureRecord2 extension method .wait() :)

https://api.dart.dev/stable/3.3.0/dart-async/FutureRecord2/wait.html

The DTD uri won't always be available here, so we can't await a completer with the guarantee that it will complete. I can add tests in https://dart-review.googlesource.com/c/sdk/+/354460 that verify the order, with a comment that if the order or strings should change, that we will need to update the Flutter tools launcher. We could already get in a broken state with the existing "Serving DevTools" string if that were ever to change.

ahhhh, I see. A test sounds great, thanks!

kenzieschmoll · 2024-02-28T00:23:56Z

Just added testing. Please review with "Hide whitespace" enabled. I fixed a formatting issue as a yak-shave.

kenzieschmoll · 2024-02-28T00:28:01Z

packages/flutter_tools/lib/src/runner/flutter_command_runner.dart

+    argParser.addFlag(
+      FlutterGlobalOptions.kPrintDtdUri,
+      negatable: false,
+      help: 'Print the URI of the Dart Tooling Daemon, if one is hosted by the Flutter CLI.',


@christopherfujino is it okay to say "the Flutter CLI" here? Technically it is hosted by DevTools server, but that seemed like an implementation detail that wouldn't mean anything to the end user.

christopherfujino

LGTM

christopherfujino · 2024-02-28T21:16:10Z

I'm seeing test failures that I'm 99% sure are not related to your change:

00:59 +597: test/commands.shard/hermetic/attach_test.dart: attach with one device and no specified target file local engine artifacts are passed to runner                                             
Error: Unable to find a built dart sdk at: "/path/to/local/engine/src/out/host_debug_unopt/dart-sdk" or a prebuilt dart sdk at: "/path/to/local/engine/src/flutter/prebuilts/linux-x64/dart-sdk"

https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8754867664597824945/+/u/run_test.dart_for_tool_tests_shard_and_subshard_commands/stdout

christopherfujino · 2024-02-28T21:38:30Z

merging in upstream master, in case it was fixed there. it might also have been a recipe change...

kenzieschmoll · 2024-02-29T16:50:12Z

The failure is being triggered from this line:
https://github.com/flutter/flutter/blob/master/packages/flutter_tools/lib/executable.dart/#L113

@christopherfujino any ideas on why that artifact would not be available in a test environment? Or how to make it so that it is available?

Another thought is to wrap DevtoolsLauncher.instance!.printDtdUri = shouldPrintDtdUri; in a try / catch in flutter_command_runner.dart and fail gracefully if this throws an exception. We likely don't want to crash the command runner if this fails anyway.

… print-dtd-uri

kenzieschmoll · 2024-02-29T17:50:30Z

All tests are passing with the try / catch. @christopherfujino let me know if you think this is an acceptable solution.

christopherfujino · 2024-02-29T18:18:33Z

All tests are passing with the try / catch. @christopherfujino let me know if you think this is an acceptable solution.

Hmm, a lot of this code (not from your PR) looks wrong to me, but I'm not fully grokking what happened. I'm investigating now...

…evTools server (flutter/flutter#144272)

Add a --print-dtd-uri flag to print the DTD address served by DevTo…

b20c693

…ols server

github-actions bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Feb 27, 2024

kenzieschmoll requested review from bkonyi and christopherfujino February 27, 2024 23:08

Add devtools_launcher test.

46fdf55

christopherfujino reviewed Feb 27, 2024

View reviewed changes

Add tests.

ebecaae

fix test strings

a6e027d

kenzieschmoll commented Feb 28, 2024

View reviewed changes

kenzieschmoll added 2 commits February 27, 2024 21:50

whitespace

5e8bfc4

Rename flag to fix args test

958730e

kenzieschmoll changed the title ~~Add a --print-dtd-uri flag to print the DTD address served by DevTools server~~ Add a --print-dtd flag to print the DTD address served by DevTools server Feb 28, 2024

fix comments

ae084bc

kenzieschmoll mentioned this pull request Feb 28, 2024

[dart cli] Add a flag to optionally print the Dart tooling daemon URI dart-lang/sdk#55034

Closed

Merge branch 'master' of github.com:flutter/flutter into print-dtd-uri

230bf91

christopherfujino approved these changes Feb 28, 2024

View reviewed changes

Merge branch 'master' into print-dtd-uri

d5cbf7c

bkonyi approved these changes Feb 28, 2024

View reviewed changes

kenzieschmoll added 3 commits February 29, 2024 08:55

add a try catch

e99004a

Merge branch 'print-dtd-uri' of github.com:kenzieschmoll/flutter into…

3def8ad

… print-dtd-uri

Merge branch 'master' of github.com:flutter/flutter into print-dtd-uri

4f04a08

fix test

43940dd

kenzieschmoll added 6 commits March 22, 2024 12:11

Merge branch 'master' of github.com:flutter/flutter into print-dtd-uri

7eac989

fix bug with printDtdUri getter

0b3f61b

fix test assertion

e78f8ad

add new line after DTD print

cd87138

kick bots

f47593a

Merge branch 'master' of github.com:flutter/flutter into print-dtd-uri

f02339e

kenzieschmoll merged commit 31f4f2b into flutter:master Mar 25, 2024

kenzieschmoll deleted the print-dtd-uri branch March 25, 2024 20:04

engine-flutter-autoroll mentioned this pull request Mar 27, 2024

Roll Flutter from dbdcead93225 to c881eee1d4dc (32 revisions) flutter/packages#6406

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 27, 2024

31f4f2b Add a --print-dtd flag to print the DTD address served by D…

e5ffbd0

…evTools server (flutter/flutter#144272)

engine-flutter-autoroll mentioned this pull request Mar 27, 2024

Roll Flutter from dbdcead93225 to 4eab267b4050 (33 revisions) flutter/packages#6407

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 27, 2024

31f4f2b Add a --print-dtd flag to print the DTD address served by D…

36d235c

…evTools server (flutter/flutter#144272)

engine-flutter-autoroll mentioned this pull request Mar 27, 2024

Roll Flutter from dbdcead93225 to 6c0a4caa8ad6 (34 revisions) flutter/packages#6408

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 27, 2024

31f4f2b Add a --print-dtd flag to print the DTD address served by D…

7a7a325

…evTools server (flutter/flutter#144272)

engine-flutter-autoroll mentioned this pull request Mar 27, 2024

Roll Flutter from dbdcead93225 to 4190b2ab7be7 (35 revisions) flutter/packages#6409

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 27, 2024

31f4f2b Add a --print-dtd flag to print the DTD address served by D…

dd0cca9

…evTools server (flutter/flutter#144272)

engine-flutter-autoroll mentioned this pull request Mar 27, 2024

Roll Flutter from dbdcead93225 to c6f9cdf1564d (36 revisions) flutter/packages#6410

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 27, 2024

31f4f2b Add a --print-dtd flag to print the DTD address served by D…

7c345ae

…evTools server (flutter/flutter#144272)

engine-flutter-autoroll mentioned this pull request Mar 28, 2024

Manual roll Flutter from dbdcead93225 to f5b65bad0f81 (41 revisions) flutter/packages#6418

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 28, 2024

31f4f2b Add a --print-dtd flag to print the DTD address served by D…

9a4fcad

…evTools server (flutter/flutter#144272)

engine-flutter-autoroll mentioned this pull request Mar 28, 2024

Roll Flutter from dbdcead93225 to 1940955e89a3 (45 revisions) flutter/packages#6420

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 28, 2024

31f4f2b Add a --print-dtd flag to print the DTD address served by D…

7165f9d

…evTools server (flutter/flutter#144272)

engine-flutter-autoroll mentioned this pull request Mar 28, 2024

Roll Flutter from dbdcead93225 to 66d1872cd5cd (46 revisions) flutter/packages#6421

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 28, 2024

31f4f2b Add a --print-dtd flag to print the DTD address served by D…

2e433b9

…evTools server (flutter/flutter#144272)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 28, 2024

31f4f2b Add a --print-dtd flag to print the DTD address served by D…

0c83712

…evTools server (flutter/flutter#144272)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 28, 2024

31f4f2b Add a --print-dtd flag to print the DTD address served by D…

311e5d0

…evTools server (flutter/flutter#144272)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 28, 2024

31f4f2b Add a --print-dtd flag to print the DTD address served by D…

d4f3baf

…evTools server (flutter/flutter#144272)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 28, 2024

31f4f2b Add a --print-dtd flag to print the DTD address served by D…

031e1f6

…evTools server (flutter/flutter#144272)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 28, 2024

31f4f2b Add a --print-dtd flag to print the DTD address served by D…

32a6c30

…evTools server (flutter/flutter#144272)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 14, 2024

31f4f2b Add a --print-dtd flag to print the DTD address served by D…

06c3b33

…evTools server (flutter/flutter#144272)

Conversation

kenzieschmoll commented Feb 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flutter-dashboard bot commented Feb 27, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kenzieschmoll Feb 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kenzieschmoll commented Feb 28, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christopherfujino left a comment

Choose a reason for hiding this comment

Uh oh!

christopherfujino commented Feb 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

christopherfujino commented Feb 28, 2024

Uh oh!

kenzieschmoll commented Feb 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kenzieschmoll commented Feb 29, 2024

Uh oh!

christopherfujino commented Feb 29, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kenzieschmoll commented Feb 27, 2024 •

edited

Loading

kenzieschmoll Feb 28, 2024 •

edited

Loading

christopherfujino commented Feb 28, 2024 •

edited

Loading

kenzieschmoll commented Feb 29, 2024 •

edited

Loading