Bug 173937 - Update configure-xcode-for-ios-development for iOS 11
Summary: Update configure-xcode-for-ios-development for iOS 11
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: Jonathan Bedard
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-06-28 14:04 PDT by Jonathan Bedard
Modified: 2017-08-24 16:18 PDT (History)
6 users (show)

See Also:


Attachments
Patch (2.78 KB, patch)
2017-08-24 11:51 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (2.85 KB, patch)
2017-08-24 14:11 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (3.44 KB, patch)
2017-08-24 14:42 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch for landing (4.02 KB, patch)
2017-08-24 15:37 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Bedard 2017-06-28 14:04:12 PDT
Some headers have been moved, renamed or removed in iOS 11.  Update configure-xcode-for-ios-development to handle the iOS 11 SDK.
Comment 1 Radar WebKit Bug Importer 2017-07-04 04:32:00 PDT
<rdar://problem/33122072>
Comment 2 Jonathan Bedard 2017-08-24 11:51:00 PDT
Created attachment 319000 [details]
Patch
Comment 3 Daniel Bates 2017-08-24 12:52:06 PDT
Comment on attachment 319000 [details]
Patch

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

> Tools/Scripts/configure-xcode-for-ios-development:91
> +    my $macSDKDirectory = sdkDirectory("macosx");

How does this work if a person has more than one Mac SDK installed?

> Tools/Scripts/configure-xcode-for-ios-development:96
> +        my $macSDKPah = File::Spec->canonpath(File::Spec->catfile($macSDKDirectory, $header));

macSDKPah => macSDKPath
(Please fix throughout this patch.)

> Tools/Scripts/configure-xcode-for-ios-development:107
> +            system("/usr/bin/ditto", $macSDKPah, $iphonesimulatorSDKPath);

Eww, duplicate code. We should factor out the code to call ditto, die and print success to a shared function. I don't think the combined success message in line 111 is worth duplicating code.
Comment 4 Jonathan Bedard 2017-08-24 14:08:01 PDT
Comment on attachment 319000 [details]
Patch

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

>> Tools/Scripts/configure-xcode-for-ios-development:91
>> +    my $macSDKDirectory = sdkDirectory("macosx");
> 
> How does this work if a person has more than one Mac SDK installed?

It will use whichever SDK is returned from `xcrun --sdk macosx --show-sdk-path`.  Which seems like the correct approach.  This is the same way we generally look for the current SDK in WebKit tools.
Comment 5 Jonathan Bedard 2017-08-24 14:11:35 PDT
Created attachment 319016 [details]
Patch
Comment 6 Daniel Bates 2017-08-24 14:18:26 PDT
Comment on attachment 319000 [details]
Patch

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

>> Tools/Scripts/configure-xcode-for-ios-development:107
>> +            system("/usr/bin/ditto", $macSDKPah, $iphonesimulatorSDKPath);
> 
> Eww, duplicate code. We should factor out the code to call ditto, die and print success to a shared function. I don't think the combined success message in line 111 is worth duplicating code.

Alternatively, we could making this function abstract: move @missingHeaders out of this function, have this function take a source SDK and destination SDK and call this function copyMissingHeadersFromSDKToSDK(). Then define a function called copyMissingHeadersToIPhoneOSSDKIfNeeded() and define a function called copyMissingHeadersToIOSSimulatorSDKIfNeeded() that turns around and calls copyMissingHeadersFromSDKToSDK("iphonesimulator", "iphoneos") and copyMissingHeadersFromSDKToSDK("macosx", "iphonesimulator"), respectively. Then this script would call copyMissingHeadersToIOSSimulatorSDKIfNeeded() first and then call copyMissingHeadersToIPhoneOSSDKIfNeeded(). The benefit of this approach is that we avoid duplication, unnecessary branches, and the names of the functions clearly describe their purpose.
Comment 7 Daniel Bates 2017-08-24 14:21:47 PDT
Comment on attachment 319016 [details]
Patch

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

> Tools/Scripts/configure-xcode-for-ios-development:115
> +            print "Successfully copied $macSDKPath to $iphonesimulatorSDKPath and $iphoneosSDKPath.\n";

You seem to have disregarded the last sentence of my remark in comment #3 without explanation.I don't think the combined success message here is worth much. Regardless, I suggest we take the approach I outlined in comment #4.
Comment 8 Jonathan Bedard 2017-08-24 14:31:32 PDT
Comment on attachment 319016 [details]
Patch

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

>> Tools/Scripts/configure-xcode-for-ios-development:115
>> +            print "Successfully copied $macSDKPath to $iphonesimulatorSDKPath and $iphoneosSDKPath.\n";
> 
> You seem to have disregarded the last sentence of my remark in comment #3 without explanation.I don't think the combined success message here is worth much. Regardless, I suggest we take the approach I outlined in comment #4.

I isolated the duplicated code and placed it in a function.  I would be open to moving the print statements to the function as well, but I don't feel like the print statements are really duplicated code.

As for the approach you suggest in comment #4, I think you may be suggesting some functions we don't need, but I like the general idea.  I'll update this bug shortly.
Comment 9 Jonathan Bedard 2017-08-24 14:42:49 PDT
Created attachment 319018 [details]
Patch
Comment 10 Daniel Bates 2017-08-24 15:22:35 PDT
Comment on attachment 319018 [details]
Patch

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

> Tools/ChangeLog:9
> +        Xcode 9 has removed some headers required by WebKit builds for testing.

If it is straightforward to do so it would be great to list the headers that were removed and what SDK they were removed from. We should also explain what we are doing to fix this issue: we are copying the missing headers from the active macOS SDK.

> Tools/Scripts/configure-xcode-for-ios-development:71
> +copyMissingHeadersFromSDKToSDKIfNeeded("macosx", "iphonesimulator");

Out of curiosity what headers would actually be copied (i.e. what headers are missing from the iphonesimulator 11 SDK)?
Comment 11 Jonathan Bedard 2017-08-24 15:29:25 PDT
(In reply to Daniel Bates from comment #10)
> Comment on attachment 319018 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=319018&action=review
> 
> > Tools/ChangeLog:9
> > +        Xcode 9 has removed some headers required by WebKit builds for testing.
> 
> If it is straightforward to do so it would be great to list the headers that
> were removed and what SDK they were removed from. We should also explain
> what we are doing to fix this issue: we are copying the missing headers from
> the active macOS SDK.
> 
> > Tools/Scripts/configure-xcode-for-ios-development:71
> > +copyMissingHeadersFromSDKToSDKIfNeeded("macosx", "iphonesimulator");
> 
> Out of curiosity what headers would actually be copied (i.e. what headers
> are missing from the iphonesimulator 11 SDK)?

I'll include this information in the changelog as well, the following headers were removed from the iOS 11 Simulator SDK:

crt_externs.h
mach_types.defs
machine_types.def
std_types.def
objc-class.h
objc-runtime.h
Protocol.h
history.h
readline.h
Comment 12 Jonathan Bedard 2017-08-24 15:37:48 PDT
Created attachment 319028 [details]
Patch for landing
Comment 13 WebKit Commit Bot 2017-08-24 16:18:19 PDT
Comment on attachment 319028 [details]
Patch for landing

Clearing flags on attachment: 319028

Committed r221168: <http://trac.webkit.org/changeset/221168>
Comment 14 WebKit Commit Bot 2017-08-24 16:18:20 PDT
All reviewed patches have been landed.  Closing bug.