Bug 228290 – [WPE][GTK] SVN_REVISION drifting away if bots don't re-run cmake

• Component: CMake
• Status: RESOLVED FIXED

[Philippe Normand]

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...

[Philippe Normand]

Created attachment 434421 [details]
Patch

[EWS Watchlist]

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]

Created attachment 434422 [details]
Patch

[Michael Catanzaro]

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 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]

(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]

Created attachment 434427 [details]
[fast-cq] Patch

[EWS]

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]

(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]

This caused some build issues, see bug 228628 and 228629

[Carlos Alberto Lopez Perez]

This caused some build issues, see bug 228628 and bug 228629

[Philippe Normand]

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]

.

[Philippe Normand]

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]

Created attachment 434822 [details]
Patch

[Michael Catanzaro]

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]

Committed r280590 (240212@main): <https://commits.webkit.org/240212@main>