Bug 235751 – [XCBuild] Add headers-only dependencies to projects in Tools/

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Elliott Williams]

[XCBuild] Add product dependencies for headers-only dependencies in Tools/

[Elliott Williams]

Created attachment 450199 [details]
Patch

[Elliott Williams]

This change adds dependency information for targets which have a dependency on an upstream product that is not already expressed. For example: a static library which imports headers from WebKit.framework but does not link it needs to have a product dependency declared, so that it doesn't build too early.

It's the same logic as was landed For Source/ projects in https://commits.webkit.org/246026@main.

[Alexey Proskuryakov]

Comment on attachment 450199 [details]
Patch

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

> Source/ThirdParty/gtest/xcode/Config/StaticLibraryTarget.xcconfig:12
> +// Copy headers into /usr/local/include/gtest
> +PUBLIC_HEADERS_FOLDER_PATH = $(inherited)/gtest
> +PRIVATE_HEADERS_FOLDER_PATH = $(inherited)/gtest

Searching for _HEADERS_FOLDER_PATH in existing xcconfigs, it looks like we tend to do it slightly differently, without $inherited. We also always have a prefix - which maybe doesn't matter because we don't build gtest in production builds, but then I don't understand why these are the correct variables to use. If they don't affect anything in Xcode, and are only used by the project to set script phase dstPath, would it be more elegant to define our own variables?

> Tools/DumpRenderTree/mac/Configurations/DumpRenderTreeLibrary.xcconfig:33
> +EXCLUDED_SOURCE_FILE_NAMES = $(EXCLUDED_SOURCE_FILE_NAMES_$(WK_COCOA_TOUCH)) $(EXCLUDED_PRODUCT_DEPENDENCY_NAMES_$(WK_WHICH_BUILD_SYSTEM));

We don't need this in other xcconfigs in this directory?

Can this use $(inherited)?

> Tools/TestWebKitAPI/Configurations/TestWTFLibrary.xcconfig:30
> +EXCLUDED_SOURCE_FILE_NAMES = $(EXCLUDED_PRODUCT_DEPENDENCY_NAMES_$(WK_WHICH_BUILD_SYSTEM));

This overwrites EXCLUDED_SOURCE_FILE_NAMES from Base.xconfig AFAICT.

[Elliott Williams]

Created attachment 450253 [details]
Patch

[Elliott Williams]

(In reply to Alexey Proskuryakov from comment #3)
> Comment on attachment 450199 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=450199&action=review
> 
> > Source/ThirdParty/gtest/xcode/Config/StaticLibraryTarget.xcconfig:12
> > +// Copy headers into /usr/local/include/gtest
> > +PUBLIC_HEADERS_FOLDER_PATH = $(inherited)/gtest
> > +PRIVATE_HEADERS_FOLDER_PATH = $(inherited)/gtest
> 
> Searching for _HEADERS_FOLDER_PATH in existing xcconfigs, it looks like we
> tend to do it slightly differently, without $inherited. We also always have
> a prefix - which maybe doesn't matter because we don't build gtest in
> production builds, but then I don't understand why these are the correct
> variables to use. If they don't affect anything in Xcode, and are only used
> by the project to set script phase dstPath, would it be more elegant to
> define our own variables?

These variables control the "Headers" directory that Xcode's build phases can copy into [1]. For static libraries, this defaults to /usr/local/include. Looking at our other uses, I think it's clearer to spell out the full path. Fixed in the latest patch.

> > Tools/DumpRenderTree/mac/Configurations/DumpRenderTreeLibrary.xcconfig:33
> > +EXCLUDED_SOURCE_FILE_NAMES = $(EXCLUDED_SOURCE_FILE_NAMES_$(WK_COCOA_TOUCH)) $(EXCLUDED_PRODUCT_DEPENDENCY_NAMES_$(WK_WHICH_BUILD_SYSTEM));
> 
> We don't need this in other xcconfigs in this directory?
> 
> Can this use $(inherited)?

Looks like this is redundantly defined, fixed in the latest patch.

> 
> > Tools/TestWebKitAPI/Configurations/TestWTFLibrary.xcconfig:30
> > +EXCLUDED_SOURCE_FILE_NAMES = $(EXCLUDED_PRODUCT_DEPENDENCY_NAMES_$(WK_WHICH_BUILD_SYSTEM));
> 
> This overwrites EXCLUDED_SOURCE_FILE_NAMES from Base.xconfig AFAICT.

Good catch, it needed an $(inherited). Fixed.


[1]: https://help.apple.com/xcode/mac/11.4/#/itcaec37c2a6?sub=devdbb795497

[Elliott Williams]

> Layout test failure:
> 
> - accessibility/mac/spinbutton-valuedescription.html

Pretty sure this is a flake. This test has been consistently failing on this builder: https://results.webkit.org/?suite=layout-tests&test=accessibility/mac/spinbutton-valuedescription.html

[Alexey Proskuryakov]

Comment on attachment 450253 [details]
Patch

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

>These variables control the "Headers" directory that Xcode's build phases can copy into [1]. 

I still don't quite understand. The project now specifies the path in copy phases with this:

dstPath = "$(PRIVATE_HEADERS_FOLDER_PATH)/internal";

Is there some other effect that's implicit?

> Source/ThirdParty/gtest/xcode/Config/StaticLibraryTarget.xcconfig:11
> +PUBLIC_HEADERS_FOLDER_PATH = /usr/local/include/gtest
> +PRIVATE_HEADERS_FOLDER_PATH = /usr/local/include/gtest

Perhaps we don't care for gtest, or this is necessary to work around other peculiarities in this project, but normally the former is /usr/include, and the latter is /usr/local/include.

> Source/ThirdParty/gtest/xcode/gtest.xcodeproj/project.pbxproj:209
> +			dstPath = "$(PRIVATE_HEADERS_FOLDER_PATH)/internal";

It smells strange that "internal" headers need to be copied into any kind of "SDK" for other projects to use.

> Source/ThirdParty/gtest/xcode/gtest.xcodeproj/project.pbxproj:515
> +		DD0EDF6F2798A073005152AD /* Headers */ = {

Perhaps this is where the implicit effect that I'm asking about is, this phase doesn't have dstPath.

> Tools/DumpRenderTree/mac/Configurations/Base.xcconfig:125
> +EXCLUDED_SOURCE_FILE_NAMES = $(EXCLUDED_PRODUCT_DEPENDENCY_NAMES_$(WK_WHICH_BUILD_SYSTEM));
> +EXCLUDED_PRODUCT_DEPENDENCY_NAMES_legacy = WebKit.framework;

This is inherited in DumpRenderTreeLibrary.xcconfig, but I'm not sure about other xcconfigs:

DumpRenderTree.xcconfig, DumpRenderTreeApp.xcconfig and TestNetscapePlugIn.xcconfig have EXCLUDED_SOURCE_FILE_NAMES too, although conditional by SDK. Do they overwrite this? Ot does it not matter for some reason?

[Elliott Williams]

Sorry for the delay here. I took a fresh look at this, and split the gtest headers fix out to a separate bug: https://bugs.webkit.org/show_bug.cgi?id=236045

Hopefully the description there (and the greatly simplified patch) helps answer some of your questions.

(In reply to Alexey Proskuryakov from comment #7)
> > Tools/DumpRenderTree/mac/Configurations/Base.xcconfig:125
> > +EXCLUDED_SOURCE_FILE_NAMES = $(EXCLUDED_PRODUCT_DEPENDENCY_NAMES_$(WK_WHICH_BUILD_SYSTEM));
> > +EXCLUDED_PRODUCT_DEPENDENCY_NAMES_legacy = WebKit.framework;
> 
> This is inherited in DumpRenderTreeLibrary.xcconfig, but I'm not sure about
> other xcconfigs:
> 
> DumpRenderTree.xcconfig, DumpRenderTreeApp.xcconfig and
> TestNetscapePlugIn.xcconfig have EXCLUDED_SOURCE_FILE_NAMES too, although
> conditional by SDK. Do they overwrite this? Ot does it not matter for some
> reason?

This setting was poorly organized on my part. It should only apply to DumpRenderTreeLibrary, which is why the override was not causing build failures. I'll move it to DumpRenderTreeLibrary.xcconfig in the next patch revision.

[Elliott Williams]

Created attachment 450716 [details]
Add dependencies to DumpRenderTree and TestWebKitAPI

[Elliott Williams]

rdar://86871388

[Elliott Williams]

Comment on attachment 450716 [details]
Add dependencies to DumpRenderTree and TestWebKitAPI

I believe the windows test failures are flakes. cq+

[EWS]

Committed r289093 (246792@main): <https://commits.webkit.org/246792@main>

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