Skip to content

Conversation

@kevmoo
Copy link
Contributor

@kevmoo kevmoo commented May 2, 2023

Also fixed some comments

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

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 Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on 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.

@kevmoo kevmoo requested a review from eyebrowsoffire May 2, 2023 20:22
@kevmoo kevmoo force-pushed the wasm_cli_tweaks branch from a038e81 to 43b9598 Compare May 2, 2023 20:56
@kevmoo kevmoo added the platform-web Web applications specifically label May 2, 2023
help: 'Run wasm-opt on the output wasm module.',
help:
'Optimize output wasm using the Binaryen (https://github.com/WebAssembly/binaryen) tool.\n'
'Increases the build time, but will yield a smaller, faster output.',
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be opt out. The main goal is to show that wasm is faster, so the default should zipline straight to that goal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Uh. Okay. Funny. I was going to add a --debuginfo line for wasmopt.

Maybe we opt-in to wasmopt by default but also add --debuginfo so we can debug issues.

Copy link
Contributor

@eyebrowsoffire eyebrowsoffire May 3, 2023

Choose a reason for hiding this comment

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

Maybe we should just make this dependent on debug vs profile vs release. That would be

  • debug = no wasm-opt
  • profile = wasm-opt but add -g so that we get debug info
  • release = wasm-opt and no -g

That seems most consistent with normal expectations, and it means that wasm-opt is opt-out instead of opt-in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a GREAT idea. Will do in a follow-up!

@kevmoo kevmoo added the autosubmit Merge PR when tree becomes green via auto submit App label May 2, 2023
@auto-submit
Copy link
Contributor

auto-submit bot commented May 2, 2023

auto label is removed for flutter/flutter, pr: 125907, due to - The status or check suite Windows build_tests_2_3 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 May 2, 2023
@kevmoo kevmoo added the autosubmit Merge PR when tree becomes green via auto submit App label May 2, 2023
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label May 2, 2023
@auto-submit
Copy link
Contributor

auto-submit bot commented May 2, 2023

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

@Hixie
Copy link
Contributor

Hixie commented May 2, 2023

test-exempt: changes documentation lines in help; help lines already have some level of verification and none of the fixes here seem amenable to automation.

@Hixie Hixie closed this May 2, 2023
@Hixie Hixie reopened this May 2, 2023
@Hixie
Copy link
Contributor

Hixie commented May 2, 2023

Sorry, didn't mean to close it, was just in the triage meeting where that was the button I click all the time. 😆

@kevmoo kevmoo added the autosubmit Merge PR when tree becomes green via auto submit App label May 2, 2023
@auto-submit
Copy link
Contributor

auto-submit bot commented May 2, 2023

auto label is removed for flutter/flutter, pr: 125907, due to - The status or check suite customer_testing-linux 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 May 2, 2023
@kevmoo kevmoo added the autosubmit Merge PR when tree becomes green via auto submit App label May 2, 2023
@auto-submit
Copy link
Contributor

auto-submit bot commented May 2, 2023

auto label is removed for flutter/flutter, pr: 125907, due to - The status or check suite customer_testing-linux 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 May 2, 2023
@kevmoo kevmoo added the autosubmit Merge PR when tree becomes green via auto submit App label May 3, 2023
@auto-submit auto-submit bot merged commit 2c56a25 into master May 3, 2023
@auto-submit auto-submit bot deleted the wasm_cli_tweaks branch May 3, 2023 21:02
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 4, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 4, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 4, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 4, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 4, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 4, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 16, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants