Bug 180063 - [CMake] Add script to copy forwarding headers
Summary: [CMake] Add script to copy forwarding headers
Status: NEW
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: 180064
  Show dependency treegraph
 
Reported: 2017-11-27 18:01 PST by Don Olmstead
Modified: 2017-11-30 11:22 PST (History)
7 users (show)

See Also:


Attachments
[WIP] Patch (2.54 KB, patch)
2017-11-27 20:03 PST, Don Olmstead
no flags Details | Formatted Diff | Diff
This patch creates forwarding headers that #include the target header. (7.36 KB, patch)
2017-11-29 16:14 PST, Mark Salisbury
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Don Olmstead 2017-11-27 18:01:51 PST
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.
Comment 1 Don Olmstead 2017-11-27 20:03:12 PST
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.
Comment 2 Build Bot 2017-11-27 22:19:23 PST
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.
Comment 3 Konstantin Tokarev 2017-11-28 01:40:30 PST
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.
Comment 4 Konstantin Tokarev 2017-11-28 01:44:01 PST
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
Comment 5 Alex Christensen 2017-11-28 10:40:52 PST
(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.
Comment 6 Konstantin Tokarev 2017-11-28 10:43:33 PST
So https://bugs.webkit.org/show_bug.cgi?id=165686 should be reverted?
Comment 7 Konstantin Tokarev 2017-11-28 10:47:43 PST
Actually no, it shouldn't be reverted, just forwarding headers should be made PUBLIC and internal headers PRIVATE
Comment 8 Don Olmstead 2017-11-28 11:03:29 PST
Alex are we in the ballpark with this particular setup? Should it all be in CMake or should there be a script.
Comment 9 Alex Christensen 2017-11-28 11:04:47 PST
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.
Comment 10 Konstantin Tokarev 2017-11-28 11:13:31 PST
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
Comment 11 Don Olmstead 2017-11-29 13:16:27 PST
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.
Comment 12 Konstantin Tokarev 2017-11-29 15:45:49 PST
>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 ()
Comment 13 Konstantin Tokarev 2017-11-29 15:48:12 PST
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.
Comment 14 Konstantin Tokarev 2017-11-29 15:53:13 PST
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.
Comment 15 Mark Salisbury 2017-11-29 16:14:34 PST
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.
Comment 16 Konstantin Tokarev 2017-11-29 16:32:53 PST
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
Comment 17 Michael Catanzaro 2017-11-30 11:22:21 PST
(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.