Bug 212623

<rdar://problem/63856997>

Created attachment 400795 [details]
Patch

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

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.

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

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?

By the way thanks Keith for picking this up!

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

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

Created attachment 400876 [details]
Patch

Created attachment 400877 [details]
Patch

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

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

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

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