Bug 180063 – [CMake] Add script to copy forwarding headers

• Component: Tools / Tests
• Status: NEW

[Don Olmstead]

All Apple ports are currently copying headers during the build process. AppleWin and WinCairo follow this convention as well. However GTK and WPE do not follow this. For consistency between the ports copying should be the default.

A macro/function should be created for copying headers into the correct location for the builds.

[Don Olmstead]

Created attachment 327722 [details]
[WIP] Patch

Here's a first stab at copying files within CMake to start a conversation.

This is currently just adding a custom_command for each include file which might not be ideal performance-wise.

[EWS Watchlist]

Attachment 327722 [details] did not pass style-queue:


ERROR: Source/cmake/WebKitMacros.cmake:261:  One space between command "endforeach" and its parentheses, should be "endforeach ("  [whitespace/parentheses] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Konstantin Tokarev]

Copying headers is fundamentally wrong.

If anyone needs separate builds, they should do make install, which will copy public headers, libraries to installation prefix, and can also export cmake targets for use in other projects.

[Konstantin Tokarev]

If you just want to kill out xcopy, I'm fine with that, but please remove xcopy call first so we don't have 4th way to make forwarding headers in the tree.

However, I liked a way from https://bugs.webkit.org/show_bug.cgi?id=179814 - script can be easily customized to do copies for AppleWin a don't do for other ports

[Alex Christensen]

(In reply to Konstantin Tokarev from comment #3)
> Copying headers is fundamentally wrong.
> 
> If anyone needs separate builds, they should do make install, which will
> copy public headers, libraries to installation prefix, and can also export
> cmake targets for use in other projects.

Apple has a hard requirement to build WTF with only the WTF directory, build JavaScriptCore with only the JavaScriptCore directory, etc.  Copying headers is necessary and not including directly other Source subdirectories is absolutely necessary.  This will not change.

[Konstantin Tokarev]

So https://bugs.webkit.org/show_bug.cgi?id=165686 should be reverted?

[Konstantin Tokarev]

Actually no, it shouldn't be reverted, just forwarding headers should be made PUBLIC and internal headers PRIVATE

[Don Olmstead]

Alex are we in the ballpark with this particular setup? Should it all be in CMake or should there be a script.

[Alex Christensen]

Comment on attachment 327722 [details]
[WIP] Patch

I think it needs to not be in CMake.  CMake is only run when a CMakeLists.txt file is touched like when we add or remove a file.  We need the copying to be done whenever a header is touched.

[Konstantin Tokarev]

It needs to be a custom target or custom command, preferredly separate command for each header which has that header as input and forwarding header as output. In this case, only changed headers will be processed in the incremental build. Implementation language is less important, though me personally would prefer real script instead of cmake -E

[Don Olmstead]

So I'm of the opinion that we should try and match what XCode is up to as closely as possible.

It appears there are two paths here. With WTF and PAL there's an rsync style step while with the other projects the copying appears to be brought in through Xcode through a section PBXHeadersBuildPhase.

I'm thinking of doing a python script and have it behave similarly to the generate-bindings script. So there would be a tmp file that contains the list of headers. This file would get read and then we could use filecmp to get a list of files that have changed. Then copy those all.

[Konstantin Tokarev]

>I'm thinking of doing a python script and have it behave similarly to the generate-bindings script. So there would be a tmp file that contains the list of headers. This file would get read and then we could use filecmp to get a list of files that have changed. Then copy those all.

This solution is overcomplicated. What should be done is just a cycle iterating over all header files to be copied, that does add_custom_command() for each header with output being file in ForwardingHeaders. 

Like

# Collect *.h files to copy
foreach (directory ${JavaScriptCore_FORWARDING_HEADERS_DIRECTORIES})
    file(GLOB _headers "${JAVASCRIPTCORE_DIR}/${_directory}/*.h")
    list(APPEND JavaScriptCore_FORWARDED_HEADERS ${_headers})
    unset(_headers)
endforeach ()

# Create targets
foreach (header ${JavaScriptCore_FORWARDED_HEADERS})
    get_filename_component(_header_filename ${header} NAME)
    set(_fwd_header "${DERIVED_SOURCES_DIR}/ForwardingHeaders/JavaScriptCore/${_header_filename}")
    add_cusom_command(OUTPUT ${_fwd_header}
        DEPENDS ${header}
        COMMAND ${CMAKE_COMMAND} -E copy_if_different ${header} ${_fwd_header}
        VERBATIM
    )
endforeach ()

[Konstantin Tokarev]

You still can use a helper script instead of copy_if_different, but it can be useful for customization of process, e.g. it could create shallow forwarding headers for ports which don't need copying headers, or create symlinks instead of copies when OS allows that.

[Konstantin Tokarev]

Note that binding generation is more complicated out of necessity: idl files depend on each other and nobody implemented proper dependency extraction for them. If that was done it would be also possible to process them as separate makefile targets, increasing overall parallelism and avoiding a lot of excessive work.

[Mark Salisbury]

Created attachment 327920 [details]
This patch creates forwarding headers that #include the target header.

If it'd be acceptable to have forwarding headers point to the real headers, instead of copying the files, consider this patch.

[Konstantin Tokarev]

I think that making forward headers that point to original ones is not only acceptable, but even preferred solution for ports which are always built as a single project and so don't have a requirement to copy headers. However, there is also opinion that it's better to standardize on one kludgy way of doing things than use different approaches in different ports.

However now I think that whatever paths we take, it's really neccessary to create separate makefile rules for each header, so only modified files are processed in incremental builds. This means that globs should be resolved on cmake side, and add_custom_command called for each header, possibly invoking script inside

[Michael Catanzaro]

(In reply to Konstantin Tokarev from comment #16)
> I think that making forward headers that point to original ones is not only
> acceptable, but even preferred solution for ports which are always built as
> a single project and so don't have a requirement to copy headers. However,
> there is also opinion that it's better to standardize on one kludgy way of
> doing things than use different approaches in different ports.

Yeah, agreed on both counts. I don't have a strong opinion about this, but Apple has a hard requirement to copy, and other ports don't really care either way, so copying would certainly reduce the difference between ports.

However, if we go the copying route, then I suggest tightening up our include directories. Don has suggested on IRC that if forwarding headers are copied, only the forwarding header directories would need to be PUBLIC include directories; the rest can be PRIVATE. That would probably fix Mark's build problem, or at least be a step in the right direction.

> However now I think that whatever paths we take, it's really neccessary to
> create separate makefile rules for each header, so only modified files are
> processed in incremental builds. This means that globs should be resolved on
> cmake side, and add_custom_command called for each header, possibly invoking
> script inside

Makes sense.