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...
Created attachment 434421 [details] Patch
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
Created attachment 434422 [details] Patch
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 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” 🤔
(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! :)
Created attachment 434427 [details] [fast-cq] Patch
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].
(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.
This caused some build issues, see bug 228628 and 228629
This caused some build issues, see bug 228628 and bug 228629
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 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...
Created attachment 434822 [details] Patch
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!
Committed r280590 (240212@main): <https://commits.webkit.org/240212@main>