Introduce architecture subdirectory for Windows build (#129805, #116196) by p-b-o · Pull Request #131843 · flutter/flutter

p-b-o · 2023-08-03T11:49:52Z

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.

p-b-o · 2023-08-03T11:52:01Z

@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.

p-b-o · 2023-08-03T12:02:28Z

Fixed error reported by flutter analyze.

p-b-o · 2023-08-03T12:19:52Z

Fixed license message.

p-b-o · 2023-08-03T14:08:02Z

@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 · 2023-08-03T20:31:47Z

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

loic-sharma · 2023-08-03T20:32:45Z

Yup will do!

packages/flutter_tools/lib/src/build_info.dart

loic-sharma · 2023-08-04T21:48:13Z

packages/flutter_tools/lib/src/windows/build_windows.dart

Can we move this to the generated configs in _writeGeneratedFlutterConfig?

flutter/packages/flutter_tools/lib/src/cmake.dart

Lines 101 to 109 in b7acafa

# 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)

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?

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!

Thanks for accepting this.

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

packages/flutter_tools/lib/src/windows/migrations/build_architecture_migration.dart

...s/flutter_tools/test/general.shard/windows/migrations/build_architecture_migration_test.dart

packages/flutter_tools/lib/src/windows/migrations/build_architecture_migration.dart

loic-sharma · 2023-08-04T22:23:39Z

@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:

flutter/dev/devicelab/lib/tasks/plugin_tests.dart

Lines 296 to 300 in 53082f6

    
           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!

p-b-o · 2023-08-07T08:54:14Z

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

p-b-o · 2023-08-07T11:25:20Z

Pushed resolved changes.

p-b-o · 2023-08-07T11:48:17Z

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

packages/flutter_tools/templates/app_shared/windows.tmpl/flutter/CMakeLists.txt

p-b-o · 2023-08-07T20:11:22Z

Added change to hardcode TargetPlatform to getWindowsBuildDirectory.

packages/flutter_tools/lib/src/windows/build_windows.dart

loic-sharma · 2023-08-08T17:53:46Z

packages/flutter_tools/lib/src/windows/build_windows.dart

Ideally this should have the same logic as the clean command's deletion:

flutter/packages/flutter_tools/lib/src/commands/clean.dart

Lines 116 to 143 in d9cb50e

void deleteFile(FileSystemEntity file) {

// This will throw a FileSystemException if the directory is missing permissions.

try {

if (!file.existsSync()) {

return;

}

} on FileSystemException catch (err) {

globals.printError('Cannot clean ${file.path}.\n$err');

return;

}

final Status deletionStatus = globals.logger.startProgress(

'Deleting ${file.basename}...',

);

try {

file.deleteSync(recursive: true);

} on FileSystemException catch (error) {

final String path = file.path;

if (globals.platform.isWindows) {

globals.printError('Failed to remove $path. '

'A program may still be using a file in the directory or the directory itself. '

'To find and stop such a program, see: '

'https://superuser.com/questions/1333118/cant-delete-empty-folder-because-it-is-used');

} else {

globals.printError('Failed to remove $path: $error');

}

} finally {

deletionStatus.stop();

}

Namely:

Show a progress bar

If a file in the build directory is in use, show a helpful error message describing how to stop the program using the file

@christopherfujino How would you feel about moving CleanCommand.deleteFile to FileSystemUtils?

Hmm, this seems to do a lot more than I would expect a generic method named deleteFile. I'm ok with moving htis to FileSystemUtils if we use a more descriptive method name, like recursivelyDeleteEntity({bool showProgress = false})

Thanks, I'll move it and use that method then.

After trying to move this, it creates several changes that are not so welcome on this PR. (tests moving, etc).
Would that be acceptable to first use the initial version, and then refactor this in a subsequent PR?

p-b-o · 2023-08-11T10:14:58Z

Pushed fix for latest comments (cmake file, nit). I left open the discussion about deleteFile method, that should be moved in another PR IMHO.

FYI, I'll be off next week, so there will be some delay before I can answer.

packages/flutter_tools/lib/src/windows/migrations/build_architecture_migration.dart

…ter#131843)

github-actions bot added tool Affects the "flutter" command-line tool. See also t: labels. a: desktop Running on desktop labels Aug 3, 2023

loic-sharma self-requested a review August 3, 2023 20:32