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] dry-run option #51

Closed
dcharkes opened this issue May 23, 2023 · 15 comments
Closed

[native_assets_cli] dry-run option #51

dcharkes opened this issue May 23, 2023 · 15 comments
Labels
P1 A high priority bug; for example, a single project is unusable or has many test failures package:native_assets_cli

Comments

@dcharkes
Copy link
Collaborator

In the context of Flutter, we need to know the list of dynamic libraries to bundle before we know the architecture and build mode.

The generated CMake (Windows, Linux), XCodeProject/CocoaPods/SwiftPackageManager (MacOS, iOS), Gradle (Android) need to know the list of dynamic libaries to bundle, they don't support "dynamic dependencies".

Moreover, the generated projects can be used to build for another architecture and another build-mode.

Consequently, the protocol in this package native_assets_cli needs a list command that lists the native assets that are going to be be build.

The list command will be invoked on the "outer" flutter build/flutter run command that generates the various build system projects.
The build command will then be invoked in the "inner" flutter assemble that is called from within these projects will then produce the native assets.

Currently, it is already the case that if you change your pub dependencies, you have to rerun the outer flutter command to have the right XCode/Gradle/CMake project. For native assets we'd have the same constraint, if you change the list of native assets (by having new dependency or a different version of a dependency) you need to run the "outer" flutter command.

We don't need a list command for the Dart ecosystem, because we don't have to deal with the constraints of existing build systems not supporting dynamic dependencies. We use no native build system for Dart apps.

@stuartmorgan @vashworth Do you have some references to previous issues with "dynamic dependencies" and workarounds for those with the build systems that Flutter is using? Feel free to add more color to this constraint.

cc @mkustermann @HosseinYousefi

@dcharkes dcharkes added P1 A high priority bug; for example, a single project is unusable or has many test failures package:native_assets_cli labels May 23, 2023
@dcharkes
Copy link
Collaborator Author

@stuartmorgan @vashworth I presume the XCode projects also have a late value for ios-device vs ios-simulator and the minimum ios-sdk-version?

@stuartmorgan
Copy link

I presume the XCode projects also have a late value for ios-device vs ios-simulator

Yes, target device in general (which isn't just device vs simulator; when devices have arch differences, which has happened before, there are more arch options controlled by this).

and the minimum ios-sdk-version?

In theory any build setting can be gated on build mode (debug/release/etc.), so this could be dynamic.

@vashworth
Copy link

vashworth commented May 23, 2023

Okay here's how we handle it for the Flutter.framework/FlutterMacOS.framework.

On the Cocoapods side of things, we generate a "fake" podspec for the Flutter framework so that Cocoapods does not try to download it from the Cocoapod trunk.
macOS - https://github.com/flutter/flutter/blob/ffbeb72336660cee22760d47a0b8a737ea99f9cc/packages/flutter_tools/bin/podhelper.rb#L229-L252
iOS - https://github.com/flutter/flutter/blob/ffbeb72336660cee22760d47a0b8a737ea99f9cc/packages/flutter_tools/bin/podhelper.rb#L191-L215

And then we update the build setting's Framework Search Paths to the corresponding engine path.
macOS - https://github.com/flutter/flutter/blob/ffbeb72336660cee22760d47a0b8a737ea99f9cc/packages/flutter_tools/bin/podhelper.rb#L124-L142
iOS - https://github.com/flutter/flutter/blob/ffbeb72336660cee22760d47a0b8a737ea99f9cc/packages/flutter_tools/bin/podhelper.rb#L42-L87

So before the compile phase, we run xcode_backend.sh build for iOS and macos_assemble.sh build for macOS, which in turn runs a flutter assemble command:
macOS - https://github.com/flutter/flutter/blob/ffbeb72336660cee22760d47a0b8a737ea99f9cc/packages/flutter_tools/bin/macos_assemble.sh#L46-L128
iOS - https://github.com/flutter/flutter/blob/ffbeb72336660cee22760d47a0b8a737ea99f9cc/packages/flutter_tools/bin/xcode_backend.dart#L314-L408

The assemble command will trigger the tool to copy the correct framework from the engine cache to the build directory. For iOS, this is where codesigning takes place.
macOS - https://github.com/flutter/flutter/blob/ffbeb72336660cee22760d47a0b8a737ea99f9cc/packages/flutter_tools/lib/src/build_system/targets/macos.dart#L48-L82
iOS - https://github.com/flutter/flutter/blob/ffbeb72336660cee22760d47a0b8a737ea99f9cc/packages/flutter_tools/lib/src/build_system/targets/ios.dart#L281-L302

Then after the compiling and linking phases, we run another script, xcode_backend.sh embed_and_thin for iOS and macos_assemble.sh embed for macOS. This will copy the .framework from the build directory to the .app. For macOS, this is also were codesigning takes place.
macOS - https://github.com/flutter/flutter/blob/ffbeb72336660cee22760d47a0b8a737ea99f9cc/packages/flutter_tools/bin/macos_assemble.sh#L132-L150
iOS - https://github.com/flutter/flutter/blob/ffbeb72336660cee22760d47a0b8a737ea99f9cc/packages/flutter_tools/bin/xcode_backend.dart#L181-L221

@dcharkes
Copy link
Collaborator Author

Notes from discussion with @mkustermann :

  • We should probably have a dry-run argument instead of different commands build/list.
  • It would be better to not have dry-run be build-mode & architecture agnostic, that would break symmetry. Instead, we should just run it for the cross-product of the parameters that we need for the XCode/Gradle/CMake project that we're targetting.
  • We should introduce a way to run the native_asset_clis for multiple configurations in a single process invocation.

@mkustermann
Copy link
Member

Just some clarifications

Then after the compiling and linking phases, we run another script, xcode_backend.sh embed_and_thin for iOS and macos_assemble.sh embed for macOS. This will copy the .framework from the build directory to the .app
macOS - https://github.com/flutter/flutter/blob/ffbeb72336660cee22760d47a0b8a737ea99f9cc/packages/flutter_tools/bin/macos_assemble.sh#L132-L150
iOS - https://github.com/flutter/flutter/blob/ffbeb72336660cee22760d47a0b8a737ea99f9cc/packages/flutter_tools/bin/xcode_backend.dart#L181-L221

It seems like this embedding step is using rsync to copy contents of a folder to the final place. Couldn't this also copy contents of another folder (containing dynamic number of shared libraries) to the final place?

The embedding step runs after linking, so one would need a different approach for that. The flutter build could always make one static library to be linked into the app - and we could combine all static libraries into one, making the build rules simple.

@dcharkes dcharkes changed the title [native_assets_cli] architecture & build-mode agnostic list command [native_assets_cli] dry-run option May 25, 2023
@dcharkes
Copy link
Collaborator Author

dcharkes commented May 25, 2023

One thing I forgot to mention in the discussions earlier: for hot-restart in flutter run to work, we need the list of dynamic libraries to be compiled into the kernel compilation in the "outer" flutter run invocation. When starting an app with flutter run, the first kernel that's loaded into the device is the one from the "inner" flutter assemble. See my analysis of the flow in internal doc.

For this, we could also use the dry-run command.

@stuartmorgan
Copy link

  • We should probably have a dry-run argument instead of different commands build/list.
  • It would be better to not have dry-run be build-mode & architecture agnostic, that would break symmetry. Instead, we should just run it for the cross-product of the parameters that we need for the XCode/Gradle/CMake project that we're targetting.

I don't understand; what happens when someone provides different output from the different runs?

As we discussed before, breaking symmetry is a feature, because it makes it unambiguous, and enforced, that the output list cannot be a function of those inputs.

@dcharkes
Copy link
Collaborator Author

dcharkes commented Jun 6, 2023

I don't understand; what happens when someone provides different output from the different runs?

Flutter would run all the dry-runs for all the combinations of params that a XCode/Gradle/CMake project that is generated can target. Flutter would then issue an error if the list of dynamic libraries is not equal.

As we discussed before, breaking symmetry is a feature, because it makes it unambiguous, and enforced, that the output list cannot be a function of those inputs.

This is a Flutter-specific requirement, and might not be specific to other embedders/launchers of Dart. Maybe some other launcher generates files in a build system which will have a different set of parameters that should not be creating differences in the output (something else than arch and debug/release). Baking in this Flutter-specific requirement into the protocol is the wrong place. The Flutter specific aspect can be done in Flutter by doing multiple dry-runs.

We will enforce that the list is identical in Flutter by running dry-run N (arch x debug/release) times and checking that the list of dylibs is identical.

I'm happy to set up a chat to discuss this further.

@stuartmorgan
Copy link

Flutter would run all the dry-runs for all the combinations of params that a XCode/Gradle/CMake project that is generated can target.

How are you planning to enumerate every possible value of those parameters?

@dcharkes
Copy link
Collaborator Author

dcharkes commented Jun 7, 2023

Flutter would run all the dry-runs for all the combinations of params that a XCode/Gradle/CMake project that is generated can target.

How are you planning to enumerate every possible value of those parameters?

The set of architecture, build modes, and iOS sdks is limited.
The NDK/iOS versions doesn't really make sense to enumerate. I'd hope that users don't branch on that.
(Users branching on all kinds of things would also create complications with closed source packages that download a prebuilt binary such as package:realm, they would need a prebuilt binary for each combination.)

As an alternative to enumerating, we could keep passing in the native-assets list from the "outer" invocation, set it as a property so that it will also be passed when running directly from xcode. We can then check that list of dylibs against the list of dylibs produced by the "inner" invocation in flutter assemble. (Maybe we need this anyway, as a sanity check that users don't do something completely different in dry-run ...)

@stuartmorgan do we have reasons to believe that --dry-run only having an OS and packaging preference would generalize to any Dart embedder/launcher we could ever have on the planet? E.g. is it intrinsically how other build systems work?

(Thinking about it, we probably need to also at least pass the dependency-metadata, if you want to depend or statically link on native code in other packages.)

@stuartmorgan
Copy link

The set of architecture, build modes, and iOS sdks is limited.

At least CMake allows defining arbitrary build modes.

@stuartmorgan do we have reasons to believe that --dry-run only having an OS and packaging preference would generalize to any Dart embedder/launcher we could ever have on the planet?

I'm not sure what this question means exactly. Are you asking if it's possible to construct a build system that requires creating a different artifact set based on parameters other than those? Maybe, but it seems like the safest/easiest way to figure out if we can get away with not providing information that's a foot-gun in some build systems would be to start out with the most restrictive set of information possible, and then see if there are any actual use cases for expanding that, rather than providing everything (which isn't actually everything that would generalize to any embedder we could ever have on the planet, but only the things we include now based on a best guess of what the relevant information might be to future theoretical embedder) first.

I'm approaching this not from the standpoint of baking in something inherently Flutter-specific, but that starting with a structure where we assert, structurally, that the artifact list can only vary on a very, very limited set of inputs reduces complexity (including for Flutter, but just in general), and so we should start there until/unless we have a concrete use case for increasing the complexity.

@dcharkes
Copy link
Collaborator Author

dcharkes commented Jun 7, 2023

limited set of inputs reduces complexity

I was thinking that having the same set of inputs and having a --dry-run flag reduces complexity, because it's just a boolean flag (like --verbose). @mkustermann Did you have any other reasons for making dry-run just a boolean flag?

I do agree though that if we lock it down at first, and open it up later if needed, that that is less breaking than the other way around.

@stuartmorgan
Copy link

limited set of inputs reduces complexity

I was thinking that having the same set of inputs and having a --dry-run flag reduces complexity, because it's just a boolean flag (like --verbose).

Sorry, "complexity" was a pretty vague way for me to describe that. I agree it's somewhat more complexity in the tool, but it reduces the potential complexity in the set of all possible behaviors that package developers can create and which we would be stuck supporting (which is a scarier type of complexity IMO, since it's outside of our direct control).

@mkustermann
Copy link
Member

mkustermann commented Jun 8, 2023

The main thing we should take into account is that a package's build.dart may produce a different set of assets for different configurations (e.g. windows vs linux, desktop vs phones, ...). So whatever solution comes out should support this.

Maybe, but it seems like the safest/easiest way to figure out if we can get away with not providing information that's a foot-gun in some build systems would be to start out with the most restrictive set of information possible, a

I fully agree with this. The communication between flutter tools / dart tools and the build.dart scripts is via a json-like interface. Some examples of how this may look for a build command

{
  'mode' : 'release',
  'target-platform': 'native',
  'native' : {
    'target-os', 'linux',
    'arch' : 'x64',
  },
  'c_compiler' : {
     'cflags': '...'
  },
  ...
}

or for producing native assets for dart2wasm:

{
  'mode' : 'release',
  'target-platform': 'wasm'
  'wasm' : {
    '...' : '...',
  }
  ...
}

The build.dart scripts will use helper packages that operate on this json-like data structure. They may look like this

  // The package needs the `libfoo.so` only on linux, on mac the package has a different implementation.
  if (config.targetPlatform == 'native' && config.native.targetOs == 'linux') {
    final cbuilder = CBuilder.library(
      name: 'libfoo.so',
      assetName: 'package:<foo>/<bar>',
      sources: ['src/foo.c', ...],
    );
    await cbuilder.run(config, output);
  }

Given this structure, I suggested that we re-use this configuration mechanism for a dry-run / list mechanism. This may avoid changing build.dart scripts - only the few helper packages like CBuilder need to branch on config.onlyListAssets and instead of actually building they'll simply note the set of assets they would produce into output.

I agree with @stuartmorgan that there's no need to supply config.c_compiler.cflags and various other details if we're only interested in the set of assets. It's a json after all - we can just leave those out.

@dcharkes
Copy link
Collaborator Author

The dry_run now does not accept architecture, build_mode, and some other parameters anymore:

/// The architecture being compiled for.
///
/// Not available in [dryRun].
Architecture get targetArchitecture {
_ensureNotDryRun();
return _targetArchitecture;
}

They cannot be supplied by the launcher, nor read by the user.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 A high priority bug; for example, a single project is unusable or has many test failures package:native_assets_cli
Projects
None yet
Development

No branches or pull requests

4 participants