Bug 206607 – WebKitTestRunner: use ad hoc signing for internal Production builds

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Jiewen Tan]

WebKitTestRunner: move code sign configurations from WebKitTestRunner.xcconfig to DebugRelease.xcconfig.

[Jiewen Tan]

<rdar://problem/56087327>

[Jiewen Tan]

Created attachment 388456 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 388456 [details]
Patch

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

> Tools/ChangeLog:3
> +        WebKitTestRunner: move code sign configurations from WebKitTestRunner.xcconfig to DebugRelease.xcconfig

This is probably fine, but I don't understand why this changes any behavior. These Xcode settings shouldn't matter for production builds AFAIK.

[Jonathan Bedard]

This more closely matches what we've done for JavaScriptCore and WebKit, so even if this doesn't fix Jiewen's problem (and I'm not convinced it will) it does seem more correct than what we were doing previously.

[Jonathan Bedard]

Comment on attachment 388456 [details]
Patch

Lets wait for a more authoritative answer from EWS, if this is going to break open source, EWS should catch it.

[Jiewen Tan]

Created attachment 388476 [details]
Patch V2

[Jiewen Tan]

(In reply to Jiewen Tan from comment #6)
> Created attachment 388476 [details]
> Patch V2

This is an attempt to address those layout test failures.

[Jiewen Tan]

Created attachment 388483 [details]
Patch V3

[Jiewen Tan]

Created attachment 388486 [details]
Patch V4

[Alexey Proskuryakov]

Comment on attachment 388486 [details]
Patch V4

Yes, I think that this is right. May be worth doing a production build locally to be super safe.

[Jiewen Tan]

(In reply to Alexey Proskuryakov from comment #10)
> Comment on attachment 388486 [details]
> Patch V4
> 
> Yes, I think that this is right. May be worth doing a production build
> locally to be super safe.

Thanks Alexey for r+ this patch.

[Jiewen Tan]

Comment on attachment 388486 [details]
Patch V4

cq+ as mac-wk2 is green.

[WebKit Commit Bot]

Comment on attachment 388486 [details]
Patch V4

Clearing flags on attachment: 388486

Committed r254955: <https://trac.webkit.org/changeset/254955>

[Jiewen Tan]

Reverted r254955 for reason:

Broke internal builds

Committed r254963: <https://trac.webkit.org/changeset/254963>

[Jiewen Tan]

Created attachment 388596 [details]
Patch V5

[Jiewen Tan]

Created attachment 388597 [details]
Patch V5

[Jonathan Bedard]

Comment on attachment 388597 [details]
Patch V5

Not a super great way to test this one, so we probably need to actually tests this with production builders.

Keith and Jiewen talked about this, and think it will fix our problems.

I'm going to r+ so we can test this, Jiewen said he's going to watch the bots to rollout if everything catches on fire.

[Jiewen Tan]

(In reply to Jonathan Bedard from comment #17)
> Comment on attachment 388597 [details]
> Patch V5
> 
> Not a super great way to test this one, so we probably need to actually
> tests this with production builders.
> 
> Keith and Jiewen talked about this, and think it will fix our problems.
> 
> I'm going to r+ so we can test this, Jiewen said he's going to watch the
> bots to rollout if everything catches on fire.

Thank you, Jonathan. Will land it manually to catch things faster.

[Jiewen Tan]

Committed r255041: <https://trac.webkit.org/changeset/255041>

[Jiewen Tan]

A speculative test fix:
Committed r255111: <https://trac.webkit.org/changeset/255111>

[Jiewen Tan]

A build fix after r255111:
Committed r255188: <https://trac.webkit.org/changeset/255188>

[Jiewen Tan]

Reopening to attach new patch.

[Jiewen Tan]

Created attachment 389282 [details]
Another speculative test fix

[Alexey Proskuryakov]

Comment on attachment 389282 [details]
Another speculative test fix

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

> Source/WebKit/ChangeLog:3
> +        Unreviewed, another speculative test fix after r255041

This says "unreviewed", but the patch is marked for review. Looks reasonable, rs=me with one question.

> Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm:359
> +        (id)kSecAttrAccessible: (id)kSecAttrAccessibleAfterFirstUnlock,

Why not kSecAttrAccessibleAlways? I don't think that there is a practical difference, but "always" is what we want for testing, so it seems better to request that explicitly.

[Jiewen Tan]

Thanks, Alexey.

(In reply to Alexey Proskuryakov from comment #24)
> Comment on attachment 389282 [details]
> Another speculative test fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=389282&action=review
> 
> > Source/WebKit/ChangeLog:3
> > +        Unreviewed, another speculative test fix after r255041
> 
> This says "unreviewed", but the patch is marked for review. Looks
> reasonable, rs=me with one question.
> 
> > Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm:359
> > +        (id)kSecAttrAccessible: (id)kSecAttrAccessibleAfterFirstUnlock,
> 
> Why not kSecAttrAccessibleAlways? I don't think that there is a practical
> difference, but "always" is what we want for testing, so it seems better to
> request that explicitly.

The API is marked as deprecated. I guess we should avoid using deprecated API.

[Jiewen Tan]

Committed r255466: <https://trac.webkit.org/changeset/255466>