Skip to content

[tool] ⚡️ Install the corresponding APK in flutter run#112768

Merged
auto-submit[bot] merged 6 commits intoflutter:masterfrom
AlexV525:correct-installing-apk-filename
Oct 18, 2022
Merged

[tool] ⚡️ Install the corresponding APK in flutter run#112768
auto-submit[bot] merged 6 commits intoflutter:masterfrom
AlexV525:correct-installing-apk-filename

Conversation

@AlexV525
Copy link
Member

@AlexV525 AlexV525 commented Oct 2, 2022 •
edited
Loading

Fix #111585. The PR makes the tool installs exactly the generated APK file, which corrects the console's output too. Also, SHA1 will be generated for all generated APKs in the batch.

Filename comparison (DEBUG Mode)

Install APK file Before After
No flavor app.apk app-debug.apk
Flavor configured app.apk app-flavor-debug.apk

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, including Features we expect every widget to implement.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

@flutter-dashboard flutter-dashboard bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Oct 2, 2022
@AlexV525 AlexV525 marked this pull request as ready for review October 3, 2022 17:07
@AlexV525 AlexV525 changed the title [tool] 🐛 Produce more precise filename for the installing APK [tool] ⚡️ Install the corresponding APK in flutter run Oct 4, 2022
@christopherfujino christopherfujino requested review from christopherfujino and removed request for jonahwilliams October 6, 2022 20:43
christopherfujino
christopherfujino reviewed Oct 17, 2022
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.

This looks like the correct fix to make, I'll defer final approval to @GaryQian though.

@AlexV525
Copy link
Member Author

AlexV525 commented Oct 17, 2022

I'm not sure if any external tools are using app.apk in their usages, that's the only thing I'm concerning.

@GaryQian
Copy link
Contributor

GaryQian commented Oct 17, 2022 •
edited
Loading

It's likely that there are tools depending on the app.apk copy. It may be reasonable for backwards compatibility to continue copying the first apk and deprecate the usage of the copy. We can add a TODO and phase it out later.

Otherwise, the change looks good!

@AlexV525
Copy link
Member Author

AlexV525 commented Oct 17, 2022

https://github.com/flutter/flutter/pull/112768/checks?check_run_id=8940587074 The failure doesn't seem to be related.

GaryQian
GaryQian approved these changes Oct 17, 2022
View reviewed changes
Copy link
Contributor

@GaryQian GaryQian left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks!

@GaryQian GaryQian added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 18, 2022
@auto-submit auto-submit bot merged commit 0ad71e8 into flutter:master Oct 18, 2022
@AlexV525 AlexV525 deleted the correct-installing-apk-filename branch October 18, 2022 02:39
@AlexV525
Copy link
Member Author

AlexV525 commented Oct 18, 2022

It may be reasonable for backwards compatibility to continue copying the first apk and deprecate the usage of the copy.

It's unclear how to deprecate a usage like this. The tool itself is not using the file at present, and other tools won't access the file through flutter_tools, so we can't raise a deprecate message for it.

@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Oct 18, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Oct 18, 2022
@engine-flutter-autoroll
0ad71e8 [tool] ⚡️ Install the corresponding APK in flutter run (flu…
91de9c6 
…tter/flutter#112768)
@christopherfujino
Copy link
Contributor

christopherfujino commented Oct 18, 2022

It may be reasonable for backwards compatibility to continue copying the first apk and deprecate the usage of the copy.

It's unclear how to deprecate a usage like this. The tool itself is not using the file at present, and other tools won't access the file through flutter_tools, so we can't raise a deprecate message for it.

Agreed, not sure what we can do other than stop creating it and see what breaks.

@godofredoc godofredoc added revert Autorevert PR (with "Reason for revert:" comment) and removed revert Autorevert PR (with "Reason for revert:" comment) labels Oct 18, 2022
godofredoc added a commit that referenced this pull request Oct 18, 2022
@godofredoc
Revert "[tool] ⚡️ Install the corresponding APK in flutter run (#11…
c745145 
…2768)"

This reverts commit 0ad71e8.
@godofredoc godofredoc mentioned this pull request Oct 18, 2022
Closed
@godofredoc
Copy link
Contributor

godofredoc commented Oct 18, 2022

This PR has been reverted because is blocking the tree. \cc @christopherfujino @AlexV525

@godofredoc godofredoc mentioned this pull request Oct 18, 2022
Merged
godofredoc added a commit that referenced this pull request Oct 18, 2022
@godofredoc
Revert "[tool] ⚡️ Install the corresponding APK in flutter run (#11…
4fa91c9 
…2768)" (#113614)

This reverts commit 0ad71e8.
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Oct 18, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Oct 18, 2022
@engine-flutter-autoroll
0ad71e8 [tool] ⚡️ Install the corresponding APK in flutter run (flu…
8785ec6 
…tter/flutter#112768)
@AlexV525
Copy link
Member Author

AlexV525 commented Oct 18, 2022 •
edited
Loading

OK so in the next PR I think we should try to remove the copy and continue to track if any breaks.

@AlexV525
Copy link
Member Author

AlexV525 commented Oct 18, 2022

@christopherfujino @gspencergoog Could you help me with dumping the tests log? I can't access to the tree's dashboard due to my network issues currently, or maybe a tracking issue. Thanks!

@christopherfujino
Copy link
Contributor

christopherfujino commented Oct 18, 2022

@christopherfujino @gspencergoog Could you help me with dumping the tests log? I can't access to the tree's dashboard due to my network issues currently, or maybe a tracking issue. Thanks!

I will open a tracking issue and link from here

@christopherfujino
Copy link
Contributor

christopherfujino commented Oct 18, 2022

@AlexV525 looks like an easy fix #113620

AlexV525 added a commit to AlexV525/flutter that referenced this pull request Oct 18, 2022
@AlexV525
Revert "Revert "[tool] ⚡️ Install the corresponding APK in `flutter r…
2d31b10 
…un` (flutter#112768)" (flutter#113614)"

This reverts commit 4fa91c9.
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Oct 18, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Oct 18, 2022
@engine-flutter-autoroll
0ad71e8 [tool] ⚡️ Install the corresponding APK in flutter run (flu…
705cd8e 
…tter/flutter#112768)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Oct 19, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 19, 2022
@engine-flutter-autoroll
0ad71e8 [tool] ⚡️ Install the corresponding APK in flutter run (flu…
6931ebf 
…tter/flutter#112768)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@christopherfujino christopherfujino christopherfujino left review comments

@GaryQian GaryQian GaryQian 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.

[tool] flutter run runs only app.apk

4 participants

@AlexV525 @GaryQian @christopherfujino @godofredoc