Bug 193792 - Add .xcfilelists to Run Script build phases
Summary: Add .xcfilelists to Run Script build phases
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keith Rollin
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-01-24 15:49 PST by Keith Rollin
Modified: 2019-01-29 12:05 PST (History)
11 users (show)

See Also:


Attachments
Patch (14.44 KB, patch)
2019-01-25 15:58 PST, Keith Rollin
no flags Details | Formatted Diff | Diff
Patch (15.55 KB, patch)
2019-01-28 16:01 PST, Keith Rollin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keith Rollin 2019-01-24 15:49:33 PST
As part of supporting XCBuild, update the necessary Run Script build phases in their Xcode projects to refer to their associated .xcfilelist files.

Make special accommodations for incorporation .xcfilelists from WebKitAdditions.
Comment 1 Keith Rollin 2019-01-24 15:49:44 PST
<rdar://problem/47201785>
Comment 2 Keith Rollin 2019-01-25 15:58:29 PST
Created attachment 360181 [details]
Patch
Comment 3 Alex Christensen 2019-01-25 16:02:08 PST
Comment on attachment 360181 [details]
Patch

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

> Source/WebKit/Configurations/DebugRelease.xcconfig:64
> +WK_WEBKIT_ADDITIONS_PATH = ../../../Internal/WebKit/WebKitAdditions/Additions/WebKit

What's this used for?  It looks iffy.

> Tools/DumpRenderTree/DumpRenderTree.xcodeproj/project.pbxproj:6
> -	objectVersion = 46;
> +	objectVersion = 51;

What happens if we don't make this change?
Comment 4 Keith Rollin 2019-01-25 16:54:36 PST
(In reply to Alex Christensen from comment #3)
> Comment on attachment 360181 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=360181&action=review
> 
> > Source/WebKit/Configurations/DebugRelease.xcconfig:64
> > +WK_WEBKIT_ADDITIONS_PATH = ../../../Internal/WebKit/WebKitAdditions/Additions/WebKit
> 
> What's this used for?  It looks iffy.

This is for the case where we're in the Xcode IDE and doing a full workspace build. When that happens, XCBuild creates a build plan for all projects simultaneously. In order to do that, it needs to find all the .xcfilelist files. The WebKitAdditions step has not had a chance to run yet, so we can't find the WebKitAdditions .xcfilelist files in whatever temp directory it ultimately gets copied to. In order to find the .xcfilelists, we need to tell XCBuild the source location for them.


> > Tools/DumpRenderTree/DumpRenderTree.xcodeproj/project.pbxproj:6
> > -	objectVersion = 46;
> > +	objectVersion = 51;
> 
> What happens if we don't make this change?

I haven't tried. But I believe that one thing that will happen is that they will get updated again the next time someone touches the project.

The bumping of this version number essentially scraps compatibility with the IDE from Xcode 9 (building from the command line still works). I've checked with a number of stakeholders, and they're mostly OK with that.
Comment 5 Alex Christensen 2019-01-28 10:48:00 PST
(In reply to Keith Rollin from comment #4)
> (In reply to Alex Christensen from comment #3)
> > Comment on attachment 360181 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=360181&action=review
> > 
> > > Source/WebKit/Configurations/DebugRelease.xcconfig:64
> > > +WK_WEBKIT_ADDITIONS_PATH = ../../../Internal/WebKit/WebKitAdditions/Additions/WebKit
> > 
> > What's this used for?  It looks iffy.
> 
> This is for the case where we're in the Xcode IDE and doing a full workspace
> build. When that happens, XCBuild creates a build plan for all projects
> simultaneously. In order to do that, it needs to find all the .xcfilelist
> files. The WebKitAdditions step has not had a chance to run yet, so we can't
> find the WebKitAdditions .xcfilelist files in whatever temp directory it
> ultimately gets copied to. In order to find the .xcfilelists, we need to
> tell XCBuild the source location for them.

I don't think we should make the open source repository that dependent on internal structure of the internal repository.  I think we should stop at ../../../Internal or maybe ../../../Internal/WebKit and put logic in Internal to handle it.
Comment 6 Keith Rollin 2019-01-28 11:53:56 PST
(In reply to Alex Christensen from comment #5)
> I don't think we should make the open source repository that dependent on
> internal structure of the internal repository.  I think we should stop at
> ../../../Internal or maybe ../../../Internal/WebKit and put logic in
> Internal to handle it.

Could you elaborate? Does this mean storing the .xcfilelists directly in Internal or Internal/WebKit? That's the only way I can imagine implementing your suggestion. But we can't "put logic in Internal to handle it" because any logic that we implement will not execute before those files are needed. When you hit Build in Xcode, it's going to look for those .xcfilelists before executing any actual build steps, because it needs the information in those files in order to determine what build steps to execute.

Also, putting those files in Internal or Internal/WebKit would put those files outside of the project that likely would hold the logic for handling them. Does Xcode support a structure like that? Do our company build tools? I would hope so, but I wouldn't be surprised if they didn't.
Comment 7 Keith Rollin 2019-01-28 15:28:09 PST
Another patch coming soon. I realized that the current patch adds the .xcfilelists for the Derived Sources phases, but I forgot to add those for the Generate Unified Sources phases. The new patch will include that change, but doesn't respond to Alex's comments until we figure out what the solution will look like.
Comment 8 Keith Rollin 2019-01-28 16:01:35 PST
Created attachment 360394 [details]
Patch
Comment 9 Alex Christensen 2019-01-29 11:54:37 PST
Comment on attachment 360394 [details]
Patch

I don't like this, but then again I don't like the whole direction the build system is going in.  If anyone complains, I'll send them to you.  R=me for this patch.
Comment 10 WebKit Commit Bot 2019-01-29 12:05:54 PST
Comment on attachment 360394 [details]
Patch

Clearing flags on attachment: 360394

Committed r240668: <https://trac.webkit.org/changeset/240668>
Comment 11 WebKit Commit Bot 2019-01-29 12:05:56 PST
All reviewed patches have been landed.  Closing bug.