-
Notifications
You must be signed in to change notification settings - Fork 6k
[windows] Support win_debug_x86 compilation target in engine #30417
Conversation
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. 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. |
This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold. |
da069c8
to
30d3921
Compare
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. |
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.
A few things:
- It looks like this PR is breaking unit tests, so at a minimum that will need to be addressed.
- I'd like @cbracken to weigh in on this PR.
- Even if the unit tests are fixed, my comments about the
gn
script are addressed, and @cbracken signs-off, this configuration will still be in Flutter's "Unsupported" support category, meaning that we may accidentally break it at any time. If this configuration is important to you, you'll be expected to keep it working on your own.
38c6098
to
6d0571b
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Thank you for your advise! : ) This change is ready for review now. @zanderso
|
dart_target_arch can be "x86", which is handled here: https://github.com/dart-lang/sdk/blob/main/runtime/BUILD.gn#L113. This CL also handles it in the remaining places. This came up in flutter/engine#30417 TEST=N/A Change-Id: Ib6dcd0a7236938026a0cbcb3955433960b396667 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/237120 Reviewed-by: Ryan Macnak <rmacnak@google.com> Commit-Queue: Zach Anderson <zra@google.com>
Let's have the dialog about support in this issue flutter/flutter#37777, rather than in a pull request. Looks like this is ready for @cbracken to sign off as a prerequisite to merging. |
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.
The change itself LGTM. We've historically said we wouldn't do anything to stand in the way of fixes that make life better for those who'd like to get Flutter working on 32-bit Windows so long as those fixes don't complicate support for 64-bit Windows, which is what we officially target. These fixes fall into that category, though I'm quite sure a fair bit more will be required.
Happy to have the discussion around support separately on flutter/flutter#37777. The short version is that while there are a few dedicated contributors who want to see 32-bit Windows support, there aren't currently enough contributors focusing on maintaining/improving our 32-bit Windows for it to successful "officially supported" target (nor do we have 32-bit Windows test infra, for that matter).
* 9f13454 [windows] Support win_debug_x86 compilation target in engine (flutter/engine#30417) * c5e958e Make tracing safe (flutter/engine#32042) * cf875d4 [macOS] Forward key events to NSTextInputContext when there's composing text (flutter/engine#32051) * ffd5c9c Roll Fuchsia Linux SDK from Ee9OX2o6P... to mVqiTwaVa... (flutter/engine#32055) * b0018c3 Roll Skia from a9bc6c643791 to 8afe53fd76d3 (1 revision) (flutter/engine#32056) * df12321 Roll Skia from 8afe53fd76d3 to 0d81bc7bb04e (2 revisions) (flutter/engine#32057) * 398a274 Roll Fuchsia Mac SDK from jvlI1s78T... to vWlaMIVkM... (flutter/engine#32060) * 3ffa9cf Roll Skia from 0d81bc7bb04e to e253cc3e55d3 (1 revision) (flutter/engine#32063) * 275cd2b [web] Position spans absolutely within paragraph (flutter/engine#31907) * 46c2cca Roll Skia from e253cc3e55d3 to 9301fe3779bb (1 revision) (flutter/engine#32064)
@eggfly Excuse me. Is it possible to run applications developed with Flutter on Windows 7 32-bit systems? |
It added
--windows
param for gn, addedx86
target_arch, and fixed windows win_debug_x86 32-bit debug target compilation errors in Flutter engine. That is, using this command can do a successful compilation for x86 target on windows x64 host:(And windows jit_release_x86 (
tools\gn --runtime-mode jit_release --windows --windows-cpu x86
) is a WIP.)Design doc:
Related issues:
In our Windows desktop scenarios and some customers like hospital, bank, embedded industrial control hardwares, the OS are not updated for years. So there are still many x86 users and devices.
Many cross platform frameworks, such as QT and electron, support 32-bit. I think Flutter can also support this CPU architecture.
So we want to contribute this PR. 😄
Details:
LPARAM
change:Which cause compile error on 32-bit target:
I think we can direcly use
LPARAM
type instead ofuint32_t
, and do some mod in tests to avoid this test failure:@dkwingsmt may I have your double check about this?
The fixing:
https://docs.microsoft.com/en-us/cpp/preprocessor/predefined-macros says:
Pre-launch Checklist
writing and running engine tests.
///
).