Add option to build app for Windows on Arm #113928
Conversation
flutter-dashboard
bot
added
the
tool
|
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. |
|
As a native Windows on Arm system is needed to test this, I'm not sure it's possible to add automated test. If I'm wrong with it, don't hesitate to say! |
|
@cbracken @stuartmorgan Polite ping for this MR, as it's needed to enable Windows on Arm support. Does the approach sounds good to you? |
loic-sharma
added
platform-windows
|
/cc @christopherfujino as this affects the Flutter tool |
There was a problem hiding this comment.
Please add unit tests to /packages/flutter_tools/test/commands.shard/hermetic/build_windows_test.dart that verify this option works as expected
We won't be able to add integration tests for this yet, but, you should be able to unit test this change under the |
Codecov Report
@@ Coverage Diff @@
## master #113928 +/- ##
===========================================
- Coverage 91.46% 90.81% -0.66%
===========================================
Files 523 531 +8
Lines 94569 95506 +937
===========================================
+ Hits 86498 86730 +232
- Misses 8071 8776 +705
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
@loic-sharma Thanks for your review. I'll integrate those changes, add test, and come back. |
|
I just rebased and pushed a new version. 2 tests (for items opened) still needs to be added. |
|
@loic-sharma Is there a way to simply format only changes? (like git clang-format). I tried to use flutter format but it results in a lot of differences alas. |
|
@loic-sharma Could you help about current CI failures? I can't reproduce those on my side alas. (analyze command fails with lots of errors related to "dynamic calls") |
The Flutter repo has to be formatted manually. Please refer to the guide above for formatting guidelines. We will likely call out lots of little style nits - please don't be discouraged by that!
You'll want to run |
|
Thanks for your help :) I'm now fixing issues reported by analyze command. Out of curiosity, why flutter itself does not use its own format command? If you need specifics, maybe a --flutter-repo option could be added there too. On this PR, could you make nit comments after the PR content is fully accepted (and tests are ok)? That limits noise with issues reported. |
There was a problem hiding this comment.
I'm confused; why are you returning x64 artifacts when targeting ARM? If there aren't ARM artifacts yet, then this shouldn't land. The order should be to update the engine repo to build and upload the necessary artifacts, then upload the tool to use them.
There was a problem hiding this comment.
For now, on arm64, it should behave like x64 (because artifacts are not available). As mentioned in the commit message, you need a prebuilt arm64 flutter engine (built by hand) to create an arm64 app.
I'd love to have arm64 artifacts available from today, but I wanted first to show that a full app can be built with "vanilla" flutter. Kind of chicken and egg problem here.
If you think we should do it the other way, who can implement this for flutter? (build and upload engine)
There was a problem hiding this comment.
As mentioned in the commit message, you need a prebuilt arm64 flutter engine (built by hand) to create an arm64 app.
I'm not sure what message you are referring to; the PR description just says that it adds an option to build for Windows ARM. This doesn't actually do that though; it adds a flag that doesn't work. That's extremely likely to confuse people who will, reasonably, assume that if there's a flag that says to target Windows ARM, that's what it will do.
Kind of chicken and egg problem here.
Having it locally working end to end is useful, and having branch in your fork to point people to can be helpful in some cases, but there's no chicken-and-egg problem when it comes to actually creating/landing production PRs; this is a layered system, with the tool depending on the engine artifacts, but not the reverse. There's nothing about landing a (non-functional) flag in the tool that would make landing the engine changes easier.
If you think we should do it the other way, who can implement this for flutter? (build and upload engine)
What's the current state of engine support for ARM? If you can cross-compile the Windows ARM engine from an x64 machine, then anyone can make the PRs to add it to the recipes. If not, then nobody can do it until/unless we have Windows ARM infra. (This is discussed in the issue referenced in your PR description.)
There was a problem hiding this comment.
I'm a bit disappointed, as this PR was opened two weeks ago, mainly to get those kind of comments "Hey, we won't do it this way, thanks". At least, now it's clearer about the way to integrate this.
I agree this flag does not work, except if you build your own engine (as stated in the commit message).
So from there, how can we advance concretely on this?
Cross compile Windows ARM engine is the first step?
If that works, are you willing to help on making it available as an official artifact? What will be needed in more?
There was a problem hiding this comment.
In this specific comment, you mentioned having a native arm64 build would be enough. Did things changed since? If we have a dedicated Windows on Arm machine that could be used, would that be enough for you?
There was a problem hiding this comment.
I'm a bit disappointed, as this PR was opened two weeks ago, mainly to get those kind of comments "Hey, we won't do it this way, thanks".
I'm sorry I wasn't able to review earlier; I have a very deep review queue at the moment.
However, what I'm saying here isn't any different than what I said in the issue months ago. I clearly said in the issue that having engine artifacts was a pre-requisite for tool changes.
I agree this flag does not work, except if you build your own engine (as stated in the commit message).
Oh, generally nobody is looking at the commit messages for individual commits in a PR. Important information needs to be in the PR description.
So from there, how can we advance concretely on this? Cross compile Windows ARM engine is the first step? [...] What will be needed in more?
@cbraken laid out a pretty detailed list in the issue: #62597 (comment)
If that works, are you willing to help on making it available as an official artifact?
I'm no longer actively working on desktop, so would not be involved in setting up artifact recipes.
In this specific comment, you mentioned having a native arm64 build would be enough. Did things changed since?
Nothing has changed since, that's just not what I said. Bolding the important missing part:
The first goal though would be to get the engine cross-compiling (x64->arm64), or compiling natively on arm64, or both. (And if only the later, a CI solution for creating engine artifacts).
Having only a native arm64 build without also having ARM CI doesn't address the need for engine artifacts.
There was a problem hiding this comment.
No problem, I understand you were busy.
I'll continue discussion on original issue #62597.
There was a problem hiding this comment.
Is this PR about host, or target, or both? The PR description currently sounds like it's only about the target, but this is changing host.
There was a problem hiding this comment.
Initially, I wanted to upstream everything to prepare arm64 support.
Basically, the option --target-platform could use this new Host Platform to derive automatically default target.
This code won't be used right now though, as the --target-platform flag has a windows-x64 default value. If you think it's not worth it (it will have to be committed at some point), I can drop it.
There was a problem hiding this comment.
What is the supported set of host/target combinations for Windows? That's something that needs to be clearly addressed (ideally in a design document, but failing that in detail in the PR description) before I could offer an opinion about the value of each piece in isolation.
There was a problem hiding this comment.
For now, only native workflows have been tried (x64 -> x64, arm64 -> arm64). Doing cross compilation implies work with gen_snapshot, which I didn't start to look into.
There was a problem hiding this comment.
I would strongly recommend a design document for any tool changes related to this; it doesn't really make sense to decide what the tool commands, output, etc. should look like without a discussion of whether cross-compilation will be supported. If same-arch builds are the only option then it doesn't seem like we even need a target flag.
There was a problem hiding this comment.
I'll focus on getting flutter engine built and uploaded for windows-arm64 for now. Then, we can reopen that discussion.
It allows to build an app for x64 or arm64. By default, build for x64. For now, a local engine build is needed to get an arm64 build. From an Windows on Arm machine: $ fetch flutter $ cd src $ python3 ./flutter/tools/gn --no-prebuilt-dart-sdk --no-goma --windows-cpu arm64 --target-dir host_debug $ ninja -C out/host_debug $ cd C:/to/your/app $ flutter.bat build windows --target-platform windows-arm64 --local-engine-src-path=C:/path/to/flutter/src --local-engine host_debug You'll then have native arm64 app available! This commits adds platform windows-arm64. For now, artifacts are not available for it, so fallback to x64 when downloading them.
|
For now, this PR is closed, as artifacts for this new platform should be available before considering to add an option targeting it (see latest discussions for details). |
5 participants
This PR adds Windows on Arm as a known platform,
and an option --target-plaftorm to "build windows" command, similar to what exists for build-linux.
Default behavior was not changed. See commits for details.
Tested by building https://github.com/flutter/gallery with this.
Issue: #62597
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.