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
[Impeller] moved validation layers on by default logic to gni scripts #45682
[Impeller] moved validation layers on by default logic to gni scripts #45682
Conversation
27ad47c
to
c6982ae
Compare
According to this build step the validation layers are now building, matching the behavior of the impeller vulkan backend: https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8770207159619809201/+/u/build_android_debug_arm64_flutter_flutter_shell_platform_android:abi_jars/stdout |
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.
LGTM
So the CI bots aren't passing enable_impeller_vulkan while building for Android? I guess this is working though 🤔
gn_args['enable_vulkan_validation_layers'] = True | ||
gn_args['impeller_enable_vulkan_validation_layers'] = True |
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.
Why do we have both enable_vulkan_validation_layers
and impeller_enable_vulkan_validation_layers
?
Or rather, what's the difference between impeller_enable_vulkan_validation_layers
and (enable_impeller_vulkan && enable_vulkan_validation_layers)
(Not sure that enable_impeller_vulkan
is the right spelling, but there's probably something we already have that could go there?)
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.
enable_vulkan_validation_layers
is for the fuchsia code, impeller_enable_vulkan_validation_layers
is for impeller. There is also a angle_enable_vulkan_validation_layers
. enable_vulkan_validation_layers
predates impeller. I tried to share the variables. That's how it was previously, but we need a different default value. That's why we need a different variable here.
Yep, that appears to be the case. I looked at the post build bots and didn't see one that turned on enable_impeller_vulkan, the bot is just relying on the default value from the gni file. I'm not 100% certain these are the source of the artifacts that are used by flutter/flutter, but it is likely that fixing this fixes it for whoever is making the artifacts if it isn't those bots. |
…134569) flutter/engine@d4698c6...496ef6a 2023-09-12 skia-flutter-autoroll@skia.org Roll Skia from 7cafb622ee7f to a4f8f5177c8b (1 revision) (flutter/engine#45719) 2023-09-12 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from vGleXqh2SRUNJM7JN... to MWWrSP9mSVlGIOaDo... (flutter/engine#45718) 2023-09-12 skia-flutter-autoroll@skia.org Roll Skia from 438ec87ea2be to 7cafb622ee7f (1 revision) (flutter/engine#45716) 2023-09-12 matanlurey@users.noreply.github.com [Impeller] Make `CreateMockVulkanContext()` thread-safe (flutter/engine#45687) 2023-09-12 30870216+gaaclarke@users.noreply.github.com [Impeller] moved validation layers on by default logic to gni scripts (flutter/engine#45682) 2023-09-12 skia-flutter-autoroll@skia.org Roll Skia from f3f0cab7efd0 to 438ec87ea2be (1 revision) (flutter/engine#45714) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from vGleXqh2SRUN to MWWrSP9mSVlG If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC bdero@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
fixes flutter/flutter#134460
Test will happen in the flutter repo as a result of flutter/flutter#134175
Pre-launch Checklist
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.