Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

fuchsia: Enable AOT builds of Dart test packages #29048

Merged

Conversation

richkadel
Copy link
Contributor

@richkadel richkadel commented Oct 6, 2021

The integration test works in debug/jit mode only, and building in
profile and release modes was not working, breaking some builders.

The workaround was to disable the test.

This PR will enable release/aot mode so it will build, and unblock both
the debug-mode version of the test (tested by CI) and release mode
builds of the repo.

Due to the request to unblock, this PR does not yet build packages in
profile mode that will run in Fuchsia, but it should at least build
without errors. Ongoing work is in progress to also support running in
release mode.

List which issues are fixed by this PR. You must list at least one issue.

Partial fix to fxbug.dev/86055

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
    writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@google-cla google-cla bot added the cla: yes label Oct 6, 2021
@richkadel richkadel force-pushed the enable-aot-of-flutter-embedder-test-2 branch from 905fbb2 to f0743c8 Compare October 18, 2021 22:20
@richkadel richkadel changed the title [WIP] Attempting to enable AOT builds of Fuchsia Dart test packages Enable AOT builds of Fuchsia Dart test packages Oct 18, 2021
@richkadel richkadel marked this pull request as ready for review October 19, 2021 14:07
@richkadel
Copy link
Contributor Author

@arbreng - Sorry for the delay. I was hoping that I could easily see the problem running the test with release mode and fix it for this PR, but it will take more digging, and I don't want to delay re-enabling the test.

CI succeeded on this PR last night on all except Android AOT, and that error appeared to be an infra failure. Re-running checks now.

@arbreng arbreng changed the title Enable AOT builds of Fuchsia Dart test packages fuchsia: Enable AOT builds of Dart test packages Oct 19, 2021
@@ -65,7 +65,7 @@ third_party/gn/
.packages
.pub-cache/
.pub/
build/
*/**/build/
Copy link
Contributor

Choose a reason for hiding this comment

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

Can split this into it's own PR? It doesn't seem like it belongs in this change

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 put it into its own commit instead, as discussed offline

# Product AOT
# dart_default_build_cfg = dart_release_build_cfg
# } else if (is_debug) {
# TODO(fxbug.dev/64153) renable dart runner aot builds
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a "TODO(fxbug.dev/86941) enable dart_runner_integration_tests" here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -157,10 +157,6 @@ template("dart_kernel") {
_kernel_deps += invoker.deps
}

# TODO(richkadel): The manifest is currently used by flutter_dart_component, to populate the file
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should keep this comment

We are still having to do the non-standard code in "fuchsia_component" below, and using the "resources_in_json_file"

These comments help me track what forked-logic needs to be reconciled

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 re-added the TODO and updated it to match the current approach.

"")

resources_in_json_files = [ rebase_path(_convert_kernel_manifest_file) ]
if (!build_cfg.is_aot) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you should add a TODO(richkadel) here

This is non-standard, and I'll need to reconcile it with the build rules in the GN SDK / fuchsia somehow.

The TODO makes sure I don't forget about this later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed offline, I added a comment next to the resources_in_json_files assignment.

The integration test works in debug/jit mode only, and building in
`profile` and `release` modes was not working, breaking some builders.

The workaround was to disable the test.

This PR will enable release/aot mode so it will build, and unblock both
the debug-mode version of the test (tested by CI) and release mode
builds of the repo.

Due to the request to unblock, this PR does not yet build packages in
`profile` mode that will run in Fuchsia, but it should at least build
without errors. Ongoing work is in progress to also support running in
release mode.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants