Bug 212623 – Change ANGLE's header postprocessing script to not rely on timestamps

• Status: RESOLVED
• Resolution: FIXED
• Component: ANGLE

[Kenneth Russell]

In discussions related to Bug 212444, krollin@ points out that the script adjust-angle-include-paths.sh added in Bug 199212 must be changed to stop using timestamps to determine whether it needs to do work. Instead it needs to do something like compute a hash of the contents of each file, and skip touching the file if the hash is unchanged since the last time the script ran.

[Radar WebKit Bug Importer]

<rdar://problem/63856997>

[Keith Rollin]

Created attachment 400795 [details]
Patch

[EWS Watchlist]

Note that there are important steps to take when updating ANGLE. See http://trac.webkit.org/wiki/UpdatingANGLE

[Keith Rollin]

To the GTK developers and CMakeLists.txt maintainers: I tried updating the CMakeLists.txt file to take into account that we no longer use an angle.timestamp file. I have no idea if I got it right, so please look at that change closely.

[David Kilzer (:ddkilzer)]

Comment on attachment 400795 [details]
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.)

[Kenneth Russell]

Comment on attachment 400795 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=400795&action=review

Looks good to me with David's comments addressed; one additional question.

>> Source/ThirdParty/ANGLE/CMakeLists.txt:155
>> +        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.

Will /tmp work portably? I suspect it won't on Windows. Can the temporary files be generated into the output directory, and deleted if they don't differ from the already-generated output files?

[Kenneth Russell]

By the way thanks Keith for picking this up!

[Keith Rollin]

(In reply to David Kilzer (:ddkilzer) from comment #5)
> Comment on attachment 400795 [details]
> > 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.)

Actually, that's the way the patch should have been, and the way it was originally. I removed the -q and piped the output to a file for debugging purposes, and forgot to revert that scaffolding. Good catch.

[Keith Rollin]

(In reply to Kenneth Russell from comment #6)
> Comment on attachment 400795 [details]
> Patch
> 
> >> Source/ThirdParty/ANGLE/CMakeLists.txt:155
> >> +        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.
> 
> Will /tmp work portably? I suspect it won't on Windows. Can the temporary
> files be generated into the output directory, and deleted if they don't
> differ from the already-generated output files?

OK, I'll take this approach. I didn't like using /tmp, but I wasn't aware of where the scratch build space was in our cmake facility (if any).

[Keith Rollin]

Created attachment 400876 [details]
Patch

[Keith Rollin]

Created attachment 400877 [details]
Patch

[ChangSeok Oh]

(In reply to Keith Rollin from comment #4)
> To the GTK developers and CMakeLists.txt maintainers: I tried updating the
> CMakeLists.txt file to take into account that we no longer use an
> angle.timestamp file. I have no idea if I got it right, so please look at
> that change closely.

Nothing broken for the gtk port with USE_ANGLE_WEBGL enabled. =)

[Keith Rollin]

Thanks! My local macOS-based tests look good, too. I'll go ahead and land this.

[EWS]

Committed r262474: <https://trac.webkit.org/changeset/262474>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 400877 [details].