Bug 182555 - [CMake] Add libsoup platform file
Summary: [CMake] Add libsoup platform file
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Don Olmstead
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-02-06 15:52 PST by Don Olmstead
Modified: 2018-02-13 21:43 PST (History)
6 users (show)

See Also:


Attachments
Patch (3.75 KB, patch)
2018-02-06 15:56 PST, Don Olmstead
no flags Details | Formatted Diff | Diff
Patch (3.73 KB, patch)
2018-02-06 18:07 PST, Don Olmstead
no flags Details | Formatted Diff | Diff
Patch (3.74 KB, patch)
2018-02-06 18:11 PST, Don Olmstead
no flags Details | Formatted Diff | Diff
Patch (3.95 KB, patch)
2018-02-06 18:15 PST, Don Olmstead
mcatanzaro: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Don Olmstead 2018-02-06 15:52:58 PST
Add a Soup.cmake file within WebCore/platform for sharing CMake information on libsoup.
Comment 1 Don Olmstead 2018-02-06 15:56:01 PST
Created attachment 333232 [details]
Patch

So I realize that with the unified sources build this seems a bit excessive to have an include. However with https://bugs.webkit.org/show_bug.cgi?id=182512 the forwarding headers are all going to be enumerated which would increase the amount of information within this file so I'd like to land this unless there are major objections.
Comment 2 Don Olmstead 2018-02-06 18:07:22 PST
Created attachment 333241 [details]
Patch

Fix the include type
Comment 3 EWS Watchlist 2018-02-06 18:09:40 PST
Attachment 333241 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Don Olmstead 2018-02-06 18:11:55 PST
Created attachment 333243 [details]
Patch
Comment 5 Don Olmstead 2018-02-06 18:15:20 PST
Created attachment 333245 [details]
Patch
Comment 6 Konstantin Tokarev 2018-02-07 03:49:44 PST
I think this is not a right way to go. When I introduced these "platform" cmake files (for ImageDecoders, GStreamer and TextureMapper), I had following motivations:

* Share file lists
* Share conditional logic

In addition, I haven't understood cmake's imported targets well yet at that point.

For this particual case, it would be better to define imported "LibSoup" target with library and include directories in FindLibSoup.cmake, and link to "LibSoup" instead of using ${LIBSOUP_LIBRARIES} and ${LIBSOUP_INCLUDE_DIRS}. Remaining unshared part is SourcesSoup.txt, but I'm not really sure that this files need to be part of unified build, and even if they do, it's just one extra line of cmake code.
Comment 7 Michael Catanzaro 2018-02-07 05:28:47 PST
But note "libsoup" is all lowercase.

(In reply to Konstantin Tokarev from comment #6)
> For this particual case, it would be better to define imported "LibSoup"
> target with library and include directories in FindLibSoup.cmake, and link
> to "LibSoup" instead of using ${LIBSOUP_LIBRARIES} and
> ${LIBSOUP_INCLUDE_DIRS}.

Its own build target? Isn't that overkill?

> Remaining unshared part is SourcesSoup.txt, but I'm
> not really sure that this files need to be part of unified build, and even
> if they do, it's just one extra line of cmake code.

They should remain part of the unified build, but yes, one extra line of CMake code is not hard.
Comment 8 Konstantin Tokarev 2018-02-07 05:55:41 PST
(In reply to Michael Catanzaro from comment #7)
> Its own build target? Isn't that overkill?

Not a build target but imported target, and no, it's not an overkill, it's rather recommended approach for modern cmake find modules. See e.g. FindZLIB.cmake in sufficiently new cmake

> They should remain part of the unified build, but yes, one extra line of
> CMake code is not hard.

I see, thanks
Comment 9 Konstantin Tokarev 2018-02-07 05:56:59 PST
(In reply to Michael Catanzaro from comment #7)
> But note "libsoup" is all lowercase.

Huh? Find modules is named FindLibSoup, so it's LibSoup for cmake :)
Comment 10 Michael Catanzaro 2018-02-07 08:29:54 PST
The find module is wrong!
Comment 11 Konstantin Tokarev 2018-02-07 08:45:15 PST
That was (partially) kidding, as any target name can be used. However, it's common to name targets that resemble cmake module name, optionally with namespace.

Naming target as "libsoup" can be confusing (because one can use raw library names in target_link_libraries etc.), but WebKit::libsoup should be fine.

Of course we can continue using ${LIBSOUP_LIBRARIES} with changed content of variable, and drop ${LIBSOUP_INCLUDE_DIRS}, but with target names it looks somewhat cleaner
Comment 12 Don Olmstead 2018-02-07 11:04:46 PST
I think we're losing sight of why I'm proposing this change.

I'm working towards copying all WebCore headers needed by the individual ports. In this case I am enumerating ALL headers within WebCore to copy. This is over 1000 headers that are being declared.

Because GTK and WPE use Soup and there are associated headers to copy I thought it would be a good idea to create a common Soup.cmake so I could put them all in there to remove duplication and to make it easier to keep things in sync.
Comment 13 Konstantin Tokarev 2018-02-07 11:08:55 PST
Enumerations of headers doubles effort needed to maintain file lists. Why do we need it at all?
Comment 14 Don Olmstead 2018-02-07 11:40:35 PST
(In reply to Konstantin Tokarev from comment #13)
> Enumerations of headers doubles effort needed to maintain file lists. Why do
> we need it at all?

For the same reason you enumerate all the source files in CMake rather than using globs. The macro made in https://bugs.webkit.org/show_bug.cgi?id=180921 creates a target which copies the headers with each being an input and each declaring an output. My plan is to use that directly and I'm going through the trouble of enumerating all the headers because I might as well do it right.
Comment 15 Don Olmstead 2018-02-07 14:53:44 PST
Michael and Konstantin so I'm prepping a patch for https://bugs.webkit.org/show_bug.cgi?id=182512.

If this patch lands I would be adding into that in a similar manner to the modifications within Curl. So within Soup.platform there would be the following which would be shared by WPE/GTK.

list(APPEND WebCore_FORWARDING_HEADERS
    platform/network/soup/AuthenticationChallenge.h
    platform/network/soup/CertificateInfo.h
    platform/network/soup/GRefPtrSoup.h
    platform/network/soup/GUniquePtrSoup.h
    platform/network/soup/ResourceError.h
    platform/network/soup/ResourceRequest.h
    platform/network/soup/ResourceResponse.h
    platform/network/soup/SocketStreamHandleImpl.h
)

If you guys don't want this patch lets close it and I'll just duplicate those lines in PlatformGTK/WPE within Source/WebCore.

I do think we should avoid the duplication which fits Konstantin's "share file lists" requirement but I'm not a GTK/WPE developer so these aren't things I'll be touching in the future so I can just close as WONTFIX.
Comment 16 Michael Catanzaro 2018-02-07 15:53:25 PST
Comment on attachment 333245 [details]
Patch

At this point I think adding a new file is going to make it harder, rather than easier, to edit the build system. I appreciate the desire to avoid listing headers multiple times, but it seems like only a small amount of duplication.
Comment 17 Konstantin Tokarev 2018-02-07 16:05:25 PST
I'm all for sharing file list but I'm not really convinced that we need to have header listings. I think you should use generate-forwarding-headers.pl like other ports, and maybe modify it to copy headers instead of making forward headers. It doesn't use any globbing, instead it parses #include directives and detects all headers which require "forwarding".
Comment 18 Don Olmstead 2018-02-07 20:15:28 PST
(In reply to Konstantin Tokarev from comment #17)
> I'm all for sharing file list but I'm not really convinced that we need to
> have header listings. I think you should use generate-forwarding-headers.pl
> like other ports, and maybe modify it to copy headers instead of making
> forward headers. It doesn't use any globbing, instead it parses #include
> directives and detects all headers which require "forwarding".

See https://bugs.webkit.org/show_bug.cgi?id=182512#c13 for generate-forwarding-headers.pl reasoning.
Comment 19 Fujii Hironori 2018-02-07 21:01:02 PST
(In reply to Konstantin Tokarev from comment #17)
> I think you should use generate-forwarding-headers.pl like
> other ports, and maybe modify it to copy headers instead of
> making forward headers.

This idea does work as expected for the copying header approach.
Ninja has to know which files are generated by
generate-forwarding-headers.pl for incremental builds.
Comment 20 Don Olmstead 2018-02-13 21:43:45 PST
Closing