Skip to content
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

Introduce architecture subdirectory for Windows build (#129805, #116196) #131843

Merged
merged 1 commit into from Aug 31, 2023

Conversation

pbo-linaro
Copy link
Contributor

@pbo-linaro pbo-linaro commented Aug 3, 2023

To implement windows-arm64 support, it is needed to add architecture as a subdirectory (#129805).

In short, when performing a flutter windows build, we have:

  • Before: build/windows/runner/Release/gallery.exe
  • After: build/windows/x64/runner/Release/gallery.exe

This convention follows what flutter linux build does.

The old "runner" folder is automatically cleaned to prevent confusion for users not aware of this change, and who could continue to use outdated binaries accidentally.

We didn't introduce any change in ephemeral build folder. This might be required when introducing another architecture, but it won't be a breaking change, as those files are not used by end user.

Since we use a fresh cmake folder, we introduce FLUTTER_TARGET_PLATFORM in flutter cmake file (#116196). This will be needed when targeting another architecture than x64, and will avoid to have to clean existing cmake files. An automatic migration was implemented to automate this change in existing applications.

Finally, we set a specific architecture when calling cmake. This fixes existing build and tests on Windows on Arm, where cmake tries to compile native windows-arm64 binaries by default (which fails to link x64 libraries). In more, it will avoid need to clean existing cmake files when introducing another architecture.

Design doc: flutter.dev/go/windows-arm64

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 Aug 3, 2023
@pbo-linaro
Copy link
Contributor Author

@loic-sharma Here it is!
Your comments are welcome, and those from other flutter devs as well.

For now, I focused on a minimal change, with tests working (on x64 and arm64).
Probably I can update some documentation as well, don't hesitate to say.

@pbo-linaro
Copy link
Contributor Author

Fixed error reported by flutter analyze.

@pbo-linaro
Copy link
Contributor Author

Fixed license message.

@pbo-linaro
Copy link
Contributor Author

pbo-linaro commented Aug 3, 2023

@loic-sharma I'm not sure if the failure (windows plugin_test) means that there is something missing, or if the test script itself (located elsewhere?) should be updated to reflect new path to binary plugintest_test.exe.

If you could help on this, that would be nice :).

@andrewkolos
Copy link
Contributor

@loic-sharma Would you be interested in reviewing this?

@loic-sharma loic-sharma self-requested a review August 3, 2023 20:32
@loic-sharma
Copy link
Member

Yup will do!

@@ -168,6 +189,9 @@ Future<void> _runCmakeGeneration({
buildDir.path,
'-G',
generator,
'-A',
arch,
'-DFLUTTER_TARGET_PLATFORM=$flutterTargetPlatform',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move this to the generated configs in _writeGeneratedFlutterConfig?

# Generated code do not commit.
file(TO_CMAKE_PATH "$escapedFlutterRoot" FLUTTER_ROOT)
file(TO_CMAKE_PATH "$escapedProjectDir" PROJECT_DIR)
set(FLUTTER_VERSION "$version" PARENT_SCOPE)
set(FLUTTER_VERSION_MAJOR ${version.major} PARENT_SCOPE)
set(FLUTTER_VERSION_MINOR ${version.minor} PARENT_SCOPE)
set(FLUTTER_VERSION_PATCH ${version.patch} PARENT_SCOPE)
set(FLUTTER_VERSION_BUILD ${buildVersion ?? 0} PARENT_SCOPE)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Linux build adds it directly to the cmake command, and IMHO, it's more explicit that putting it in another function.
FLUTTER_VERSION_* stuff is the same for all architecture, thus it makes sense to factorize it in a single place.

What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm I agree we should align with Linux. If we move FLUTTER_TARGET_PLATFORM to the generated CMake config file, we would need to also update Linux. That seems out-of-scope for this PR, and we can follow-up with this change later if needed.

That said, I'm curious as to why Linux chose this approach. The generated cmake config file makes it easier to use the CMake project without the Flutter tool. Regardless, I'll close this off!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for accepting this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if someone directly invokes a build via cmake without providing -DFLUTTER_TARGET_PLATFORM?

@loic-sharma
Copy link
Member

@loic-sharma I'm not sure if the failure (windows plugin_test) means that there is something missing, or if the test script itself (located elsewhere?) should be updated to reflect new path to binary plugintest_test.exe.

It looks like you'll need to update this:

if (await exec(
path.join(rootPath, 'build', 'windows', 'plugins', 'plugintest', 'Release', 'plugintest_test.exe'),
<String>[],
canFail: true,
) != 0) {

You should be able to run this test locally using these instructions: https://github.com/flutter/flutter/tree/master/dev/devicelab#running-tests-locally

TLDR, this should work:

cd path/to/flutter
cd dev/devicelab
../../bin/cache/dart-sdk/bin/dart bin/test_runner.dart test -t plugin_test_windows

Let me know if you run into any problems!

@pbo-linaro
Copy link
Contributor Author

First fixing failed test, before addressing other comments in review.

@pbo-linaro pbo-linaro force-pushed the windows_build_layout_v1 branch 2 times, most recently from 58332e9 to 78f5688 Compare August 7, 2023 11:25
@pbo-linaro
Copy link
Contributor Author

Pushed resolved changes.

@pbo-linaro
Copy link
Contributor Author

Fixed analyze not happy with raw cmake strings 'if(...'.

@pbo-linaro
Copy link
Contributor Author

Added change to hardcode TargetPlatform to getWindowsBuildDirectory.

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 2, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 2, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 3, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 3, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 3, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 4, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 4, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 5, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 5, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 5, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 6, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 6, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 6, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 6, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 6, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 6, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 6, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 6, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 7, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 7, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 7, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 7, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 7, 2023
@pbo-linaro pbo-linaro deleted the windows_build_layout_v1 branch October 5, 2023 08:54
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: desktop Running on desktop d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos framework flutter/packages/flutter repository. See also f: labels. platform-windows Building on or for Windows specifically tool Affects the "flutter" command-line tool. See also t: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants