Skip to content

Add missing files in the Gradle wrapper directory#39145

Merged
blasten merged 9 commits intoflutter:masterfrom
blasten:fix_aar_properties
Aug 28, 2019
Merged

Add missing files in the Gradle wrapper directory#39145
blasten merged 9 commits intoflutter:masterfrom
blasten:fix_aar_properties

Conversation

@blasten
Copy link

@blasten blasten commented Aug 23, 2019

Description

We currently assume that if <app>/android/gradlew exists, then there's a wrapper and gradle-wrapper.properties. However, this isn't true in some cases. (This plugin for example: https://github.com/sroddy/flutter_string_encryption)

Related Issues

Fixes #38691

Tests

I added the following tests:

Unit tests in gradle_test.dart and filesystem_test.dart

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.

@fluttergithubbot fluttergithubbot added the tool Affects the "flutter" command-line tool. See also t: labels. label Aug 23, 2019
@blasten blasten added the t: gradle "flutter build" and "flutter run" on Android label Aug 23, 2019
@blasten blasten mentioned this pull request Aug 23, 2019
Closed
zanderso
zanderso approved these changes Aug 23, 2019
View reviewed changes
Copy link
Member

@zanderso zanderso left a comment

Choose a reason for hiding this comment

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

lgtm w/ nit

christopherfujino
christopherfujino reviewed Aug 23, 2019
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.

two questions

packages/flutter_tools/lib/src/base/file_system.dart
Directory srcDir,
Directory destDir,
{
bool shouldCopyFile(File srcFile, File destFile),
Copy link
Contributor

@christopherfujino christopherfujino Aug 23, 2019

Choose a reason for hiding this comment

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

rather than a callback closure, what if this instead was a optional boolean argument shouldOverwrite that defaulted to true?

Copy link
Author

@blasten blasten Aug 23, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I thought about it. I ended up going with a callback because I see this as a generic utility--someone may want to filter based on other criteria.

Copy link
Contributor

@christopherfujino christopherfujino Aug 23, 2019

Choose a reason for hiding this comment

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

👍

packages/flutter_tools/lib/src/base/file_system.dart
entity,
destDir.fileSystem.directory(newPath),
shouldCopyFile: shouldCopyFile,
onFileCopied: onFileCopied,
Copy link
Contributor

@christopherfujino christopherfujino Aug 23, 2019

Choose a reason for hiding this comment

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

is onFileCopied supposed to be passed on to recursive calls? It seems in the old version it was not.

Copy link
Author

@blasten blasten Aug 23, 2019

Choose a reason for hiding this comment

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

Based on the original description of this function, it looks like it was a bug :)

Copy link
Contributor

@christopherfujino christopherfujino Aug 23, 2019

Choose a reason for hiding this comment

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

sgtm

christopherfujino
christopherfujino approved these changes Aug 23, 2019
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.

lgtm

@blasten blasten force-pushed the fix_aar_properties branch from 097b4ce to f6dfcd5 Compare August 28, 2019 00:01
@codecov
Copy link

codecov bot commented Aug 28, 2019 •
edited
Loading

Codecov Report

Merging #39145 into master will increase coverage by 1.32%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #39145      +/-   ##
==========================================
+ Coverage   55.75%   57.08%   +1.32%     
==========================================
  Files         195      195              
  Lines       18496    18502       +6     
==========================================
+ Hits        10312    10561     +249     
+ Misses       8184     7941     -243
Flag Coverage Δ
#flutter_tool 57.08% <100%> (+1.32%) ⬆️
Impacted Files Coverage Δ
packages/flutter_tools/lib/src/android/gradle.dart 62.39% <100%> (+0.92%) ⬆️
...ackages/flutter_tools/lib/src/commands/create.dart 72.45% <100%> (ø) ⬆️
packages/flutter_tools/lib/src/project.dart 83.7% <100%> (ø) ⬆️
...ckages/flutter_tools/lib/src/base/file_system.dart 69.64% <100%> (ø) ⬆️
...ckages/flutter_tools/lib/src/commands/channel.dart 43.07% <0%> (-29.24%) ⬇️
...ges/flutter_tools/lib/src/build_system/source.dart 82.43% <0%> (-4.06%) ⬇️
...ools/lib/src/build_runner/resident_web_runner.dart 67.16% <0%> (-2.99%) ⬇️
packages/flutter_tools/lib/src/cache.dart 43.02% <0%> (-0.73%) ⬇️
packages/flutter_tools/lib/src/vmservice.dart 37.46% <0%> (-0.46%) ⬇️
packages/flutter_tools/lib/src/base/logger.dart 82.06% <0%> (-0.39%) ⬇️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c1cb680...3c28abb. Read the comment docs.

@blasten blasten merged commit 4a1c62c into flutter:master Aug 28, 2019
@blasten blasten deleted the fix_aar_properties branch August 28, 2019 21:52
@christopherfujino christopherfujino mentioned this pull request Aug 29, 2019
Merged
@blasten blasten mentioned this pull request Sep 10, 2019
Closed
@jmagman jmagman mentioned this pull request Apr 7, 2020
Closed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@zanderso zanderso zanderso 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

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.

Executing flutter build aar failed with exit code 1

5 participants

@blasten @zanderso @christopherfujino @googlebot @fluttergithubbot