Bug 229418 - Build error preprocessing sandbox
Summary: Build error preprocessing sandbox
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Per Arne Vollan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-08-23 13:40 PDT by Per Arne Vollan
Modified: 2021-08-25 15:46 PDT (History)
3 users (show)

See Also:


Attachments
Patch (1.31 KB, patch)
2021-08-23 13:44 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (1.42 KB, patch)
2021-08-24 10:02 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (1.23 KB, patch)
2021-08-25 14:45 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Per Arne Vollan 2021-08-23 13:40:17 PDT
In some cases there are build errors when preprocessing the WebContent sandbox.
Comment 1 Per Arne Vollan 2021-08-23 13:40:52 PDT
<rdar://81480311>
Comment 2 Per Arne Vollan 2021-08-23 13:44:14 PDT
Created attachment 436231 [details]
Patch
Comment 3 Alexey Proskuryakov 2021-08-23 13:59:52 PDT
Comment on attachment 436231 [details]
Patch

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

> Source/WebKit/Scripts/generate-derived-sources.sh:12
> +if [ ! -z ${JAVASCRIPTCORE_PRIVATE_HEADERS_DIR} ]; then

-z is "the length of string is zero", can you elaborate on the logic of this check?

I expected this to check for sandbox-profiles-ios argument that's passed when building the profiles.
Comment 4 Per Arne Vollan 2021-08-23 14:33:16 PDT
(In reply to Alexey Proskuryakov from comment #3)
> Comment on attachment 436231 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=436231&action=review
> 
> > Source/WebKit/Scripts/generate-derived-sources.sh:12
> > +if [ ! -z ${JAVASCRIPTCORE_PRIVATE_HEADERS_DIR} ]; then
> 
> -z is "the length of string is zero", can you elaborate on the logic of this
> check?
>

Yes, this checks that the env variable exists (does not have zero length) before using it. If the env variable does not exist, the ln command will fail. 
 
> I expected this to check for sandbox-profiles-ios argument that's passed
> when building the profiles.

I think the above test will cover more cases, in case this should happen under other circumstances. Also, it will continue to work if we should rename the argument.

Thanks for reviewing!
Comment 5 Alexey Proskuryakov 2021-08-23 14:49:29 PDT
My concern is that we will get a way more confusing error from make if this happens under other circumstances. 

So I'd prefer it to be as focused as possible, so that the check is only true when we know it should be true - not when it happens to be true.
Comment 6 Per Arne Vollan 2021-08-23 16:47:19 PDT
(In reply to Alexey Proskuryakov from comment #5)
> My concern is that we will get a way more confusing error from make if this
> happens under other circumstances. 
> 
> So I'd prefer it to be as focused as possible, so that the check is only
> true when we know it should be true - not when it happens to be true.

That makes sense, I will update the patch.

Thanks for reviewing!
Comment 7 Per Arne Vollan 2021-08-24 10:02:25 PDT
Created attachment 436301 [details]
Patch
Comment 8 Per Arne Vollan 2021-08-25 13:29:54 PDT
Comment on attachment 436301 [details]
Patch

Thanks for reviewing!
Comment 9 EWS 2021-08-25 13:33:51 PDT
Committed r281577 (240941@main): <https://commits.webkit.org/240941@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 436301 [details].
Comment 10 Per Arne Vollan 2021-08-25 14:45:58 PDT
Reopening to attach new patch.
Comment 11 Per Arne Vollan 2021-08-25 14:45:59 PDT
Created attachment 436428 [details]
Patch
Comment 12 Per Arne Vollan 2021-08-25 14:49:39 PDT
Comment on attachment 436428 [details]
Patch

Thanks for reviewing!
Comment 13 EWS 2021-08-25 15:46:05 PDT
Committed r281595 (240957@main): <https://commits.webkit.org/240957@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 436428 [details].