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

[tools,pigeon] Update tooling to handle Windows build output changes #4826

Merged
merged 7 commits into from
Sep 1, 2023
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 18 additions & 3 deletions packages/pigeon/tool/shared/test_suites.dart
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

// ignore_for_file: avoid_print

import 'dart:ffi';
import 'dart:io' show File, Directory;

import 'package:meta/meta.dart';
Expand Down Expand Up @@ -313,9 +314,23 @@ Future<int> _runWindowsUnitTests() async {
return compileCode;
}

return runProcess(
'$examplePath/build/windows/plugins/test_plugin/Debug/test_plugin_test.exe',
<String>[]);
// Depending on the Flutter version, the build output path is different.
// Check for the new location first, and fall back to the old location, to
// support running against both stable and master.
// TODO(stuartmorgan): Remove all this when these tests no longer need to
// support Flutter 3.13, and just construct the version of the path with the
// architecture.
const String buildDirBase = '$examplePath/build/windows';
const String buildRelativeBinaryPath =
'plugins/test_plugin/Debug/test_plugin_test.exe';
final String arch = Abi.current() == Abi.windowsArm64 ? 'arm64' : 'x64';
Copy link
Member

@loic-sharma loic-sharma Aug 31, 2023

Choose a reason for hiding this comment

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

Currently, the tool creates x64 executables on Windows Arm64 machines (the executable runs using emulation). Creating Windows Arm64 executables on Arm64 hosts by default is tracked by flutter/flutter#129807.

In other words, in the long-term this script is correct, but in the short-term (until flutter/flutter#129807 lands) this script should use build/windows/x64 on Windows Arm64 hosts. Feel free to keep this as-is if you don't plan to run this script on Windows Arm64 hosts until after flutter/flutter#129807 lands.

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, I misremembered what the roadmap ordering was on the arm64 support.

To avoid having a mini fire-drill when flutter/flutter#129807 lands, I've made the scripts messier for now but future-proof, so that they will work with the new structure with only x64, but then use the new structure with arm64 when it starts working.

final String newPath = '$buildDirBase/$arch/$buildRelativeBinaryPath';
const String oldPath = '$buildDirBase/$buildRelativeBinaryPath';
if (File(newPath).existsSync()) {
return runProcess(newPath, <String>[]);
} else {
return runProcess(oldPath, <String>[]);
}
}

Future<int> _runWindowsIntegrationTests() async {
Expand Down
19 changes: 12 additions & 7 deletions script/tool/lib/src/common/cmake.dart
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ class CMakeProject {
required this.buildMode,
this.processRunner = const ProcessRunner(),
this.platform = const LocalPlatform(),
this.arch,
});

/// The directory of a Flutter project to run Gradle commands in.
Expand All @@ -31,6 +32,11 @@ class CMakeProject {
/// The platform that commands are being run on.
final Platform platform;

/// The architecture subdirectory of the build.
// TODO(stuartmorgan): Make this non-nullable once Flutter 3.13 is no longer
// supported, since at that point there will always be a subdirectory.
final String? arch;

/// The build mode (e.g., Debug, Release).
///
/// This is a constructor paramater because on Linux many properties depend
Expand All @@ -46,14 +52,13 @@ class CMakeProject {
Directory get buildDirectory {
Directory buildDir =
flutterProject.childDirectory('build').childDirectory(_platformDirName);
if (arch != null) {
buildDir = buildDir.childDirectory(arch!);
}
if (platform.isLinux) {
buildDir = buildDir
// TODO(stuartmorgan): Support arm64 if that ever becomes a supported
// CI configuration for the repository.
.childDirectory('x64')
// Linux uses a single-config generator, so the base build directory
// includes the configuration.
.childDirectory(buildMode.toLowerCase());
// Linux uses a single-config generator, so the base build directory
// includes the configuration.
buildDir = buildDir.childDirectory(buildMode.toLowerCase());
}
return buildDir;
}
Expand Down
39 changes: 33 additions & 6 deletions script/tool/lib/src/native_test_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

import 'dart:ffi';

import 'package:file/file.dart';
import 'package:meta/meta.dart';

Expand Down Expand Up @@ -41,7 +43,9 @@ class NativeTestCommand extends PackageLoopingCommand {
super.packagesDir, {
super.processRunner,
super.platform,
}) : _xcode = Xcode(processRunner: processRunner, log: true) {
Abi? abi,
}) : _abi = abi ?? Abi.current(),
_xcode = Xcode(processRunner: processRunner, log: true) {
argParser.addOption(
_iOSDestinationFlag,
help: 'Specify the destination when running iOS tests.\n'
Expand All @@ -63,6 +67,9 @@ class NativeTestCommand extends PackageLoopingCommand {
help: 'Runs native integration (UI) tests', defaultsTo: true);
}

// The ABI of the host.
final Abi _abi;

// The device destination flags for iOS tests.
List<String> _iOSDestinationFlags = <String>[];

Expand Down Expand Up @@ -548,9 +555,10 @@ this command.
isTestBinary: isTestBinary);
}

/// Finds every file in the [buildDirectoryName] subdirectory of [plugin]'s
/// build directory for which [isTestBinary] is true, and runs all of them,
/// returning the overall result.
/// Finds every file in the relevant (based on [platformName], [buildMode],
/// and [arch]) subdirectory of [plugin]'s build directory for which
/// [isTestBinary] is true, and runs all of them, returning the overall
/// result.
///
/// The binaries are assumed to be Google Test test binaries, thus returning
/// zero for success and non-zero for failure.
Expand All @@ -563,11 +571,30 @@ this command.
final List<File> testBinaries = <File>[];
bool hasMissingBuild = false;
bool buildFailed = false;
String? arch;
if (platform.isWindows) {
arch = _abi == Abi.windowsX64 ? 'x64' : 'arm64';
} else if (platform.isLinux) {
// TODO(stuartmorgan): Support arm64 if that ever becomes a supported
// CI configuration for the repository.
arch = 'x64';
}
for (final RepositoryPackage example in plugin.getExamples()) {
final CMakeProject project = CMakeProject(example.directory,
CMakeProject project = CMakeProject(example.directory,
buildMode: buildMode,
processRunner: processRunner,
platform: platform);
platform: platform,
arch: arch);
if (platform.isWindows && !project.isConfigured()) {
// Check again without the arch subdirectory, since 3.13 doesn't
// have it yet.
// TODO(stuartmorgan): Remove this once Flutter 3.13 is no longer
// supported by the main repository CI.
project = CMakeProject(example.directory,
buildMode: buildMode,
processRunner: processRunner,
platform: platform);
}
if (!project.isConfigured()) {
printError('ERROR: Run "flutter build" on ${example.displayName}, '
'or run this tool\'s "build-examples" command, for the target '
Expand Down
Loading