Bug 237112

Summary: [XCBuild] libwebrtc's headers are copied via rsync and do not emit task outputs
Product: WebKit Reporter: Elliott Williams <emw>
Component: Tools / TestsAssignee: Elliott Williams <emw>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, ap, commit-queue, eric.carlson, ews-watchlist, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 237582    
Bug Blocks:    
Attachments:
Description Flags
Copy headers natively, add headers-only absl target
none
Copy headers natively, add headers-only absl target r2
ews-feeder: commit-queue-
/usr/local/include diff for r2
none
Copy headers natively, add headers-only absl target r2
ews-feeder: commit-queue-
Copy headers natively, add headers-only absl target r3
none
Patch for relanding
none
Patch none

Description Elliott Williams 2022-02-23 14:20:22 PST
<rdar://89053575>
Comment 1 Elliott Williams 2022-02-23 16:03:45 PST
Created attachment 453043 [details]
Copy headers natively, add headers-only absl target
Comment 2 Elliott Williams 2022-02-23 16:17:31 PST
Created attachment 453046 [details]
Copy headers natively, add headers-only absl target r2
Comment 3 Elliott Williams 2022-02-23 16:20:47 PST
Created attachment 453047 [details]
/usr/local/include diff for r2

The pbxproj changes are too messy to meaningfully review, because of all the untracked files I had to add. To help reviewers, here's a diff of /usr/local/include after building libwebrtc, before and after this patch.

There are some header directories that I didn't bother adding to the project. I tried to limit this to directories that were obviously test-only or for non-Apple platforms. We'll see in testing if any of those headers *are* actually meaningful.
Comment 4 Elliott Williams 2022-02-23 21:19:08 PST
Created attachment 453069 [details]
Copy headers natively, add headers-only absl target r2

Removing a directory which got accidentally added twice.
Comment 5 Elliott Williams 2022-02-23 21:34:46 PST
Created attachment 453072 [details]
Copy headers natively, add headers-only absl target r3
Comment 6 Elliott Williams 2022-03-07 15:51:10 PST
Comment on attachment 453072 [details]
Copy headers natively, add headers-only absl target r3

Just rebased and rebuilt locally, and this is still fresh. cq+
Comment 7 EWS 2022-03-07 17:27:43 PST
Committed r290966 (248146@main): <https://commits.webkit.org/248146@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 453072 [details].
Comment 8 Alex Christensen 2022-03-07 20:51:29 PST
After this change, doing a clean build on Monterey I hit this issue using the public SDK:

/Users/alexchristensen/code/OpenSource/WebKitBuild/Debug/usr/local/include/webrtc/api/peer_connection_interface.h:78:10: fatal error: 'absl/base/attributes.h' file not found
#include "absl/base/attributes.h"
         ^~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.
Comment 9 Alex Christensen 2022-03-07 20:53:53 PST
I see that header in WebKitBuild/Debug/usr/local/include/base/attributes.h
Comment 10 Elliott Williams 2022-03-07 21:31:29 PST
(In reply to Alex Christensen from comment #8)
> After this change, doing a clean build on Monterey I hit this issue using
> the public SDK:
> 
> /Users/alexchristensen/code/OpenSource/WebKitBuild/Debug/usr/local/include/
> webrtc/api/peer_connection_interface.h:78:10: fatal error:
> 'absl/base/attributes.h' file not found
> #include "absl/base/attributes.h"
>          ^~~~~~~~~~~~~~~~~~~~~~~~
> 1 error generated.

I can reproduce this too, looks like my testing was not sufficient. Reverting for now and will investigate more tomorrow. Thanks for letting me know!
Comment 11 WebKit Commit Bot 2022-03-07 21:33:16 PST
Re-opened since this is blocked by bug 237582
Comment 12 Alex Christensen 2022-03-07 22:41:49 PST
Reverted r290966 for reason:

Broke ews and make

Committed r290974 (248152@trunk): <https://commits.webkit.org/248152@trunk>
Comment 13 Elliott Williams 2022-03-08 16:28:37 PST
Created attachment 454171 [details]
Patch for relanding

libabsl.xcconfig was not setting PUBLIC_HEADERS_FOLDER_PATH, due to a bad copy-paste. This was causing build failures. I'm guessing I forgot to clean when I tested the patch yesterday, so I didn't notice the headers in the wrong location.

This patch runs s/PREFIX/PATH/g on libabsl.xcconfig, which fixes the issue:

	diff --git a/Source/ThirdParty/libwebrtc/Configurations/libabsl.xcconfig b/Source/ThirdParty/libwebrtc/Configurations/libabsl.xcconfig
	index 896b098ddc7d..57d61fe0fd01 100644
	--- a/Source/ThirdParty/libwebrtc/Configurations/libabsl.xcconfig
	+++ b/Source/ThirdParty/libwebrtc/Configurations/libabsl.xcconfig
	@@ -24,6 +24,6 @@
	 PRODUCT_NAME = absl;
	 
	 APPLY_RULES_IN_COPY_HEADERS = $(WK_USE_NEW_BUILD_SYSTEM);
	-PUBLIC_HEADERS_FOLDER_PREFIX = $(PUBLIC_HEADERS_FOLDER_PREFIX_$(WK_WHICH_BUILD_SYSTEM));
	-PUBLIC_HEADERS_FOLDER_PREFIX_not_legacy = /usr/local/include/absl;
	-PUBLIC_HEADERS_FOLDER_PREFIX_legacy = /usr/local/include/absl_flattened;
	+PUBLIC_HEADERS_FOLDER_PATH = $(PUBLIC_HEADERS_FOLDER_PATH_$(WK_WHICH_BUILD_SYSTEM));
	+PUBLIC_HEADERS_FOLDER_PATH_not_legacy = /usr/local/include/absl;
	+PUBLIC_HEADERS_FOLDER_PATH_legacy = /usr/local/include/absl_flattened;
Comment 14 EWS 2022-03-14 11:15:15 PDT
Committed r291239 (248394@main): <https://commits.webkit.org/248394@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 454171 [details].
Comment 15 Elliott Williams 2022-03-14 13:23:24 PDT
Reverted r291239 for reason:

Some builds failing with "fatal error: 'absl/utility/utility.h' file not found"

Committed r291249 (248402@trunk): <https://commits.webkit.org/248402@trunk>
Comment 16 Elliott Williams 2022-03-15 15:08:33 PDT
My understanding of the problem is that libwebrtc has many quote-style includes of absl headers. This is fine when webrtc *itself* is building, because it puts absl on its HEADER_SEARCH_PATHS, but it's a problem for *other* project which import webrtc through a bracket-style include.

This causes WebCore to fail with an include trace like:

    In file included from Sources/WebKit/Source/WebCore/platform/graphics/cocoa/VP9UtilitiesCocoa.mm:32:
    In file included from Sources/WebKit/Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCProvider.h:44:
    In file included from SDKs/iPhoneOS16.0.Internal.sdk/usr/local/include/webrtc/api/peer_connection_interface.h:79:
    /usr/local/include/absl/types/optional.h:39:10: fatal error: 'absl/utility/utility.h' file not found

…because it doesn't have /usr/local/include on its include search path, only its *library* search path.

As I understand it, this is really just a disagreement between the two projects as to how we differentiate between "project" and "library" headers. The right move is probably to add /usr/local/include to WebCore's HEADER_SEARCH_PATHS, or to add absl headers back to the `libwebrtc` target, so that Xcode puts them into the appropriate header map.
Comment 17 Elliott Williams 2022-04-04 16:28:01 PDT
Created attachment 456646 [details]
Patch
Comment 18 Elliott Williams 2022-04-04 16:31:47 PDT
(In reply to Elliott Williams from comment #16)
> My understanding of the problem is that libwebrtc has many quote-style
> includes of absl headers. This is fine when webrtc *itself* is building,
> because it puts absl on its HEADER_SEARCH_PATHS, but it's a problem for
> *other* project which import webrtc through a bracket-style include.
> 
> This causes WebCore to fail with an include trace like:
> 
>     In file included from
> Sources/WebKit/Source/WebCore/platform/graphics/cocoa/VP9UtilitiesCocoa.mm:
> 32:
>     In file included from
> Sources/WebKit/Source/WebCore/platform/mediastream/libwebrtc/
> LibWebRTCProvider.h:44:
>     In file included from
> SDKs/iPhoneOS16.0.Internal.sdk/usr/local/include/webrtc/api/
> peer_connection_interface.h:79:
>     /usr/local/include/absl/types/optional.h:39:10: fatal error:
> 'absl/utility/utility.h' file not found
> 
> …because it doesn't have /usr/local/include on its include search path, only
> its *library* search path.

This was incorrect: By default, HEADER_SEARCH_PATHS variables (and -I flags) apply to both quote and bracket style includes. The former _also_ search the local directory and -iquote flags, which is what makes them different.

The real problem appears to be that I missed some headers which needed to be added, and didn't notice on engineering builds because of absl headers in the SDK.
Comment 19 Alexey Proskuryakov 2022-04-04 17:31:43 PDT
Comment on attachment 456646 [details]
Patch

Sounds like this is the same approach as before but with more headers, rs=me.
Comment 20 EWS 2022-04-05 12:47:13 PDT
Committed r292411 (249274@main): <https://commits.webkit.org/249274@main>

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