[webkit-reviews] review denied: [Bug 227333] Build Default Metal library offline, and install to WebCore Resources : [Attachment 433557] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Jul 14 19:45:29 PDT 2021
Alexey Proskuryakov <ap at webkit.org> has denied Kyle Piddington
<kpiddington at apple.com>'s request for review:
Bug 227333: Build Default Metal library offline, and install to WebCore
Resources
https://bugs.webkit.org/show_bug.cgi?id=227333
Attachment 433557: Patch
https://bugs.webkit.org/attachment.cgi?id=433557&action=review
--- Comment #11 from Alexey Proskuryakov <ap at webkit.org> ---
Comment on attachment 433557
--> https://bugs.webkit.org/attachment.cgi?id=433557
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=433557&action=review
> Source/ThirdParty/ANGLE/.gitignore:31
>
+/src/libANGLE/renderer/metal/shaders/compiled/mtl_default_shaders_compiled.inc
This should probably go into DerivedSources? Generating code in place is
fraught with peril.
> Source/ThirdParty/ANGLE/ANGLE.xcodeproj/project.pbxproj:3693
> + "$(BUILT_PRODUCTS_DIR)/ANGLEMetalLib.metallib",
I didn't examine this patch closely enough yet, is this actually an input, not
an output?
This phase clearly depends on create_mtl_internal_shaders.py too. Does it
depend on anything else?
> Source/ThirdParty/ANGLE/ANGLE.xcodeproj/project.pbxproj:3698
> + outputPaths = (
> + );
Please add the output path too, so that actions that depend on it would be
ordered correctly.
> Source/ThirdParty/ANGLE/ANGLE.xcodeproj/project.pbxproj:4218
> + ALWAYS_SEARCH_USER_PATHS = NO;
> + CODE_SIGN_STYLE = Automatic;
> + IPHONEOS_DEPLOYMENT_TARGET = 15.0;
> + MTL_ENABLE_DEBUG_INFO = INCLUDE_SOURCE;
> + MTL_FAST_MATH = YES;
> + PRODUCT_NAME = "$(TARGET_NAME)";
Please put all build settings into xcconfigs.
> Source/ThirdParty/ANGLE/ANGLE.xcodeproj/project.pbxproj:4239
> + IPHONEOS_DEPLOYMENT_TARGET = 15.0;
In an xcconfig, it will be easier to follow other xcconfigs and to avoid issues
like this (obviously, we can't hardcode IPHONEOS_DEPLOYMENT_TARGET = 15.0).
> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/DisplayMtl.mm:1007
> + mtl::AutoObjCPtr<id<MTLLibrary>> mDefaultShaders =
mtl::CreateShaderLibraryFromBinary(getMetalDevice(), compiled_shader_binary,
> +
compiled_shader_binary_len, &err);
Can this call fail? E.g. if the system runs out of file handles?
Even though conditions like that are "beyond repair", we get hard to
investigate bugs when they are not handled explicitly.
>
Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/shaders/create_mtl_internal
_shaders.py:2
> +# Copyright 2021 The ANGLE Project Authors. All rights reserved.
I'm not certain about what the copyright header should look like, as it's
obviously not a standard WebKit one - but then this is a 3rd party library.
More information about the webkit-reviews
mailing list