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 Mac Android Debug Engine shard #27980

Merged
merged 1 commit into from
Aug 10, 2021
Merged

Conversation

cbracken
Copy link
Member

At one point, this shard built the Android Vulkan binaries, but during
the migration to ci.yaml-based recipes, the gn and build steps for that
build were accidentally removed. As such, this shard no longer builds
anything.

This was detected when a buildroot roll triggered a clobber build which
caused the verify_exported.dart script to fail due to a missing 'out'
directory, which caused several of us to wonder why we were:

  1. Checking that we strip release binaries on a debug bot, and
  2. Why we're checking that we strip release binaries before we actually
    build anything at all.

It appears that we used to build android vulkan binaries, such as in
steps 14-16 of Build 11130:

We migrated to ci.yaml-based infra in this commit:

A no-op rebuild was triggered in commmit:

Which triggered the following buikld, Build 11131:

That build does not contain the gn or build steps for the Android Vulkan
binaries.

HOWEVER... we have no users actually using Android Vulkan binaries, so
rather than update the build to fix this, we delete the shard altogether
and save some CPU.

This will be followed by a patch that removes the --enable-vulkan flag
from gn and throughout our build.

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.

At one point, this shard built the Android Vulkan binaries, but during
the migration to ci.yaml-based recipes, the gn and build steps for that
build were accidentally removed. As such, this shard no longer builds
anything.

This was detected when a buildroot roll triggered a clobber build which
caused the verify_exported.dart script to fail due to a missing 'out'
directory, which caused several of us to wonder why we were:
1. Checking that we strip release binaries on a debug bot, and
2. Why we're checking that we strip release binaries before we actually
   build anything at all.

It appears that we used to build android vulkan binaries, such as in
steps 14-16 of Build 11130:
* https://ci.chromium.org/ui/p/flutter/builders/prod/Mac%20Android%20Debug%20Engine/11130/overview

We migrated to ci.yaml-based infra in this commit:
* https://flutter-review.googlesource.com/c/infra/+/15660

A no-op rebuild was triggered in commmit:
* flutter#27599

Which triggered the following buikld, Build 11131:
* https://ci.chromium.org/ui/p/flutter/builders/prod/Mac%20Android%20Debug%20Engine/11131/overview

That build does not contain the gn or build steps for the Android Vulkan
binaries.

HOWEVER... we have no users actually using Android Vulkan binaries, so
rather than update the build to fix this, we delete the shard altogether
and save some CPU.

This will be followed by a patch that removes the --enable-vulkan flag
from gn and throughout our build.
@cbracken cbracken merged commit c2f2291 into flutter:master Aug 10, 2021
@cbracken cbracken deleted the made branch August 10, 2021 01:40
@CaseyHillers
Copy link
Contributor

Thanks for investigating and saving CPU in the process :)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 11, 2021
cbracken added a commit to cbracken/flutter_engine that referenced this pull request Aug 20, 2021
This eliminates Vulkan support for Android. As of flutter#27980
(c2f2291) we are no longer running Android-Vulkan builds. This
eliminates the --enable-vulkan flag from tools/gn and any code that is
only reachable when that flag is enabled.

Note that after this patch, `shell_enable_vulkan` is always false,
however the //flutter/shell/gpu:gpu_surface_vulkan target and source
files remain since they are still used when `test_enable_vulkan` is
true, which is the case when `is_fuchsia` is true. Note that these files
are *not* built as part of a regular fuchsia build (see the
`shell_gpu_configuration` in //shell/platform/fuchsia/flutter/BUILD.gn),
but may be enabled once the Fuchsia embedder is migrated to the Embedder
API.
cbracken added a commit to cbracken/flutter_engine that referenced this pull request Aug 20, 2021
This eliminates Vulkan support for Android. As of flutter#27980
(c2f2291) we are no longer running Android-Vulkan builds. This
eliminates the --enable-vulkan flag from tools/gn and any code that is
only reachable when that flag is enabled.

Note that after this patch, `shell_enable_vulkan` is always false,
however the //flutter/shell/gpu:gpu_surface_vulkan target and source
files remain since they are still used when `test_enable_vulkan` is
true, which is the case when `is_fuchsia` is true. Note that these files
are *not* built as part of a regular fuchsia build (see the
`shell_gpu_configuration` in //shell/platform/fuchsia/flutter/BUILD.gn),
but may be enabled once the Fuchsia embedder is migrated to the Embedder
API.
cbracken added a commit to cbracken/flutter_engine that referenced this pull request Aug 21, 2021
This eliminates Vulkan support for Android. As of flutter#27980
(c2f2291) we are no longer running Android-Vulkan builds. This
eliminates the --enable-vulkan flag from tools/gn and any code that is
only reachable when that flag is enabled.

Note that after this patch, `shell_enable_vulkan` is always false,
however the //flutter/shell/gpu:gpu_surface_vulkan target and source
files remain since they are still used when `test_enable_vulkan` is
true, which is the case when `is_fuchsia` is true. Note that these files
are *not* built as part of a regular fuchsia build (see the
`shell_gpu_configuration` in //shell/platform/fuchsia/flutter/BUILD.gn),
but may be enabled once the Fuchsia embedder is migrated to the Embedder
API.

Also updates TODOs to dworsham, who is the committer who'll be
transitioning the Fuchsia embedder to the embedding API.
cbracken added a commit that referenced this pull request Aug 23, 2021
This eliminates Vulkan support for Android. As of #27980
(c2f2291) we are no longer running Android-Vulkan builds. This
eliminates the --enable-vulkan flag from tools/gn and any code that is
only reachable when that flag is enabled.

Note that after this patch, `shell_enable_vulkan` is always false,
however the //flutter/shell/gpu:gpu_surface_vulkan target and source
files remain since they are still used when `test_enable_vulkan` is
true, which is the case when `is_fuchsia` is true. Note that these files
are *not* built as part of a regular fuchsia build (see the
`shell_gpu_configuration` in //shell/platform/fuchsia/flutter/BUILD.gn),
but may be enabled once the Fuchsia embedder is migrated to the Embedder
API.

Also updates TODOs to dworsham, who is the committer who'll be
transitioning the Fuchsia embedder to the embedding API.
filmil pushed a commit to filmil/engine that referenced this pull request Apr 21, 2022
At one point, this shard built the Android Vulkan binaries, but during
the migration to ci.yaml-based recipes, the gn and build steps for that
build were accidentally removed. As such, this shard no longer builds
anything.

This was detected when a buildroot roll triggered a clobber build which
caused the verify_exported.dart script to fail due to a missing 'out'
directory, which caused several of us to wonder why we were:
1. Checking that we strip release binaries on a debug bot, and
2. Why we're checking that we strip release binaries before we actually
   build anything at all.

It appears that we used to build android vulkan binaries, such as in
steps 14-16 of Build 11130:
* https://ci.chromium.org/ui/p/flutter/builders/prod/Mac%20Android%20Debug%20Engine/11130/overview

We migrated to ci.yaml-based infra in this commit:
* https://flutter-review.googlesource.com/c/infra/+/15660

A no-op rebuild was triggered in commmit:
* flutter#27599

Which triggered the following buikld, Build 11131:
* https://ci.chromium.org/ui/p/flutter/builders/prod/Mac%20Android%20Debug%20Engine/11131/overview

That build does not contain the gn or build steps for the Android Vulkan
binaries.

HOWEVER... we have no users actually using Android Vulkan binaries, so
rather than update the build to fix this, we delete the shard altogether
and save some CPU.

This will be followed by a patch that removes the --enable-vulkan flag
from gn and throughout our build.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants