Bug 207043 – Generalize configure-xcode-for-ios-development

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Jonathan Bedard]

This script needs to be generalized to serve watchOS and tvOS as well as iOS.

[Jonathan Bedard]

Created attachment 389347 [details]
Patch

[Jonathan Bedard]

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 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]

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]

Created attachment 400491 [details]
Patch

[Jonathan Bedard]

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]

<rdar://problem/63946933>

[Jonathan Bedard]

Created attachment 401036 [details]
Patch

[Jonathan Bedard]

Created attachment 401059 [details]
Patch

[David Kilzer (:ddkilzer)]

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]

(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]

Created attachment 401193 [details]
Patch for landing

[EWS]

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]

Reopening to attach new patch.

[Jonathan Bedard]

Created attachment 401197 [details]
Patch

[Keith Rollin]

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 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]

(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]

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]

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]

(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]

(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]

(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.