Skip to content

Refactor Gradle plugin #34353

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
blasten merged 2 commits into from
Jun 13, 2019
Merged

Refactor Gradle plugin #34353

blasten merged 2 commits into from
Jun 13, 2019

Conversation

@blasten
Copy link

@blasten blasten commented Jun 12, 2019 •
edited
Loading

Description

Cleans up flutter.gradle

Related Issues

Helps #33064

Tests

n/a

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I signed the CLA.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (Please read Handling breaking changes). Replace this with a link to the e-mail where you asked for input on this proposed change.
  • No, this is not a breaking change.

@blasten blasten added t: gradle "flutter build" and "flutter run" on Android tool Affects the "flutter" command-line tool. See also t: labels. labels Jun 12, 2019
@blasten blasten changed the title Fix local engine Fix local engine override in Gradle Jun 12, 2019
@blasten blasten requested a review from mklim June 12, 2019 21:04
mklim
mklim reviewed Jun 12, 2019
View reviewed changes
Copy link
Contributor

@mklim mklim left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The CI failure looks suspiciously related to me, what do you think?

packages/flutter_tools/gradle/flutter.gradle Outdated
Copy link
Contributor

@mklim mklim Jun 12, 2019

Choose a reason for hiding this comment

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

nit, subjective/optional: I think having the 20 different variables per Jar was more verbose, but ultimately safer than this. If there's a typo in either the setting or getting of any of these string keys this will end up being null, and the reason why will be hard to debug because the language won't complain about a mismatched key. With named variables it'll at least fail harder and faster if there's a typo in the variable in any of these places. You could guard against it some and still keep the map by keeping these constants, the assert you added will help catch it too.

Copy link
Author

@blasten blasten Jun 12, 2019

Choose a reason for hiding this comment

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

I added a TODO to figure out what to do with this code as part of the refactoring. Agreed, strings are less safe, but I'd like to eventually remove this code, or change the way it's structured: #33064

@blasten blasten changed the title Fix local engine override in Gradle Refactor Gradle plugin Jun 12, 2019
@blasten blasten requested a review from mklim June 12, 2019 22:14
mklim
mklim approved these changes Jun 13, 2019
View reviewed changes
Copy link
Contributor

@mklim mklim left a comment

Choose a reason for hiding this comment

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

LGTM

@blasten blasten merged commit 1c6cda9 into flutter:master Jun 13, 2019
@blasten blasten deleted the local_engine branch June 13, 2019 23:05
tango5614 added a commit to tango5614/flutter that referenced this pull request Jun 14, 2019
@tango5614
Merge branch 'master' into fix28096
51b43c2 
* master: (24 commits)
  [flutter_tool,fuchsia] Update the install flow for packaging migration. (flutter#34447)
  SliverFillRemaining flag for different use cases (flutter#33627)
  SizedBox documentation (flutter#34424)
  Change API doc link to api.dart.dev (flutter#34388)
  2589785 Roll src/third_party/skia 87e885038893..c3252a04b377 (3 commits) (flutter/engine#9327) (flutter#34484)
  ace5d59 Fix rawTypes errors in Android embedding classes (flutter/engine#9326) (flutter#34481)
  bf0def6 Roll src/third_party/skia 4c4945a25248..87e885038893 (1 commits) (flutter/engine#9325) (flutter#34471)
  Roll engine f1d821d..6f5347c (13 commits) (flutter#34466)
  Allow "from" hero state to survive hero animation in a push transition (flutter#32842)
  Roll pub dependencies (flutter#33677)
  Skip flaky test on Windows (flutter#34464)
  Allow flaky tests to pass or fail and mark web tests as flaky (flutter#34456)
  Dont depend on web SDK unless running tests on chrome (flutter#34457)
  Fix semantics_tester (flutter#34368)
  Include raw value in Diagnostics json for basic types (flutter#34417)
  Refactor Gradle plugin (flutter#34353)
  Allow web tests to fail in cirrus config (flutter#34436)
  skip bottom_sheet (flutter#34430)
  Remove unused flag `--target-platform` from `flutter run` (flutter#34369)
  Extract DiagnosticsNode serializer from WidgetInspector (flutter#34012)
  ...
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@mklim mklim mklim approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

t: gradle "flutter build" and "flutter run" on Android 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

@blasten @mklim @googlebot