WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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-
Details
Formatted Diff
Diff
Patch
(17.39 KB, patch)
2021-07-28 05:57 PDT
,
Philippe Normand
no flags
Details
Formatted Diff
Diff
[fast-cq] Patch
(17.65 KB, patch)
2021-07-28 07:17 PDT
,
Philippe Normand
no flags
Details
Formatted Diff
Diff
Patch
(2.90 KB, patch)
2021-08-03 02:42 PDT
,
Philippe Normand
mcatanzaro
: review+
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Philippe Normand
Comment 1
2021-07-28 05:52:46 PDT
Created
attachment 434421
[details]
Patch
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
Created
attachment 434422
[details]
Patch
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)
This caused some build issues, see
bug 228628
and 228629
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
Created
attachment 434822
[details]
Patch
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
Committed
r280590
(
240212@main
): <
https://commits.webkit.org/240212@main
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug