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

[flutter_tools] fix tests with no native assets running native asset build #135474

Merged
merged 4 commits into from
Sep 26, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/flutter_tools/lib/src/ios/native_assets.dart
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ Future<Uri?> dryRunNativeAssetsIOS({
required Uri projectUri,
required FileSystem fileSystem,
}) async {
if (await hasNoPackageConfig(buildRunner) || await isDisabledAndNoNativeAssets(buildRunner)) {
if (!await nativeBuildRequired(buildRunner)) {
return null;
}

Expand Down Expand Up @@ -72,7 +72,7 @@ Future<List<Uri>> buildNativeAssetsIOS({
required Uri yamlParentDirectory,
required FileSystem fileSystem,
}) async {
if (await hasNoPackageConfig(buildRunner) || await isDisabledAndNoNativeAssets(buildRunner)) {
if (!await nativeBuildRequired(buildRunner)) {
await writeNativeAssetsYaml(<Asset>[], yamlParentDirectory, fileSystem);
return <Uri>[];
}
Expand Down
4 changes: 2 additions & 2 deletions packages/flutter_tools/lib/src/linux/native_assets.dart
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ Future<Uri?> dryRunNativeAssetsLinux({
bool flutterTester = false,
required FileSystem fileSystem,
}) async {
if (await hasNoPackageConfig(buildRunner) || await isDisabledAndNoNativeAssets(buildRunner)) {
if (!await nativeBuildRequired(buildRunner)) {
return null;
}

Expand Down Expand Up @@ -90,7 +90,7 @@ Future<(Uri? nativeAssetsYaml, List<Uri> dependencies)> buildNativeAssetsLinux({
// CMake requires the folder to exist to do copying.
await buildDir.create(recursive: true);
}
if (await hasNoPackageConfig(buildRunner) || await isDisabledAndNoNativeAssets(buildRunner)) {
if (!await nativeBuildRequired(buildRunner)) {
final Uri nativeAssetsYaml = await writeNativeAssetsYaml(<Asset>[], yamlParentDirectory ?? buildUri_, fileSystem);
return (nativeAssetsYaml, <Uri>[]);
}
Expand Down
4 changes: 2 additions & 2 deletions packages/flutter_tools/lib/src/macos/native_assets.dart
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ Future<Uri?> dryRunNativeAssetsMacOS({
bool flutterTester = false,
required FileSystem fileSystem,
}) async {
if (await hasNoPackageConfig(buildRunner) || await isDisabledAndNoNativeAssets(buildRunner)) {
if (!await nativeBuildRequired(buildRunner)) {
return null;
}

Expand Down Expand Up @@ -77,7 +77,7 @@ Future<(Uri? nativeAssetsYaml, List<Uri> dependencies)> buildNativeAssetsMacOS({
}) async {
const OS targetOs = OS.macOS;
final Uri buildUri_ = nativeAssetsBuildUri(projectUri, targetOs);
if (await hasNoPackageConfig(buildRunner) || await isDisabledAndNoNativeAssets(buildRunner)) {
if (!await nativeBuildRequired(buildRunner)) {
final Uri nativeAssetsYaml = await writeNativeAssetsYaml(<Asset>[], yamlParentDirectory ?? buildUri_, fileSystem);
return (nativeAssetsYaml, <Uri>[]);
}
Expand Down
39 changes: 17 additions & 22 deletions packages/flutter_tools/lib/src/native_assets.dart
Original file line number Diff line number Diff line change
Expand Up @@ -214,45 +214,45 @@ BuildMode nativeAssetsBuildMode(build_info.BuildMode buildMode) {
///
/// Native asset builds cannot be run without a package config. If there is
/// no package config, leave a logging trace about that.
Future<bool> hasNoPackageConfig(NativeAssetsBuildRunner buildRunner) async {
Future<bool> _hasNoPackageConfig(NativeAssetsBuildRunner buildRunner) async {
final bool packageConfigExists = await buildRunner.hasPackageConfig();
if (!packageConfigExists) {
globals.logger.printTrace('No package config found. Skipping native assets compilation.');
}
return !packageConfigExists;
}

/// Checks that if native assets is disabled, none of the dependencies declare
/// native assets.
///
/// If any of the dependencies have native assets, but native assets are
/// disabled, exits the tool.
Future<bool> isDisabledAndNoNativeAssets(NativeAssetsBuildRunner buildRunner) async {
if (featureFlags.isNativeAssetsEnabled) {
Future<bool> nativeBuildRequired(NativeAssetsBuildRunner buildRunner) async {
if (await _hasNoPackageConfig(buildRunner)) {
return false;
}
final List<Package> packagesWithNativeAssets = await buildRunner.packagesWithNativeAssets();
if (packagesWithNativeAssets.isEmpty) {
return true;
return false;
}
final String packageNames = packagesWithNativeAssets.map((Package p) => p.name).join(' ');
throwToolExit(
'Package(s) $packageNames require the native assets feature to be enabled. '
'Enable using `flutter config --enable-native-assets`.',
);

if (!featureFlags.isNativeAssetsEnabled) {
final String packageNames = packagesWithNativeAssets.map((Package p) => p.name).join(' ');
throwToolExit(
'Package(s) $packageNames require the native assets feature to be enabled. '
'Enable using `flutter config --enable-native-assets`.',
);
}
return true;
}

/// Ensures that either this project has no native assets, or that native assets
/// are supported on that operating system.
///
/// Exits the tool if the above condition is not satisfied.
// TODO(fujino): this does not appear to do what it says it does.
Copy link
Contributor

Choose a reason for hiding this comment

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

wdym?

This test checks that either (A) there are no native assets at all, or (B) it throws that the OS is not supported. This function is only called from call sites where the OS is not supported yet. (And (C) it skips the check if there is no package config.)

Copy link
Member Author

Choose a reason for hiding this comment

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

ahh, I see. I still think this is a misleading function, as its correctness relies on it only being called from OSes that are not supported. Let me file a tracking issue and link to it in the TODO however, to unblock this fix to de-flake our tests.

Future<void> ensureNoNativeAssetsOrOsIsSupported(
Uri workingDirectory,
String os,
FileSystem fileSystem,
NativeAssetsBuildRunner buildRunner,
) async {
if (await hasNoPackageConfig(buildRunner)) {
if (await _hasNoPackageConfig(buildRunner)) {
return;
}
final List<Package> packagesWithNativeAssets = await buildRunner.packagesWithNativeAssets();
Expand Down Expand Up @@ -345,12 +345,7 @@ Future<Uri?> dryRunNativeAssets({
buildRunner: buildRunner,
);
} else {
await ensureNoNativeAssetsOrOsIsSupported(
projectUri,
const LocalPlatform().operatingSystem,
fileSystem,
buildRunner,
);
await nativeBuildRequired(buildRunner);
nativeAssetsYaml = null;
}
case build_info.TargetPlatform.linux_arm64:
Expand Down Expand Up @@ -389,7 +384,7 @@ Future<Uri?> dryRunNativeAssetsMultipeOSes({
required FileSystem fileSystem,
required Iterable<build_info.TargetPlatform> targetPlatforms,
}) async {
if (await hasNoPackageConfig(buildRunner) || await isDisabledAndNoNativeAssets(buildRunner)) {
if (await nativeBuildRequired(buildRunner)) {
return null;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import 'package:file/file.dart';
import 'package:file/memory.dart';
import 'package:file_testing/file_testing.dart';
import 'package:flutter_tools/src/artifacts.dart';
import 'package:flutter_tools/src/base/common.dart';
import 'package:flutter_tools/src/base/file_system.dart';
import 'package:flutter_tools/src/base/logger.dart';
import 'package:flutter_tools/src/base/platform.dart';
Expand Down Expand Up @@ -86,6 +87,24 @@ void main() {
);
});

testUsingContext('does not throw if clang not present but no native assets present', overrides: <Type, Generator>{
FeatureFlags: () => TestFeatureFlags(isNativeAssetsEnabled: true),
ProcessManager: () => FakeProcessManager.empty(),
}, () async {
final File packageConfig = environment.projectDir.childFile('.dart_tool/package_config.json');
await packageConfig.create(recursive: true);
await buildNativeAssetsLinux(
projectUri: projectUri,
buildMode: BuildMode.debug,
fileSystem: fileSystem,
buildRunner: _BuildRunnerWithoutClang(),
);
expect(
(globals.logger as BufferLogger).traceText,
isNot(contains('Building native assets for ')),
);
});

testUsingContext('dry run for multiple OSes with no package config', overrides: <Type, Generator>{
ProcessManager: () => FakeProcessManager.empty(),
}, () async {
Expand Down Expand Up @@ -372,3 +391,8 @@ void main() {
expect(result.cc, Uri.file('/some/path/to/clang'));
});
}

class _BuildRunnerWithoutClang extends FakeNativeAssetsBuildRunner {
@override
Future<CCompilerConfig> get cCompilerConfig async => throwToolExit('Failed to find clang++ on the PATH.');
}