Stop using sdk/system/child_process/subprocess in WebIDE

RESOLVED FIXED in Firefox 56

Status

enhancement
P1
normal
RESOLVED FIXED
2 years ago
10 months ago

People

(Reporter: pbro, Assigned: ochameau)

Tracking

unspecified
Firefox 56
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [nosdk] [qf:p3])

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
We're trying to stop using certain SDK APIs in bug 1350645, including sdk/system/child_process/subprocess which happens to be used in WebIDE. 

The usages of child_process in WebIDE are for building apps (not important anymore) and running Firefox OS simulators (also not important).

So, let's kill these parts of WebIDE so we can stop using this SDK API.
The other solution is to kill WebIDE altogether (see bug 1314811), but this is a more complicated thing to do.
Whiteboard: [qf]
If getting the WebIDE out is not on the short-term horizon (apparently, bug 1212802 is a dependency for removing WebIDE, and is not being worked on yet), then Subprocess.jsm might be the short-term solution to getting off of this SDK module.
Whiteboard: [qf] → [qf:p3]
Depends on: 1361311
WebIDE triage. Filter on TRIAGE-JD201705
Priority: -- → P2
(Reporter)

Comment 3

2 years ago
(In reply to Mike Conley (:mconley) from comment #1)
> If getting the WebIDE out is not on the short-term horizon (apparently, bug
> 1212802 is a dependency for removing WebIDE, and is not being worked on
> yet), then Subprocess.jsm might be the short-term solution to getting off of
> this SDK module.
Sounds good to me. Thanks.
Apparently sdk/system/child_process/subprocess is basically a wrapper around Subprocess.jsm, so that shouldn't be too hard.
In WebIDE, the following 2 files use sdk/system/child_process/subprocess:
devtools\client\webide\modules\build.js
devtools\client\webide\modules\simulator-process.js
(In reply to Patrick Brosset <:pbro> from comment #3)
> (In reply to Mike Conley (:mconley) from comment #1)
> > If getting the WebIDE out is not on the short-term horizon (apparently, bug
> > 1212802 is a dependency for removing WebIDE, and is not being worked on
> > yet), then Subprocess.jsm might be the short-term solution to getting off of
> > this SDK module.
> Sounds good to me. Thanks.
> Apparently sdk/system/child_process/subprocess is basically a wrapper around
> Subprocess.jsm, so that shouldn't be too hard.
> In WebIDE, the following 2 files use sdk/system/child_process/subprocess:
> devtools\client\webide\modules\build.js

build.js seems reasonable to just remove at this stage.

> devtools\client\webide\modules\simulator-process.js

This one is probably more trouble to simply remove, so converting it to use Subprocess.jsm seems like the fastest path.
No longer blocks: 1350645
Flags: qe-verify-
Whiteboard: [qf:p3] → [nosdk] [qf:p3]
(Assignee)

Comment 5

2 years ago
I'll try to cleanup some things here as I'm the last one of WebIDE era around.
Assignee: nobody → poirot.alex
Status: NEW → ASSIGNED
Priority: P2 → P1
Target Milestone: --- → Firefox 56
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8889522 - Flags: review?(jryans)
Attachment #8889523 - Flags: review?(jryans)

Comment 8

2 years ago
mozreview-review
Comment on attachment 8889523 [details]
Bug 1353656 - Remove WebIDE build step for local apps.

https://reviewboard.mozilla.org/r/160550/#review166296

Thanks, looks good overall! :) Just a few more things to remove.

::: devtools/client/webide/content/details.js
(Diff revision 1)
>    }
>  
>    AppManager.update("details");
>  }
>  
> -function showPrepackageLog() {

Please also remove `details_showPrepackageLog_button`[1], the `prePackageLog`[2] button, and line to hide it[3].

[1]: http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/devtools/client/locales/en-US/webide.dtd#93
[2]: http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/devtools/client/webide/content/details.xhtml#47
[3]: http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/devtools/client/webide/content/details.js#43
Attachment #8889523 - Flags: review?(jryans) → review+

Comment 9

2 years ago
mozreview-review
Comment on attachment 8889522 [details]
Bug 1353656 - Use SubProcess.jsm from WebIDE instead of SDK equivalent.

https://reviewboard.mozilla.org/r/160548/#review166306

Great, thanks for working on this! :)

::: devtools/client/webide/modules/simulator-process.js:92
(Diff revision 1)
>  
>      // Spawn a B2G instance.
> -    this.process = Subprocess.call({
> -      command: b2g,
> +    Subprocess.call({
> +      command: b2g.path,
>        arguments: this.args,
> +      environmentAppend: true,

Hmm, do we really want this `evironmentAppend` mode?  Seems like a behavior change, and this file carefully gardens the environment...
Attachment #8889522 - Flags: review?(jryans) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 12

2 years ago
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #8)
> Comment on attachment 8889523 [details]
> Bug 1353656 - Remove WebIDE build step for local apps.
> 
> https://reviewboard.mozilla.org/r/160550/#review166296
> 
> Thanks, looks good overall! :) Just a few more things to remove.

I removed this.

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #9)
>
> Hmm, do we really want this `evironmentAppend` mode?  Seems like a behavior
> change, and this file carefully gardens the environment...

No idea why, it looks like previous code was defining a precise environment,
but it doesn't work without environmentAppend... Simulator doesn't show up and you get a timeout.
(In reply to Alexandre Poirot [:ochameau] from comment #12)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #9)
> >
> > Hmm, do we really want this `evironmentAppend` mode?  Seems like a behavior
> > change, and this file carefully gardens the environment...
> 
> No idea why, it looks like previous code was defining a precise environment,
> but it doesn't work without environmentAppend... Simulator doesn't show up
> and you get a timeout.

Ah well... sounds like you are doing the right thing then!

Comment 14

2 years ago
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/42801a80bc6c
Use SubProcess.jsm from WebIDE instead of SDK equivalent. r=jryans
https://hg.mozilla.org/integration/autoland/rev/df6fd4e616d9
Remove WebIDE build step for local apps. r=jryans

Comment 15

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/42801a80bc6c
https://hg.mozilla.org/mozilla-central/rev/df6fd4e616d9
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED

Updated

10 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.