-
Notifications
You must be signed in to change notification settings - Fork 29.8k
Add initial implementation of flutter assemble #32816
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
Add initial implementation of flutter assemble #32816
Conversation
|
From discussion, this is currently missing a way to specify tool inputs. |
|
Need to fix my File/Uri logic |
…into a_real_human_bean
|
I'll need to create an artifacts class which can correctly delegate to the environment.cacheDir value for testing purposes |
|
Update with an artifact source, which can map to cache or local engine. |
3611877 to
30207cc
Compare
zanderso
left a comment
There was a problem hiding this 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Description
A re-imagining of how we structure and describe builds for the flutter tool
Target build/system API
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
Example: list the inputs for the macOS target
Example: Direct integration into a makefile
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.///).flutter analyze --flutter-repo) does not report any problems on my PR.Breaking Change
Does your PR require Flutter developers to manually update their apps to accommodate your change?