Skip to content

[native_assets_cli] Move BuildMode to [native_toolchain_c] #1800

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Dec 10, 2024
Merged

[native_assets_cli] Move BuildMode to [native_toolchain_c] #1800

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9fc9f15
[native_assets_cli] Move `BuildMode` to [native_toolchain_c]
dcharkes Dec 10, 2024
3ab2ed8
cleanup
dcharkes Dec 10, 2024
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
4 changes: 0 additions & 4 deletions pkgs/native_assets_builder/lib/src/build_runner/build_runner.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,6 @@ class NativeAssetsBuildRunner {
required BuildValidator buildValidator,
required ApplicationAssetValidator applicationAssetValidator,
required OS targetOS,
required BuildMode buildMode,
required Uri workingDirectory,
PackageLayout? packageLayout,
String? runPackageName,
Expand Down Expand Up @@ -124,7 +123,6 @@ class NativeAssetsBuildRunner {
..setupHookConfig(
targetOS: targetOS,
buildAssetTypes: buildAssetTypes,
buildMode: buildMode,
packageName: package.name,
packageRoot: packageLayout.packageRoot(package.name),
)
Expand Down Expand Up @@ -200,7 +198,6 @@ class NativeAssetsBuildRunner {
required LinkConfigValidator configValidator,
required LinkValidator linkValidator,
required OS targetOS,
required BuildMode buildMode,
required Uri workingDirectory,
required ApplicationAssetValidator applicationAssetValidator,
PackageLayout? packageLayout,
Expand All @@ -225,7 +222,6 @@ class NativeAssetsBuildRunner {
..setupHookConfig(
targetOS: targetOS,
buildAssetTypes: buildAssetTypes,
buildMode: buildMode,
packageName: package.name,
packageRoot: packageLayout.packageRoot(package.name),
);
Expand Down
2 changes: 0 additions & 2 deletions pkgs/native_assets_builder/test/build_runner/build_runner_reusability_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ void main() async {
await buildRunner.build(
configCreator: configCreator,
targetOS: OS.current,
buildMode: BuildMode.release,
workingDirectory: packageUri,
linkingEnabled: false,
buildAssetTypes: [],
Expand All @@ -46,7 +45,6 @@ void main() async {
);
await buildRunner.build(
configCreator: configCreator,
buildMode: BuildMode.release,
targetOS: OS.current,
workingDirectory: packageUri,
linkingEnabled: false,
Expand Down
1 change: 0 additions & 1 deletion pkgs/native_assets_builder/test/build_runner/concurrency_shared_test_helper.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ void main(List<String> args) async {
dartExecutable: dartExecutable,
).build(
configCreator: BuildConfigBuilder.new,
buildMode: BuildMode.release,
targetOS: target.os,
workingDirectory: packageUri,
linkingEnabled: false,
Expand Down
1 change: 0 additions & 1 deletion pkgs/native_assets_builder/test/build_runner/concurrency_test_helper.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ void main(List<String> args) async {
cCompilerConfig: dartCICompilerConfig,
targetMacOSVersion: OS.current == OS.macOS ? defaultMacOSVersion : null,
),
buildMode: BuildMode.release,
targetOS: OS.current,
workingDirectory: packageUri,
linkingEnabled: false,
Expand Down
4 changes: 0 additions & 4 deletions pkgs/native_assets_builder/test/build_runner/helpers.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,6 @@ Future<BuildResult?> build(
return configBuilder;
},
configValidator: configValidator,
buildMode: BuildMode.release,
targetOS: targetOS,
workingDirectory: packageUri,
packageLayout: packageLayout,
Expand Down Expand Up @@ -137,7 +136,6 @@ Future<LinkResult?> link(
return configBuilder;
},
configValidator: configValidator,
buildMode: BuildMode.release,
targetOS: target?.os ?? OS.current,
workingDirectory: packageUri,
packageLayout: packageLayout,
Expand Down Expand Up @@ -195,7 +193,6 @@ Future<(BuildResult?, LinkResult?)> buildAndLink(
targetAndroidNdkApi: targetAndroidNdkApi,
),
configValidator: buildConfigValidator,
buildMode: BuildMode.release,
targetOS: target?.os ?? OS.current,
workingDirectory: packageUri,
packageLayout: packageLayout,
Expand Down Expand Up @@ -228,7 +225,6 @@ Future<(BuildResult?, LinkResult?)> buildAndLink(
targetAndroidNdkApi: targetAndroidNdkApi,
),
configValidator: linkConfigValidator,
buildMode: BuildMode.release,
targetOS: target?.os ?? OS.current,
workingDirectory: packageUri,
packageLayout: packageLayout,
Expand Down
15 changes: 8 additions & 7 deletions pkgs/native_assets_builder/test/test_data/native_dynamic_linking_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,15 +32,16 @@ void main() async {

final configBuilder = BuildConfigBuilder()
..setupHookConfig(
packageName: name,
packageRoot: testPackageUri,
targetOS: OS.current,
buildAssetTypes: [CodeAsset.type],
buildMode: BuildMode.debug)
packageName: name,
packageRoot: testPackageUri,
targetOS: OS.current,
buildAssetTypes: [CodeAsset.type],
)
..setupBuildConfig(dryRun: false, linkingEnabled: false)
..setupBuildRunConfig(
outputDirectory: outputDirectory,
outputDirectoryShared: outputDirectoryShared)
outputDirectory: outputDirectory,
outputDirectoryShared: outputDirectoryShared,
)
..setupCodeConfig(
targetArchitecture: Architecture.current,
linkModePreference: LinkModePreference.dynamic,
Expand Down
15 changes: 8 additions & 7 deletions pkgs/native_assets_builder/test/test_data/transformer_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,15 +47,16 @@ void main() async {
Future<void> runBuild(Architecture architecture) async {
final configBuilder = BuildConfigBuilder()
..setupHookConfig(
packageName: packageName,
packageRoot: packageUri,
targetOS: OS.current,
buildAssetTypes: [DataAsset.type],
buildMode: BuildMode.debug)
packageName: packageName,
packageRoot: packageUri,
targetOS: OS.current,
buildAssetTypes: [DataAsset.type],
)
..setupBuildConfig(dryRun: false, linkingEnabled: false)
..setupBuildRunConfig(
outputDirectory: outputDirectory,
outputDirectoryShared: outputDirectoryShared)
outputDirectory: outputDirectory,
outputDirectoryShared: outputDirectoryShared,
)
..setupCodeConfig(
targetArchitecture: architecture,
linkModePreference: LinkModePreference.dynamic,
Expand Down
3 changes: 3 additions & 0 deletions pkgs/native_assets_builder/test_data/native_dynamic_linking/hook/build.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ void main(List<String> args) async {
sources: [
'src/debug.c',
],
buildMode: BuildMode.debug,
),
CBuilder.library(
name: 'math',
Expand All @@ -27,6 +28,7 @@ void main(List<String> args) async {
'src/math.c',
],
libraries: ['debug'],
buildMode: BuildMode.debug,
),
CBuilder.library(
name: 'add',
Expand All @@ -35,6 +37,7 @@ void main(List<String> args) async {
'src/add.c',
],
libraries: ['math'],
buildMode: BuildMode.debug,
)
];

Expand Down
4 changes: 4 additions & 0 deletions pkgs/native_assets_cli/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,10 @@
missing. This enables (1) code to run if an asset is missing but that code is
not invoked at runtime, and (2) doing fallback implementations in Dart if an
asset is missing.
- **Breaking change** Move `BuildMode` to `package:native_toolchain_c`. This way
it can be controlled in the build hook together with the `OptimizationLevel`.
Most likely, every package should ship with `release`. `BuildMode.debug`
should only be used while developing the package locally.
- Update pubspec.yaml of examples to use 0.9.0 of `package:native_assets_cli`.
- Consolidate [CodeAsset] specific things into `lib/src/code_assets/*`

Expand Down
1 change: 0 additions & 1 deletion pkgs/native_assets_cli/lib/code_assets.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ export 'native_assets_cli.dart'
EncodedAsset,
EncodedAssetBuildOutputBuilder,
EncodedAssetLinkOutputBuilder;
export 'src/build_mode.dart' show BuildMode;
export 'src/code_assets/architecture.dart' show Architecture;
export 'src/code_assets/c_compiler_config.dart' show CCompilerConfig;
export 'src/code_assets/code_asset.dart' show CodeAsset, OSLibraryNaming;
Expand Down
1 change: 0 additions & 1 deletion pkgs/native_assets_cli/lib/native_assets_cli.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ export 'src/api/build.dart' show build;
export 'src/api/builder.dart' show Builder;
export 'src/api/link.dart' show link;
export 'src/api/linker.dart' show Linker;
export 'src/build_mode.dart' show BuildMode;
export 'src/config.dart'
show
BuildConfig,
Expand Down
2 changes: 0 additions & 2 deletions pkgs/native_assets_cli/lib/src/code_assets/testing.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ Future<void> testCodeBuildHook({
// ignore: inference_failure_on_function_return_type
required Function(List<String> arguments) mainMethod,
required FutureOr<void> Function(BuildConfig, BuildOutput) check,
BuildMode? buildMode,
Architecture? targetArchitecture,
OS? targetOS,
IOSSdk? targetIOSSdk,
Expand Down Expand Up @@ -46,7 +45,6 @@ Future<void> testCodeBuildHook({
expect(await validateCodeAssetBuildOutput(config, output), isEmpty);
await check(config, output);
},
buildMode: buildMode,
targetOS: targetOS,
buildAssetTypes: buildAssetTypes,
linkingEnabled: linkingEnabled,
Expand Down
34 changes: 9 additions & 25 deletions pkgs/native_assets_cli/lib/src/config.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import 'package:crypto/crypto.dart' show sha256;
import 'package:pub_semver/pub_semver.dart';

import 'api/deprecation_messages.dart';
import 'build_mode.dart';
import 'code_assets/architecture.dart';
import 'encoded_asset.dart';
import 'json_utils.dart';
Expand Down Expand Up @@ -65,13 +64,6 @@ sealed class HookConfig {
/// The operating system being compiled for.
final OS targetOS;

/// The [BuildMode] that the code should be compiled in.
///
/// Currently [BuildMode.debug] and [BuildMode.release] are the only modes.
///
/// Not available during a dry run.
final BuildMode? _buildMode;

/// The asset types that the invoker of this hook supports.
final List<String> buildAssetTypes;

Expand All @@ -91,18 +83,7 @@ sealed class HookConfig {
targetOS = OS.fromString(json.string(_targetOSConfigKey)),
buildAssetTypes = json.optionalStringList(_buildAssetTypesKey) ??
json.optionalStringList(_supportedAssetTypesKey) ??
const [],
_buildMode = switch (json.optionalString(_buildModeConfigKey)) {
String value => BuildMode.fromString(value),
null => null,
};

BuildMode get buildMode {
if (_buildMode == null) {
throw StateError('Build mode should not be accessed in dry-run mode.');
}
return _buildMode;
}
const [];

@override
String toString() => const JsonEncoder.withIndent(' ').convert(json);
Expand All @@ -118,16 +99,12 @@ sealed class HookConfigBuilder {
required String packageName,
required OS targetOS,
required List<String> buildAssetTypes,
required BuildMode? buildMode,
}) {
json[_packageNameConfigKey] = packageName;
json[_packageRootConfigKey] = packageRoot.toFilePath();
json[_targetOSConfigKey] = targetOS.toString();
json[_buildAssetTypesKey] = buildAssetTypes;
json[_supportedAssetTypesKey] = buildAssetTypes;
if (buildMode != null) {
json[_buildModeConfigKey] = buildMode.toString();
}
}

/// Constructs a checksum for a [BuildConfig].
Expand Down Expand Up @@ -159,7 +136,8 @@ sealed class HookConfigBuilder {
}

const _targetOSConfigKey = 'target_os';
const _buildModeConfigKey = 'build_mode';
// TODO: Bump min-SDK constraint to 3.7 and remove once stable.
const _buildModeConfigKeyDeprecated = 'build_mode';
const _metadataConfigKey = 'metadata';
const _outDirConfigKey = 'out_dir';
const _outDirSharedConfigKey = 'out_dir_shared';
Expand Down Expand Up @@ -207,6 +185,10 @@ final class BuildConfigBuilder extends HookConfigBuilder {
json[_dependencyMetadataKey] = {
for (final key in metadata.keys) key: metadata[key]!.toJson(),
};
// TODO: Bump min-SDK constraint to 3.7 and remove once stable.
Copy link
Member

Choose a reason for hiding this comment

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

This function should no longer take dryRun instead it should hardcode it to false in the json.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That would break the tests in native_toolchain_c that check that no assets are built if dryRun is true. We can only remove such functionality once we can bump the Dart SDK constraint to 3.7 stable (assuming we get all the refactorings landed in both Dart and Flutter before the branchcut for 3.7).

Copy link
Member

Choose a reason for hiding this comment

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

This code here is the builder code (i.e. code for Dart SDK & Flutter SDK - which no longer set this to true), no?

The package:native_toolchain_c is for hook writers, it can still read the dry run (possibly produced by older versions of Dart SDK & older versions of Flutter SDK).

Copy link
Collaborator Author

@dcharkes dcharkes Dec 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The builder code is used to produce a config in the unit tests of native_toolchain_c (pkgs/native_toolchain_c/test/cbuilder/cbuilder_test.dart). Edit: It seems we don't have any expect that no asset is produced no dry run though, we should have had that. 🤓

if (!dryRun) {
json[_buildModeConfigKeyDeprecated] = 'release';
}
}

void setupBuildRunConfig({
Expand Down Expand Up @@ -237,6 +219,8 @@ final class LinkConfigBuilder extends HookConfigBuilder {
required List<EncodedAsset> assets,
}) {
json[_assetsKey] = [for (final asset in assets) asset.toJson()];
// TODO: Bump min-SDK constraint to 3.7 and remove once stable.
json[_buildModeConfigKeyDeprecated] = 'release';
}

void setupLinkRunConfig({
Expand Down
5 changes: 5 additions & 0 deletions pkgs/native_assets_cli/lib/src/model/build_config_CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,11 @@
Compatibility with older SDKs: Create a sibling dir next to the output
directory. This does not facilitate caching, but should not break the hook.
Compatibility with older hooks: These will not read this field.
- `BuildConfig.buildMode` is removed. Instead it is specified by hook writers
in the `CBuilder` constructors.
Compatibility with older SDKs: The new hooks will not read the field.
Compatibility with older hooks: The field is now always passed as 'release'
until we can bump the SDK constraint in `package:native_assets_cli`.

## 1.5.0

Expand Down
2 changes: 0 additions & 2 deletions pkgs/native_assets_cli/lib/test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ Future<void> testBuildHook({
required FutureOr<void> Function(List<String> arguments) mainMethod,
required FutureOr<void> Function(BuildConfig config, BuildOutput output)
check,
BuildMode? buildMode,
OS? targetOS,
List<String>? buildAssetTypes,
bool? linkingEnabled,
Expand All @@ -44,7 +43,6 @@ Future<void> testBuildHook({
packageName: _readPackageNameFromPubspec(),
targetOS: targetOS ?? OS.current,
buildAssetTypes: buildAssetTypes ?? [],
buildMode: buildMode ?? BuildMode.release,
)
..setupBuildConfig(
dryRun: false,
Expand Down
1 change: 0 additions & 1 deletion pkgs/native_assets_cli/test/api/build_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ void main() async {
packageName: packageName,
targetOS: OS.iOS,
buildAssetTypes: ['foo'],
buildMode: BuildMode.release,
)
..setupBuildConfig(
dryRun: false,
Expand Down
5 changes: 0 additions & 5 deletions pkgs/native_assets_cli/test/build_config_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ void main() async {
packageName: packageName,
packageRoot: packageRootUri,
targetOS: OS.android,
buildMode: BuildMode.release,
buildAssetTypes: ['my-asset-type'],
)
..setupBuildConfig(
Expand Down Expand Up @@ -87,7 +86,6 @@ void main() async {
expect(config.packageName, packageName);
expect(config.packageRoot, packageRootUri);
expect(config.targetOS, OS.android);
expect(config.buildMode, BuildMode.release);
expect(config.buildAssetTypes, ['my-asset-type']);

expect(config.linkingEnabled, false);
Expand All @@ -101,7 +99,6 @@ void main() async {
packageName: packageName,
packageRoot: packageRootUri,
targetOS: OS.android,
buildMode: null, // not available in dry run
buildAssetTypes: ['my-asset-type'],
)
..setupBuildConfig(
Expand Down Expand Up @@ -138,7 +135,6 @@ void main() async {
expect(config.packageRoot, packageRootUri);
expect(config.targetOS, OS.android);
expect(config.buildAssetTypes, ['my-asset-type']);
expect(() => config.buildMode, throwsStateError);

expect(config.linkingEnabled, true);
expect(config.dryRun, true);
Expand Down Expand Up @@ -210,7 +206,6 @@ void main() async {
'package_root': packageRootUri.toFilePath(),
'target_os': 'android',
'linking_enabled': true,
'build_mode': BuildMode.release.name,
'build_asset_types': ['my-asset-type'],
'dependency_metadata': {
'bar': {'key': 'value'},
Expand Down
Loading
Loading