Bug 228290

Summary: [WPE][GTK] SVN_REVISION drifting away if bots don't re-run cmake
Product: WebKit Reporter: Philippe Normand <pnormand>
Component: CMakeAssignee: Philippe Normand <pnormand>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, aperez, berto, bugs-noreply, cgarcia, clopez, ews-watchlist, gustavo, gyuyoung.kim, keith_miller, mark.lam, mcatanzaro, msaboff, ryuan.choi, saam, sergio, tzagallo
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=228628
https://bugs.webkit.org/show_bug.cgi?id=228629
Attachments:
Description Flags
Patch
ews-feeder: commit-queue-
Patch
none
[fast-cq] Patch
none
Patch mcatanzaro: review+, ews-feeder: commit-queue-

Description Philippe Normand 2021-07-26 11:52:06 PDT
https://webkitgtk-release.igalia.com/built-products/release_r280292.zip has its revision in install/lib/pkgconfig/webkit2gtk-4.1.pc set to r280287 instead of r280292. I suspect it's because the bot didn't re-run cmake since r280292.

Ideally SVN_REVISION should reflect the actual revision being built. Whether cmake configure step runs or not...
Comment 1 Philippe Normand 2021-07-28 05:52:46 PDT
Created attachment 434421 [details]
Patch
Comment 2 EWS Watchlist 2021-07-28 05:54:29 PDT
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment 3 Philippe Normand 2021-07-28 05:57:04 PDT
Created attachment 434422 [details]
Patch
Comment 4 Michael Catanzaro 2021-07-28 06:05:11 PDT
Comment on attachment 434422 [details]
Patch

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

> Source/cmake/OptionsCommon.cmake:172
> +if (PORT STREQUAL "GTK" OR PORT STREQUAL "WPE")

Nit: I would move this to OptionsGTK.cmake and OptionsWPE.cmake to try to keep this file as port-agnostic as possible.
Comment 5 Adrian Perez 2021-07-28 06:35:29 PDT
Comment on attachment 434422 [details]
Patch

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

>> Source/cmake/OptionsCommon.cmake:172
>> +if (PORT STREQUAL "GTK" OR PORT STREQUAL "WPE")
> 
> Nit: I would move this to OptionsGTK.cmake and OptionsWPE.cmake to try to keep this file as port-agnostic as possible.

Drive-by comment: Maybe at some point we should split common things into a
new “CommonOptionsGLib.cmake” (or similarly named) file and include it from
both “OptionsGTK.cmake” and “OptionsWPE.cmake” 🤔
Comment 6 Philippe Normand 2021-07-28 06:50:49 PDT
(In reply to Adrian Perez from comment #5)
> Comment on attachment 434422 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=434422&action=review
> 
> >> Source/cmake/OptionsCommon.cmake:172
> >> +if (PORT STREQUAL "GTK" OR PORT STREQUAL "WPE")
> > 
> > Nit: I would move this to OptionsGTK.cmake and OptionsWPE.cmake to try to keep this file as port-agnostic as possible.
> 
> Drive-by comment: Maybe at some point we should split common things into a
> new “CommonOptionsGLib.cmake” (or similarly named) file and include it from
> both “OptionsGTK.cmake” and “OptionsWPE.cmake” 🤔

Big explicit +1 that I explicitely support! :)
Comment 7 Philippe Normand 2021-07-28 07:17:49 PDT
Created attachment 434427 [details]
[fast-cq] Patch
Comment 8 EWS 2021-07-28 08:01:58 PDT
Committed r280382 (240026@main): <https://commits.webkit.org/240026@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 434427 [details].
Comment 9 Michael Catanzaro 2021-07-28 12:19:16 PDT
(In reply to Adrian Perez from comment #5)
> Drive-by comment: Maybe at some point we should split common things into a
> new “CommonOptionsGLib.cmake” (or similarly named) file and include it from
> both “OptionsGTK.cmake” and “OptionsWPE.cmake” 🤔

Yeah it's probably time to try that.
Comment 10 Carlos Alberto Lopez Perez 2021-07-29 18:21:54 PDT Comment hidden (obsolete)
Comment 11 Carlos Alberto Lopez Perez 2021-07-29 18:22:10 PDT
This caused some build issues, see bug 228628 and bug 228629
Comment 12 Philippe Normand 2021-08-03 01:19:17 PDT
Comment on attachment 434427 [details]
[fast-cq] Patch

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

> Source/WebKit/PlatformGTK.cmake:21
> +        DEPENDS ${WebKit2Gtk_FRAMEWORK_HEADERS_DIR}/BuildRevision.h ${WebKit2_PKGCONFIG_FILE} ${WebKit2WebExtension_PKGCONFIG_FILE}

Seems like this doesn't work in case the files exist already and the revision in generated files start drifting again :(
Comment 13 Philippe Normand 2021-08-03 01:19:38 PDT
.
Comment 14 Philippe Normand 2021-08-03 01:46:01 PDT
Comment on attachment 434427 [details]
[fast-cq] Patch

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

> Source/WebKit/PlatformGTK.cmake:20
> +        python3 "${TOOLS_DIR}/glib/apply-build-revision-to-files.py" ${WebKit2Gtk_FRAMEWORK_HEADERS_DIR}/BuildRevision.h ${WebKit2_PKGCONFIG_FILE} ${WebKit2WebExtension_PKGCONFIG_FILE}

The bug's here, the python script works the first time it runs, and afterwards doesn't, because the template variable (${BUILD_REVISION}) is not present anymore...
Comment 15 Philippe Normand 2021-08-03 02:42:04 PDT
Created attachment 434822 [details]
Patch
Comment 16 Michael Catanzaro 2021-08-03 05:59:15 PDT
Comment on attachment 434822 [details]
Patch

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

> Tools/glib/apply-build-revision-to-files.py:50
> +            print("Processing {} as it is, $BUILD_REVISION might not be expanded as expected.".format(in_file))

I would return a failure status here. You don't want the build to succeed if the revision isn't really updated, do you? That would have caught this sooner!
Comment 17 Philippe Normand 2021-08-03 06:26:44 PDT
Committed r280590 (240212@main): <https://commits.webkit.org/240212@main>