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

[native_assets_cli] Move BuildMode to [native_toolchain_c] #1800

Merged
merged 2 commits into from
Dec 10, 2024

Conversation

dcharkes
Copy link
Collaborator

@dcharkes dcharkes commented Dec 10, 2024

The first in a series of refactorings to address:

We align BuildMode with OptimizationLevel, it's configured by the hook author in the build hook. Likely, every package should ship with max optimization level and build mode set to release. Only while developing a package (when it is the root package or path-depsed in) would the build mode be set to debug.

This will require a manual roll into dartdev and flutter tools due to the removal of BuildMode from the native_assets_builder API.

The native assets builder will still emit the field in the JSON, until we can bump the native_assets_cli SDK constraint to 3.7 stable. Then only people with older versions of the native_assets_cli package but a newer SDK break.

@coveralls
Copy link

coveralls commented Dec 10, 2024

Coverage Status

coverage: 88.699% (-0.02%) from 88.719%
when pulling 3ab2ed8 on remove-buildmode
into d37df9c on main.

@@ -118,16 +99,13 @@ sealed class HookConfigBuilder {
required String packageName,
required OS targetOS,
required List<String> buildAssetTypes,
required BuildMode? buildMode,
// required BuildMode? buildMode,
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this.

@@ -207,6 +186,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

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

@auto-submit auto-submit bot merged commit 00aae9e into main Dec 10, 2024
24 checks passed
@auto-submit auto-submit bot deleted the remove-buildmode branch December 10, 2024 20:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants