Skip to content

Commit

Permalink
Move !is_android to build_engine_artifacts declaration (flutter#54006)
Browse files Browse the repository at this point in the history
The `//flutter/build/archives:artifacts` target is used to build a zip
archive (artifacts.zip) of host tools such as flutter_tester, the Dart
kernel compiler, the impellerc shader compiler, and other tooling that
is bundled in debug-mode host builds.

This moves the `!is_android` to the definition of
`build_engine_artifacts`. This is required because of the way that we
produce 32-bit arm gen_snapshot for Android on Windows hosts, which
relies on the regular x64 host toolchain due to us having no 32-bit arm
toolchain for Windows. As such, `current_toolchain == host_toolchain` on
that platform.

```
build_engine_artifacts =
    flutter_build_engine_artifacts &&
    (current_toolchain == host_toolchain ||
     (is_linux && !is_chromeos && current_cpu != "arm") || is_mac || is_win)
```

On iOS builds, we don't have this issue since `current_toolchain` will
be one of:
* `//build/toolchain/mac:ios_clang_arm`
* `//build/toolchain/mac:ios_clang_arm_sim`
* `//build/toolchain/mac:ios_clang_x64_sim`

Whereas `host_toolchain` will be one of:
* `//build/toolchain/mac:clang_arm64`
* `//build/toolchain/mac:clang_x64`

This patch also adds documentation to clarify the purpose of this target
and where related artifacts are produced so that future readers don't
need to do a deep dive into our build plumbing to figure this out.

While the target itself is primarily intended for producing host
binaries, one target binary (gen_snapshot) is bundled into the same
archive bundle as the host tools. This should be refactored such that
just like iOS and Android, they are bundled into their own
target-platform-specific archive, and the tool code accordingly updated
to pull these down into the appropriate cache directory.

As a side-note, on macOS we do rely on this archive target for the host
tools, but the bundled gen_snapshot is unused -- instead, one produced
by the //flutter/sky/tools/create_macos_gen_snapshots.py script used.
This should be fixe in a followup patch.

Related: flutter/flutter#38935

Identified while trying to resolve:
Issue: flutter/flutter#101138
Issue: flutter/flutter#69157

## Pre-launch Checklist

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

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

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#overview
[Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene
[test-exempt]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#tests
[Flutter Style Guide]:
https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo
[C++, Objective-C, Java style guides]:
https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
[testing the engine]:
https://github.com/flutter/flutter/wiki/Testing-the-engine
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes
[Discord]: https://github.com/flutter/flutter/wiki/Chat
  • Loading branch information
cbracken authored Jul 19, 2024
1 parent eb7c880 commit 5810b3f
Show file tree
Hide file tree
Showing 2 changed files with 30 additions and 3 deletions.
23 changes: 22 additions & 1 deletion build/archives/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,28 @@ generated_file("artifacts_entitlement_config") {
}
}

if (build_engine_artifacts && !is_android) {
# This target is used to build a zip archive (artifacts.zip) of host tools such
# as flutter_tester, the Dart kernel compiler, the impellerc shader compiler,
# and other tooling that is bundled in debug-mode host builds.
#
# For historical reasons, this bundle also includes gen_snapshot (which is a
# target-platform-specific binary) for desktop platforms, though the
# un-suffixed gen_snapshot is unused on macOS (gen_snapshot_arm64 and
# gen_snapshot_x64 are used in its place).
#
# Target-specific iOS artifacts.zip are produced in:
# //flutter/sky/tools/create_full_ios_framework.py
#
# Target-specific Android archives are produced in:
# //flutter/shell/platform/android:gen_snapshot
#
# macOS-specific gen_snapshot_arm64 and gen_snapshot_x64 are produced in:
# //flutter/sky/tools/create_macos_gen_snapshots.py
#
# TODO: https://github.com/flutter/flutter/issues/38935
# Target-specific binaries such as gen_snapshot should be separated into a
# separate archive target from host binaries such as flutter_tester.
if (build_engine_artifacts) {
zip_bundle("artifacts") {
deps = [
"$dart_src/runtime/bin:gen_snapshot",
Expand Down
10 changes: 8 additions & 2 deletions common/config.gni
Original file line number Diff line number Diff line change
Expand Up @@ -162,9 +162,15 @@ if (flutter_prebuilt_dart_sdk) {

# Flutter SDK artifacts should only be built when either doing host builds, or
# for cross-compiled desktop targets.
#
# !is_android is necessary because of the way that Android 32-bit arm
# gen_snapshot is built on Windows using the host toolchain, since we have no
# 32-bit arm toolchain for Windows. See comments in //flutter/tools/gn for
# details.
#
# TODO: We can't build the engine artifacts for arm (32-bit) right now;
# see https://github.com/flutter/flutter/issues/74322
# see https://github.com/flutter/flutter/issues/74322.
build_engine_artifacts =
flutter_build_engine_artifacts &&
flutter_build_engine_artifacts && !is_android &&
(current_toolchain == host_toolchain ||
(is_linux && !is_chromeos && current_cpu != "arm") || is_mac || is_win)

0 comments on commit 5810b3f

Please sign in to comment.