Summary: | [PATCH] fix webkitgtk build on macOS | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dmitry Kalinkin <veprbl+webkit> | ||||||||||||||||||||||||
Component: | Platform | Assignee: | youenn fablet <youennf> | ||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||
Severity: | Normal | CC: | benjamin, bugs-noreply, cdumez, clopez, cmarcelo, eric.carlson, ews-watchlist, keith_miller, mark.lam, mcatanzaro, msaboff, saam, tzagallo, webkit-bug-importer, youennf | ||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||
Hardware: | Mac (Intel) | ||||||||||||||||||||||||||
OS: | macOS 10.14 | ||||||||||||||||||||||||||
Attachments: |
|
Description
Dmitry Kalinkin
2021-05-15 23:15:09 PDT
Created attachment 428774 [details]
1/4
Created attachment 428775 [details]
2/4
Created attachment 428776 [details]
2/4
Created attachment 428777 [details]
3/4
Created attachment 428778 [details]
4/4
Created attachment 432216 [details]
1/4
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 Created attachment 437405 [details]
Combined patch
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. Created attachment 437417 [details]
Combined patch (revised)
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 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. [...] 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 Created attachment 437423 [details]
Combined patch (re-revised)
(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. :) 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]. Reopening to attach new patch. Created attachment 437467 [details]
Patch
(In reply to youenn fablet from comment #19) > Created attachment 437467 [details] > Patch This patch seems for another bug than this one Reopening to attach new patch. Created attachment 437497 [details]
Patch
Comment on attachment 437497 [details]
Patch
Wrong bug
|