RESOLVED FIXED 228290
[WPE][GTK] SVN_REVISION drifting away if bots don't re-run cmake
https://bugs.webkit.org/show_bug.cgi?id=228290
Summary [WPE][GTK] SVN_REVISION drifting away if bots don't re-run cmake
Philippe Normand
Reported 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...
Attachments
Patch (15.71 KB, patch)
2021-07-28 05:52 PDT, Philippe Normand
ews-feeder: commit-queue-
Patch (17.39 KB, patch)
2021-07-28 05:57 PDT, Philippe Normand
no flags
[fast-cq] Patch (17.65 KB, patch)
2021-07-28 07:17 PDT, Philippe Normand
no flags
Patch (2.90 KB, patch)
2021-08-03 02:42 PDT, Philippe Normand
mcatanzaro: review+
ews-feeder: commit-queue-
Philippe Normand
Comment 1 2021-07-28 05:52:46 PDT
EWS Watchlist
Comment 2 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
Philippe Normand
Comment 3 2021-07-28 05:57:04 PDT
Michael Catanzaro
Comment 4 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.
Adrian Perez
Comment 5 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” 🤔
Philippe Normand
Comment 6 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! :)
Philippe Normand
Comment 7 2021-07-28 07:17:49 PDT
Created attachment 434427 [details] [fast-cq] Patch
EWS
Comment 8 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].
Michael Catanzaro
Comment 9 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.
Carlos Alberto Lopez Perez
Comment 10 2021-07-29 18:21:54 PDT Comment hidden (obsolete)
Carlos Alberto Lopez Perez
Comment 11 2021-07-29 18:22:10 PDT
This caused some build issues, see bug 228628 and bug 228629
Philippe Normand
Comment 12 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 :(
Philippe Normand
Comment 13 2021-08-03 01:19:38 PDT
.
Philippe Normand
Comment 14 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...
Philippe Normand
Comment 15 2021-08-03 02:42:04 PDT
Michael Catanzaro
Comment 16 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!
Philippe Normand
Comment 17 2021-08-03 06:26:44 PDT
Note You need to log in before you can comment on or make changes to this bug.