Bug 140828 – [iOS] Make it possible to build WebKit using iphoneos SDK without a developer certificate installed

• Component: Tools / Tests
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

For the ios-ews queue and for developers who want to make sure their changes build for the iOS port of WebKit using the iphoneos SDK, we need a way to disable code-signing so that Xcode doesn't stop building if it can't find a developer certificate.

This will add a --disable-code-signing switch to the configure-xcode-for-ios-development script.

[David Kilzer (:ddkilzer)]

Created attachment 245238 [details]
Patch v1

[Daniel Bates]

Comment on attachment 245238 [details]
Patch v1

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

> Tools/ChangeLog:11
> +        (disableCodeSigning): Add.  Calls

Nit: "Add" => "Added".

> Tools/ChangeLog:14
> +        (disableCodeSigningForProductType): Add.  Does the work of

Ditto.

> Tools/Scripts/configure-xcode-for-ios-development:65
> +  -h|--help               Show this help message

Nit: I suggest we list the --help option at the top of the usage message so as to be consistent with the order we use in other scripts, including build-webkit and run-safari.

> Tools/Scripts/configure-xcode-for-ios-development:212
> +    for ( ; ; ++$index) {
> +        `/usr/libexec/PlistBuddy -c "Print :$index" "$plist" > /dev/null 2>&1`;
> +        die "Failed to disable code signing for $productType: $!" if exitStatus($?);
> +
> +        chomp(my $name = `/usr/libexec/PlistBuddy -c "Print :$index:Name" "$plist" 2> /dev/null`);
> +        next unless $name eq $productType;
> +
> +        chomp(my $value = `/usr/libexec/PlistBuddy -c "Print :$index:DefaultBuildProperties:CODE_SIGNING_ALLOWED" "$plist" 2> /dev/null`);
> +        return if ($value eq "NO");
> +
> +        system("/usr/libexec/PlistBuddy", "-x", "-c", "Set :$index:DefaultBuildProperties:CODE_SIGNING_ALLOWED NO", $plist);
> +        die "Failed to disable code signing for $productType: $!" if exitStatus($?);
> +        print "Successfully disabled code signing for $productType.\n";
> +        return;
> +    }

Notice that we have existing parsing code for finding the index position of a <dict> with child a <key> called Identifier in a plist in readXcodeSpecificationById(). Can we make use of this function or extract the common functionality into a helper function that is used by both readXcodeSpecificationById() and disableCodeSigningForProductType()?

[David Kilzer (:ddkilzer)]

Comment on attachment 245238 [details]
Patch v1

There is some duplicate code that Dan pointed out, and I want to add an --enable-code-signing switch to undo the change that --disable-code-signing does (in case someone needs to do that).

We may also want to add a check for a developer certificate in the keychain, and bail out (or ask) before continuing with disabling code signing.

[David Kilzer (:ddkilzer)]

(In reply to comment #2)
> Comment on attachment 245238 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=245238&action=review
> 
> > Tools/ChangeLog:11
> > +        (disableCodeSigning): Add.  Calls
> 
> Nit: "Add" => "Added".
> 
> > Tools/ChangeLog:14
> > +        (disableCodeSigningForProductType): Add.  Does the work of
> 
> Ditto.

I prefer the imperative, present tense:

<http://stackoverflow.com/questions/3580013/should-i-use-past-or-present-tense-in-git-commit-messages>

> > Tools/Scripts/configure-xcode-for-ios-development:65
> > +  -h|--help               Show this help message
> 
> Nit: I suggest we list the --help option at the top of the usage message so
> as to be consistent with the order we use in other scripts, including
> build-webkit and run-safari.

Will fix.

> > Tools/Scripts/configure-xcode-for-ios-development:212
> > +    for ( ; ; ++$index) {
> > +        `/usr/libexec/PlistBuddy -c "Print :$index" "$plist" > /dev/null 2>&1`;
> > +        die "Failed to disable code signing for $productType: $!" if exitStatus($?);
> > +
> > +        chomp(my $name = `/usr/libexec/PlistBuddy -c "Print :$index:Name" "$plist" 2> /dev/null`);
> > +        next unless $name eq $productType;
> > +
> > +        chomp(my $value = `/usr/libexec/PlistBuddy -c "Print :$index:DefaultBuildProperties:CODE_SIGNING_ALLOWED" "$plist" 2> /dev/null`);
> > +        return if ($value eq "NO");
> > +
> > +        system("/usr/libexec/PlistBuddy", "-x", "-c", "Set :$index:DefaultBuildProperties:CODE_SIGNING_ALLOWED NO", $plist);
> > +        die "Failed to disable code signing for $productType: $!" if exitStatus($?);
> > +        print "Successfully disabled code signing for $productType.\n";
> > +        return;
> > +    }
> 
> Notice that we have existing parsing code for finding the index position of
> a <dict> with child a <key> called Identifier in a plist in
> readXcodeSpecificationById(). Can we make use of this function or extract
> the common functionality into a helper function that is used by both
> readXcodeSpecificationById() and disableCodeSigningForProductType()?

Will reuse this code in the next patch.  Thanks!

[David Kilzer (:ddkilzer)]

<rdar://problem/19520599>

[Alexey Proskuryakov]

What's the next step on this? Is this patch still necessary?

[Daniel Bates]

Created attachment 262463 [details]
Patch

[Daniel Bates]

Comment on attachment 262463 [details]
Patch

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

> Tools/Scripts/webkitdirs.pm:2012
> +sub hasIOSDevelopmentCertificate()

Although, this function takes no arguments, will forward declare this function at the top of the file so that Perl can check its prototype.

[Daniel Bates]

Created attachment 262468 [details]
Patch

Actually, moved hasIOSDevelopmentCertificate() to come before XcodeOptions() instead of forward declaring it. Added missing semicolon in hasIOSDevelopmentCertificate().

[Alexey Proskuryakov]

Comment on attachment 262468 [details]
Patch

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

Neat.

EWS is broken due to a (new) unrelated issue.

> Tools/Scripts/webkitdirs.pm:688
> +    push @options, ("-configuration", $configuration);
> +    push @options, ("-xcconfig", sourceDir() . "/Tools/asan/asan.xcconfig", "ASAN_IGNORE=" . sourceDir() . "/Tools/asan/webkit-asan-ignore.txt") if $asanIsEnabled;

I think that parentheses are not needed here.

[Daniel Bates]

(In reply to comment #10)
> > Tools/Scripts/webkitdirs.pm:688
> > +    push @options, ("-configuration", $configuration);
> > +    push @options, ("-xcconfig", sourceDir() . "/Tools/asan/asan.xcconfig", "ASAN_IGNORE=" . sourceDir() . "/Tools/asan/webkit-asan-ignore.txt") if $asanIsEnabled;
> 
> I think that parentheses are not needed here.

Yes, parentheses are not needed here. I will keep them because I feel they improves the readability of this code.

[Daniel Bates]

Comment on attachment 262468 [details]
Patch

Clearing flags on attachment: 262468

Committed r190583: <http://trac.webkit.org/changeset/190583>

[Daniel Bates]

All reviewed patches have been landed.  Closing bug.