[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]

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.)