Bug 213727

Summary: [webkitperl] nativeArchitecture() needs to support remote devices
Product: WebKit Reporter: Jonathan Bedard <jbedard>
Component: Tools / TestsAssignee: Jonathan Bedard <jbedard>
Status: RESOLVED FIXED    
Severity: Normal CC: aakash_jain, guijemont, saam, webkit-bug-importer, zhifei_fang
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=213487
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Jonathan Bedard 2020-06-29 07:23:38 PDT
When targeting a remote platform, nativeArchitecture() needs to support return the architecture of the remote device.
Comment 1 Radar WebKit Bug Importer 2020-06-29 08:50:14 PDT
<rdar://problem/64892021>
Comment 2 Jonathan Bedard 2020-06-29 08:58:13 PDT
Created attachment 403066 [details]
Patch
Comment 3 Jonathan Bedard 2020-06-29 09:41:58 PDT
Created attachment 403073 [details]
Patch
Comment 4 Guillaume Emont 2020-06-29 10:20:15 PDT
Comment on attachment 403066 [details]
Patch

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

The changes look sensible at first sight. I'll try to run a couple tests locally, though the arm EWS should pick it up soon.

> Tools/Scripts/webkitdirs.pm:131
> +my %nativeArchitectureMap = ();

Do we need this to be a map? It looks like we would always have exactly one element in it, or are there some nativeArchitecture() calls without arguments that are left there on purpose?
Comment 5 Jonathan Bedard 2020-06-29 10:34:10 PDT
(In reply to Guillaume Emont from comment #4)
> Comment on attachment 403066 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=403066&action=review
> 
> The changes look sensible at first sight. I'll try to run a couple tests
> locally, though the arm EWS should pick it up soon.
> 
> > Tools/Scripts/webkitdirs.pm:131
> > +my %nativeArchitectureMap = ();
> 
> Do we need this to be a map? It looks like we would always have exactly one
> element in it, or are there some nativeArchitecture() calls without
> arguments that are left there on purpose?

It's hard to get rid of all the nativeArchitecture() calls without arguments since other places in webkitdirs may call it. Seemed better to just cache the results on a per-host basis.
Comment 6 Jonathan Bedard 2020-06-29 10:35:39 PDT
Comment on attachment 403073 [details]
Patch

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

> Tools/Scripts/webkitdirs.pm:362
> +    if ($target eq 'localhost') {

Just realized this won't work, embedded devices will target localhost on a different port.
Comment 7 Jonathan Bedard 2020-06-29 10:41:01 PDT
Created attachment 403080 [details]
Patch
Comment 8 Jonathan Bedard 2020-06-29 13:56:01 PDT
Created attachment 403109 [details]
Patch
Comment 9 Jonathan Bedard 2020-06-29 14:39:50 PDT
Created attachment 403115 [details]
Patch
Comment 10 Jonathan Bedard 2020-06-29 15:56:02 PDT
Created attachment 403127 [details]
Patch
Comment 11 Jonathan Bedard 2020-06-29 16:13:02 PDT
Created attachment 403132 [details]
Patch
Comment 12 Jonathan Bedard 2020-06-29 17:03:12 PDT
Created attachment 403141 [details]
Patch
Comment 13 Jonathan Bedard 2020-06-29 19:26:19 PDT
Created attachment 403160 [details]
Patch
Comment 14 Guillaume Emont 2020-06-30 04:39:24 PDT
The patch makes sense to me, I could locally check that it works with --remote, and the jsc-armv7 ews confirms that it fixes the issues introduced in r263569. I am not a reviewer, but I think that this patch should be landed ASAP as it will unbreak the armv7 and mips bots (EWS and post-commit).
Comment 15 Aakash Jain 2020-06-30 04:49:44 PDT
Comment on attachment 403160 [details]
Patch

rs=me
Comment 16 EWS 2020-06-30 04:52:05 PDT
Committed r263742: <https://trac.webkit.org/changeset/263742>

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