Bug 198984

Summary: Relocate some test tools in non-mac builds
Product: WebKit Reporter: Keith Rollin <krollin>
Component: Tools / TestsAssignee: Keith Rollin <krollin>
Status: RESOLVED FIXED    
Severity: Normal CC: aestes, ap, commit-queue, mitz, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Keith Rollin 2019-06-18 17:02:46 PDT
The tools DumpRenderTree, WebKitTestRunner, LayoutTestHelper, and TestNetscapePlugin get created in WebKit.framework/Versions/A/Resources on non-mac builds. This is incorrect, as those bundles are shallow bundles that don't use the Versions hierarchy. Instead, store these files directly in WebKit.framework. (Note that getting rid of just the "Versions/A" path components and putting the files in WebKit.framework/Resources doesn't work as `codesign` treats this as an invalid layout.)
Comment 1 Radar WebKit Bug Importer 2019-06-18 17:03:01 PDT
<rdar://problem/51873261>
Comment 2 Keith Rollin 2019-06-18 17:07:32 PDT
Created attachment 372408 [details]
Patch
Comment 3 Andy Estes 2019-06-18 17:30:36 PDT
Comment on attachment 372408 [details]
Patch

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

> Tools/DumpRenderTree/mac/Configurations/Base.xcconfig:116
> +WEBKIT_FRAMEWORK_RESOURCES_PATH = WebKit.framework;
> +WEBKIT_FRAMEWORK_RESOURCES_PATH[sdk=macosx*] = WebKit.framework/Versions/A/Resources;

I see some other places in WebKit that use the value of $(SHALLOW_BUNDLE) rather than checking for specific SDKs. Can we do that here too?
Comment 4 Keith Rollin 2019-06-18 17:37:05 PDT
Not really. I tried that. The value of $(SHALLOW_BUNDLE) bundle that we'd want would be that used by the WebKit.framework build. That's not available when building WebKitTestRunner, et al. And, it turns out $(SHALLOW_BUNDLE) can be set to NO for some of these projects, which are strict executables and not bundles themselves.
Comment 5 Andy Estes 2019-06-18 17:57:19 PDT
Comment on attachment 372408 [details]
Patch

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

> Tools/DumpRenderTree/DumpRenderTree.xcodeproj/project.pbxproj:-1389
> -				WEBKIT_FRAMEWORK_RESOURCES_PATH = WebKit.framework/Versions/A/Resources;

It'd be nice to also move INSTALL_PATH from the pbxproj to an .xcconfig file. WEBKIT_FRAMEWORK_RESOURCES_PATH's effect is a little mysterious otherwise (at least it was to me while reviewing).
Comment 6 Alexey Proskuryakov 2019-06-19 00:59:39 PDT
IIRC the location was chosen for Mac as one that didn't make code signing get unhappy about unexpected or missing binaries. Not sure if this is still relevant as we tend to re-sign a lot more eagerly, but anyway - how did you test this?
Comment 7 Keith Rollin 2019-06-19 11:57:49 PDT
> how did you test this?

I ran it through spade to create a temporary-install. I modified spade to allow me to specify local roots. See Safari pull request 7255. This is how I found out about codesign complaining about my trying to use a Resources directory.
Comment 8 Keith Rollin 2019-06-19 15:16:16 PDT
Created attachment 372495 [details]
Patch
Comment 9 WebKit Commit Bot 2019-06-19 17:14:20 PDT
Comment on attachment 372495 [details]
Patch

Clearing flags on attachment: 372495

Committed r246617: <https://trac.webkit.org/changeset/246617>
Comment 10 WebKit Commit Bot 2019-06-19 17:14:22 PDT
All reviewed patches have been landed.  Closing bug.