This repository was archived by the owner on Feb 25, 2025. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 6k
extend const_finder to allow skipping particular classes #37257
Merged
Merged
Changes from 1 commit
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
f35217c
wip
christopherfujino f06d70a
extend const_finder_test to compile web dills
christopherfujino 779e03a
add test
christopherfujino 6785ce6
add back tests, including non-const for web
christopherfujino 7abff6d
update run_tests.py
christopherfujino 66c5236
fix whitespace
christopherfujino e7d1185
clean up test file
christopherfujino 6d0431d
add new options to cli
christopherfujino 4f2a9a3
use annotation rather than explicit deny list
christopherfujino 1263b6d
clean up
christopherfujino ac499c7
Apply suggestions from code review
christopherfujino 0977118
clean up
christopherfujino 5894e4b
code review
christopherfujino File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
extend const_finder_test to compile web dills
- Loading branch information
commit f06d70a9d122ddbca1a45820514207cb2239fadd
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,37 +35,26 @@ void expectInstances(dynamic value, dynamic expected) { | |
| } | ||
| } | ||
|
|
||
| final String basePath = | ||
| path.canonicalize(path.join(path.dirname(Platform.script.toFilePath()), '..')); | ||
| final String fixtures = path.join(basePath, 'test', 'fixtures'); | ||
| final String box = path.join(fixtures, 'lib', 'box.dart'); | ||
| final String consts = path.join(fixtures, 'lib', 'consts.dart'); | ||
| final String packageConfig = path.join(fixtures, '.dart_tool', 'package_config.json'); | ||
| final String constsAndNon = path.join(fixtures, 'lib', 'consts_and_non.dart'); | ||
| final String boxDill = path.join(fixtures, 'box.dill'); | ||
| final String constsDill = path.join(fixtures, 'consts.dill'); | ||
| final String constsAndNonDill = path.join(fixtures, 'consts_and_non.dill'); | ||
|
|
||
| // This test is assuming the `dart` used to invoke the tests is compatible | ||
| // with the version of package:kernel in //third-party/dart/pkg/kernel | ||
| final String dart = Platform.resolvedExecutable; | ||
| final String bat = Platform.isWindows ? '.bat' : ''; | ||
|
|
||
| void _checkRecursion() { | ||
| void _checkRecursion(String dillPath) { | ||
| stdout.writeln('Checking recursive calls.'); | ||
| final ConstFinder finder = ConstFinder( | ||
| kernelFilePath: boxDill, | ||
| kernelFilePath: dillPath, | ||
| classLibraryUri: 'package:const_finder_fixtures/box.dart', | ||
| className: 'Box', | ||
| ); | ||
| // Will timeout if we did things wrong. | ||
| jsonEncode(finder.findInstances()); | ||
| } | ||
|
|
||
| void _checkConsts() { | ||
| void _checkConsts(String dillPath) { | ||
| stdout.writeln('Checking for expected constants.'); | ||
| final ConstFinder finder = ConstFinder( | ||
| kernelFilePath: constsDill, | ||
| kernelFilePath: dillPath, | ||
| classLibraryUri: 'package:const_finder_fixtures/target.dart', | ||
| className: 'Target', | ||
| ); | ||
|
|
@@ -99,7 +88,7 @@ void _checkConsts() { | |
| ); | ||
|
|
||
| final ConstFinder finder2 = ConstFinder( | ||
| kernelFilePath: constsDill, | ||
| kernelFilePath: dillPath, | ||
| classLibraryUri: 'package:const_finder_fixtures/target.dart', | ||
| className: 'MixedInTarget', | ||
| ); | ||
|
|
@@ -114,10 +103,10 @@ void _checkConsts() { | |
| ); | ||
| } | ||
|
|
||
| void _checkNonConsts() { | ||
| void _checkNonConsts(String dillPath) { | ||
| stdout.writeln('Checking for non-constant instances.'); | ||
| final ConstFinder finder = ConstFinder( | ||
| kernelFilePath: constsAndNonDill, | ||
| kernelFilePath: dillPath, | ||
| classLibraryUri: 'package:const_finder_fixtures/target.dart', | ||
| className: 'Target', | ||
| ); | ||
|
|
@@ -168,68 +157,168 @@ void _checkNonConsts() { | |
| ); | ||
| } | ||
|
|
||
| void checkProcessResult(ProcessResult result) { | ||
| if (result.exitCode != 0) { | ||
| stdout.writeln(result.stdout); | ||
| stderr.writeln(result.stderr); | ||
| } | ||
| expect(result.exitCode, 0); | ||
| } | ||
|
|
||
| Future<void> main(List<String> args) async { | ||
| if (args.length != 2) { | ||
| stderr.writeln('The first argument must be the path to the forntend server dill.'); | ||
| if (args.length != 3) { | ||
| stderr.writeln('The first argument must be the path to the frontend server dill.'); | ||
| stderr.writeln('The second argument must be the path to the flutter_patched_sdk'); | ||
| stderr.writeln('The third argument must be the path to libraries.json'); | ||
| exit(-1); | ||
| } | ||
| final String frontendServer = args[0]; | ||
| final String sdkRoot = args[1]; | ||
| try { | ||
| void checkProcessResult(ProcessResult result) { | ||
| if (result.exitCode != 0) { | ||
| stdout.writeln(result.stdout); | ||
| stderr.writeln(result.stderr); | ||
|
|
||
| TestRunner( | ||
| frontendServer: args[0], | ||
| sdkRoot: args[1], | ||
| librariesSpec: args[2], | ||
| ).test(); | ||
| } | ||
|
|
||
| final String basePath = | ||
| path.canonicalize(path.join(path.dirname(Platform.script.toFilePath()), '..')); | ||
| final String fixtures = path.join(basePath, 'test', 'fixtures'); | ||
| final String packageConfig = path.join(fixtures, '.dart_tool', 'package_config.json'); | ||
|
|
||
| class TestRunner { | ||
| TestRunner({ | ||
| required this.frontendServer, | ||
| required this.sdkRoot, | ||
| required this.librariesSpec, | ||
| }); | ||
|
|
||
| //static final String box = path.join(fixtures, 'lib', 'box.dart'); | ||
| //static final String consts = path.join(fixtures, 'lib', 'consts.dart'); | ||
| //static final String constsAndNon = path.join(fixtures, 'lib', 'consts_and_non.dart'); | ||
|
|
||
| final String frontendServer; | ||
| final String sdkRoot; | ||
| final String librariesSpec; | ||
|
|
||
| void test() { | ||
| final List<_Test> tests = <_Test>[ | ||
| _Test( | ||
| name: 'box', | ||
| dartSource: path.join(fixtures, 'lib', 'box.dart'), | ||
| frontendServer: frontendServer, | ||
| sdkRoot: sdkRoot, | ||
| librariesSpec: librariesSpec, | ||
| verify: _checkRecursion, | ||
| ), | ||
| _Test( | ||
| name: 'consts', | ||
| dartSource: path.join(fixtures, 'lib', 'consts.dart'), | ||
| frontendServer: frontendServer, | ||
| sdkRoot: sdkRoot, | ||
| librariesSpec: librariesSpec, | ||
| verify: _checkConsts, | ||
| ), | ||
| _Test( | ||
| name: 'consts_and_non', | ||
| dartSource: path.join(fixtures, 'lib', 'consts_and_non.dart'), | ||
| frontendServer: frontendServer, | ||
| sdkRoot: sdkRoot, | ||
| librariesSpec: librariesSpec, | ||
| verify: _checkNonConsts, | ||
| ), | ||
| ]; | ||
| try { | ||
| stdout.writeln('Generating kernel fixtures...'); | ||
|
|
||
| for (final _Test test in tests) { | ||
| test.run(); | ||
| } | ||
| } finally { | ||
| try { | ||
| for (final _Test test in tests) { | ||
| test.dispose(); | ||
| } | ||
| } finally { | ||
| stdout.writeln('Tests ${exitCode == 0 ? 'succeeded' : 'failed'} - exit code: $exitCode'); | ||
| } | ||
| expect(result.exitCode, 0); | ||
| } | ||
| } | ||
|
|
||
| stdout.writeln('Generating kernel fixtures...'); | ||
| stdout.writeln(consts); | ||
| } | ||
|
|
||
| checkProcessResult(Process.runSync(dart, <String>[ | ||
| frontendServer, | ||
| '--sdk-root=$sdkRoot', | ||
| '--target=flutter', | ||
| '--aot', | ||
| '--tfa', | ||
| '--packages=$packageConfig', | ||
| '--output-dill=$boxDill', | ||
| box, | ||
| ])); | ||
| class _Test { | ||
| _Test({ | ||
| required this.name, | ||
| required this.dartSource, | ||
| required this.sdkRoot, | ||
| required this.verify, | ||
| required this.frontendServer, | ||
| required this.librariesSpec, | ||
| }); | ||
|
|
||
| final String name; | ||
| final String dartSource; | ||
| final String sdkRoot; | ||
| final String frontendServer; | ||
| final String librariesSpec; | ||
| void Function(String dillPath) verify; | ||
|
|
||
| final List<String> resourcesToDispose = <String>[]; | ||
|
|
||
| void run() { | ||
| final String tfaDill = path.join(fixtures, '$name-tfa.dill'); | ||
| stdout.writeln('Compiling $dartSource to $tfaDill'); | ||
| _compileTFADill(tfaDill); | ||
| stdout.writeln('Testing $tfaDill'); | ||
| verify(tfaDill); | ||
|
|
||
| final String webDill = path.join(fixtures, '$name-web.dill'); | ||
| stdout.writeln('Compiling $dartSource to $webDill'); | ||
| _compileWebDill(webDill, dartSource); | ||
| verify(webDill); | ||
| stdout.writeln('Testing $webDill'); | ||
| } | ||
|
|
||
| void dispose() { | ||
| for (final String resource in resourcesToDispose) { | ||
| stdout.writeln('Deleting $resource'); | ||
| File(resource).deleteSync(); | ||
| } | ||
| } | ||
|
|
||
| void _compileTFADill(String dillPath) { | ||
| checkProcessResult(Process.runSync(dart, <String>[ | ||
| frontendServer, | ||
| '--sdk-root=$sdkRoot', | ||
| '--target=flutter', | ||
| '--aot', | ||
| '--tfa', | ||
| '--packages=$packageConfig', | ||
| '--output-dill=$constsDill', | ||
| consts, | ||
| '--output-dill=$dillPath', | ||
| dartSource, | ||
| ])); | ||
|
|
||
| resourcesToDispose.add(dillPath); | ||
| } | ||
|
|
||
| void _compileWebDill(String dillPath, String dartSource) { | ||
| checkProcessResult(Process.runSync(dart, <String>[ | ||
| frontendServer, | ||
| '--sdk-root=$sdkRoot', | ||
| '--target=flutter', | ||
| '--aot', | ||
| '--tfa', | ||
| 'compile', | ||
| 'js', | ||
| '--libraries-spec=$librariesSpec', | ||
| '-Ddart.vm.product=true', | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sigmundch Does
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It does. The Flutter Web uses the constant within the flutter framework/engine code. They use it, for example, to choose whether certain code path is executed (e.g. debug only code). |
||
| '-o', | ||
| dillPath, | ||
| '--packages=$packageConfig', | ||
| '--output-dill=$constsAndNonDill', | ||
| constsAndNon, | ||
| '--cfe-only', | ||
| dartSource, | ||
| ])); | ||
|
|
||
| _checkRecursion(); | ||
| _checkConsts(); | ||
| _checkNonConsts(); | ||
| } finally { | ||
| try { | ||
| File(constsDill).deleteSync(); | ||
| File(constsAndNonDill).deleteSync(); | ||
| } finally { | ||
| stdout.writeln('Tests ${exitCode == 0 ? 'succeeded' : 'failed'} - exit code: $exitCode'); | ||
| } | ||
| resourcesToDispose.add(dillPath); | ||
| } | ||
| } | ||
|
|
||
| enum Compiler { | ||
| frontendServer, | ||
| dart2js, | ||
| } | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line needs to be wrapped in an exception handler. The failure to find and delete this file in the cleanup phase is causing the test to fail, and our HHH builders to fail (testing head of flutter/flutter, flutter/engine, and dart-lang/sdk together).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will open a PR to catch this. However, I'm wondering why the file doesn't exist on HHH? Is it never getting created or is some other process deleting it first?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, it looks like the problem is a little bit earlier:
So even if I catch the delete exception, I think this test would fail anyway since it depends on the dart2js snapshot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may have been failing because of a change that tried to use the prebuilt SDK, which was reverted. Could it be interacting with flutter/flutter#115242?
The hhh and monorepo builders should be able to build the full SDK, or whatever is needed. But they do not have a prebuilt Dart SDK that they download. The monorepo builder and HHH builder do use --local-engine when running flutter commands: https://dart.googlesource.com/monorepo/+/refs/heads/main
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @eyebrowsoffire @jonahwilliams may know the answer to @whesse's question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really have context into this issue. I reverted flutter/flutter#115242 because it broke the flutter tool for me on an m1. I can't imagine that any recepie changes depended on that yet, because the build would have broken after I reverted, or before I landed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@whesse I think the action item here is--when building the Dart SDK on the HHH bot, ensure that //engine/src/out/host_debug_unopt/dart-sdk/bin/snapshots/dart2js.dart.snapshot is built (I'm assuming that //engine/src/out/host_debug_unopt/dart-sdk/bin/dart is present, since this script also references that binary).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has caused our HHH builds to fail. The modes where we run
const_finder.dartneed to have full dart sdk built, since it uses now dart2js. So the GN invocation needs--full-dart-sdk.On Dart side, I'll updat our recipes in recipes/cl/271382 and monorepo/cl/271384