Bug 225850 – [PATCH] fix webkitgtk build on macOS

• Component: Platform
• Status: RESOLVED FIXED

[Dmitry Kalinkin]

There are several issues preventing GTK backend from successfully compiling on macOS.

[Dmitry Kalinkin]

Created attachment 428774 [details]
1/4

[Dmitry Kalinkin]

Created attachment 428775 [details]
2/4

[Dmitry Kalinkin]

Created attachment 428776 [details]
2/4

[Dmitry Kalinkin]

Created attachment 428777 [details]
3/4

[Dmitry Kalinkin]

Created attachment 428778 [details]
4/4

[Dmitry Kalinkin]

Created attachment 432216 [details]
1/4

[Carlos Alberto Lopez Perez]

Thanks for the patches.

Can you please rebase them on top of current trunk/main and merge all of them in one single patch? Thanks

[Dmitry Kalinkin]

Created attachment 437405 [details]
Combined patch

[Michael Catanzaro]

Comment on attachment 437405 [details]
Combined patch

View in context: https://bugs.webkit.org/attachment.cgi?id=437405&action=review

It's probably been a long time since somebody got WebKitGTK to build on macOS. Impressive!

Your changes all look fine to me. I'm just going to leave nits about the line spacing.

> Source/WebKit/ChangeLog:8
> +        implementations to allow compiling webkitgtk on macOS.
> +        https://bugs.webkit.org/show_bug.cgi?id=225850

There should be one blank line in the ChangeLog above the URL.

> Source/WebKit/Platform/IPC/unix/ConnectionUnix.cpp:50
> +#endif
>  // Although it's available on Darwin, SOCK_SEQPACKET seems to work differently

Leave a blank line here, please.

[Dmitry Kalinkin]

Created attachment 437417 [details]
Combined patch (revised)

[Dmitry Kalinkin]

Carlos, Michael, thank you for your kind feedback. This patch has been tested in nixpkgs [1] and in a homebrew tap [2]. This fix was mostly done for fun, but there appears to be an interest in maintaining the effort from the community of a Nyxt browser users [3][4]. There are additional changes needed to fix webkitgtk on main, but I don't have those fully ready yet. In case I do, I'd rather submit a separate patch. Let me know what's left to do for this one and thanks again for taking time to review this.

[1] https://github.com/NixOS/nixpkgs/pull/123298
[2] https://github.com/incidentist/homebrew-nyxt
[3] https://github.com/NixOS/nixpkgs/pull/126101
[4] https://github.com/atlas-engineer/nyxt/issues/577

[Carlos Alberto Lopez Perez]

Comment on attachment 437417 [details]
Combined patch (revised)

View in context: https://bugs.webkit.org/attachment.cgi?id=437417&action=review

Patch looks fine, but the change-logs have a formatting that don't follow the standard we usually use. See my comments

> Source/WTF/ChangeLog:6
> +        Define HAVE_AUDIT_TOKEN only for Cocoa backend to fix webkitgtk on
> +        macOS.
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=225850

Please, don't wrap the subject lines and don't leave a blank line between the subject and the bug number.
Also, please use the same subject on all the changelog files.

For example use this:

Fix WebKitGTK build on MacOS
https://bugs.webkit.org/show_bug.cgi?id=225850

Reviewed by NOBODY (OOPS!).

Define HAVE_AUDIT_TOKEN only for Cocoa backend to fix webkitgtk on macOS.

[...]

> Source/WebKit/ChangeLog:11
> +        Don't require availability of MSG_NOSIGNAL to fix webkitgtk build on
> +        macOS.
> +
> +        Streamline conditionals for consistency between definitions and
> +        implementations to allow compiling webkitgtk on macOS.
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=225850
> +
> +        Reviewed by NOBODY (OOPS!).

This changelog should kept the same title than the other changelog, and the explanation or the details comes after the "reviewed by" line. For example:


Fix WebKitGTK build on MacOS
https://bugs.webkit.org/show_bug.cgi?id=225850

Reviewed by NOBODY (OOPS!).

Don't require availability of MSG_NOSIGNAL to fix webkitgtk build on macOS.
Streamline conditionals for consistency between definitions and implementations to allow compiling webkitgtk on macOS. 

[...]

[Carlos Alberto Lopez Perez]

Comment on attachment 437405 [details]
Combined patch

View in context: https://bugs.webkit.org/attachment.cgi?id=437405&action=review

>> Source/WebKit/ChangeLog:8
>> +        https://bugs.webkit.org/show_bug.cgi?id=225850
> 
> There should be one blank line in the ChangeLog above the URL.

I think this is incorrect. Above the url it should be the subject and not a blank line.
The formatting of ChangeLog files is:

"""
Subject (same subject for all the changelog entries, and without wrapping long lines)
https://bugs.webkit.org/show_bug.cgi?id=${bugnumber}

Reviewed by NOBODY (OOPS!).

Explanation of what this patch does.

* List of files modified
"""

Check how other changelog entries are written in case of doubt.

The tool Tools/Scripts/prepare-ChangeLog can help to automate this.
You can pass to it the bug number, for example: Tools/Scripts/prepare-ChangeLog -b 225850

[Dmitry Kalinkin]

Created attachment 437423 [details]
Combined patch (re-revised)

[Michael Catanzaro]

(In reply to Carlos Alberto Lopez Perez from comment #13)
> Comment on attachment 437405 [details]
> Combined patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=437405&action=review
> 
> >> Source/WebKit/ChangeLog:8
> >> +        https://bugs.webkit.org/show_bug.cgi?id=225850
> > 
> > There should be one blank line in the ChangeLog above the URL.
> 
> I think this is incorrect. Above the url it should be the subject and not a
> blank line.

Oops, yes. :)

[EWS]

Committed r282061 (241361@main): <https://commits.webkit.org/241361@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 437423 [details].

[Radar WebKit Bug Importer]

<rdar://problem/82797509>

[youenn fablet]

Reopening to attach new patch.

[youenn fablet]

Created attachment 437467 [details]
Patch

[Carlos Alberto Lopez Perez]

(In reply to youenn fablet from comment #19)
> Created attachment 437467 [details]
> Patch

This patch seems for another bug than this one

[youenn fablet]

Reopening to attach new patch.

[youenn fablet]

Created attachment 437497 [details]
Patch

[youenn fablet]

Comment on attachment 437497 [details]
Patch

Wrong bug