Skip to content

flutter_tool: only enable wasm compile in master channel#121755

Merged
auto-submit[bot] merged 3 commits intomasterfrom
wasm_experimental
Mar 2, 2023
Merged

flutter_tool: only enable wasm compile in master channel#121755
auto-submit[bot] merged 3 commits intomasterfrom
wasm_experimental

Conversation

@kevmoo
Copy link
Contributor

@kevmoo kevmoo commented Mar 2, 2023 •
edited
Loading

Added an associated Feature

@flutter-dashboard flutter-dashboard bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Mar 2, 2023
@flutter-dashboard

This comment was marked as outdated.

Sign in to view
@kevmoo kevmoo marked this pull request as draft March 2, 2023 00:46
@kevmoo
Copy link
Contributor Author

kevmoo commented Mar 2, 2023

Need to wait on the parent PR to land, first

@kevmoo kevmoo force-pushed the wasm_experimental branch 3 times, most recently from b3457bb to 3602381 Compare March 2, 2023 01:59
@kevmoo kevmoo changed the title wasm experimental flutter_tool: only enable wasm compile in master channel Mar 2, 2023
@kevmoo
Copy link
Contributor Author

kevmoo commented Mar 2, 2023

@christopherfujino – suggestions on adding tests for this? Or not needed?

@kevmoo kevmoo marked this pull request as ready for review March 2, 2023 02:06
@kevmoo kevmoo requested a review from eyebrowsoffire March 2, 2023 02:06
@kevmoo
Copy link
Contributor Author

kevmoo commented Mar 2, 2023

@eyebrowsoffire – the only thing I worry about here is flutter_build_wasm_test.dart – will start failing on non-master branches. Not sure how to wire up things there. An environment variable?

@christopherfujino
Copy link
Contributor

christopherfujino commented Mar 2, 2023

@christopherfujino – suggestions on adding tests for this? Or not needed?

Yeah, can you add a test here: https://github.com/flutter/flutter/blob/master/packages/flutter_tools/test/general.shard/features_test.dart

eyebrowsoffire
eyebrowsoffire approved these changes Mar 2, 2023
View reviewed changes
Copy link
Contributor

@eyebrowsoffire eyebrowsoffire left a comment

Choose a reason for hiding this comment

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

LGTM!

christopherfujino
christopherfujino reviewed Mar 2, 2023
View reviewed changes
christopherfujino
christopherfujino approved these changes Mar 2, 2023
View reviewed changes
Copy link
Contributor

@christopherfujino christopherfujino left a comment

Choose a reason for hiding this comment

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

nit about capitalization of JavaScript, otherwise LGTM

@kevmoo kevmoo force-pushed the wasm_experimental branch from bf0b573 to dfd85e6 Compare March 2, 2023 20:27
@kevmoo kevmoo added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 2, 2023
@auto-submit
Copy link
Contributor

auto-submit bot commented Mar 2, 2023

auto label is removed for flutter/flutter, pr: 121755, due to - The status or check suite Mac tool_integration_tests_1_4 has failed. Please fix the issues identified (or deflake) before re-applying this label.

@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Mar 2, 2023
kevmoo
kevmoo commented Mar 2, 2023
View reviewed changes
packages/flutter_tools/lib/src/features.dart
/// All current Flutter feature flags that can be configured.
///
/// [Feature.configSetting] is not `null`.
Iterable<Feature> get allConfigurableFeatures => allFeatures.where((Feature feature) => feature.configSetting != null);
Copy link
Contributor Author

@kevmoo kevmoo Mar 2, 2023

Choose a reason for hiding this comment

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

@christopherfujino – seems there are a number of tests that assume every feature is configurable. Figured this was a clean way to address it.

@kevmoo kevmoo added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 2, 2023
@auto-submit auto-submit bot merged commit a8cc95e into master Mar 2, 2023
@auto-submit auto-submit bot deleted the wasm_experimental branch March 2, 2023 22:21
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Mar 3, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 3, 2023
@engine-flutter-autoroll
a8cc95e flutter_tool: only enable wasm compile in master channel (flu…
0e4a318 
…tter/flutter#121755)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Mar 3, 2023
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 3, 2023
@engine-flutter-autoroll
a8cc95e flutter_tool: only enable wasm compile in master channel (flu…
4a9c445 
…tter/flutter#121755)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 10, 2023
@engine-flutter-autoroll
a8cc95e flutter_tool: only enable wasm compile in master channel (flu…
eb163bf 
…tter/flutter#121755)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 10, 2023
@engine-flutter-autoroll
a8cc95e flutter_tool: only enable wasm compile in master channel (flu…
f9061d2 
…tter/flutter#121755)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@eyebrowsoffire eyebrowsoffire eyebrowsoffire approved these changes

+1 more reviewer

@christopherfujino christopherfujino christopherfujino approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

autosubmit Merge PR when tree becomes green via auto submit App tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@kevmoo @christopherfujino @eyebrowsoffire