Skip to content

Conversation

@pbo-linaro
Copy link
Contributor

@pbo-linaro pbo-linaro commented Oct 31, 2023

It's now possible to natively compile a flutter app for
windows-arm64. Cross-compilation is not yet implemented.

Uses arm64 artifacts now available for Dart/Flutter.
Platform detection is based on Abi class, provided by Dart. Depending if
Dart is an arm64 or x64 binary, the Abi is set accordingly.
Initial bootstrap of dart artifacts (update_dart_sdk.ps1) is checking
PROCESSOR_ARCHITECTURE environment variable, which is the way to detect
host architecture on Windows.

This is available only for master channel (on other channels, it
fallbacks to windows-x64).

On windows-x64, it produces an x64 app. On windows-arm64, it produces an
arm64 app.

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.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@github-actions github-actions bot added tool Affects the "flutter" command-line tool. See also t: labels. a: desktop Running on desktop labels Oct 31, 2023
@pbo-linaro
Copy link
Contributor Author

pbo-linaro commented Oct 31, 2023

@loic-sharma This is the first version of PR to support windows-arm64.

There are probably some missing tests, and some style issues. If possible, I would appreciate if we could focus on current content, and fill testing and tweak style once we all agree on this.

To complete PR description:

  • native compilation (x64 -> x64 and arm64 -> arm64) works!
  • current tests work on both platforms (at least what I could ran)
  • native assets are implemented

Yours and flutter team comments are welcome :)
I'll be back on Monday, so I won't be able to give feedback until then.

@loic-sharma loic-sharma self-requested a review November 1, 2023 19:35
@loic-sharma
Copy link
Member

@pbo-linaro This looks good! My only concern so far is that only the "master" channel should allow targeting Windows Arm64. The "beta" and "stable" channels should...

  1. Error if the --target-platform option is provided
  2. Not download Windows Arm64 artifacts
  3. Warn when compiling on Windows Arm64 that Arm64 is not a supported target platform and x64 will be used instead.

Excellent work! :)

@pbo-linaro
Copy link
Contributor Author

@pbo-linaro This looks good! My only concern so far is that only the "master" channel should allow targeting Windows Arm64. The "beta" and "stable" channels should...

Dart-sdk is only built for master channel, which is what prevents us from doing all this for beta and stable channels.
Considering this, it means the update_dart_sdk.ps1 (or any .bat file) should not try to download any arm64 version at all. I'm not sure if checking the channel in this script is the right thing to do. Any opinion or advice?

@cbracken cbracken self-requested a review November 10, 2023 01:17
@christopherfujino
Copy link
Contributor

@pbo-linaro This looks good! My only concern so far is that only the "master" channel should allow targeting Windows Arm64. The "beta" and "stable" channels should...

Dart-sdk is only built for master channel, which is what prevents us from doing all this for beta and stable channels. Considering this, it means the update_dart_sdk.ps1 (or any .bat file) should not try to download any arm64 version at all. I'm not sure if checking the channel in this script is the right thing to do. Any opinion or advice?

I definitely would discourage checking the branch name in a powershell script. I think we would need to discuss this further, perhaps in a doc, before merging this change.

@pbo-linaro
Copy link
Contributor Author

I definitely would discourage checking the branch name in a powershell script. I think we would need to discuss this further, perhaps in a doc, before merging this change.

I agree checking this in script is not a good way. I added a new section in windows-arm64 design document, "Enable windows-arm64 support for master branch only".

After looking closely at this, I plan to not check current branch from powershell script, but simply fallback to windows-x64 engine if windows-arm64 one is not available. From there, we can check the channel in flutter code as normally expected.

@christopherfujino
Copy link
Contributor

I definitely would discourage checking the branch name in a powershell script. I think we would need to discuss this further, perhaps in a doc, before merging this change.

I agree checking this in script is not a good way. I added a new section in windows-arm64 design document, "Enable windows-arm64 support for master branch only".

After looking closely at this, I plan to not check current branch from powershell script, but simply fallback to windows-x64 engine if windows-arm64 one is not available. From there, we can check the channel in flutter code as normally expected.

This sounds reasonable 👍

@pbo-linaro
Copy link
Contributor Author

Thanks for the feedback @christopherfujino.

@pbo-linaro
Copy link
Contributor Author

Implemented described change (fallback on x64 engine when arm64 is not available) and implemented channel check for --target-platform option.

@pbo-linaro
Copy link
Contributor Author

Updated comments and rebased.

@loic-sharma
Copy link
Member

@pbo-linaro You'll also need to update the Devicelab tests to pass post-submit verification. I would run the Devicelab tests locally on your Windows Arm64 machine to verify this.

Here are instructions on how to run Devicelab tests locally: https://github.com/flutter/flutter/blob/master/dev/devicelab/README.md#running-tests-locally

You can find Devicelab tests using the .ci.yaml file, all the builders whose name are prefixed by Windows_arm64 .

You'll want to use the task_name property for test_runner.dart's --task option. For example, for Windows_arm64 run_debug_test_windows:

cd dev/devicelab
rm C:\Code\f\flutter\bin\cache\flutter_tools.snapshot # Forces Flutter tool rebuild
../../bin/cache/dart-sdk/bin/dart bin/test_runner.dart test -t run_debug_test_windows

It looks like you'll need to update these files:

static final RegExp _builtOutput = RegExp(
r'Built build\\windows\\x64\\runner\\(Debug|Release)\\\w+\.exe( \(\d+(\.\d+)?MB\))?\.',
);

'Built build\\windows\\x64\\runner\\$buildMode\\app.exe',

applicationBinaryPath = path.join(
testDirectory,
'build',
'windows',
'x64',
'runner',
'Profile',
'$basename.exe'
);

final String exePath = path.join(
cwd,
'build',
'windows',
'x64',
'runner',
'release',
'$basename.exe');

@pbo-linaro
Copy link
Contributor Author

pbo-linaro commented Dec 8, 2023

To be direct, I won't work/update any test as long as we don't 100% agree on content of PR.
We already had an experience doing all this work, to see the request being rejected in the last straight line (#113928, #121999).
So I'm prudent on investing lots of time for this.
If possible, I would appreciate if we keep on fixing content, then approve it collectively (i.e. you and other devs from Flutter team), and then we can talk about tests missing and tests to update.

@Jasguerrero Jasguerrero added the revert Autorevert PR (with "Reason for revert:" comment) label Jan 18, 2024
auto-submit bot pushed a commit that referenced this pull request Jan 18, 2024
@auto-submit auto-submit bot removed the revert Autorevert PR (with "Reason for revert:" comment) label Jan 18, 2024
auto-submit bot added a commit that referenced this pull request Jan 18, 2024
Reverts #137618
Initiated by: Jasguerrero
This change reverts the following previous change:
Original Description:
It's now possible to natively compile a flutter app for
windows-arm64. Cross-compilation is not yet implemented.

Uses arm64 artifacts now available for Dart/Flutter.
Platform detection is based on Abi class, provided by Dart. Depending if
Dart is an arm64 or x64 binary, the Abi is set accordingly.
Initial bootstrap of dart artifacts (update_dart_sdk.ps1) is checking
PROCESSOR_ARCHITECTURE environment variable, which is the way to detect
host architecture on Windows.

This is available only for master channel (on other channels, it
fallbacks to windows-x64).

On windows-x64, it produces an x64 app. On windows-arm64, it produces an
arm64 app.
@pbo-linaro
Copy link
Contributor Author

@Jasguerrero Is that possible to get a public link or details on the failure?

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 19, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 19, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 19, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 19, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 19, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 19, 2024
@loic-sharma
Copy link
Member

@pbo-linaro I'll try to handle this. I'll tag you on this thread if I need your help :)

@pbo-linaro
Copy link
Contributor Author

Thanks very much!

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 19, 2024
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Jan 19, 2024
Manual roll Flutter from f77f824 to 684247a (39 revisions)

Manual roll requested by [email protected]

flutter/flutter@f77f824...684247a

2024-01-19 [email protected] Use Integer instead of int in map in flutter.groovy  (flutter/flutter#141895)
2024-01-19 [email protected] Roll Flutter Engine from c953c83112ba to f2b441a26416 (4 revisions) (flutter/flutter#141894)
2024-01-19 [email protected] Native assets: package in framework on iOS and MacOS (flutter/flutter#140907)
2024-01-19 [email protected] Revert "Make tests more resilient to Skia gold failures and refactor flutter_goldens for extensive technical debt removal (#140101)" (flutter/flutter#141814)
2024-01-19 [email protected] Roll Flutter Engine from 538975f2511b to c953c83112ba (3 revisions) (flutter/flutter#141886)
2024-01-19 [email protected] Add `showDragHandle` to `showBottomSheet` (flutter/flutter#141754)
2024-01-19 [email protected] Roll Flutter Engine from 9a6c64de8a46 to 538975f2511b (8 revisions) (flutter/flutter#141881)
2024-01-19 [email protected] Make pumpWidget's arguments named (flutter/flutter#141728)
2024-01-19 [email protected] Tiny fix inaccurate documentations about bindings (flutter/flutter#140282)
2024-01-19 [email protected] Roll engine to 9a6c64de8a4694cef59a338cd33ac1a9e7d23d9d (flutter/flutter#141870)
2024-01-19 [email protected] Roll Packages from 83c2c4d to 129e08c (13 revisions) (flutter/flutter#141865)
2024-01-19 [email protected] Add mac_x64_ios configuration. (flutter/flutter#141828)
2024-01-19 [email protected] Roll Flutter Engine from 90be25d8aac3 to d1afda52d254 (1 revision) (flutter/flutter#141825)
2024-01-19 [email protected] Move the requestKeyboard up to the widgets layer (flutter/flutter#141655)
2024-01-19 [email protected] Roll Flutter Engine from dde3ebf6551a to 90be25d8aac3 (1 revision) (flutter/flutter#141817)
2024-01-18 [email protected] enable more tests in web mode (flutter/flutter#141791)
2024-01-18 [email protected] Roll Flutter Engine from 9dded186bcff to dde3ebf6551a (2 revisions) (flutter/flutter#141811)
2024-01-18 [email protected] Update margin between label and icon in Tab to better reflect Material specs (flutter/flutter#140698)
2024-01-18 [email protected] Roll Flutter Engine from 3106e08e1219 to 9dded186bcff (2 revisions) (flutter/flutter#141807)
2024-01-18 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Enable native compilation for windows-arm64 " (flutter/flutter#141809)
2024-01-18 [email protected] Run framework_tests_misc in arm64 and x64. (flutter/flutter#141797)
2024-01-18 [email protected] Roll Flutter Engine from f4a4f046b173 to 3106e08e1219 (1 revision) (flutter/flutter#141802)
2024-01-18 49699333+dependabot[bot]@users.noreply.github.com Bump actions/upload-artifact from 4.1.0 to 4.2.0 (flutter/flutter#141803)
2024-01-18 [email protected] Update labeler.yml (flutter/flutter#141697)
2024-01-18 [email protected] Roll Flutter Engine from 75400c49fa0b to f4a4f046b173 (2 revisions) (flutter/flutter#141800)
2024-01-18 [email protected] Reland "Remove hack from PageView." (flutter/flutter#141533)
2024-01-18 [email protected] ScaleGestureRecognizer pointerCount=2 for trackpad gestures (flutter/flutter#140745)
2024-01-18 [email protected] Roll Flutter Engine from de68e7612948 to 75400c49fa0b (2 revisions) (flutter/flutter#141796)
2024-01-18 [email protected] Run `flutter_gallery_ios__start_up` test on Mac-14 in staging (flutter/flutter#141795)
2024-01-18 [email protected] Roll Flutter Engine from d80fe1cb5854 to de68e7612948 (1 revision) (flutter/flutter#141789)
2024-01-18 [email protected] Enable native compilation for windows-arm64  (flutter/flutter#137618)
2024-01-18 [email protected] [github actions] Fix token issue on actions/checkout package (flutter/flutter#141652)
2024-01-18 [email protected] Roll Flutter Engine from b75d6d80d813 to d80fe1cb5854 (2 revisions) (flutter/flutter#141785)
2024-01-18 [email protected] Revert "Native assets: roll deps" (flutter/flutter#141748)
2024-01-18 [email protected] Deprecate M2 curves (flutter/flutter#134417)
2024-01-18 [email protected] Fix: TextField can inherit `errorStyle` from `InputDecorationTheme`. (flutter/flutter#141227)
2024-01-18 [email protected] Add check for Bank of Brazil security module to Windows Flutter Doctor validators (flutter/flutter#141135)
2024-01-18 [email protected] Fix gradle lints No semantic change should be present. (flutter/flutter#141692)
2024-01-18 [email protected] Roll Packages from 1a2b780 to 83c2c4d (5 revisions) (flutter/flutter#141778)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human
...
@loic-sharma
Copy link
Member

loic-sharma commented Jan 19, 2024

@pbo-linaro Could you open a new pull request to re-land these changes? I have a fix pending on the Google side that I'll attach to this new pull request. That should allow us to reland your changes.

@pbo-linaro
Copy link
Contributor Author

New PR is available at #141930.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: desktop Running on desktop a: text input Entering text in a text field or keyboard related problems 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

Development

Successfully merging this pull request may close these issues.

4 participants