Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Add WASM target in gn #31670

Merged
merged 26 commits into from
Mar 16, 2022
Merged

Conversation

harryterkelsen
Copy link
Contributor

Rolls to latest version of ICU which supports building with WASM.
Adds emsdk to DEPS to get the emscripten toolchain working.

Addresses flutter/flutter#52588

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
    writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

Rolls to latest version of ICU which supports building with WASM.
Adds emsdk to DEPS to get the emscripten toolchain working.
@harryterkelsen harryterkelsen added the Work in progress (WIP) Not ready (yet) for review! label Feb 24, 2022
Copy link
Contributor

@dnfield dnfield left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly nits/discussion

tools/gn Outdated
Comment on lines 150 to 152
if args.target_os != 'wasm':
gn_args['flutter_use_fontconfig'] = args.enable_fontconfig
gn_args['flutter_enable_skshaper'] = args.enable_skshaper
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? Don't we want to use SkShaper if wasm?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

flutter_enable_skshaper is unused if you don't build Flutter, so setting it will cause gn to fail

tools/gn Outdated
if args.target_os == 'winuwp':
gn_args['skia_enable_winuwp'] = True
gn_args['is_official_build'] = True # Disable Skia test utilities.
gn_args['dart_component_kind'] = 'static_library' # Always link Dart in statically.
if args.target_os != 'wasm':
gn_args['dart_component_kind'] = 'static_library' # Always link Dart in statically.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the idea that this arg is unused by wasm an GN/Ninja complain?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. If an arg is unused then gn will fail.

@skia-gold
Copy link

Gold has detected about 2 new digest(s) on patchset 9.
View them at https://flutter-engine-gold.skia.org/cl/github/31670

@harryterkelsen
Copy link
Contributor Author

/cc @eyebrowsoffire

Copy link
Contributor

@yjbanov yjbanov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@@ -110,7 +110,8 @@ Future<void> copySkiaTestImages() async {
'images',
));

for (final io.File imageFile in testImagesDir.listSync(recursive: true).whereType<io.File>()) {
for (final io.File imageFile
in testImagesDir.listSync(recursive: true).whereType<io.File>()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: please undo formatting-only changes. Flutter style guide does not use dartfmt.

.listSync(recursive: true, followLinks: true)
.whereType<io.File>();
Iterable<io.File> canvasKitFiles;
if (builtLocalCanvasKit) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This if/else can be combined with the if/else below. Unless I'm missing something, there no shared logic applied to canvasKitFiles. I think it can also reduce the amount of file path massaging done for the profile build.

@@ -184,7 +220,8 @@ Future<void> compileTests(List<FilePath> testFiles) async {
}

// Maximum number of concurrent dart2js processes to use.
const int _dart2jsConcurrency = int.fromEnvironment('FELT_DART2JS_CONCURRENCY', defaultValue: 8);
const int _dart2jsConcurrency =
int.fromEnvironment('FELT_DART2JS_CONCURRENCY', defaultValue: 8);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto: undo

@harryterkelsen harryterkelsen removed the Work in progress (WIP) Not ready (yet) for review! label Mar 15, 2022
@skia-gold
Copy link

Gold has detected about 18 new digest(s) on patchset 26.
View them at https://flutter-engine-gold.skia.org/cl/github/31670

@flutter-dashboard
Copy link

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

Changes reported for pull request #31670 at sha 5ef3e78

@harryterkelsen harryterkelsen merged commit 2309bcc into flutter:main Mar 16, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 17, 2022
zanderso pushed a commit to flutter/flutter that referenced this pull request Mar 17, 2022
* 09d7bcc Optionally specify the target dir in tools/gn (flutter/engine#32065)

* a00ba24 Fix done button click not blur in iOS keyboard (flutter/engine#31718)

* 81547d1 Add a display list op to clear to transformation stack. (flutter/engine#32050)

* 2309bcc Add WASM target in gn (flutter/engine#31670)

* 852e800 [web] Remove the --passfail flag when calling goldctl in post-submit (flutter/engine#32071)

* eb1c50d Fix issues with nested gradients in html renderer. (flutter/engine#31887)

* fb0fd74 Update the magic number for JPEG to just FF D8 FF. (flutter/engine#32076)

* 233c17c Wrap the global timeline event handler callback in a std::atomic (flutter/engine#32073)

* dfde2aa Roll Dart SDK from 24bf86f16411 to 5bc905e69609 (9 revisions) (flutter/engine#32075)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants