Bug 236683 – [XCBuild] Use native build phases to copy PAL's headers

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

[Elliott Williams]

Similar to WTF in https://bugs.webkit.org/show_bug.cgi?id=235744, we need to use native build system tasks to copy PAL's headers, instead of relying on an rsync-based script.

[Elliott Williams]

Created attachment 452123 [details]
Native PAL Headers via build rules

See the ChangeLog for more details, but I figured out a way to do nested headers without adding a bunch of copy file phases, like was done for WTF. This makes the project easier for devs to understand (all headers go in the same phase), plus, PAL has more nested directories than WTF does.

The key idea is that if you know the *depth* of a nested header, you can use build setting macros to produce its relative path. For example, given:

    PRIVATE_HEADERS_FOLDER_PATH = /usr/local/include/pal
    INPUT_FILE_DIR = Source/WebCore/PAL/pal/text
    INPUT_FILE_NAME = EncodingTables.h
    
the path for <pal/text/EncodingTables.h> is:

    $(PRIVATE_HEADERS_FOLDER_PATH)/$(INPUT_FILE_DIR:dir:standardizepath:base)/$(INPUT_FILE_NAME)

For deeper headers like <pal/crypto/tasn1/Utilities.h>, we give the last *two* directory components:

    $(PRIVATE_HEADERS_FOLDER_PATH)/$(INPUT_FILE_DIR:dir:standardizepath:dir:standardizepath:base)/$(INPUT_FILE_DIR:dir:standardizepath:base)/$(INPUT_FILE_NAME)

and so on.

These paths form the outputs of separate build rules which match each depth of nested header we need.

[Radar WebKit Bug Importer]

<rdar://problem/89001407>

[Elliott Williams]

Created attachment 452143 [details]
Native PAL Headers via build rules v2

For compatibility, this patch keeps the "Copy PAL Headers" rsync script and and runs it when using the legacy build system.

[Alexey Proskuryakov]

Comment on attachment 452143 [details]
Native PAL Headers via build rules v2

Supporting two different code paths until we completely drop the legacy build system support seems potentially troublesome. I did not think through this patch enough to have a feel of how likely it is to break the build in just one build system when making changes.

[Elliott Williams]

(In reply to Alexey Proskuryakov from comment #4)
> Comment on attachment 452143 [details]
> Native PAL Headers via build rules v2
> 
> Supporting two different code paths until we completely drop the legacy
> build system support seems potentially troublesome. I did not think through
> this patch enough to have a feel of how likely it is to break the build in
> just one build system when making changes.

Agreed, I don't like it either. This codepath will be tested in production builds which use XCBuild, and by the builds.webkit.org bot we are setting up in https://bugs.webkit.org/show_bug.cgi?id=236726.

An alternative approach I tried was to have the legacy build system copy to a flattened directory (/usr/local/include/pal_flattened), and then have a script go through and "nest" those headers back to their proper hierarchy. It's still quite a bit different from the XCBuild code path, but does ensure that all the headers PAL is using are part of Headers phase. I'll put that patch up too and we can decide which is best.

[Elliott Williams]

Created attachment 452262 [details]
Use a legacy-only script phase to share more of the codepath

[Elliott Williams]

Created attachment 452285 [details]
Native PAL Headers via build rules + a legacy-only script phase, v2

Per offline discussion, I think we should move forward with an approach that deletes the rsync script entirely, and uses a legacy-only script phase to un-flatten the headers. This has the benefit of enforcing that, on the legacy build system, all headers must be part of the target's Headers phase.

[Alexey Proskuryakov]

Comment on attachment 452285 [details]
Native PAL Headers via build rules + a legacy-only script phase, v2

View in context: https://bugs.webkit.org/attachment.cgi?id=452285&action=review

> Source/WebCore/PAL/Configurations/PAL.xcconfig:51
> +PRIVATE_HEADERS_FOLDER_PATH_legacy = /usr/local/include/pal_flattened;

This is a temporary directory, correct? I feel like there must be a more appropriate place for intermediate files.

[Elliott Williams]

(In reply to Alexey Proskuryakov from comment #8)
> Comment on attachment 452285 [details]
> Native PAL Headers via build rules + a legacy-only script phase, v2
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=452285&action=review
> 
> > Source/WebCore/PAL/Configurations/PAL.xcconfig:51
> > +PRIVATE_HEADERS_FOLDER_PATH_legacy = /usr/local/include/pal_flattened;
> 
> This is a temporary directory, correct? I feel like there must be a more
> appropriate place for intermediate files.

I'll try again to find a way to put this in TARGET_TEMP_DIR, but the problem seems to be that Headers phases are Special, and Xcode insists on copying them into BUILT_PRODUCTS_DIR or DSTROOT (for non-install or install builds respectively).

This is why it isn't trying to copy them into the system's `/usr/local` directory even though that's what PRIVATE_HEADERS_FOLDER_PATH specifies.

[EWS]

Tools/Scripts/svn-apply failed to apply attachment 452285 [details] to trunk.
Please resolve the conflicts and upload a new patch.

[Elliott Williams]

Created attachment 452581 [details]
Rebase and fix conflicts

[Elliott Williams]

Comment on attachment 452581 [details]
Rebase and fix conflicts

This patch resolves conflicts only, no need for additional review.

[Elliott Williams]

Comment on attachment 452581 [details]
Rebase and fix conflicts

Windows test failure looks like a flake.

[Elliott Williams]

Comment on attachment 452581 [details]
Rebase and fix conflicts

Mac test failures were fixed in r290192, back to cq+

[EWS]

Committed r290260 (?): <https://commits.webkit.org/r290260>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 452581 [details].