Fix Arm64 detection logic for devices by rpzrpzrpz · Pull Request #40373 · flutter/flutter

rpzrpzrpz · 2019-09-12T18:34:42Z

Description

Flutter tools assumes that all 64 bit ARM cpus are able to run 64 bit code.

There are manufacturers that use 64 bit ARM cpus while installing 32 bit Android OS on it.

Added code that looks at the OS for 32 or 64 bit arm so dev tools do not try to download
64 bit code to 32 bit Operating systems.

Tests

As far as tests, I have tested with my available android devices including Kindle Fire 8 and Motorola LG v30.

But this is device detection, so it requires physical devices more than I have available.

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.

[ x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
[ x] I signed the [CLA].
[ x] 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.

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.
[X ] No, this is not a breaking change.

fluttergithubbot · 2019-09-12T18:34:43Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. While there are exceptions to this rule, if this patch modifies code it is probably not an exception.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

googlebot · 2019-09-12T18:34:50Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

rpzrpzrpz · 2019-09-12T19:08:12Z

@googlebot I signed it!

googlebot · 2019-09-12T19:08:15Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

jmagman

You have analyzer issues, braces go on the same line, spaces between // and the comment text. Match the code around where you are changing.

   info • Prefer final for variable declarations if they are not reassigned • packages/flutter_tools/lib/src/android/android_device.dart:195:18 • prefer_final_locals

Check out the style guide: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo
2. You will needs tests for this. See android_device_test.dart.

jmagman · 2019-09-12T19:33:15Z

 Diego Velásquez <[email protected]>
-Sarbagya Dhaubanjar <[email protected]>
+Sarbagya Dhaubanjar <[email protected]>
+Mark Diener <[email protected]>


Re-add the newline.

jmagman · 2019-09-12T19:34:37Z

        case 'arm64-v8a':
-          _platform = TargetPlatform.android_arm64;
+
+          //Original code assumed that CPU was enough. But you need to verify 64 bit OS as well.


This code will be around for a long time and edited by many people, so comments like "original code" aren't super informative. A better comment would explain why just checking the CPU isn't sufficient.

jmagman · 2019-09-12T19:35:10Z

+          String gabilist = await _getProperty('ro.product.cpu.abilist');
+          if (gabilist == null)
+          {
+            //Have not seen a device without an abilist in adb, but it is possible.


Very possible. #10330.

jmagman · 2019-09-12T19:35:27Z

+          }
+
+          //Confirm that our 64 bit arm cpu has a 64 bit OS running on it
+          if (gabilist.contains('arm64-v8a') == true)


Don't need the == true.

jmagman · 2019-09-12T19:38:01Z

+          }
+          else
+          {
+            //Sorry Amazon Kindle Fire 8, You tried trick me, but you failed.


This comment is cute, but not super informative. Maybe something like:

Some devices, like the Kindle Fire 8, misreport abilist as <blah>. Fall back to android_arm64.

jonahwilliams

We should have some existing tests that cover this code. They should be updated with new cases + synthetic devices to ensure this logic is covered and doesn't get accidentally reverted

jonahwilliams · 2019-09-12T19:40:12Z

+          //Original code assumed that CPU was enough. But you need to verify 64 bit OS as well.
+          String gabilist = await _getProperty('ro.product.cpu.abilist');
+          if (gabilist == null)
+          {


Please follow the style guide at https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo

In this case place the opening brace on the same line as the if.

jonahwilliams · 2019-09-12T19:40:46Z

        case 'arm64-v8a':
-          _platform = TargetPlatform.android_arm64;
+
+          //Original code assumed that CPU was enough. But you need to verify 64 bit OS as well.


Some links to external documentation or issues here would be helpful too.

jmagman · 2019-09-12T22:00:33Z

You will needs tests for this. See android_device_test.dart.

To clarify, that probably means mock out out the results of the 'ro.product.cpu.abilist' property fetch, to make sure the TargetPlatform gets set correctly, and also making sure other tests have it mocked out so they pass (returns null instead of actually fetching some value from a plugged in device, for example).

rpzrpzrpz · 2019-10-03T14:00:11Z

I will get back to this very shortly. I am just trying to get some other deadlines met in the meantime.

jonahwilliams · 2019-11-15T18:47:57Z

Closing due to lack of activity. I've filled #44999 to capture the issue here

Fix Arm64 detection logic for devices

c915b28

fluttergithubbot added the tool Affects the "flutter" command-line tool. See also t: labels. label Sep 12, 2019

googlebot added the cla: no label Sep 12, 2019

googlebot added cla: yes and removed cla: no labels Sep 12, 2019

jmagman requested changes Sep 12, 2019

View reviewed changes

jonahwilliams suggested changes Sep 12, 2019

View reviewed changes

zanderso added the waiting for response The Flutter team cannot make further progress on this issue until the original reporter responds label Sep 20, 2019

zanderso assigned jonahwilliams Oct 24, 2019

jonahwilliams mentioned this pull request Nov 15, 2019

arm64 device detection is broken for certain Android devices #44999

Closed

jonahwilliams closed this Nov 15, 2019

jonahwilliams mentioned this pull request Nov 19, 2019

Update Android CPU device detection #45139

Merged

github-actions Bot locked as resolved and limited conversation to collaborators Aug 3, 2021

Uh oh!

Conversation

rpzrpzrpz commented Sep 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Tests

Checklist

Uh oh!

fluttergithubbot commented Sep 12, 2019

Uh oh!

googlebot commented Sep 12, 2019

What to do if you already signed the CLA

Individual signers

Corporate signers

Uh oh!

rpzrpzrpz commented Sep 12, 2019

Uh oh!

googlebot commented Sep 12, 2019

Uh oh!

jmagman left a comment

Choose a reason for hiding this comment

Uh oh!

jmagman Sep 12, 2019

Choose a reason for hiding this comment

Uh oh!

jmagman Sep 12, 2019

Choose a reason for hiding this comment

Uh oh!

jmagman Sep 12, 2019

Choose a reason for hiding this comment

Uh oh!

jmagman Sep 12, 2019

Choose a reason for hiding this comment

Uh oh!

jmagman Sep 12, 2019

Choose a reason for hiding this comment

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

jonahwilliams Sep 12, 2019

Choose a reason for hiding this comment

Uh oh!

jonahwilliams Sep 12, 2019

Choose a reason for hiding this comment

Uh oh!

jmagman commented Sep 12, 2019

Uh oh!

rpzrpzrpz commented Oct 3, 2019

Uh oh!

jonahwilliams commented Nov 15, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

rpzrpzrpz commented Sep 12, 2019 •

edited

Loading