-
Notifications
You must be signed in to change notification settings - Fork 6k
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
Changes from 4 commits
f86860c
e06c9c9
dcd1c5e
29c7edc
408e7f5
be3271b
43e202a
1c05893
1cecd26
d72221b
fd68de3
a453f7f
ab9a072
26dc17f
4e2eeac
2cf6407
79231ec
9e378b1
b8bcfcb
f3ee710
eea1bce
7917811
fb6e3ee
b7031ba
a38397f
de0fa35
2d6fdfb
c7f4b80
dfa2b37
dfd121f
a8f9e41
1a2b88d
7b6514c
bf9d430
50fcdde
412912b
2cec37c
34ce5b6
c53ff77
50cfa9d
3d105f0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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!"; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 || | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why does this one have There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
||
|
@@ -1253,19 +1247,12 @@ def parse_args(args): | |
|
||
parser.add_argument( | ||
'--enable-impeller-vulkan-playgrounds', | ||
default=False, | ||
default=True, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
|
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.
TODO: remove
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.
Done