Summary: | [WPE][GTK] SVN_REVISION drifting away if bots don't re-run cmake | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Philippe Normand <pnormand> | ||||||||||
Component: | CMake | Assignee: | 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
Philippe Normand
2021-07-26 11:52:06 PDT
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> |