The current assumption about only embedded device sdk's appending .internal is too limiting. Generalize this code to cover all possible trailing sdk specifications.
<rdar://problem/40853947>
Created attachment 342051 [details] Patch
(In reply to Jonathan Bedard from comment #2) > Created attachment 342051 [details] > Patch Change still needs to be verified on a few different configurations and some help messages need to be updated.
Comment on attachment 342051 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=342051&action=review > Tools/Scripts/webkitdirs.pm:507 > +sub availableXcodeSDKS A function with this name would need to return the SDKs. I think that you want "determineAvailableXcodeSDKs". > Tools/Scripts/webkitdirs.pm:553 > + # Prefer the more specific version of an sdk, if it exists. Should just say that we prefer the internal version. > Tools/Scripts/webkitdirs.pm:558 > + next if (index($sdk, "$xcodeSDK.") == -1); and check for exactly that.
Created attachment 342060 [details] Patch
(In reply to Alexey Proskuryakov from comment #4) > Comment on attachment 342051 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=342051&action=review > > > Tools/Scripts/webkitdirs.pm:507 > > +sub availableXcodeSDKS > > A function with this name would need to return the SDKs. I think that you > want "determineAvailableXcodeSDKs". Fixed in the updated patch. > > > Tools/Scripts/webkitdirs.pm:553 > > + # Prefer the more specific version of an sdk, if it exists. > > Should just say that we prefer the internal version. > > > Tools/Scripts/webkitdirs.pm:558 > > + next if (index($sdk, "$xcodeSDK.") == -1); > > and check for exactly that.
Not a fan of this parsing dance. Preferred the old style.
Comment on attachment 342060 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=342060&action=review > Tools/Scripts/package-root:52 > + --ios-simulator Use "iphonesimulator.*" SDK if installed, else "iphonesimulator" SDK (iOS only) Is this correct?
Comment on attachment 342060 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=342060&action=review > Tools/Scripts/webkitdirs.pm:513 > + if ($line =~ /-sdk (\D+)([\d\.]+)(\D*)\n/) { Specifically, not a fan of using regular expressions for this. I prefer hardcoidng the SDK name over this regex. Also prefer the failure mode of a hard coded name over a regex that may silently match something it shouldn’t. If we really need to use a regex then please split it out into its own function and add unit tests for this.
Created attachment 342069 [details] Patch
Created attachment 342096 [details] Patch
Comment on attachment 342096 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=342096&action=review > Tools/Scripts/webkitdirs.pm:508 > +sub parseAvailableXcodeSDKS($) I think that the last s should be lower case. > Tools/Scripts/webkitdirs.pm:528 > +sub availableXcodeSDKS Ditto.
Created attachment 342171 [details] Patch for landing
Comment on attachment 342171 [details] Patch for landing Clearing flags on attachment: 342171 Committed r232583: <https://trac.webkit.org/changeset/232583>
All reviewed patches have been landed. Closing bug.