Skip to content

Conversation

@jonahwilliams
Copy link
Contributor

@jonahwilliams jonahwilliams commented May 16, 2019

Description

A re-imagining of how we structure and describe builds for the flutter tool

Target build/system API

/// Assemble the assets used in the application into a build directory.
const Target copyAssets = Target(
  name: 'copy_assets',
  inputs: <Object>[
    '{PROJECT_DIR}/pubspec.yaml',
    listAssets, // <- parses pubspec file and outputs file paths.
  ],
  outputs: <Object>[
    '{BUILD_DIR}/flutter_assets/AssetManifest.json',
    '{BUILD_DIR}/flutter_assets/FontManifest.json',
    '{BUILD_DIR}/flutter_assets/LICENSE',
    listOutputAssets,
  ],
  dependencies: <Target>[],
  invocation: copyAssetsInvocation,
);

All files and directories are specified as file URIs relative to some "magic environment variable" defined below. This keeps us sane in the face of Windows. The {PROJECT_DIR}, {CACHE_DIR}, {BUILD_DIR} are automatically substituted variables that can be configured.

listAssets and listOutputAssets are top level or static functions which take a configuration object and return a list of files. These are used for dependency analysis when executing a target.

copyAssetsInvocation is a top level or static function which takes a configuration object. The tooling will verify that it produces the expected outputs. Placing all of the relevant configuration on single object (instead of providing some ambiently) makes it easier for the system to correctly invalidate and for authors and reviewers to identify whether the implementation is correct.

assemble command line API

The ability to inspect and execute targets is provided via a command line API. This is intended to be much lower level than the build APIs for integrating with external build systems. This is intended to initially replace the xcode_backend and desktop specific shell scripts with an API that can be directly embedded in the host build system.

Example: invoke a particular target

flutter assemble run -dbuildMode=debug  -dFlavor=cheese android_apk

Example: list the inputs for the macOS target

flutter assemble inputs macos_debug
> lib/main.dart
> pubspec.yaml
> assets/foo.png

Example: Direct integration into a makefile

$(FLUTTER_ROOT/bin/flutter assemble inputs linux_debug): flutter
flutter: $(FLUTTER_ROOT/bin/flutter assemble run linux_debug \ 
                --target-platform=linux-x64 \
                --build=mode=$(MODE) 

Related Issues

#27720
#28415
#16604

Tests

I added the following tests: Several

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I signed the CLA.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (Please read Handling breaking changes). Replace this with a link to the e-mail where you asked for input on this proposed change.
  • No, this is not a breaking change.

@Piinks Piinks added the tool Affects the "flutter" command-line tool. See also t: labels. label May 16, 2019
@jonahwilliams jonahwilliams marked this pull request as ready for review May 17, 2019 17:59
@jonahwilliams
Copy link
Contributor Author

From discussion, this is currently missing a way to specify tool inputs.

@jonahwilliams
Copy link
Contributor Author

Need to fix my File/Uri logic

@jonahwilliams
Copy link
Contributor Author

I'll need to create an artifacts class which can correctly delegate to the environment.cacheDir value for testing purposes

@jonahwilliams
Copy link
Contributor Author

Update with an artifact source, which can map to cache or local engine.

@jonahwilliams jonahwilliams requested a review from zanderso July 10, 2019 22:09
Copy link
Member

@zanderso zanderso left a comment

Choose a reason for hiding this comment

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

lgtm w/ misc concerns and nits.


/// Supports compiling a dart kernel file to an assembly file.
///
/// If more than one iOS arch is provided, then this rule will
Copy link
Member

Choose a reason for hiding this comment

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

It feels a bit like a layering violation for there to be iOS specific code here, but since that is what the AOTSnapshotter is expecting, I don't see an easy way around it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll attempt to address the layering a bit when integrating the iOS implementation. It seems likely that there will be no generic "dart" rules beyond generation of the kernel.

// If we're building multiple iOS archs the binaries need to be lipo'd
// together.
final List<Future<int>> pending = <Future<int>>[];
for (IOSArch iosArch in iosArchs) {
Copy link
Member

Choose a reason for hiding this comment

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

Further thought along these lines: the existence of a special case for iOS here (and probably also future special cases for Fuchsia) implies that dart artifact build rules will need to be target platform specific. This is okay for now though, I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Responded to above

outputs: <Source>[],
dependencies: <Target>[
copyAssets,
aotAssemblyProfile,
Copy link
Member

Choose a reason for hiding this comment

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

Again, okay if not in this PR, but the iOS-specific AOT rules could go in this file, for example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Responded to above

fs.file(fs.path.join('bin', 'cache', 'pkg', 'sky_engine', 'lib', 'ui', 'ui.dart')).createSync(recursive: true);
fs.file(fs.path.join('bin', 'cache', 'pkg', 'sky_engine', 'sdk_ext', 'vmservice_io.dart')).createSync(recursive: true);
fs.file(fs.path.join('bin', 'cache', 'dart-sdk', 'bin', 'dart')).createSync(recursive: true);
fs.file(fs.path.join('bin', 'cache', 'artifacts', 'engine', getNameForHostPlatform(hostPlatform), 'frontend_server.dart.snapshot')).createSync(recursive: true);
Copy link
Member

Choose a reason for hiding this comment

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

These lines are getting a bit long.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried to shorten most of them. Will look for a better solution for the testbed though.

@jonahwilliams jonahwilliams merged commit e91b98a into flutter:master Jul 11, 2019
@jonahwilliams jonahwilliams deleted the a_real_human_bean branch July 11, 2019 23:53
johnsonmh pushed a commit to johnsonmh/flutter that referenced this pull request Jul 30, 2019
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants