Support specifying explicit SDK version #28696

ojhunt · 2024-05-17T06:08:14Z

`9140fbd`

Support specifying explicit SDK version
https://bugs.webkit.org/show_bug.cgi?id=274298

Reviewed by Elliott Williams.

Strip the version number from the specified SDK as well when setting
the platform name

* Tools/Scripts/webkitdirs.pm:
(determineXcodeSDKPlatformName):

Canonical link: https://commits.webkit.org/279009@main

6163619

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	~~🧪 wpe-wk2~~	✅ 🧪 wincairo-tests
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	~~🧪 ios-wk2-wpt~~	✅ 🧪 mac-wk1	✅ 🛠 wpe-cairo
✅ 🛠 🧪 jsc	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
✅ 🛠 🧪 jsc-arm64	✅ 🛠 tv	~~🧪 mac-AS-debug-wk2~~	~~🧪 gtk-wk2~~
	~~🛠 tv-sim~~	✅ 🧪 mac-wk2-stress	~~🧪 api-gtk~~
✅ 🛠 🧪 merge	~~🛠 watch~~		✅ 🛠 jsc-armv7
	✅ 🛠 watch-sim		✅ 🧪 jsc-armv7-tests

webkit-early-warning-system · 2024-05-17T06:08:20Z

EWS run on previous version of this PR (hash d98521d)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2	✅ 🧪 wincairo-tests
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	🧪 ios-wk2-wpt	🧪 mac-wk1	✅ 🛠 wpe-cairo
🛠 🧪 jsc	🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
✅ 🛠 🧪 jsc-arm64	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	🧪 gtk-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2-stress	✅ 🧪 api-gtk
	✅ 🛠 watch		✅ 🛠 jsc-armv7
	✅ 🛠 watch-sim		✅ 🧪 jsc-armv7-tests

aproskuryakov · 2024-05-17T16:38:22Z

Tools/Scripts/webkitdirs.pm

@@ -917,6 +917,8 @@ sub availableXcodeSDKs

 sub isValidXcodeSDKPlatformName($) {
    my $name = shift;
+    # strip sdk version and variant qualifiers prior to validating platform


This comment says what the code does, but not why. I do not immediately understand why - make always works with the active Xcode anyway, so it seems like there would be no need to specify SDK version, as there's only one.

@aproskuryakov if you have multiple sdk versions installed you cannot currently use the sdk argument to specify which sdk to use, e.g. build-webkit --sdk macosx14.1 will fail because "macosx14.1" is not in the list of valid platforms.

emw-apple

I agree with @aproskuryakov, I'm not entirely sure what you're trying to accomplish. Do you have a special Xcode configuration where you have multiple SDKs for the same platform, differing only in version?

emw-apple · 2024-05-18T00:30:45Z

Tools/Scripts/webkitdirs.pm

@@ -917,6 +917,8 @@ sub availableXcodeSDKs

 sub isValidXcodeSDKPlatformName($) {
    my $name = shift;
+    # strip sdk version and variant qualifiers prior to validating platform
+    $name =~ s/(\d+\.[\d\.]+)?(\.internal)?$//;


I don't think stripping the version in the validation function makes sense, because then we'd have to make sure that the rest of webkitdirs can handle when $xcodeSDKPlatformName contains a version number. What I would recommend instead is a modification to determineXcodeSDKPlatformName:

if (checkForArgumentAndRemoveFromARGVGettingValue("--sdk", \$sdk)) { $xcodeSDK = lc $sdk; $xcodeSDKPlatformName ||= $sdk; - $xcodeSDKPlatformName =~ s/\.internal$//; + $xcodeSDKPlatformName =~ s/(\d+\.[\d\.]+)?(\.internal)?$//; die "Couldn't determine platform name from Xcode SDK" unless isValidXcodeSDKPlatformName($xcodeSDKPlatformName); return; }

that seems more correct to me assuming that's what gets passed to the validation code, I will try to verify in a bit

webkit-early-warning-system · 2024-05-18T21:42:04Z

EWS run on previous version of this PR (hash a2b2dad)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	~~🧪 wpe-wk2~~	✅ 🧪 wincairo-tests
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	✅ 🧪 api-mac	✅ 🧪 api-wpe
	~~🧪 ios-wk2-wpt~~	✅ 🧪 mac-wk1	✅ 🛠 wpe-cairo
~~🛠 🧪 jsc~~	~~🧪 api-ios~~	✅ 🧪 mac-wk2	✅ 🛠 gtk
✅ 🛠 🧪 jsc-arm64	✅ 🛠 tv	~~🧪 mac-AS-debug-wk2~~	~~🧪 gtk-wk2~~
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2-stress	✅ 🧪 api-gtk
	✅ 🛠 watch		✅ 🛠 jsc-armv7
	✅ 🛠 watch-sim		✅ 🧪 jsc-armv7-tests

ojhunt · 2024-05-18T21:44:21Z

Updated to the much more sensible suggestion of "just set the platform name correctly" approach, have verified correct behavior, but I'm wondering if this change should also include logic to verify the requested SDK actually exists, or if that should be a follow on change?

webkit-early-warning-system · 2024-05-18T22:27:00Z

EWS run on previous version of this PR (hash 4814eae)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2	✅ 🧪 wincairo-tests
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 wpe-cairo
✅ 🛠 🧪 jsc	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
✅ 🛠 🧪 jsc-arm64	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2-stress	✅ 🧪 api-gtk
	✅ 🛠 watch		✅ 🛠 jsc-armv7
	✅ 🛠 watch-sim		✅ 🧪 jsc-armv7-tests

ojhunt · 2024-05-18T22:29:59Z

Actually checking "does the requested sdk exist" is actually trivial so just including it

emw-apple

r=me

emw-apple · 2024-05-20T18:31:50Z

Tools/Scripts/webkitdirs.pm

+        `xcrun --sdk $xcodeSDK --show-sdk-path 2>&1`;
+        die "Unable to find requested sdk '" . $xcodeSDK . "' requested" if exitStatus($?);


Talked privately but: I've tried to avoid making calls to xcrun when executing XcodeOptions(), since xcrun can be a little slow (~2 sec) when uncached. So, I'd suggest to not unconditionally run this, especially since most people pass an unversioned SDK name.

webkit-early-warning-system · 2024-05-20T19:00:55Z

EWS run on current version of this PR (hash 6163619)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	~~🧪 wpe-wk2~~	✅ 🧪 wincairo-tests
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	~~🧪 ios-wk2-wpt~~	✅ 🧪 mac-wk1	✅ 🛠 wpe-cairo
✅ 🛠 🧪 jsc	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
✅ 🛠 🧪 jsc-arm64	✅ 🛠 tv	~~🧪 mac-AS-debug-wk2~~	~~🧪 gtk-wk2~~
	~~🛠 tv-sim~~	✅ 🧪 mac-wk2-stress	~~🧪 api-gtk~~
✅ 🛠 🧪 merge	~~🛠 watch~~		✅ 🛠 jsc-armv7
	✅ 🛠 watch-sim		✅ 🧪 jsc-armv7-tests

https://bugs.webkit.org/show_bug.cgi?id=274298 Reviewed by Elliott Williams. Strip the version number from the specified SDK as well when setting the platform name * Tools/Scripts/webkitdirs.pm: (determineXcodeSDKPlatformName): Canonical link: https://commits.webkit.org/279009@main

webkit-commit-queue · 2024-05-20T20:39:18Z

Committed 279009@main (9140fbd): https://commits.webkit.org/279009@main

Reviewed commits have been landed. Closing PR #28696 and removing active labels.

ojhunt requested a review from JonWBedard as a code owner May 17, 2024 06:08

aproskuryakov requested a review from emw-apple May 17, 2024 16:36

aproskuryakov reviewed May 17, 2024

View reviewed changes

emw-apple reviewed May 18, 2024

View reviewed changes

ojhunt force-pushed the eng/Support-specifying-explicit-SDK-version branch from d98521d to a2b2dad Compare May 18, 2024 21:41

ojhunt force-pushed the eng/Support-specifying-explicit-SDK-version branch from a2b2dad to 4814eae Compare May 18, 2024 22:26

emw-apple approved these changes May 20, 2024

View reviewed changes

ojhunt force-pushed the eng/Support-specifying-explicit-SDK-version branch from 4814eae to 6163619 Compare May 20, 2024 19:00

emw-apple added the merge-queue Applied to send a pull request to merge-queue label May 20, 2024

webkit-commit-queue force-pushed the eng/Support-specifying-explicit-SDK-version branch from 6163619 to 9140fbd Compare May 20, 2024 20:39

webkit-commit-queue merged commit 9140fbd into WebKit:main May 20, 2024

webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label May 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support specifying explicit SDK version #28696

Support specifying explicit SDK version #28696

ojhunt commented May 17, 2024 •

edited by webkit-early-warning-system

webkit-early-warning-system commented May 17, 2024 •

edited

aproskuryakov May 17, 2024

ojhunt May 18, 2024

emw-apple left a comment

emw-apple May 18, 2024

ojhunt May 18, 2024

webkit-early-warning-system commented May 18, 2024 •

edited

ojhunt commented May 18, 2024

webkit-early-warning-system commented May 18, 2024 •

edited

ojhunt commented May 18, 2024

emw-apple left a comment

emw-apple May 20, 2024

webkit-early-warning-system commented May 20, 2024 •

edited

webkit-commit-queue commented May 20, 2024

		`xcrun --sdk $xcodeSDK --show-sdk-path 2>&1`;
		die "Unable to find requested sdk '" . $xcodeSDK . "' requested" if exitStatus($?);

Support specifying explicit SDK version #28696

Support specifying explicit SDK version #28696

Conversation

ojhunt commented May 17, 2024 • edited by webkit-early-warning-system

9140fbd

webkit-early-warning-system commented May 17, 2024 • edited

aproskuryakov May 17, 2024

Choose a reason for hiding this comment

ojhunt May 18, 2024

Choose a reason for hiding this comment

emw-apple left a comment

Choose a reason for hiding this comment

emw-apple May 18, 2024

Choose a reason for hiding this comment

ojhunt May 18, 2024

Choose a reason for hiding this comment

webkit-early-warning-system commented May 18, 2024 • edited

ojhunt commented May 18, 2024

webkit-early-warning-system commented May 18, 2024 • edited

ojhunt commented May 18, 2024

emw-apple left a comment

Choose a reason for hiding this comment

emw-apple May 20, 2024

Choose a reason for hiding this comment

webkit-early-warning-system commented May 20, 2024 • edited

webkit-commit-queue commented May 20, 2024

ojhunt commented May 17, 2024 •

edited by webkit-early-warning-system

`9140fbd`

webkit-early-warning-system commented May 17, 2024 •

edited

webkit-early-warning-system commented May 18, 2024 •

edited

webkit-early-warning-system commented May 18, 2024 •

edited

webkit-early-warning-system commented May 20, 2024 •

edited