Bug 195528 – [iOS] Copy sandbox profiles into $BUILT_PRODUCTS_DIR during engineering builds

• Component: New Bugs
• Status: NEW

[Andy Estes]

[iOS] Copy sandbox profiles into $BUILT_PRODUCTS_DIR during engineering builds

[Andy Estes]

Created attachment 364156 [details]
Patch

Is there any way to retrofit the existing Sandbox Profiles target do this in engineering builds?

Perhaps just changing the shared scheme (and engineering makefile) to build Sandbox Profiles in addition to All?

[Alexey Proskuryakov]

Comment on attachment 364156 [details]
Patch

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

> Source/WebKit/ChangeLog:11
> +        To make this task easier, this patch teaches WebKit to copy the iOS sandbox profiles into

Does this mean that perf testing results on iOS will be misleading when using release builds? When discussing a change like this in the past, some people thought that that would be a show stopper.

[Andy Estes]

(In reply to Alexey Proskuryakov from comment #4)
> Comment on attachment 364156 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=364156&action=review
> 
> > Source/WebKit/ChangeLog:11
> > +        To make this task easier, this patch teaches WebKit to copy the iOS sandbox profiles into
> 
> Does this mean that perf testing results on iOS will be misleading when
> using release builds? When discussing a change like this in the past, some
> people thought that that would be a show stopper.

I don't think this would change iOS perf testing results at all, because nothing in this patch causes the copied sandbox profiles to be installed on an iOS device. That still requires a manual step.

(Of course, it's not clear to me how iOS perf testing is dealing with this problem now. Surely not applying a necessary sandbox profile change on a device will affect perf testing results.)

[EWS Watchlist]

Comment on attachment 364156 [details]
Patch

Attachment 364156 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11444112

New failing tests:
imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-constructor.html

[EWS Watchlist]

Created attachment 364165 [details]
Archive of layout-test-results from ews107 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[Alexey Proskuryakov]

Comment on attachment 364156 [details]
Patch

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

>>> Source/WebKit/ChangeLog:11
>>> +        To make this task easier, this patch teaches WebKit to copy the iOS sandbox profiles into
>> 
>> Does this mean that perf testing results on iOS will be misleading when using release builds? When discussing a change like this in the past, some people thought that that would be a show stopper.
> 
> I don't think this would change iOS perf testing results at all, because nothing in this patch causes the copied sandbox profiles to be installed on an iOS device. That still requires a manual step.
> 
> (Of course, it's not clear to me how iOS perf testing is dealing with this problem now. Surely not applying a necessary sandbox profile change on a device will affect perf testing results.)

Perhaps I don’t understand what the patch does, going from the ChangeLog. But if it puts profiles into /usr/local/share/sandbox in the built archive, then what is the missing manual step?

As for perf impact - not installing profiles means that we are missing on testing the impact of changes, but installing them into this directory results in enormous process launch slowdown. At least that’s what I’ve been told. So it’s better to not perf test these changes than to run all testing in a wildly different scenario.

[Alexey Proskuryakov]

Perhaps you are saying that you don’t expect package-root to be the def in this scenario?

[Andy Estes]

(In reply to Alexey Proskuryakov from comment #9)
> Perhaps you are saying that you don’t expect package-root to be the def in
> this scenario?

Yes, the manual steps would be to run package-root (or otherwise package your built products into a root, install the root on a device, and run the sbutil command (or reboot I think).

I think for performance testing we build with build-root, and my change has no impact on Production builds.

Comment on attachment 364156 [details]
Patch

r- just as a reminder to address my comment #2 and comment #3. If it turns out that there’s no way to reuse the existing target, you can r? this version again.

[Alexey Proskuryakov]

> I think for performance testing we build with build-root, and my change has no impact on Production builds.

Bots do, but I've heard it many times from folks who require preserving the ability to do perf testing with local release builds.