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

[Impeller] attempt to get validation errors from CI unittests. #51341

Merged
merged 41 commits into from
Mar 13, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
f86860c
testing.
jonahwilliams Mar 11, 2024
e06c9c9
make fatal
jonahwilliams Mar 11, 2024
dcd1c5e
Add validation layers to deps?
jonahwilliams Mar 11, 2024
29c7edc
more validation
jonahwilliams Mar 12, 2024
408e7f5
more cleanups
jonahwilliams Mar 12, 2024
be3271b
start with test skips.
jonahwilliams Mar 12, 2024
43e202a
++
jonahwilliams Mar 12, 2024
1c05893
++
jonahwilliams Mar 12, 2024
1cecd26
add back gles flag.
jonahwilliams Mar 12, 2024
d72221b
add more skips.
jonahwilliams Mar 12, 2024
fd68de3
++
jonahwilliams Mar 12, 2024
a453f7f
++
jonahwilliams Mar 12, 2024
ab9a072
more skips.
jonahwilliams Mar 12, 2024
26dc17f
++
jonahwilliams Mar 12, 2024
4e2eeac
++
jonahwilliams Mar 12, 2024
2cf6407
++
jonahwilliams Mar 12, 2024
79231ec
++
jonahwilliams Mar 12, 2024
9e378b1
++
jonahwilliams Mar 12, 2024
b8bcfcb
++
jonahwilliams Mar 12, 2024
f3ee710
++
jonahwilliams Mar 12, 2024
eea1bce
more fixes
jonahwilliams Mar 12, 2024
7917811
point field fixes.
jonahwilliams Mar 12, 2024
fb6e3ee
++
jonahwilliams Mar 12, 2024
b7031ba
make skips actually work.
jonahwilliams Mar 12, 2024
a38397f
fix gaussians.
jonahwilliams Mar 12, 2024
de0fa35
fix validation disable
jonahwilliams Mar 12, 2024
2d6fdfb
more skips.
jonahwilliams Mar 12, 2024
c7f4b80
++
jonahwilliams Mar 12, 2024
dfa2b37
update linux builders.
jonahwilliams Mar 12, 2024
dfd121f
add vvl config to impeller unittests for linux.
jonahwilliams Mar 12, 2024
a8f9e41
++
jonahwilliams Mar 12, 2024
1a2b88d
++
jonahwilliams Mar 12, 2024
7b6514c
cleanups.
jonahwilliams Mar 12, 2024
bf9d430
++
jonahwilliams Mar 12, 2024
50fcdde
merge dictionaries.
jonahwilliams Mar 13, 2024
412912b
++
jonahwilliams Mar 13, 2024
2cec37c
remove unused build_dir.
jonahwilliams Mar 13, 2024
34ce5b6
++
jonahwilliams Mar 13, 2024
c53ff77
remove unused gn flag.
jonahwilliams Mar 13, 2024
50cfa9d
Merge branch 'main' into turn_on_validation
jonahwilliams Mar 13, 2024
3d105f0
more adjustments.
jonahwilliams Mar 13, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions impeller/aiks/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,8 @@ template("aiks_unittests_component") {
"//flutter/impeller/typographer/backends/stb:typographer_stb_backend",
"//flutter/testing:testing_lib",
"//flutter/third_party/txt",
"//third_party/vulkan_validation_layers",
Copy link
Member Author

Choose a reason for hiding this comment

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

TODO: remove

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

"//third_party/vulkan_validation_layers:vulkan_gen_json_files",
]
if (defined(invoker.public_configs)) {
public_configs = invoker.public_configs
Expand Down
1 change: 1 addition & 0 deletions impeller/playground/playground_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ std::unique_ptr<PlaygroundImpl> PlaygroundImpl::Create(
#endif // IMPELLER_ENABLE_OPENGLES
#if IMPELLER_ENABLE_VULKAN
case PlaygroundBackend::kVulkan:
switches.enable_vulkan_validation = true;
return std::make_unique<PlaygroundImplVK>(switches);
#endif // IMPELLER_ENABLE_VULKAN
default:
Expand Down
2 changes: 1 addition & 1 deletion impeller/renderer/backend/vulkan/capabilities_vk.cc
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ CapabilitiesVK::CapabilitiesVK(bool enable_validations) {
validations_enabled_ =
enable_validations && HasLayer("VK_LAYER_KHRONOS_validation");
if (enable_validations && !validations_enabled_) {
FML_LOG(ERROR)
FML_LOG(FATAL)
Copy link
Member

Choose a reason for hiding this comment

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

I think the safer thing is to add a test that makes sure this doesn't get printed out. I'm concerned about sending users down this path on accident.

Copy link
Member Author

Choose a reason for hiding this comment

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

For now I'm keeping this fatal for testing.

So we have two approaches here:

  1. Keep the existing behavior but add checks that it is running as expected on CI.
  2. Make it fatal but do more work to make sure users don't fall down the path.

Copy link
Member

Choose a reason for hiding this comment

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

What I'm doing for impeller validations is that it is off by default (existing behavior) but I swap it on in the main() for the test runner and adding a test to make sure it it is on.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm moving this back to error, as a follow up we need to make sure accidentally turning off validations causes a test somewhere to fail - ideally once for each shard.

Copy link
Member Author

Choose a reason for hiding this comment

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

<< "Requested Impeller context creation with validations but the "
"validation layers could not be found. Expect no Vulkan validation "
"checks!";
Expand Down
4 changes: 2 additions & 2 deletions impeller/tools/impeller.gni
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,10 @@ declare_args() {

# Whether the OpenGLES backend is enabled.
impeller_enable_opengles =
(is_linux || is_win || is_android) && target_os != "fuchsia"
(is_linux || is_win || is_android || is_mac) && target_os != "fuchsia"

# Whether the Vulkan backend is enabled.
impeller_enable_vulkan = (is_linux || is_win || is_android ||
impeller_enable_vulkan = (is_linux || is_win || is_android || is_mac ||
Copy link
Member

Choose a reason for hiding this comment

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

Why does this one have enable_unittests but the opengles one doesn't?

Copy link
Member Author

Choose a reason for hiding this comment

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

Lets try it

enable_unittests) && target_os != "fuchsia"

# Whether playgrounds should run with Vulkan.
Expand Down
15 changes: 1 addition & 14 deletions tools/gn
Original file line number Diff line number Diff line change
Expand Up @@ -788,15 +788,9 @@ def to_gn_args(args):
if args.enable_impeller_trace_canvas:
gn_args['impeller_trace_canvas'] = True

if args.enable_impeller_vulkan:
gn_args['impeller_enable_vulkan'] = True

if args.enable_impeller_vulkan_playgrounds:
gn_args['impeller_enable_vulkan_playgrounds'] = True
Copy link
Member

Choose a reason for hiding this comment

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

Remove impeller_enable_vulkan_playgrounds from tools/gn now that it no longer exists in the GN scripts

Copy link
Member Author

Choose a reason for hiding this comment

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

Done


if args.enable_impeller_opengles:
gn_args['impeller_enable_opengles'] = True

if args.prebuilt_impellerc is not None:
gn_args['impeller_use_prebuilt_impellerc'] = args.prebuilt_impellerc

Expand Down Expand Up @@ -1253,19 +1247,12 @@ def parse_args(args):

parser.add_argument(
'--enable-impeller-vulkan-playgrounds',
default=False,
default=True,
Copy link
Member

Choose a reason for hiding this comment

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

The GN invocations all live in this repo, so if this flag is a no-op now, and is no longer passed in the json files, then it can be safely removed from here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

action='store_true',
help='Enable the Impeller Vulkan Playgrounds. ' +
'The Impeller Vulkan backend needs to be enabled.'
)

parser.add_argument(
'--enable-impeller-opengles',
default=False,
action='store_true',
help='Enable the Impeller OpenGL ES backend.'
)

parser.add_argument(
'--prebuilt-impellerc',
default=None,
Expand Down