-
Notifications
You must be signed in to change notification settings - Fork 24
Use aot-snapshot in pkg-standalone-dev #183
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
Conversation
|
@nex3 A few things I would like to discuss:
|
I'm fine bumping the minimum Dart version. Pub has good SDK version solving, so this won't break users who are locked to older SDKs, and the fact that this package is targeted specifically at people who are generally distributing to non-Dart users makes the marginal value of supporting older SDKs lower.
I'm assuming the code complexity increase here would be from having separate code paths for cross-compiling Linux but not other operating systems? I agree that this is low-priority. If we get a request for it, maybe we can bump it up, but as it stands I'm happy to keep using QEMU and suggesting others do the same for the time being.
I have mixed feelings about this. I think generally it's more important to provide something that makes sense to the end users who are running the applications, and who have no idea what |
The cross compile works as the Dart SDK hard codes the download path from storage.googleapis.com, meaning on dart-musl/dart-android it will not be able to download the correct cross-compiler. However, these files can be download manually:
The "path" part looks like:
These files are downloaded to:
The biggest problem here is the dart-gnu and dart-musl literally have the conflicting filenames, because the Dart SDK itself cannot tell the difference. We can to download and overwrite those files as needed, but it introduces possible race conditions for concurrent compilation and may cause unintended side effect on which one becomes the default cross compile without using cli_pkg. I can think of a few solutions but non of them are perfect:
Looks good to me. |
I bumped it to 3.8.0 and updated the dev builds to always be aot-snapshot. As for the snapshot naming, I decided to not change that for now in this PR. I realized that when cross-compiling gets supported at a later time, the This PR is ready for review. In my local test, switching to AOT cuts the |
Yeah, that's not worth the hassle right now. Can you file an issue against the Dart SDK requesting a CLI flag to pass in a specific path for the SDK cache? |
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.
Since we're no longer supporting Dart 3.7.x, let's also simplify CliPlatform.useNative to remove the IA32 check (since there are no longer any Dart SDKs that support IA32).
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.
The ia32 Abi classes are not removed from SDK yet, because dart is technically still support the kernel/JIT runtime on ia32. As far as I understand, it's for Google/Flutter's internal use and no ia32 sdk/binary is published for external users. Therefore we still need to keep the logic to filter out ia32.
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.
But we shouldn't need the && !arch.isIA32 check in CliPlatform.useNative, since there aren't actually any IA32 SDKs and so we won't have any IA32 binaries.
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.
Thanks. Updated.
Co-authored-by: Natalie Weizenbaum <nweiz@google.com>
I did a bit more testing and found that if we only support |
As dart-lang/sdk#39973 has been addressed and can be closed, this PR uses
dart compile aot-snapshot --enable-assertsfor faster dev snapshot experience.A few clarifications:
--enable-assertsduring compilation does NOT change the behavior of generated snapshot,dart --enable-assertsmust be used at the runtime.dart compile kernel --helpdoes not show it as a valid option, it's "valid" only because it's an global option to the dart command for running.dartsource file.--enable-assertsduring compilation does change the behavior of generated snapshot,dartaotruntime --enable-assertsdoes NOT affect assertion behavior which has been determined during compilation.-Dvariables are determined during compilation, specify variables during runtime does not change any behavior.Minimum required SDK version is bumped up to 3.8.0.