Skip to content

Commit 2563591

Browse files
mkustermannmkustermann
authored
Fix duplicate work in native assets release builds (flutter#158980)
In release builds linking of native assets is enabled. The build step is only a temprary step, it's output is given to the link step which then returns all final assets (effectively a map-reduce system). Assets that aren't sent to a specific linker could be conceptually viewed as sent to a linker that will emit it's input as-is. => The code currently took output of build & link step and therefore accumulated assets that aren't explicitly sent to a linker twice. => This led to performing work twice for those (e.g. copying them twice) This PR changes this such that if linking mode is enabled, we only rely on the output of the link phase. That in return means many tests that mock the native asset builds need to be updated to mock the output of the link phase.
1 parent eaa99f2 commit 2563591

File tree

8 files changed

+207
-87
lines changed

8 files changed

+207
-87
lines changed

‎packages/flutter_tools/lib/src/isolated/native_assets/native_assets.dart‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -750,9 +750,10 @@ Future<DartBuildResult> _runDartBuild({
750750
if (buildResult == null) {
751751
_throwNativeAssetsBuildFailed();
752752
}
753-
assets.addAll(buildResult.encodedAssets);
754753
dependencies.addAll(buildResult.dependencies);
755-
if (linkingEnabled) {
754+
if (!linkingEnabled) {
755+
assets.addAll(buildResult.encodedAssets);
756+
} else {
756757
final LinkResult? linkResult = await buildRunner.link(
757758
supportedAssetTypes: <String>[CodeAsset.type],
758759
configCreator: () => LinkConfigBuilder()

‎packages/flutter_tools/test/general.shard/isolated/android/native_assets_test.dart‎

Lines changed: 15 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -67,21 +67,26 @@ void main() {
6767
final File dylibAfterCompiling = fileSystem.file('libbar.so');
6868
// The mock doesn't create the file, so create it here.
6969
await dylibAfterCompiling.create();
70+
71+
final List<CodeAsset> codeAssets = <CodeAsset>[
72+
CodeAsset(
73+
package: 'bar',
74+
name: 'bar.dart',
75+
linkMode: DynamicLoadingBundled(),
76+
os: OS.android,
77+
architecture: Architecture.arm64,
78+
file: Uri.file('libbar.so'),
79+
),
80+
];
7081
final FakeFlutterNativeAssetsBuildRunner buildRunner = FakeFlutterNativeAssetsBuildRunner(
7182
packagesWithNativeAssetsResult: <Package>[
7283
Package('bar', projectUri),
7384
],
7485
buildResult: FakeFlutterNativeAssetsBuilderResult.fromAssets(
75-
codeAssets: <CodeAsset>[
76-
CodeAsset(
77-
package: 'bar',
78-
name: 'bar.dart',
79-
linkMode: DynamicLoadingBundled(),
80-
os: OS.android,
81-
architecture: Architecture.arm64,
82-
file: Uri.file('libbar.so'),
83-
),
84-
],
86+
codeAssets: codeAssets,
87+
),
88+
linkResult: FakeFlutterNativeAssetsBuilderResult.fromAssets(
89+
codeAssets: codeAssets,
8590
),
8691
);
8792
await runFlutterSpecificDartBuild(

‎packages/flutter_tools/test/general.shard/isolated/build_system/targets/native_assets_test.dart‎

Lines changed: 29 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -190,23 +190,27 @@ void main() {
190190
() async {
191191
await createPackageConfig(iosEnvironment);
192192

193+
final List<CodeAsset> codeAssets = <CodeAsset>[
194+
CodeAsset(
195+
package: 'foo',
196+
name: 'foo.dart',
197+
linkMode: DynamicLoadingBundled(),
198+
os: OS.iOS,
199+
architecture: Architecture.arm64,
200+
file: Uri.file('foo.framework/foo'),
201+
),
202+
];
193203
final FlutterNativeAssetsBuildRunner buildRunner = FakeFlutterNativeAssetsBuildRunner(
194204
packagesWithNativeAssetsResult: <Package>[Package('foo', iosEnvironment.buildDir.uri)],
195205
buildResult: FakeFlutterNativeAssetsBuilderResult.fromAssets(
196-
codeAssets: <CodeAsset>[
197-
CodeAsset(
198-
package: 'foo',
199-
name: 'foo.dart',
200-
linkMode: DynamicLoadingBundled(),
201-
os: OS.iOS,
202-
architecture: Architecture.arm64,
203-
file: Uri.file('foo.framework/foo'),
204-
),
205-
],
206+
codeAssets: codeAssets,
206207
dependencies: <Uri>[
207208
Uri.file('src/foo.c'),
208209
],
209210
),
211+
linkResult: FakeFlutterNativeAssetsBuilderResult.fromAssets(
212+
codeAssets: codeAssets,
213+
),
210214
);
211215
await NativeAssets(buildRunner: buildRunner).build(iosEnvironment);
212216

@@ -252,26 +256,30 @@ void main() {
252256
await createPackageConfig(androidEnvironment);
253257
await fileSystem.file('libfoo.so').create();
254258

259+
final List<CodeAsset> codeAssets = <CodeAsset>[
260+
if (hasAssets)
261+
CodeAsset(
262+
package: 'foo',
263+
name: 'foo.dart',
264+
linkMode: DynamicLoadingBundled(),
265+
os: OS.android,
266+
architecture: Architecture.arm64,
267+
file: Uri.file('libfoo.so'),
268+
),
269+
];
255270
final FakeFlutterNativeAssetsBuildRunner buildRunner = FakeFlutterNativeAssetsBuildRunner(
256271
packagesWithNativeAssetsResult: <Package>[
257272
Package('foo', androidEnvironment.buildDir.uri)
258273
],
259274
buildResult: FakeFlutterNativeAssetsBuilderResult.fromAssets(
260-
codeAssets: <CodeAsset>[
261-
if (hasAssets)
262-
CodeAsset(
263-
package: 'foo',
264-
name: 'foo.dart',
265-
linkMode: DynamicLoadingBundled(),
266-
os: OS.android,
267-
architecture: Architecture.arm64,
268-
file: Uri.file('libfoo.so'),
269-
),
270-
],
275+
codeAssets: codeAssets,
271276
dependencies: <Uri>[
272277
Uri.file('src/foo.c'),
273278
],
274279
),
280+
linkResult: FakeFlutterNativeAssetsBuilderResult.fromAssets(
281+
codeAssets: codeAssets,
282+
),
275283
);
276284
await NativeAssets(buildRunner: buildRunner).build(androidEnvironment);
277285
expect(

‎packages/flutter_tools/test/general.shard/isolated/fake_native_assets_build_runner.dart‎

Lines changed: 35 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,8 @@ import 'package:native_assets_builder/native_assets_builder.dart';
1212
import 'package:native_assets_cli/code_assets_builder.dart';
1313
import 'package:package_config/package_config_types.dart';
1414

15+
export 'package:native_assets_cli/code_assets_builder.dart' show CodeAsset, DynamicLoadingBundled;
16+
1517
/// Mocks all logic instead of using `package:native_assets_builder`, which
1618
/// relies on doing process calls to `pub` and the local file system.
1719
class FakeFlutterNativeAssetsBuildRunner
@@ -20,6 +22,7 @@ class FakeFlutterNativeAssetsBuildRunner
2022
this.hasPackageConfigResult = true,
2123
this.packagesWithNativeAssetsResult = const <Package>[],
2224
this.onBuild,
25+
this.onLink,
2326
this.buildDryRunResult = const FakeFlutterNativeAssetsBuilderResult(),
2427
this.buildResult = const FakeFlutterNativeAssetsBuilderResult(),
2528
this.linkResult = const FakeFlutterNativeAssetsBuilderResult(),
@@ -30,6 +33,7 @@ class FakeFlutterNativeAssetsBuildRunner
3033
ndkCCompilerConfigResult ?? CCompilerConfig();
3134

3235
final BuildResult? Function(BuildConfig)? onBuild;
36+
final LinkResult? Function(LinkConfig)? onLink;
3337
final BuildResult? buildResult;
3438
final LinkResult? linkResult;
3539
final BuildDryRunResult? buildDryRunResult;
@@ -99,11 +103,29 @@ class FakeFlutterNativeAssetsBuildRunner
99103
required Uri workingDirectory,
100104
required BuildResult buildResult,
101105
}) async {
102-
for (final Package _ in packagesWithNativeAssetsResult) {
106+
LinkResult? result = linkResult;
107+
for (final Package package in packagesWithNativeAssetsResult) {
108+
final LinkConfigBuilder configBuilder = configCreator()
109+
..setupHookConfig(
110+
packageRoot: package.root,
111+
packageName: package.name,
112+
targetOS: targetOS,
113+
supportedAssetTypes: supportedAssetTypes,
114+
buildMode: buildMode,
115+
)
116+
..setupLinkRunConfig(
117+
outputDirectory: Uri.parse('build-out-dir'),
118+
outputDirectoryShared: Uri.parse('build-out-dir-shared'),
119+
recordedUsesFile: null,
120+
);
121+
final LinkConfig buildConfig = LinkConfig(configBuilder.json);
122+
if (onLink != null) {
123+
result = onLink!(buildConfig);
124+
}
103125
lastBuildMode = buildMode;
104126
linkInvocations++;
105127
}
106-
return linkResult;
128+
return result;
107129
}
108130

109131
@override
@@ -148,12 +170,22 @@ final class FakeFlutterNativeAssetsBuilderResult
148170

149171
factory FakeFlutterNativeAssetsBuilderResult.fromAssets({
150172
List<CodeAsset> codeAssets = const <CodeAsset>[],
173+
Map<String, List<CodeAsset>> codeAssetsForLinking = const <String, List<CodeAsset>>{},
151174
List<Uri> dependencies = const <Uri>[],
152175
}) {
153176
return FakeFlutterNativeAssetsBuilderResult(
154177
encodedAssets: <EncodedAsset>[
155178
for (final CodeAsset codeAsset in codeAssets) codeAsset.encode(),
156-
], dependencies: dependencies);
179+
],
180+
encodedAssetsForLinking: <String, List<EncodedAsset>>{
181+
for (final String linkerName in codeAssetsForLinking.keys)
182+
linkerName: <EncodedAsset>[
183+
for (final CodeAsset codeAsset in codeAssetsForLinking[linkerName]!)
184+
codeAsset.encode(),
185+
],
186+
},
187+
dependencies: dependencies,
188+
);
157189
}
158190

159191
@override

‎packages/flutter_tools/test/general.shard/isolated/ios/native_assets_test.dart‎

Lines changed: 24 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -162,31 +162,35 @@ void main() {
162162
final Uri nonFlutterTesterAssetUri = environment.buildDir.childFile('native_assets.yaml').uri;
163163
await packageConfig.parent.create();
164164
await packageConfig.create();
165+
166+
List<CodeAsset> codeAssets(OS targetOS, CodeConfig codeConfig) => <CodeAsset>[
167+
CodeAsset(
168+
package: 'bar',
169+
name: 'bar.dart',
170+
linkMode: DynamicLoadingBundled(),
171+
os: targetOS,
172+
architecture: codeConfig.targetArchitecture,
173+
file: Uri.file('${codeConfig.targetArchitecture}/libbar.dylib'),
174+
),
175+
CodeAsset(
176+
package: 'buz',
177+
name: 'buz.dart',
178+
linkMode: DynamicLoadingBundled(),
179+
os: targetOS,
180+
architecture: codeConfig.targetArchitecture,
181+
file: Uri.file('${codeConfig.targetArchitecture}/libbuz.dylib'),
182+
),
183+
];
165184
final FakeFlutterNativeAssetsBuildRunner buildRunner = FakeFlutterNativeAssetsBuildRunner(
166185
packagesWithNativeAssetsResult: <Package>[
167186
Package('bar', projectUri),
168187
],
169188
onBuild: (BuildConfig config) =>
170-
FakeFlutterNativeAssetsBuilderResult.fromAssets(
171-
codeAssets: <CodeAsset>[
172-
CodeAsset(
173-
package: 'bar',
174-
name: 'bar.dart',
175-
linkMode: DynamicLoadingBundled(),
176-
os: config.targetOS,
177-
architecture: config.codeConfig.targetArchitecture,
178-
file: Uri.file('${config.codeConfig.targetArchitecture}/libbar.dylib'),
179-
),
180-
CodeAsset(
181-
package: 'buz',
182-
name: 'buz.dart',
183-
linkMode: DynamicLoadingBundled(),
184-
os: config.targetOS,
185-
architecture: config.codeConfig.targetArchitecture,
186-
file: Uri.file('${config.codeConfig.targetArchitecture}/libbuz.dylib'),
187-
),
188-
],
189-
),
189+
FakeFlutterNativeAssetsBuilderResult.fromAssets(codeAssets: codeAssets(config.targetOS, config.codeConfig)),
190+
onLink: (LinkConfig config) =>
191+
buildMode == BuildMode.debug
192+
? null
193+
: FakeFlutterNativeAssetsBuilderResult.fromAssets(codeAssets: codeAssets(config.targetOS, config.codeConfig)),
190194
);
191195
await runFlutterSpecificDartBuild(
192196
environmentDefines: <String, String>{

‎packages/flutter_tools/test/general.shard/isolated/macos/native_assets_test.dart‎

Lines changed: 23 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -268,31 +268,34 @@ void main() {
268268
final Uri nonFlutterTesterAssetUri = environment.buildDir.childFile('native_assets.yaml').uri;
269269
await packageConfig.parent.create();
270270
await packageConfig.create();
271+
272+
List<CodeAsset> codeAssets(OS targetOS, CodeConfig codeConfig) => <CodeAsset>[
273+
CodeAsset(
274+
package: 'bar',
275+
name: 'bar.dart',
276+
linkMode: DynamicLoadingBundled(),
277+
os: targetOS,
278+
architecture: codeConfig.targetArchitecture,
279+
file: Uri.file('${codeConfig.targetArchitecture}/libbar.dylib'),
280+
),
281+
CodeAsset(
282+
package: 'buz',
283+
name: 'buz.dart',
284+
linkMode: DynamicLoadingBundled(),
285+
os: targetOS,
286+
architecture: codeConfig.targetArchitecture,
287+
file: Uri.file('${codeConfig.targetArchitecture}/libbuz.dylib'),
288+
),
289+
];
271290
final FakeFlutterNativeAssetsBuildRunner buildRunner = FakeFlutterNativeAssetsBuildRunner(
272291
packagesWithNativeAssetsResult: <Package>[
273292
Package('bar', projectUri),
274293
],
275294
onBuild: (BuildConfig config) =>
276-
FakeFlutterNativeAssetsBuilderResult.fromAssets(
277-
codeAssets: <CodeAsset>[
278-
CodeAsset(
279-
package: 'bar',
280-
name: 'bar.dart',
281-
linkMode: DynamicLoadingBundled(),
282-
os: config.targetOS,
283-
architecture: config.codeConfig.targetArchitecture,
284-
file: Uri.file('${config.codeConfig.targetArchitecture}/libbar.dylib'),
285-
),
286-
CodeAsset(
287-
package: 'buz',
288-
name: 'buz.dart',
289-
linkMode: DynamicLoadingBundled(),
290-
os: config.targetOS,
291-
architecture: config.codeConfig.targetArchitecture,
292-
file: Uri.file('${config.codeConfig.targetArchitecture}/libbuz.dylib'),
293-
),
294-
],
295-
),
295+
FakeFlutterNativeAssetsBuilderResult.fromAssets(codeAssets: codeAssets(config.targetOS, config.codeConfig)),
296+
onLink: (LinkConfig config) => buildMode == BuildMode.debug
297+
? null
298+
: FakeFlutterNativeAssetsBuilderResult.fromAssets(codeAssets: codeAssets(config.targetOS, config.codeConfig)),
296299
);
297300
final (_, Uri nativeAssetsYaml) = await runFlutterSpecificDartBuild(
298301
environmentDefines: <String, String>{

‎packages/flutter_tools/test/general.shard/isolated/native_assets_test.dart‎

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -300,4 +300,67 @@ void main() {
300300
),
301301
);
302302
});
303+
304+
testUsingContext('Native assets: no duplicate assets with linking', overrides: <Type, Generator>{
305+
FeatureFlags: () => TestFeatureFlags(isNativeAssetsEnabled: true),
306+
ProcessManager: () => FakeProcessManager.empty(),
307+
}, () async {
308+
final File packageConfig =
309+
environment.projectDir.childFile('.dart_tool/package_config.json');
310+
final Uri nonFlutterTesterAssetUri = environment.buildDir.childFile('native_assets.yaml').uri;
311+
await packageConfig.parent.create();
312+
await packageConfig.create();
313+
314+
final File directSoFile = environment.projectDir.childFile('direct.so');
315+
directSoFile.writeAsBytesSync(<int>[]);
316+
final File linkableAFile = environment.projectDir.childFile('linkable.a');
317+
linkableAFile.writeAsBytesSync(<int>[]);
318+
final File linkedSoFile = environment.projectDir.childFile('linked.so');
319+
linkedSoFile.writeAsBytesSync(<int>[]);
320+
321+
CodeAsset makeCodeAsset(String name, Uri file, LinkMode linkMode)
322+
=> CodeAsset(
323+
package: 'bar',
324+
name: name,
325+
linkMode: linkMode,
326+
os: OS.linux,
327+
architecture: Architecture.x64,
328+
file: file,
329+
);
330+
331+
final (DartBuildResult result, _) = await runFlutterSpecificDartBuild(
332+
environmentDefines: <String, String>{
333+
// Release mode means the dart build has linking enabled.
334+
kBuildMode: BuildMode.release.cliName,
335+
},
336+
targetPlatform: TargetPlatform.linux_x64,
337+
projectUri: projectUri,
338+
nativeAssetsYamlUri: nonFlutterTesterAssetUri,
339+
fileSystem: fileSystem,
340+
buildRunner: FakeFlutterNativeAssetsBuildRunner(
341+
packagesWithNativeAssetsResult: <Package>[
342+
Package('bar', projectUri),
343+
],
344+
buildResult: FakeFlutterNativeAssetsBuilderResult.fromAssets(
345+
codeAssets: <CodeAsset>[
346+
makeCodeAsset('direct', directSoFile.uri, DynamicLoadingBundled()),
347+
],
348+
codeAssetsForLinking: <String, List<CodeAsset>>{
349+
'package:bar' : <CodeAsset>[
350+
makeCodeAsset('linkable', linkableAFile.uri, StaticLinking()),
351+
],
352+
},
353+
),
354+
linkResult: FakeFlutterNativeAssetsBuilderResult.fromAssets(
355+
codeAssets: <CodeAsset>[
356+
makeCodeAsset('direct', directSoFile.uri, DynamicLoadingBundled()),
357+
makeCodeAsset('linked', linkedSoFile.uri, DynamicLoadingBundled()),
358+
],
359+
),
360+
),
361+
);
362+
expect(
363+
result.codeAssets.map((CodeAsset c) => c.file!.toString()).toList()..sort(),
364+
<String>[directSoFile.uri.toString(), linkedSoFile.uri.toString()]);
365+
});
303366
}

0 commit comments

Comments
 (0)