refactor(GUI): remove the intermediate child writer proxy process #1910
Conversation
|
I tested this on macOS and GNU/Linux, both locally and packaged, and I went through all the flashing/elevation related items in the manual testing document on macOS. |
| @@ -222,8 +222,7 @@ rules: | |||
| - error | |||
| - max: 1 | |||
| multiline-ternary: | |||
| - error | |||
| - never | |||
| - off | |||
There was a problem hiding this comment.
If you agree, of course :) Ternary operations are very handy, but the lines can get long very quickly,
There was a problem hiding this comment.
Wait, does this enable the use of multiline ternaries? Hell yes! 👍
| @@ -27,7 +27,7 @@ | |||
| // an older equivalent of `ELECTRON_RUN_AS_NODE` that still gets set when | |||
| // using `child_process.fork()`. | |||
| if (process.env.ELECTRON_RUN_AS_NODE || process.env.ATOM_SHELL_INTERNAL_RUN_AS_NODE) { | |||
| require('./cli/etcher') | |||
| require('./gui/modules/child-writer') | |||
There was a problem hiding this comment.
You call its entry point directly (ie node lib/cli/etcher.js)
| log(`Umount on success: ${unmountOnSuccess}`) | ||
| log(`Validate on success: ${validateWriteOnSuccess}`) | ||
|
|
||
| writer.writeImage(process.env.OPTION_IMAGE, process.env.OPTION_DEVICE, { |
There was a problem hiding this comment.
Should we pass this over IPC instead of through env vars? If we want to add persistence and stopping it might make more sense.
There was a problem hiding this comment.
Yeah, that's next step (otherwise this PR will keep growing). I want to get most of this merged, and will send and follow up PR that elevates on long-living process at startup that does scanning and flashing using IPC commands.
jviotti
force-pushed
the
remove-child-writer
branch
from
e0a8f4c
to
012125a
Compare
|
Ping @jhermsmeier @Shou @lurch |
There was a problem hiding this comment.
@jviotti, this is not sadly working for me on Windows. Looks like a combination of an issue with the elevation & the child exiting unexpectedly (prob. due to lack of permissions)
jviotti
force-pushed
the
remove-child-writer
branch
from
012125a
to
0708abb
Compare
|
… On Wed, Jan 03, 2018 at 09:26:53AM -0800, Jonas Hermsmeier wrote:
jhermsmeier requested changes on this pull request.
@jviotti, this is not sadly working for me on Windows. Looks like a combination of an issue with the elevation & the child exiting unexpectedly (prob. due to lack of permissions)
--
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#1910 (review)
--
Juan Cruz Viotti
Software Engineer
|
Etcher currently elevates a child writer proxy that itself spawns the Etcher CLI in robot mode, parses the output, and proxies those messages to the GUI application over IPC. After these set of changes, Etcher elevates a single child writer process that directly communicates back with the GUI using IPC. The main purpose behind these changes is to simplify the overall architecture and fix various issues caused by the current complex child process tree. Here's a summary of the changes: - Stop wrapping the Etcher CLI to perform writing - Remove the robot option from the Etcher CLI (along with related documentation) - Elevate a new `child-write.js` standalone executable - Move the relevant bits of `lib/child-writer` to the `image-writer` GUI module - Remove the `lib/child-writer` directory - Add a new "Child died unexpectedly" Mixpanel event - Floor state percentage in the flash state model The above changes made is possible to tackle all the remaining issues where the writer process would remain alive even if the parent died. Change-Type: patch Changelog-Entry: Ensure the writer process dies when the GUI application is killed. See: #1873 See: #1843 Signed-off-by: Juan Cruz Viotti <jv@jviotti.com>
jviotti
force-pushed
the
remove-child-writer
branch
from
0708abb
to
c48b176
Compare
|
Rebased! It should work fine now. |
| if (!flashResults.cancelled && !flashResults.bytesWritten) { | ||
| throw errors.createUserError({ | ||
| title: 'The writer process ended unexpectedly', | ||
| description: 'Please try again, and contact the Etcher team if the problem persists', |
There was a problem hiding this comment.
Sorry if this is a stupid question, but why does the identical string appear both here and in lib/shared/messages.js ?
|
I guess |
Etcher currently elevates a child writer proxy that itself spawns the
Etcher CLI in robot mode, parses the output, and proxies those messages
to the GUI application over IPC.
After these set of changes, Etcher elevates a single child writer
process that directly communicates back with the GUI using IPC. The main
purpose behind these changes is to simplify the overall architecture and
fix various issues caused by the current complex child process tree.
Here's a summary of the changes:
documentation)
child-write.jsstandalone executablelib/child-writerto theimage-writerGUImodule
lib/child-writerdirectoryThe above changes made is possible to tackle all the remaining issues
where the writer process would remain alive even if the parent died.
Change-Type: patch
Changelog-Entry: Ensure the writer process dies when the GUI application is killed.
See: #1873
See: #1843
Signed-off-by: Juan Cruz Viotti jv@jviotti.com