[native_assets] Fix flutter build ios-framework#181507
Conversation
There was a problem hiding this comment.
Code Review
The pull request effectively addresses the issues related to native asset bundling for flutter build ios-framework. The introduction of the DartBuildEnvironment target correctly separates environment-dependent build parameters, improving caching and preventing build overwrites. The adoption of .xcframework for packaging native assets is a significant step forward for iOS framework integration. The updated integration tests provide good coverage for the new .xcframework structure and architecture validation.
packages/flutter_tools/lib/src/commands/build_ios_framework.dart
Outdated
Show resolved
Hide resolved
packages/flutter_tools/lib/src/build_system/targets/native_assets.dart
Outdated
Show resolved
Hide resolved
| Uri nativeAssetsBuildUri(Uri projectUri, String osName, {Map<String, String>? environmentDefines}) { | ||
| final String? override = environmentDefines?[kNativeAssetsInstallDirectory]; | ||
| if (override != null) { | ||
| return Uri.parse(override); | ||
| } | ||
| final String buildDir = getBuildDirectory(); | ||
| return projectUri.resolve('$buildDir/native_assets/$osName/'); |
There was a problem hiding this comment.
In nativeAssetsBuildUri, when kNativeAssetsInstallDirectory is provided via environmentDefines, the projectUri and osName parameters are effectively ignored. While this is the intended behavior for an override, it could be clearer by either making projectUri and osName nullable when environmentDefines is present, or by explicitly documenting that these parameters are bypassed in such cases. This is a minor clarity improvement.
bkonyi
left a comment
There was a problem hiding this comment.
LGTM, but let's wait for input from @vashworth to confirm the iOS project logic.
| } | ||
| for (final frameworkDirectorySimulator in frameworkDirectoriesSimulator) { | ||
| final Directory frameworkDirectoryDevice = nativeAssetDeviceDirectory.childDirectory( | ||
| frameworkDirectorySimulator.basename, |
There was a problem hiding this comment.
I don't think they are guaranteed to have the same name. For example, I've seen one that has them named as sqlite3arm64ios_sim.framework and sqlite3arm64ios.framework using build hooks.
There was a problem hiding this comment.
You might consider getting the NativeAssetsManifest.json from the build App.framework and then parsing the assets out of it. #179251 did something similar.
That's what I was planning to do for a new SwiftPM version of this command. See prototype:
https://github.com/vashworth/flutter/blob/2a62d6647f8917938d0bd3150c5ae32f20a984bb/packages/flutter_tools/lib/src/commands/build_swift_packages.dart#L671
https://github.com/vashworth/flutter/blob/2a62d6647f8917938d0bd3150c5ae32f20a984bb/packages/flutter_tools/lib/src/commands/build_swift_packages.dart#L725-L771
There was a problem hiding this comment.
@dcharkes any plan for merge this PR? I didn't a get chance to verify the fix.
There was a problem hiding this comment.
@vashworth The fix for #181724 means we will only have frameworks for the right build mode and SDK in the directory.
@HyperlinkKishanr Yes, but I uncovered more bugs and refactorings I had to land first.
e3edd7e to
3473bb9
Compare
|
@bkonyi @vashworth I completely reworked this PR. I removed the new |
bkonyi
left a comment
There was a problem hiding this comment.
LGTM once verified by @vashworth
packages/flutter_tools/lib/src/commands/build_ios_framework.dart
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Can this be simplified to now just copy from the nativeAssetsInBuiltProducts?
There was a problem hiding this comment.
Yes, provided we empty the directory in the Target so we don't see any frameworks that were removed. Done!
packages/flutter_tools/lib/src/commands/build_ios_framework.dart
Outdated
Show resolved
Hide resolved
| ...BuildFrameworkCommand.findFrameworkNames(simulatorBuildOutput), | ||
| ...BuildFrameworkCommand.findFrameworkNames(iPhoneBuildOutput), | ||
| }; | ||
| for (final frameworkName in frameworkNames) { |
There was a problem hiding this comment.
This still is a problem when it doesn't have the same name per build mode/arch/device. See original comment: #181507 (comment)
There was a problem hiding this comment.
I don't think so. Below we check existsSync, and otherwise we don't add it.
And the native assets mapping in the NativeAssetsManifest.json is different
for device and simulator:
- my_package/example/build/ios/framework/Release/App.xcframework/ios-arm64/App.framework/flutter_assets/NativeAssetsManifest.json
- my_package/example/build/ios/framework/Release/App.xcframework/ios-arm64_x86_64-simulator/App.framework/flutter_assets/NativeAssetsManifest.json
So the result would be a different framework for device and simulator and then
the different framework being loaded when run on the different one.
And, different names per architecture are overwritten to the first name found in fatAssetTargetLocationsIOS. (And then also the dylib names are rewritten as well with setInstallNamesDylib.)
Different OSes are different builds, so they shouldn't influence this.
(It could be worth it to add an integration test that does all kinds of wonky stuff instead of relying on flutter create --template package_ffi which is very regular.)
There was a problem hiding this comment.
So this is a still a problem, though, because if you have a separate xcframework for simulators vs physical phones and you embed it in your app (following our instructions https://docs.flutter.dev/add-to-app/ios/project-setup), it will error.
For example, if I use the sqlite3 plugin, it'll build sqlite3arm64ios.xcframework and sqlite3x64ios_sim.xcframework. If I embed both in my iOS app and build for a physical iOS device, it will fail with this error because the sqlite3x64ios_sim.xcframework does not support physical iOS devices:
![]()
There was a problem hiding this comment.
Really it'd be best if they had the same name. However, we might be able to wrap them in the same xcframework and name the xcframework with the physical device framework's name, since that's the one that will actually be included in an archive for the App Store.
For example,
- sqlite3arm64ios.xcframework
- ios-arm64
- sqlite3arm64ios.framework
- ios-arm64_x86_64-simulator
- sqlite3x64ios_sim.framework
- ios-arm64
I tested it against App Store validation and it passed so I think it's okay
There was a problem hiding this comment.
Thanks @vashworth. I didn't get it the first time that this was an Apple constraint.
I tested it against App Store validation and it passed so I think it's okay
I am afraid Apple will tighten their validation later, and things will start failing. So, I believe it'd be better to force package authors to use the same names.
(Since we do two flutter assemble calls and those the renaming and code signing, and then combine them into a framework we can't easily rename the simulator dylibs/frameworks. Unless we start passing in some new environment variable with the code assets manifest of for the device build.)
- I've added a hard error on this now. This will trigger bug reports for package authors to start supporting
flutter build ios-framework
Also, we've been silently rewriting the dylib names if the different architectures have different dylib names.
- I've added a warning on this. It will still work, but it will nudge package authors to do a cleaner naming solution for all ios and macos
build/runcommands.
This definitely requires some documentation, so I've filed:
747036b to
a50764a
Compare
Context: * flutter/flutter#181507 Code assets reported from build hooks must have consistent naming on iOS and MacOS to support all use cases (specifically add2app) and have error messages easy to understand for developers depending on packages with code assets. This PR adds documentation to the Flutter website to provide guidance.
| simulatorBuildOutput, | ||
| ); | ||
|
|
||
| for (final String assetId in simulatorAssets.keys) { |
There was a problem hiding this comment.
If the device build is the source of truth, shouldn't we iterate through that one instead?
| 'not present in the physical device build. \n' | ||
| 'The device build is the source of truth for distributed ' | ||
| 'frameworks. \n' | ||
| 'Ensure "$assetId" is also built for physical devices.', |
There was a problem hiding this comment.
This doesn't seem actionable for the app developer. Doesn't the plugin author need to make the change? Perhaps something like "Please file an issue with the $pluginName plugin."
| ' - iphoneos: $deviceAssetPath\n' | ||
| ' - iphonesimulator: $simulatorAssetPath\n\n' | ||
| 'Ensure the "build.dart" hook produces consistent filenames for ' | ||
| 'all targets.', |
There was a problem hiding this comment.
Same as above, make the message actionable for the app developer
| '{FLUTTER_ROOT}/packages/flutter_tools/lib/src/build_system/targets/native_assets.dart', | ||
| ), | ||
| // If different packages are resolved, different native assets might need to be built. | ||
| Source.pattern('{BUILD_DIR}/${DartBuild.dartHookResultFilename}'), |
There was a problem hiding this comment.
So I checked this out again. When I build an app through Xcode, then do a clean "Product > Clean Build Folder...", native assets are deleted from the build folder as I would expect. However, when I rebuild using Xcode, the native assets are not re-generated. I'm guessing because the input did not change and we don't track what this outputs really so the target gets skipped.
We likely need to update this target's outputs to have something in the OUTPUT_DIR, so that if it's deleted, the target will re-run. It'd be best for it to output each generated framework, but that may not be possible. Easiest would be to output like the native_assets.json to the OUTPUT_DIR too
| 'different architectures. Picking "$existingName" and ' | ||
| 'ignoring "$currentName". Ensuring consistent filenames in ' | ||
| '"build.dart" is recommended.', | ||
| ); |
There was a problem hiding this comment.
Warnings within flutter assemble don't show in Xcode unless they are prefixed with "warning: " and are sent to stderr. Perhaps use printXcodeWarning?
Also, make this warning more actionable for the app developer.
vashworth
left a comment
There was a problem hiding this comment.
LGTM once tests are passing! Sorry for all the back and forth!
Thanks for the thorough review @vashworth! 🙏 Catching bugs before they land! 💪 |
This PR fixes two issues. Accidental reuse of code assets between build modes and SDKs (flutter#181724), and the bundling in ios-frameworks (flutter#181382). To fix the accidental caching, the `Target`s related to build hooks and code assets now output their files to `environment.outputDir` instead of `$projectDir/$buildDir/native_assets`. * `xcode_backend` is updated to deal with this. * `Flutter.kt` has been updated to deal with this. * Because the `Target`s are responsible for caching, the code has been refactored to provide the target directories from there. The "global-ish" function `nativeAssetsBuildUri` that was calculating the directory before has been removed. * `runFlutterSpecificHooks` has nothing to do with that directory, it's access to it has been removed. * To avoid another cmakefile migration, the Linux and Windows implementation use the same directory. (Note that output dir and build dir overlap for Linux and Windows, while they do not for MacOS, iOS, and Android.) * This also means that we don't have to read `NativeAssetsManifest.json` in `xcode_backend` anymore. Instead the `Target` clears the output directory, so we should not have any stale frameworks. * Refactored `installCodeAssets` and its platform-specific implementations to return a list of all produced files. These are now added to the `Target`'s depfile. This fixes an issue where the build system would skip re-installing native assets after an Xcode "Clean Build Folder because it wasn't tracking the frameworks/dylibs as outputs. Closes: flutter#181724 Other `Target`s related tweaks: * Added proper `Source.pattern('{BUILD_DIR}/${DartBuild.dartHookResultFilename}'),` for all `Target`s that depend on that file. These were missing. (The build system uses `dependencies` for ordering of `Target`s, but relies on `inputs` and `outputs` for caching.) * Removed code assets from `CopyAssets`. That target is supposed to make an asset-bundle that is OS-independent if I understand correctly. This PR changes the way code assets are bundled in `flutter build ios-framework`. * This PR now packages in an `.xcframework`, which is necessary to be able to package both device and simulator. * Run through the frameworks of both device and simulator and give errors on inconsistencies. Closes: flutter#181382 Other iOS related tweaks: * Use `xcrun` for invoking all the commands. (This is used for producing the app framework, but was not for code assets frameworks.) * Make sure all commands are added to the traces when running verbose. (Also to bring it in line for with the other `xcrun` commands.) Testing: * The integration test is updated to inspect the `xcframework`s. * Added a test that simulates Xcode "Product > Clean Build Folder...", to check that it now correctly triggers a rebuild. * We do _not_ have an integration test that _runs_ the frameworks output from `flutter build ios-framework` inside a host app at all as far as I'm aware. * dev/devicelab/bin/tasks/build_ios_framework_module_test.dart builds a framework, but doesn't run it in a host app * dev/integration_tests/ios_add2app_life_cycle/build_and_test.sh runs, but does so via `flutter build ios` not as a framework. * Does not add an integration test for caching behavior between switching build modes. However, the proper functioning of `flutter build ios-framework` depends on the `Target`s for different not using overlapping directories. Architectural approaches tried but didn't work: * Subclass `InstallCodeAssets` per OS to be more precise in the `output`s on what files are output. This doesn't work because other OS-independent targets on the `InstallCodeAssets` target.
This PR fixes two issues. Accidental reuse of code assets between build modes and SDKs (#181724), and the bundling in ios-frameworks (#181382).
To fix the accidental caching, the
Targets related to build hooks and code assets now output their files toenvironment.outputDirinstead of$projectDir/$buildDir/native_assets.xcode_backendis updated to deal with this.Flutter.kthas been updated to deal with this.Targets are responsible for caching, the code has been refactored to provide the target directories from there. The "global-ish" functionnativeAssetsBuildUrithat was calculating the directory before has been removed.runFlutterSpecificHookshas nothing to do with that directory, it's access to it has been removed.NativeAssetsManifest.jsoninxcode_backendanymore. Instead theTargetclears the output directory, so we should not have any stale frameworks.installCodeAssetsand its platform-specific implementations to return a list of all produced files. These are now added to theTarget's depfile. This fixes an issue where the build system would skip re-installing native assets after an Xcode "Clean Build Folder because it wasn't tracking the frameworks/dylibs as outputs.Closes: #181724
Other
Targets related tweaks:Source.pattern('{BUILD_DIR}/${DartBuild.dartHookResultFilename}'),for allTargets that depend on that file. These were missing. (The build system usesdependenciesfor ordering ofTargets, but relies oninputsandoutputsfor caching.)CopyAssets. That target is supposed to make an asset-bundle that is OS-independent if I understand correctly.This PR changes the way code assets are bundled in
flutter build ios-framework..xcframework, which is necessary to be able to package both device and simulator.Closes: #181382
Other iOS related tweaks:
xcrunfor invoking all the commands. (This is used for producing the app framework, but was not for code assets frameworks.)xcruncommands.)Testing:
xcframeworks.flutter build ios-frameworkinside a host app at all as far as I'm aware.flutter build iosnot as a framework.flutter build ios-frameworkdepends on theTargets for different not using overlapping directories.Architectural approaches tried but didn't work:
InstallCodeAssetsper OS to be more precise in theoutputs on what files are output. This doesn't work because other OS-independent targets on theInstallCodeAssetstarget.