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

Reland "Native assets support for Linux" #135097

Merged
merged 10 commits into from Sep 22, 2023
Merged

Conversation

dcharkes
Copy link
Contributor

@dcharkes dcharkes commented Sep 20, 2023

Reland of #134031. (Reverted in #135069.) Contains the fix for b/301051367 together with cl/567233346.

Support for FFI calls with @Native external functions through Native assets on Linux. This enables bundling native code without any build-system boilerplate code.

For more info see:

Implementation details for Linux.

Mainly follows the design of #130494.

Some differences are:

  • Linux does not support cross compiling or compiling for multiple architectures, so this has not been implemented.
  • Linux has no add2app.

The assets copying is done in the install-phase of the CMake build of a flutter app.
CMake requires the native assets folder to exist, so we create it also when the feature is disabled or there are no assets.

Tests

This PR adds new tests to cover the various use cases.

  • packages/flutter_tools/test/general.shard/linux/native_assets_test.dart
    • Unit tests the Linux-specific part of building native assets.

It also extends various existing tests:

  • packages/flutter_tools/test/integration.shard/native_assets_test.dart
    • Runs (incl hot reload/hot restart), builds, builds frameworks for Linux and flutter-tester.

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, including Features we expect every widget to implement.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

@github-actions github-actions bot added tool Affects the "flutter" command-line tool. See also t: labels. a: desktop Running on desktop labels Sep 20, 2023
The context does not contain a config in g3 when doing expression
evaluation. This workaround prevents expression evaluation from
crashing in g3.
This enables skipping building native assets with `build.dart` in g3
where native assets are built bazel instead.
@dcharkes
Copy link
Contributor Author

The first commit is the revert of the revert.

The third commit reverts the second commit (which was a workaround), and contains the fix for b/301051367 together with cl/567233346.

@dcharkes
Copy link
Contributor Author

Somehow, the 100something CI checks aren't triggering?

@XilaiZhang XilaiZhang requested review from CaseyHillers and removed request for XilaiZhang September 21, 2023 16:41
@drewroengoogle
Copy link
Contributor

drewroengoogle commented Sep 21, 2023

@dcharkes Is it intended to merge into main versus the current default master? Asking since the initial PR was being merged into master.

@drewroengoogle drewroengoogle changed the base branch from main to master September 21, 2023 18:55
@dcharkes
Copy link
Contributor Author

@dcharkes Is it intended to merge into main versus the current default master? Asking since the initial PR was being merged into master.

Aren't these aliases of each other?

@drewroengoogle
Copy link
Contributor

@dcharkes Is it intended to merge into main versus the current default master? Asking since the initial PR was being merged into master.

Aren't these aliases of each other?

I don't fully understand the specifics on how the targets are generated yet, but it seems that the logic in our backend that determines which targets to run handles PRs merging to the "default" branch (master in this case) to PRs merging to other branches.

@dcharkes
Copy link
Contributor Author

@dcharkes Is it intended to merge into main versus the current default master? Asking since the initial PR was being merged into master.

Aren't these aliases of each other?

I don't fully understand the specifics on how the targets are generated yet, but it seems that the logic in our backend that determines which targets to run handles PRs merging to the "default" branch (master in this case) to PRs merging to other branches.

Nice find, it looks like the CI is now running :)

Should I be using master instead of main in my local checkout? to branch out of?

@drewroengoogle
Copy link
Contributor

@dcharkes Is it intended to merge into main versus the current default master? Asking since the initial PR was being merged into master.

Aren't these aliases of each other?

I don't fully understand the specifics on how the targets are generated yet, but it seems that the logic in our backend that determines which targets to run handles PRs merging to the "default" branch (master in this case) to PRs merging to other branches.

Nice find, it looks like the CI is now running :)

Should I be using master instead of main in my local checkout? to branch out of?

Yeah I think that would prevent this from occurring. I believe there are some github issues created to completely get rid of master that would make it even simpler in the future.

@stuartmorgan
Copy link
Contributor

Should I be using master instead of main in my local checkout? to branch out of?

I exclusively branch from main locally; it shouldn't affect PR targets.

@dcharkes
Copy link
Contributor Author

Friendly ping to review this PR @chingjun @stuartmorgan.

(Apparently, all my PRs end up having merge conflicts with g3, and get "deleted" roll CLs. I've verified the fix for this PR in g3 cl/566871401. So it should be safe to set g3 to green. Though, other stuff might have landed in the meantime, invalidating the old CL.)

Copy link
Contributor

@chingjun chingjun left a comment

Choose a reason for hiding this comment

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

LGTM! The approach is a lot cleaner than I originally imagined :)

And for the g3fix, I think you only need to keep the first file in your g3fix CL, maybe that was the issue you're facing?

packages/flutter_tools/lib/src/build_info.dart Outdated Show resolved Hide resolved
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 22, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 22, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 22, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 22, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 22, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 25, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 26, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 26, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 26, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 26, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 26, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 27, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 27, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 27, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 27, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 27, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 28, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 28, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 28, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 28, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 28, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 28, 2023
Mairramer pushed a commit to Mairramer/flutter that referenced this pull request Oct 10, 2023
Reland of flutter#134031. (Reverted in flutter#135069.) Contains the fix for b/301051367 together with cl/567233346.

Support for FFI calls with `@Native external` functions through Native assets on Linux. This enables bundling native code without any build-system boilerplate code.

For more info see:

* flutter#129757

### Implementation details for Linux.

Mainly follows the design of flutter#130494.

Some differences are:

* Linux does not support cross compiling or compiling for multiple architectures, so this has not been implemented.
* Linux has no add2app.

The assets copying is done in the install-phase of the CMake build of a flutter app.
CMake requires the native assets folder to exist, so we create it also when the feature is disabled or there are no assets.

### Tests

This PR adds new tests to cover the various use cases.

* packages/flutter_tools/test/general.shard/linux/native_assets_test.dart
  * Unit tests the Linux-specific part of building native assets.

It also extends various existing tests:

* packages/flutter_tools/test/integration.shard/native_assets_test.dart
  * Runs (incl hot reload/hot restart), builds, builds frameworks for Linux and flutter-tester.
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: desktop Running on desktop autosubmit Merge PR when tree becomes green via auto submit App tool Affects the "flutter" command-line tool. See also t: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants