[webkit-reviews] review granted: [Bug 212623] Change ANGLE's header postprocessing script to not rely on timestamps : [Attachment 400795] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Jun 2 08:51:04 PDT 2020
David Kilzer (:ddkilzer) <ddkilzer at webkit.org> has granted Keith Rollin
<krollin at apple.com>'s request for review:
Bug 212623: Change ANGLE's header postprocessing script to not rely on
timestamps
https://bugs.webkit.org/show_bug.cgi?id=212623
Attachment 400795: Patch
https://bugs.webkit.org/attachment.cgi?id=400795&action=review
--- Comment #5 from David Kilzer (:ddkilzer) <ddkilzer at webkit.org> ---
Comment on attachment 400795
--> https://bugs.webkit.org/attachment.cgi?id=400795
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=400795&action=review
r=me if you adjust DERIVED_FILE_DIR for CMake and update the `diff` command
line.
> Source/ThirdParty/ANGLE/ChangeLog:32
> + * adjust-angle-include-paths.sh:
This script would be much more efficient as a Makefile (in the spirit of
Source/JavaScriptCore/DerivedSources.make or Source/WebCore/DerivedSources.make
or Source/WebKitLegacy/mac/MigrateHeaders.make). It could compare the
timestamps of the original source files with the derived output sources and
only update the individual files that had newer timestamps, but that's out of
scope of this change.
> Source/ThirdParty/ANGLE/CMakeLists.txt:155
> + add_custom_target(ANGLE-webgl-headers
> DEPENDS LibGLESv2EntryPointsHeaders ANGLEWebGLHeaders
> - COMMAND BUILT_PRODUCTS_DIR=${ANGLE_FRAMEWORK_HEADERS_DIR}/
> + COMMAND DERIVED_FILE_DIR=/tmp
DERIVED_FILE_DIR should probably not be at the top level of /tmp (since it's
going to dump every individual file into that directory). Maybe something
like:
DERIVED_FILE_DIR=/tmp/ANGLE-webgl-headers
Other projects like JavaScriptCore and WebCore have variables like
${JavaScriptCore_DERIVED_SOURCES_DIR} and ${WebCore_DERIVED_SOURCES_DIR} in
their CMake files. I can't figure out how/where they're set, but using an
${ANGLE_DERIVED_SOURCES_DIR} would be better than /tmp or
/tmp/ANGLE-webgl-headers if it's not too hard to set up.
> Source/ThirdParty/ANGLE/adjust-angle-include-paths.sh:44
> +' < "$i" > "$temp_file"
> + if ! diff "$temp_file" "$i" &> "$temp_file.diff"
> + then
You should use `diff -q` here since you're just discarding the output.
Also, why not just send diff output to /dev/null instead of a unique file? (If
this script isn't run on Windows, using /dev/null should be fine.)
More information about the webkit-reviews
mailing list