RESOLVED FIXED 207043
Generalize configure-xcode-for-ios-development
https://bugs.webkit.org/show_bug.cgi?id=207043
Summary Generalize configure-xcode-for-ios-development
Jonathan Bedard
Reported 2020-01-31 07:12:38 PST
This script needs to be generalized to serve watchOS and tvOS as well as iOS.
Attachments
Patch (30.96 KB, patch)
2020-01-31 07:20 PST, Jonathan Bedard
no flags
Patch (13.99 KB, patch)
2020-05-28 12:10 PDT, Jonathan Bedard
no flags
Patch (13.93 KB, patch)
2020-06-04 10:17 PDT, Jonathan Bedard
no flags
Patch (13.68 KB, patch)
2020-06-04 12:01 PDT, Jonathan Bedard
no flags
Patch for landing (13.74 KB, patch)
2020-06-05 13:48 PDT, Jonathan Bedard
no flags
Patch (14.01 KB, patch)
2020-06-05 14:24 PDT, Jonathan Bedard
no flags
Jonathan Bedard
Comment 1 2020-01-31 07:20:19 PST
Jonathan Bedard
Comment 2 2020-01-31 07:21:11 PST
Verified that this works on a customer install. Haven't totally gotten tvOS and watchOS working with this, but it's the first step.
Keith Rollin
Comment 3 2020-01-31 13:34:41 PST
Comment on attachment 389347 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=389347&action=review > Source/WebKit/UIProcess/WKImagePreviewViewController.mm:102 > { Why use direct pragmas rather than adding ALLOW_DEPRECATED_IMPLEMENTATIONS_BEGIN/ALLOW_DEPRECATED_IMPLEMENTATIONS_END? Is this necessary to get the compile to work, given our discussion on my having added those macros and still not getting the compile to work? > Tools/Scripts/configure-xcode-for-embedded-development:4 > +# Copyright 2020.
Keith Rollin
Comment 4 2020-01-31 13:37:03 PST
I confirm that applying the patch to an isolated copy of the WebKit checkout, running the script on a public version of Xcode, and running `build-webkit --sdk iphoneos` works for me.
Jonathan Bedard
Comment 5 2020-05-28 12:10:28 PDT
Jonathan Bedard
Comment 6 2020-05-28 12:11:45 PDT
Reviving this bug. I've made some edits to this script so it is compatible with the new build system (at least for tvOS). I think we are in a place where we should land the script, we need to edit some documentation before removing the old one, though.
Radar WebKit Bug Importer
Comment 7 2020-06-03 15:15:00 PDT
Jonathan Bedard
Comment 8 2020-06-04 10:17:46 PDT
Jonathan Bedard
Comment 9 2020-06-04 12:01:10 PDT
David Kilzer (:ddkilzer)
Comment 10 2020-06-05 13:05:03 PDT
Comment on attachment 401059 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=401059&action=review rs=me I did not try to compare logic from the old script to the new one. The best test would be to run each script and verify output is identical (equivalent) for a given SDK. > Tools/Scripts/configure-xcode-for-embedded-development:1 > +#!/usr/bin/env python3 We aren't supporting Python 2.7 in new scripts anymore? > Tools/Scripts/configure-xcode-for-embedded-development:3 > +# Copyright (C) 2020 Apple Inc. All rights reserved. Nit: Since this is based on the Perl script, should the copyright be from 2014-2020? # Copyright (C) 2014, 2015 Apple Inc. All rights reserved.
Jonathan Bedard
Comment 11 2020-06-05 13:43:28 PDT
(In reply to David Kilzer (:ddkilzer) from comment #10) > Comment on attachment 401059 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=401059&action=review > > rs=me > > I did not try to compare logic from the old script to the new one. The best > test would be to run each script and verify output is identical (equivalent) > for a given SDK. It's not exactly identical (since we are adding the watchOS architectures), and there are a few old bits that were removed. The most important piece is ensuring that the new script puts an external SDK in a state that it can build WebKit for iOS, which it does do. > > > Tools/Scripts/configure-xcode-for-embedded-development:1 > > +#!/usr/bin/env python3 > > We aren't supporting Python 2.7 in new scripts anymore? Our motivation for supporting Python 2.7 is to keep supporting Mojave. However, our embedded builds only support the latest Xcode and latest macOS, so while many scripts need to support Python 2.7, this script is guaranteed to be used on Catalina, which has Python 3. > > > Tools/Scripts/configure-xcode-for-embedded-development:3 > > +# Copyright (C) 2020 Apple Inc. All rights reserved. > > Nit: Since this is based on the Perl script, should the copyright be from > 2014-2020? > > # Copyright (C) 2014, 2015 Apple Inc. All rights reserved. Good point, updating the copyright.
Jonathan Bedard
Comment 12 2020-06-05 13:48:36 PDT
Created attachment 401193 [details] Patch for landing
EWS
Comment 13 2020-06-05 14:15:21 PDT
Committed r262653: <https://trac.webkit.org/changeset/262653> All reviewed patches have been landed. Closing bug and clearing flags on attachment 401193 [details].
Jonathan Bedard
Comment 14 2020-06-05 14:24:53 PDT
Reopening to attach new patch.
Jonathan Bedard
Comment 15 2020-06-05 14:24:54 PDT
Keith Rollin
Comment 16 2020-06-05 14:43:27 PDT
I would expect to see a comment in the ChangeLog on what this patch is for. I guess it's to "switch to the new script and remove the old script"? > Nit: Since this is based on the Perl script, should the copyright be from > 2014-2020? I'm wondering if this is the case. I'm certainly no copyright lawyer. But the new script was a rewrite. It wasn't a transliteration or anything. It was written using the old script as a checklist. It doesn't bring over any of the previous approaches or algorithms. One thing I'm thinking of here is a story Audible suggested to me. It's a modern take on Dr. Jekyll and Mr. Hyde. It's based on the original and takes the central idea, but I don't think it's copyright 1886-2020. Again, just my two bits. I don't know what I'm talking about.
Darin Adler
Comment 17 2020-06-10 11:29:09 PDT
Comment on attachment 401197 [details] Patch Why do we need a separate script for this? Is it critical that these steps *not* be done for someone who only intends to develop for Mac? I think it’s better if there are the minimum number of separate scripts for each person to run. Ideally after installing tools if you wish to work on WebKit there is only one script invocation needed.
Jonathan Bedard
Comment 18 2020-06-10 17:13:32 PDT
(In reply to Darin Adler from comment #17) > Comment on attachment 401197 [details] > Patch > > Why do we need a separate script for this? Is it critical that these steps > *not* be done for someone who only intends to develop for Mac? I think it’s > better if there are the minimum number of separate scripts for each person > to run. Ideally after installing tools if you wish to work on WebKit there > is only one script invocation needed. There isn't a similar script for Mac, if there was one, I would agree. But Mac just works with 'build-webkit' right out of the box. It's also worth noting that this script modifies your Xcode install, I don't think we want folks doing that unless they actually need to. If we really wanted to, we could probably integrate this script into build-webkit, but I haven't seen a need to do that so far. The instructions on https://webkit.org/building-webkit/ seem to have served us well so far.
EWS
Comment 19 2020-06-10 17:41:36 PDT
Committed r262884: <https://trac.webkit.org/changeset/262884> All reviewed patches have been landed. Closing bug and clearing flags on attachment 401197 [details].
Chenfeng Pan
Comment 20 2020-09-22 00:06:25 PDT
Hi, when I build WebKit for iOS according to https://github.com/WebKit/webkit/blob/master/ReadMe.md#building-ios-port I find there is no Tools/Scripts/configure-xcode-for-ios-development. Until searching in trac.webkit.org I find it has been replaced with Tools/Scripts/configure-xcode-for-embedded-development. So ReadMe.md needs to be updated now.
Keith Rollin
Comment 21 2020-09-22 00:28:15 PDT
(In reply to Jonathan Bedard from comment #18) > (In reply to Darin Adler from comment #17) > > Comment on attachment 401197 [details] > > Patch > > > > Why do we need a separate script for this? Is it critical that these steps > > *not* be done for someone who only intends to develop for Mac? I think it’s > > better if there are the minimum number of separate scripts for each person > > to run. Ideally after installing tools if you wish to work on WebKit there > > is only one script invocation needed. > > There isn't a similar script for Mac, if there was one, I would agree. But > Mac just works with 'build-webkit' right out of the box. > > It's also worth noting that this script modifies your Xcode install, I don't > think we want folks doing that unless they actually need to. > > If we really wanted to, we could probably integrate this script into > build-webkit, but I haven't seen a need to do that so far. The instructions > on https://webkit.org/building-webkit/ seem to have served us well so far. Perhaps, when a developer tries to build for iOS, we can detect if the modifications haven't been made. If they haven't, present the developer with a confirmation request (which explains the liberties we'll be taking with Xcode), and then invoke configure-code-for-embedded-development for them. If we take this route, then our update to the ReadMe.md would be to remove any reference to such a script.
Jonathan Bedard
Comment 22 2020-09-22 08:28:11 PDT
(In reply to Keith Rollin from comment #21) > .... > > Perhaps, when a developer tries to build for iOS, we can detect if the > modifications haven't been made. If they haven't, present the developer with > a confirmation request (which explains the liberties we'll be taking with > Xcode), and then invoke configure-code-for-embedded-development for them. If > we take this route, then our update to the ReadMe.md would be to remove any > reference to such a script. Might be harder than you expect, we could easily do that for build-webkit, but not sure how we we do it for Xcode builds.
Jonathan Bedard
Comment 23 2020-09-22 08:28:53 PDT
(In reply to Chenfeng Pan from comment #20) > Hi, when I build WebKit for iOS according to > https://github.com/WebKit/webkit/blob/master/ReadMe.md#building-ios-port I > find there is no Tools/Scripts/configure-xcode-for-ios-development. Until > searching in trac.webkit.org I find it has been replaced with > Tools/Scripts/configure-xcode-for-embedded-development. > So ReadMe.md needs to be updated now. Oops! Didn't realize we had a ReadMe there! I updated https://webkit.org/building-webkit/, but not the ReadMe....doing so now.
Note You need to log in before you can comment on or make changes to this bug.