-
Notifications
You must be signed in to change notification settings - Fork 3.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added chipsets to supported platforms #10174
Conversation
Visit the preview URL for this PR (updated for commit b34790e): https://flutter-docs-prod--pr10174-fix-10136-5szlvlmk.web.app |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming the chipset info is correct, lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this @atsansone! Cool idea!
A few accessibility and accuracy issues I noticed during an initial review:
Let me know if want to discuss any further. Thanks!
src/_data/chipsets.yml
Outdated
x64: '<span class="material-symbols" style="color: #158477;">verified</span>' | ||
arm32: '<span class="material-symbols" style="color: #158477;">verified</span>' | ||
arm64: '<span class="material-symbols" style="color: #158477;">verified</span> ' | ||
risc-v: '<span class="material-symbols" style="color: #13C2AD">gpp_maybe</span>' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is unsupported. I don't think any work in Flutter has gone towards adding support for this target yet. At least not anything enabled by default in the beta
channel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
src/_data/chipsets.yml
Outdated
x86: '<span class="material-symbols" style="color: #158477;">verified</span>' | ||
x64: '<span class="material-symbols" style="color: #158477;">verified</span>' | ||
arm32: '<span class="material-symbols" style="color: #158477;">verified</span>' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does Flutter actually provide the necessary artifacts for Linux x86 and arm32 support? I think a custom engine/embedder could add support, but as far as I can tell, Flutter doesn't support these two by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
src/reference/supported-platforms.md
Outdated
{% assign na = '<span class="material-symbols" style="color: | ||
#DADCE0">do_not_disturb_on</span>' %} | ||
|
||
| Platform | x86 | x64 | ARM32 | ARM64 | RISC-V | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RISC-V is very broad due to its various extensions. Consider explaining with a footnote or using the full specifier of the extensions that Dart targets: RISC-V (RV64GC)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
src/reference/supported-platforms.md
Outdated
{% assign yes = '<span class="material-symbols" style="color: #158477;">verified</span>' %} | ||
{% assign no = '<span class="material-symbols" style="color: #D43324">dangerous</span>' %} | ||
{% assign beta = '<span class="material-symbols" style="color: #13C2AD">gpp_maybe</span>' %} | ||
{% assign na = '<span class="material-symbols" style="color: | ||
#DADCE0">do_not_disturb_on</span>' %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm worried about the accessibility of these icons. For a screen reader, I believe they'll just show up as verified
, dangerous
, gpp_maybe
, etc.
Consider exploring avenues to make them more accessible. Happy to discuss further!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found a way to handle this. Take a look.
src/reference/supported-platforms.md
Outdated
{% assign na = '<span class="material-symbols" style="color: | ||
#DADCE0">do_not_disturb_on</span>' %} | ||
|
||
| Platform | x86 | x64 | ARM32 | ARM64 | RISC-V | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To prevent confusion between x86 and x86-64 on the Dart site we've referred to x86 as IA32. I think that would be worthwhile here, or some parenthetical.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We did, but not anymore. This table came from the Dart site.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason in particular you made the switch? Can we switch back? In the Dart SDK and the GCP storage archive, it is labeled as IA32, so I think it's best to be consistent across everywhere if possible.
Thanks so much for your thorough review, @parlough! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM once @parlough's changes are landed
src/reference/supported-platforms.md
Outdated
{% assign na = '<span class="material-symbols" style="color: | ||
#DADCE0">do_not_disturb_on</span>' %} | ||
|
||
| Platform | x86 | x64 | ARM32 | ARM64 | RISC-V | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, we were told by Microsoft to use the "Windows Arm64" branding instead of "Windows ARM64". It looks like that's the branding used on arm.com as well (link). Should we rebrand these to Arm32
and Arm64
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good!
src/reference/supported-platforms.md
Outdated
{% assign na = '<span class="material-symbols" style="color: | ||
#DADCE0">do_not_disturb_on</span>' %} | ||
|
||
| Platform | x86 | x64 | ARM32 | ARM64 | RISC-V | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we upgrade Windows Arm64 to "Supported in all channels"? The 3.19 blog post declared "initial support" for Windows Arm64 on beta and master channels: https://medium.com/flutter/whats-new-in-flutter-3-19-58b1aae242d2
FYI we're in a weird messy state right now: you can create Windows Arm64 executables if you re-download the engine artifacts once (examples: flutter precache
, flutter upgrade
, etc..). Until you re-download artifacts, you'll create Windows x64 executables that run on Windows Arm64 using emulation.
Some context: On Windows, we don't support cross-compilation yet and the Flutter tool targets whatever architecture the tool was built for. If on a Windows Arm64 machine you use the Flutter tool for x64 machines, you'll produce an x64 app. Although we do have Windows Arm64 artifacts, we haven't created a Windows Arm64 Flutter SDK .zip yet. As a result, Windows Arm64 users start with x64 artifacts until they re-download their artifacts.
Sorry for the long-winded explanation 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood and fixed.
src/reference/supported-platforms.md
Outdated
role="img">do_not_disturb_on</span> | ||
{% endcapture %} | ||
|
||
| Platform | x86 | x64 | ARM32 | ARM64 | RV64GC | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Platform | x86 | x64 | ARM32 | ARM64 | RV64GC | | |
| Platform | x86 | x64 | Arm32 | Arm64 | RV64GC | |
src/reference/supported-platforms.md
Outdated
{% capture beta %} | ||
<span class="material-symbols" | ||
style="color: #13C2AD" | ||
aria-label="The Flutter SDK supports ARM64 in beta the only architecture on Windows" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Windows Arm64 is "initially supported" on all channels
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for those adjustments Tony! I haven't had a chance to take a look at the a11y improvements to the icons, but thanks for the additional focus there.
Some comments as I was reading through again:
src/reference/supported-platforms.md
Outdated
role="img">do_not_disturb_on</span> | ||
{% endcapture %} | ||
|
||
| Platform | x86 | x64 | ARM32 | ARM64 | RV64GC | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Platform | x86 | x64 | ARM32 | ARM64 | RV64GC | | |
| Platform | IA32 (x86) | x64 | ARM32 | ARM64 | RV64GC | |
src/reference/supported-platforms.md
Outdated
{{yes}} Supported in all channels. | ||
{{no}} Unsupported in all channels. | ||
{{beta}} Supported in Beta channel only. | ||
{{na}} No version exists. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- For more reliable and easier to notice forced new-lines, you can use
<br>
here. We do it elsewhere on the sites as well. - Generally I think of an OS or architecture as "supported on" or "supported by".
- To highlight channels, we generally use code font or bold while keeping it lower case.
- "No version exists" could be mistaken to mean that no version exists for Flutter. I think it would be beneficial to expand a bit to different it from the case where it's just unsupported.
{{yes}} Supported in all channels. | |
{{no}} Unsupported in all channels. | |
{{beta}} Supported in Beta channel only. | |
{{na}} No version exists. | |
{{yes}} Supported on all channels.<br> | |
{{no}} Unsupported on all channels.<br> | |
{{beta}} Supported on the `beta` and `main` channels only.<br> | |
{{na}} Unsupported by the operating system.<br> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@parlough : No issue with most of this, but the N/A comes from mit's thoughts on Dart. I don't know if I want to confuse this if you look across both sites.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also like to keep markup to Markdown wherever possible.
src/reference/supported-platforms.md
Outdated
|
||
Flutter offers three tiers of support for deploying apps to target platforms. | ||
|
||
* **Supported**: Google tests these platforms on every commit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using a definition list here instead, since it seems to fit your use case and adds extra semantics.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried that, but it looked like it added a bit too much page length. Let me check again.
src/reference/supported-platforms.md
Outdated
* **Supported**: Google tests these platforms on every commit. | ||
* **Best effort**: Google intends to support these platforms | ||
through coding practices. Google tests these platforms on an ad-hoc basis. | ||
* **Unsupported**: Google doesn't test or support these platforms. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to not drag "Google" in to all three descriptions. Perhaps it makes sense for the "Supported" category, but the other two are likely better off as "The Flutter team" or "Flutter".
For the supported category, I think it also should be "The Flutter team" but can perhaps be strengthened with a mention of Google. I'm not sure exactly how yet, so I'll try to come back to this comment with ideas. Perhaps something like "The Flutter team, supported by Google, runs tests for these platforms on every commit."?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fair. The original said Google; I was just updating.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
src/_data/chipsets.yml
Outdated
x64: '<span class="material-symbols" style="color: #158477" aria-label="The Flutter SDK supports the x64 architecture on Linux" role="img">verified</span>' | ||
arm32: '<span class="material-symbols" style="color: #D43324" aria-label="The Flutter SDK does not support the Arm32 architecture on Linux" role="img">dangerous</span>' | ||
arm64: '<span class="material-symbols" style="color: #158477" aria-label="The Flutter SDK supports the Arm64 architecture on Linux" role="img">verified</span> ' | ||
risc: '<span class="material-symbols" style="color: #DADCE0" aria-label="No version of the Flutter SDK exists for the RV64GC architecture on Linux" role="img">do_not_disturb_on</span>' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the definitions, it seems like this entry should be dangerous
(Not supported) as the OS/arch combo exists, just Flutter's support for it doesn't yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have found at least one mistake on this page. Can we please revert this change and have it reviewed by more team members?
- platform: 'Android' | ||
x86: '<span class="material-symbols" style="color: #DADCE0" aria-label="No version of the Flutter SDK exists for the x86 architecture on Android" role="img">do_not_disturb_on</span>' | ||
x64: '<span class="material-symbols" style="color: #DADCE0" aria-label="No version of the Flutter SDK exists for the x64 architecture on Android" role="img">do_not_disturb_on</span>' | ||
arm32: '<span class="material-symbols" style="color: #DADCE0" aria-label="No version of the Flutter SDK exists for the Arm32 architecture on Android" role="img">do_not_disturb_on</span>' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is definitely not correct. 32-bit ARM Android is 100% supported and we have the devices in our lab.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can do another PR. Is there a definitive lust, @zanderso?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest adding the TLs from the teams that support each platform for the definitive answers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I sent #10197 to revert. To clarify a bit, this is the definitive list =)
Added hardware architectures to supported platforms. Fixes flutter#10136 Fixes flutter#6713
Added hardware architectures to supported platforms.
Fixes #10136
Fixes #6713