Bug 182555 – [CMake] Add libsoup platform file

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Tools / Tests

[Don Olmstead]

Add a Soup.cmake file within WebCore/platform for sharing CMake information on libsoup.

[Don Olmstead]

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.

[Don Olmstead]

Created attachment 333241 [details]
Patch

Fix the include type

[EWS Watchlist]

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.

[Don Olmstead]

Created attachment 333243 [details]
Patch

[Don Olmstead]

Created attachment 333245 [details]
Patch

[Konstantin Tokarev]

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.

[Michael Catanzaro]

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.

[Konstantin Tokarev]

(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

[Konstantin Tokarev]

(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 :)

[Michael Catanzaro]

The find module is wrong!

[Konstantin Tokarev]

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

[Don Olmstead]

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.

[Konstantin Tokarev]

Enumerations of headers doubles effort needed to maintain file lists. Why do we need it at all?

[Don Olmstead]

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

[Don Olmstead]

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.

[Michael Catanzaro]

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.

[Konstantin Tokarev]

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

[Don Olmstead]

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

[Fujii Hironori]

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

[Don Olmstead]

Closing