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

Eliminate snapshot/depfile options to build bundle #21507

Merged
merged 1 commit into from
Sep 7, 2018

Conversation

cbracken
Copy link
Member

@cbracken cbracken commented Sep 6, 2018

The --snapshot argument was only necessary in Dart 1. The --depfile
argument was only used in Dart 2 mode to pass to the kernel compiler,
but was inconsistent with the 'build aot' command, where the depfile was
always set to build/kernel_compile.d.

This patch updates 'build bundle' to emit the depfile to a location
consistent with the 'build aot' command; since it's not intended to be
user-configurable and flutter.gradle hardcodes the location to
build/kernel_compile.d either way, this patch also eliminates the
ability to configure the filename altogether.

@cbracken
Copy link
Member Author

cbracken commented Sep 6, 2018

I realise I deleted the --depfile param from Xcode but not Gradle in the original PR here. I'll update this PR with the cleanup I mentioned above.

@cbracken cbracken changed the title Elinimate --snapshot option to build bundle Eliminate --snapshot option to build bundle Sep 6, 2018
@cbracken cbracken force-pushed the kill-snapshot_blob.bin-refs branch 2 times, most recently from 6ea6faf to 270a769 Compare September 7, 2018 01:11
The --snapshot argument was only necessary in Dart 1. The --depfile
argument was only used in Dart 2 mode to pass to the kernel compiler,
but was inconsistent with the 'build aot' command, where the depfile was
always set to build/kernel_compile.d.

This patch updates 'build bundle' to emit the depfile to a location
consistent with the 'build aot' command; since it's not intended to be
user-configurable and flutter.gradle hardcodes the location to
build/kernel_compile.d either way, this patch also eliminates the
ability to configure the filename altogether.
@cbracken cbracken changed the title Eliminate --snapshot option to build bundle Eliminate snapshot/depfile options to build bundle Sep 7, 2018
@cbracken
Copy link
Member Author

cbracken commented Sep 7, 2018

Done.

@cbracken cbracken merged commit 43a106e into flutter:master Sep 7, 2018
@cbracken cbracken deleted the kill-snapshot_blob.bin-refs branch September 7, 2018 17:22
cbracken added a commit to cbracken/flutter that referenced this pull request Sep 7, 2018
…1507)"

This tickled a bug in KernelCompiler.compile() where the fingerprinter
doesn't include the outputFilePath in its list of dependencies. As such,
if the output .dill file is missing or corrupted, the fingerprint still
matches and re-compile is skipped, even though it shouldn't be. I'll fix
that in a followup, then look at how this triggered that issue. My
hypothesis is that that it's due to the aot kernel compile and bundle
kernel compile have separate output directories for the .dill files
(build/ vs build/aot) but the same output directory for the associated
depfiles (due to this patch).

This reverts commit 43a106e.
cbracken added a commit that referenced this pull request Sep 7, 2018
…21563)

This tickled a bug in KernelCompiler.compile() where the fingerprinter
doesn't include the outputFilePath in its list of dependencies. As such,
if the output .dill file is missing or corrupted, the fingerprint still
matches and re-compile is skipped, even though it shouldn't be. I'll fix
that in a followup, then look at how this triggered that issue. My
hypothesis is that that it's due to the aot kernel compile and bundle
kernel compile have separate output directories for the .dill files
(build/ vs build/aot) but the same output directory for the associated
depfiles (due to this patch).

This reverts commit 43a106e.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants