[flutter_tools] port bash script to use sysctl not uname on macOS #101308

christopherfujino · 2022-04-04T18:34:27Z

For context, a similar Flutter tool change was made in https://github.com/flutter/flutter/pull/67970/files

To test the bash logic, you can update line 66:

To test sysctl returning exit code 1 on x64 and newer macOS: QUERY="false"

It should log: Downloading Darwin x64 Dart SDK from Flutter engine $ENGINE_VERSION...

To test sysctl returning exit code 0 (success) but printing 0 (arm64 not present) on x64 and older macOS: QUERY="echo 0"

It should log: Downloading Darwin x64 Dart SDK from Flutter engine $ENGINE_VERSION...

To test sysctl returning exit code 0 (success) and printing 1 (arm64 present) on arm64: QUERY="echo 1"

It should log: Downloading Darwin arm64 Dart SDK from Flutter engine $ENGINE_VERSION...

jmagman · 2022-04-04T18:52:50Z

This works on macOS 12.3.1 x64 machine:

$ flutter
Downloading Darwin x64 Dart SDK from Flutter engine 801d8c530fc5b0c102d13d5913dbd98376b99c93...

A macOS 12.1 M1 machine running w/ Rosetta:

$ uname -m
x86_64
flutter$ flutter
Downloading Darwin arm64 Dart SDK from Flutter engine 801d8c530fc5b0c102d13d5913dbd98376b99c93...

Same Mac running w/ Rosetta:

$ uname -m                                            
arm64
flutter$ flutter            
Downloading Darwin arm64 Dart SDK from Flutter engine 801d8c530fc5b0c102d13d5913dbd98376b99c93...

flutter-dashboard · 2022-04-04T19:38:28Z

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.

jmagman · 2022-04-04T20:59:26Z

bin/internal/update_dart_sdk.sh

+  # `uname -m` may be running in Rosetta mode, instead query sysctl
+  if [ "$OS" = 'Darwin' ]; then
+    ARCH="arm64"
+    # This will exit 1 if the CPU arch is not ARM64


On x64 it will either exit with a failure code, or it will exit successfully and print 0 (I actually don't see that anymore, but I did see that working on #67970, though it may have been a beta version of macOS):

$ sysctl hw.optional.arm64 sysctl: unknown oid 'hw.optional.arm64' $ echo $? 1

On arm64 it will print 1 and exit successfully.

argh, we need to parse the output then?

at least it looks like we can do sysctl -n hw.optional.arm64 https://github.com/pytorch/pytorch/blob/e7ca62be08f96b07efc1271baf9b02d4e7be226a/cmake/Modules/FindARM.cmake#L44

It's annoying we have to ALSO check the exit code though

If I recall the last time I looked at this on old versions of macOS it exited 1, on new versions of macOS on x64 it exited successfully and printed 0, and on arm64 it printed 1. However on latest macOS on my x64 machine it's exiting failure with sysctl: unknown oid 'hw.optional.arm64'. So maybe that's not the behavior anymore.

Updated this to mirror the logic of #67970

zanderso

lgtm. This will need manual testing on both x64 and arm64 macs.

bin/internal/update_dart_sdk.sh

christopherfujino · 2022-04-04T23:20:47Z

I have tested this works on x64 mac.

jmagman · 2022-04-04T23:26:45Z

Confirmed 20f64ac works on x64 (downloads x64), arm64 (downloads arm64), arm64 w/ Rosetta (downloads arm64).

jmagman · 2022-04-04T23:42:41Z

To test, consider an integration test that validates

$ file bin/cache/dart-sdk/bin/dart
bin/cache/dart-sdk/bin/dart: Mach-O 64-bit executable x86_64

$ file bin/cache/dart-sdk/bin/dart
bin/cache/dart-sdk/bin/dart: Mach-O 64-bit executable arm64

similar to

flutter/packages/flutter_tools/test/integration.shard/ios_content_validation_test.dart

Lines 259 to 263 in 6ab0075

    
           final ProcessResult archs = processManager.runSync( 
        
             <String>['file', pluginFrameworkBinary.path], 
        
           ); 
        
           expect(archs.stdout, contains('Mach-O 64-bit dynamically linked shared library x86_64')); 
        
           expect(archs.stdout, contains('Mach-O 64-bit dynamically linked shared library arm64'));

We won't get arm64 test coverage until #87508 is done.

christopherfujino · 2022-04-04T23:59:32Z

To test, consider an integration test that validates
$ file bin/cache/dart-sdk/bin/dart
bin/cache/dart-sdk/bin/dart: Mach-O 64-bit executable x86_64
similar to

flutter/packages/flutter_tools/test/integration.shard/ios_content_validation_test.dart

Lines 259 to 263 in 6ab0075

final ProcessResult archs = processManager.runSync(

<String>['file', pluginFrameworkBinary.path],

);

expect(archs.stdout, contains('Mach-O 64-bit dynamically linked shared library x86_64'));

expect(archs.stdout, contains('Mach-O 64-bit dynamically linked shared library arm64'));

We won't get arm64 test coverage until #87508 is done.

Good idea, writing this test now

christopherfujino · 2022-04-05T00:29:03Z

Added an integration test, @jmagman PTAL

jmagman

LGTM

jmagman · 2022-04-05T00:32:02Z

packages/flutter_tools/test/integration.shard/bash_entrypoint_test.dart

@@ -14,14 +14,19 @@ import 'test_utils.dart';
 final String flutterRootPath = getFlutterRoot();
 final Directory flutterRoot = fileSystem.directory(flutterRootPath);

+enum MacArches {


nit: there's probably a better name than bash_entrypoint_test for this test file since it's doing more now.

It's technically testing the entrypoint, since the entrypoint is responsible for caching the dart SDK, but I'm open to suggestions.

It's fine as-is.

… macOS (flutter/flutter#101308)

…acOS (flutter/flutter#101308)

* 'stable' of https://github.com/flutter/flutter: (1203 commits) 'Update Engine revision to ffe7b86a1e5b5cb63c8385ae1adc759e372ee8f4 for stable release 3.0.3' (flutter#106431) [flutter_releases] Removing minor iOS version filter value from ci.yaml (flutter#105059) (flutter#105629) [flutter_releases] Flutter stable 3.0.2 Framework Cherrypicks (flutter#105528) [framework] ensure ink sparkle is disposed (flutter#104569) (flutter#105144) [CP] Fix Flutter doctor crash on Windows caused by bad UTF8 from vswhere.exe (flutter#105153) [tool][web] Use 'constructor' instead of 'public field declarations' in flutter.js (Safari 13) (flutter#105162) [flutter_releases] Cherrypicks for SliverReorderableList and Slider regressions (flutter#105141) 'Update Engine revision to caaafc5604ee9172293eb84a381be6aadd660317 for stable release 3.0.1' (flutter#104213) [flutter_releases] Flutter stable 2.13.0 Framework Cherrypicks (flutter#103375) [flutter_releases] Flutter beta 2.13.0-0.4.pre Framework Cherrypicks (flutter#103101) Enforce cpu explicitly for Mac devicelab test beds (flutter#101871) (flutter#102685) [flutter_releases] Flutter beta 2.13.0-0.3.pre Framework Cherrypicks (flutter#102620) [flutter_releases] Upgrade dwds to 12.1.1 (flutter#101546) [flutter_releases] Flutter beta 2.13.0-0.2.pre Framework Cherrypicks (flutter#102193) [flutter_releases] Flutter beta 2.12.0-4.1.pre Framework Cherrypicks (flutter#101771) [flutter_releases] Cherrypick engine to c341645 (flutter#101608) Revert "Refactor `ToggleButtons` (remove `RawMaterialButton`) (flutter#99493)" (flutter#101538) [Cherrypick] Partial revert of super params in tools (flutter#101436) (flutter#101527) Roll Engine from e7e7ca1 to b48d65e (10 revisions) (flutter#101370) [flutter_tools] port bash script to use sysctl not uname on macOS (flutter#101308) ...

christopherfujino force-pushed the use-sysctl-not-uname branch from 1c011a3 to 7a33043 Compare April 4, 2022 19:38

christopherfujino requested review from timsneath and Jasguerrero April 4, 2022 19:38

christopherfujino marked this pull request as ready for review April 4, 2022 19:38

christopherfujino changed the title ~~[flutter_tools] port bash script to use sysctl not uname~~ [flutter_tools] port bash script to use sysctl not uname on macOS Apr 4, 2022

christopherfujino mentioned this pull request Apr 4, 2022

Fail dart assert in Linux web_canvaskit_tests_7_last #101312

Closed

jmagman reviewed Apr 4, 2022

View reviewed changes

christopherfujino requested a review from zanderso April 4, 2022 23:11

zanderso approved these changes Apr 4, 2022

View reviewed changes

bin/internal/update_dart_sdk.sh Outdated Show resolved Hide resolved

bin/internal/update_dart_sdk.sh Outdated Show resolved Hide resolved

jmagman approved these changes Apr 5, 2022

View reviewed changes

christopherfujino added 5 commits April 5, 2022 09:49

port bash script to use sysctl not uname

eb53190

update comment

9d1c766

update comment

32eb6cd

check exit code AND stdout

b6b2b49

code review

00269de

christopherfujino force-pushed the use-sysctl-not-uname branch from 3d4c5e6 to 00269de Compare April 5, 2022 16:51

christopherfujino merged commit 6bb63ed into flutter:master Apr 5, 2022

christopherfujino deleted the use-sysctl-not-uname branch April 5, 2022 21:17

engine-flutter-autoroll mentioned this pull request Apr 5, 2022

Roll Flutter from fbdbb094ab48 to 6bb63ed0c8e8 (2 revisions) flutter/packages#1404

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 5, 2022

6bb63ed0c [flutter_tools] port bash script to use sysctl not uname on…

8ef6d1b

… macOS (flutter/flutter#101308)

engine-flutter-autoroll mentioned this pull request Apr 5, 2022

Roll Flutter from 5c6918933e15 to 5788f5ef07c2 (61 revisions) flutter/plugins#5179

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Apr 5, 2022

6bb63ed [flutter_tools] port bash script to use sysctl not uname on m…

bdac5f1

…acOS (flutter/flutter#101308)

christopherfujino mentioned this pull request Apr 8, 2022

[flutter_tools] add test for dart binary arch #101604

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[flutter_tools] port bash script to use sysctl not uname on macOS #101308

[flutter_tools] port bash script to use sysctl not uname on macOS #101308

christopherfujino commented Apr 4, 2022 •

edited

Loading

jmagman commented Apr 4, 2022 •

edited

Loading

flutter-dashboard bot commented Apr 4, 2022

jmagman Apr 4, 2022

christopherfujino Apr 4, 2022

christopherfujino Apr 4, 2022 •

edited

Loading

jmagman Apr 4, 2022

christopherfujino Apr 4, 2022

zanderso left a comment

christopherfujino commented Apr 4, 2022

jmagman commented Apr 4, 2022

jmagman commented Apr 4, 2022 •

edited

Loading

christopherfujino commented Apr 4, 2022

christopherfujino commented Apr 5, 2022

jmagman left a comment

jmagman Apr 5, 2022

christopherfujino Apr 5, 2022

jmagman Apr 5, 2022

[flutter_tools] port bash script to use sysctl not uname on macOS #101308

[flutter_tools] port bash script to use sysctl not uname on macOS #101308

Conversation

christopherfujino commented Apr 4, 2022 • edited Loading

jmagman commented Apr 4, 2022 • edited Loading

flutter-dashboard bot commented Apr 4, 2022

jmagman Apr 4, 2022

Choose a reason for hiding this comment

christopherfujino Apr 4, 2022

Choose a reason for hiding this comment

christopherfujino Apr 4, 2022 • edited Loading

Choose a reason for hiding this comment

jmagman Apr 4, 2022

Choose a reason for hiding this comment

christopherfujino Apr 4, 2022

Choose a reason for hiding this comment

zanderso left a comment

Choose a reason for hiding this comment

christopherfujino commented Apr 4, 2022

jmagman commented Apr 4, 2022

jmagman commented Apr 4, 2022 • edited Loading

christopherfujino commented Apr 4, 2022

christopherfujino commented Apr 5, 2022

jmagman left a comment

Choose a reason for hiding this comment

jmagman Apr 5, 2022

Choose a reason for hiding this comment

christopherfujino Apr 5, 2022

Choose a reason for hiding this comment

jmagman Apr 5, 2022

Choose a reason for hiding this comment

christopherfujino commented Apr 4, 2022 •

edited

Loading

jmagman commented Apr 4, 2022 •

edited

Loading

christopherfujino Apr 4, 2022 •

edited

Loading

jmagman commented Apr 4, 2022 •

edited

Loading