Update builder logic for the Dart Debug Extension #1717

elliette · 2022-08-11T23:58:37Z

Adds a shell script for building the extension
Builds the dart2js-compiled app to build/web_prod directory instead of web (this new directory is gitignored so the generated files will never be committed to github)
Builds all dart files, not just background.dart (currently background.dart is the only dart file, but with the Manifest v3 migration there will be more)
Copies the Google developer key (from a gitignored extension_key.txt file) into the manifest.json in the /build directory so that it is never accidentally committed
For tests that load the debug extension, we first build the extension (because the generated files are no longer checked into github)

elliette · 2022-08-12T17:30:12Z

Adding @natebosch for advice on improving builder logic, thanks!

natebosch

The Builder implementation looks good - using the known 1:1 correlation between the input and output defined in buildExtensions looks like a nice way to handle this.

natebosch · 2022-08-12T17:33:36Z

dwds/debug_extension/tool/copy_builder.dart

+    final allowedOutputs = buildStep.allowedOutputs;
+
+    if (allowedOutputs.isEmpty) {
+      print('Skipping ${inputAsset.path} which has no output options.');


Are these temporary? I think we typically expect builders to be silent unless there is a user actionable problem.

Have you hit these lines in practice?

I haven't hit them in practice! Removed

natebosch · 2022-08-12T17:34:16Z

dwds/debug_extension/tool/copy_builder.dart

+    if (outputAsset.path.endsWith('.png')) {
+      _copyBinaryFile(buildStep,


It should be safe to copy all files as bytes and that will simplify things a bit

Done, that works!

natebosch · 2022-08-12T17:38:36Z

dwds/debug_extension/CONTRIBUTING.md

-> *At this point, you should manually verify that everything is working by following the steps in [Local Development](#local-development).*
+1. Update the version in `web/manifest_prod.json`, `pubspec.yaml`, and in the `CHANGELOG`. 
+2. Follow the instructions above to build the dart2js-compiled release version of the extension.
+> *At this point, you should manually verify that everything is working by following the steps in [Local Development](#local-development), except load the extension from the `build/web_prod` directory. You will need to add a manifest developer key to the `manifest.json` in the `build/web_prod` directory.


[nit] reflow to 80 columns. There is an internal formatter if you'd like to use it, but you'd probably want to first format the entire file in a separate commit. go/mdformat

dwds/debug_extension/tool/copy_builder.dart

natebosch · 2022-08-18T20:03:23Z

dwds/debug_extension/tool/build_extension.sh

+        shift;;
+esac
+
+# Clean existing build directory to avoid merge conflicts.


How often did you find this was necessary? We don't expect a clean to be necessary for every build in typical workflows.

Ah, solved the problem with --delete-conflicting-outputs flag. Was trying to prevent the prompt about conflicts from showing up when running the continuous integration tests

natebosch · 2022-08-18T20:24:03Z

dwds/debug_extension/tool/copy_builder.dart

 class _CopyBuilder extends Builder {
  @override
  Map<String, List<String>> get buildExtensions => {
-        _backgroundJsId.path: [_backgroundJsCopyId.path]
+        "web/{{}}.dart.js": ["build/web_prod/{{}}.js"],


Do I understand correctly that you are outputting into <package root>/build here, but that also you run the build with -o build?

Do you get a mix of "to source" written output, and then the merged output all in the same directory?

Would it work to change this builder to be to-cache instead of to-source, and make the directory move into a subdirectory of web/?

Suggested change

"web/{{}}.dart.js": ["build/web_prod/{{}}.js"],

"web/{{}}.dart.js": ["web/web_prod/{{}}.js"],

I think that should result in the -o build copying these files into where you expect them.

Ah thank you! Updated so now:

building with DDC builds to a dev_build directory

building with dart2js builds to the build directory, but then copies just the build/web directory over to prod_build

annagrin · 2022-08-18T20:44:25Z

dwds/test/fixtures/context.dart

+    final process = await Process.run('tool/build_extension.sh', ['prod'],
+        runInShell: true);
+    print(process.stdout);
+    Directory.current = currentDir;


Can we wrap the building code in a try/finally and put the Directory.current = currentDir; in a finally block so the directory is restored on exceptions?

Good point, done!

annagrin · 2022-08-18T22:56:27Z

dwds/test/fixtures/context.dart

+      return;
+    }
+    try {
+      Directory.current = '$currentDir/debug_extension';


Does this work on windows?

Not sure how to test this because I don't have a Windows machine. I tried re-enabling the tests for this change, and at head:

this branch: https://github.com/dart-lang/webdev/runs/7909473406?check_suite_focus=true

at head: https://github.com/dart-lang/webdev/runs/7910680435?check_suite_focus=true

For both, the failure is coming from the fact that the client is null, so it doesn't even get here.

The debug extension tests have been disabled on Windows for 3 years. When we re-enable them, I'm guessing there are quite a few things we will need to change to get them to work.

The client is null probably due to some race during retry, I presume the actual failure is different and we don't see it in the test output - the tests are failing originally right after the build has succeeded. As for disabled tests - yeah it looks like an omission - probably disabled in the beginning when the windows tests were set up and we never came back to it. Do we have users of the extension on windows machines? If not, we could make the feature not supported on windows (with an error if someone tries). I think flutter allows to use the extension with the flutter run -d web-server but I might be wrong:)

If we are making changes to the extension build and I suppose subsequent changes to the extension, it would be great to understand what we are missing and have a plan - for example, how do we test extension changes until the tests start working? I presume we would need to run tests manually on a windows machine every time a change to the extension is made.

@grouma do you know what was the problem with running the extension tests on windows?

Just for context, it looks like we have never tested the extension on Windows (if I understand this correctly): #712

It looks like since adding Windows-test support, the debug extension test has been disabled.

annagrin · 2022-08-18T22:57:52Z

dwds/test/fixtures/context.dart

+          runInShell: true);
+      print(process.stdout);
+    } catch (e) {
+      _logger.warning('Error building Debug Extension: $e');


Is swallowing the exception and continuing what we would want to do in that case? Or do we want to rethrow at this point so the test fails?

Good point, rethrowing the exception

dwds/test/fixtures/context.dart

annagrin · 2022-08-18T23:35:11Z

dwds/test/fixtures/context.dart

      final capabilities = Capabilities.chrome
        ..addAll({
          Capabilities.chromeOptions: {
            'args': [
              'remote-debugging-port=$debugPort',
-              if (enableDebugExtension) '--load-extension=debug_extension/web',
+              if (enableDebugExtension)
+                '--load-extension=debug_extension/prod_build',


does this work on windows as well?

Same as previous comment, we don't get here due to other failures on Windows

annagrin · 2022-08-19T17:51:30Z

dwds/test/fixtures/context.dart

+          'Expected to be in /dwds directory, instead path was $currentDir.');
+    }
+    try {
+      Directory.current = '$currentDir/debug_extension';


We chatted offline about running the build runner directly, so the extension build works on windows as well (potentially, after we fix the tests)

One more thing I realized - we might not need to change the current directory (which is prone to races, especially on retries), but just run the build runner process with the working directory parameter set to the right directory. Would this work?

Unfortunately it does not work, looks like only top-level directories are permitted as args to build_runner build:
Only top level directories such as 'web' or 'test' are allowed as positional args, but got 'debug_extension/web'

elliette added 5 commits August 11, 2022 14:30

Modified copy builder

ffa56a9

Delete manifest.json

fffc803

Add manifest.json to gitignore

ca2667a

Add shell script for building the extension

87d0b86

Update documentation

59da8d7

elliette requested a review from annagrin August 11, 2022 23:58

Fix analyze error

7012062

elliette requested a review from natebosch August 12, 2022 17:29

natebosch approved these changes Aug 12, 2022

View reviewed changes

annagrin reviewed Aug 15, 2022

View reviewed changes

dwds/debug_extension/tool/copy_builder.dart Outdated Show resolved Hide resolved

dwds/debug_extension/tool/copy_builder.dart Outdated Show resolved Hide resolved

elliette added 10 commits August 17, 2022 12:05

Merge in master

4db90cf

Update copy builder in response to review comments

229220b

Remove background.js

6a38612

Trying to fix test

6663f6b

Format

a8f112d

Better way of handling extension key

6bc0ddc

Test passing locally

e9f2cd6

Fix analyze error

b9ccc5b

Remove new line

33c7bb2

Only add extension key in dev mode

6479625

natebosch reviewed Aug 18, 2022

View reviewed changes

annagrin reviewed Aug 18, 2022

View reviewed changes

elliette added 3 commits August 18, 2022 15:36

Respond to PR comments

ba9404d

Respond to PR comments cont

99f65ae

reorder flags and use full flag names

d5f4151

annagrin reviewed Aug 18, 2022

View reviewed changes

elliette added 2 commits August 18, 2022 17:02

Error handling and test on windows

a146c37

Skip on windows again

973bc9c

elliette mentioned this pull request Aug 19, 2022

Re-enable debug extension tests on Windows #1724

Open

annagrin reviewed Aug 19, 2022

View reviewed changes

Remove run in shell

af42b98

elliette merged commit a82a5a9 into dart-lang:master Aug 19, 2022

This was referenced Aug 19, 2022

Pass working directory to Process.run when building debug extension in tests #1726

Merged

Modifies the update_dev_manifest script #1727

Merged

		if (outputAsset.path.endsWith('.png')) {
		_copyBinaryFile(buildStep,

	"web/{{}}.dart.js": ["build/web_prod/{{}}.js"],
	"web/{{}}.dart.js": ["web/web_prod/{{}}.js"],

Update builder logic for the Dart Debug Extension #1717

Update builder logic for the Dart Debug Extension #1717

Uh oh!

Conversation

elliette commented Aug 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elliette commented Aug 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

natebosch left a comment

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!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

annagrin Aug 19, 2022 • 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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

annagrin Aug 19, 2022 • 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!

elliette Aug 19, 2022 • 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!

Uh oh!

elliette commented Aug 11, 2022 •

edited

Loading

elliette commented Aug 12, 2022 •

edited

Loading

annagrin Aug 19, 2022 •

edited

Loading

annagrin Aug 19, 2022 •

edited

Loading

elliette Aug 19, 2022 •

edited

Loading