Remove ProcessTaskRunner.findCommand, introduce process "started" event #3843

simark · 2018-12-14T20:39:02Z

In the long term goal of handling properly shell tasks (allowing the
user to specify "abc && def" as the command), we need to get rid of the
findCommand method in ProcessTaskRunner. Otherwise, it will always
return that the binary "abc && def" can't be found. Even for non-shell
tasks, it doesn't seem ideal to implement ourselves the logic of looking
through PATH. It's better to leave that to the OS.

Instead, we want to rely on the underlying thing we use to run processes
(either node's child_process or node-pty) to return us an appropriate
error code.

With node's child_process, we can do it out-of-the-box. With node-pty,
we use our own fork available at [1] which adds the necessary
features. On error, the Process emits an onError event.

For the non-error case, we need to know when the process has been
successfully started, so we can inform the client that the task has been
successfully started. To do this, I added an 'onStart' event on
Process.

[1] https://github.com/theia-ide/node-pty/tree/theia

Change-Id: Ie2db8909610129842912d312872a48aef3de23a5
Signed-off-by: Simon Marchi [email protected]

simark · 2018-12-14T21:00:50Z

Not quite ready for review, I need to investigate the failing tests (unless you want to comment on the general approach).

paul-marechal

LGTM! But it seems like it broke the terminal tests.

packages/task/src/common/process/task-protocol.ts

paul-marechal · 2018-12-21T16:49:49Z

Still some errors on Windows:

  TerminalProcess
    1) test error on non existent path
    2) test error on trying to execute a directory
    √ test exit (141ms)
    √ test pipe stream (62ms)

akosyakov · 2019-01-03T07:48:41Z

Do we really want to support our version of node-pty? Is it temporary? Is there a PR against node-pty to introduce this feature? Do we need CQ for our version?

// cc @svenefftinge @marcdumais-work

simark · 2019-01-03T14:37:12Z

Do we really want to support our version of node-pty? Is it temporary? Is there a PR against node-pty to introduce this feature? Do we need CQ for our version?

Hopefully it is temporary. Once we confirmed that the approach works well, I would try to get the functionality merged upstream.

akosyakov · 2019-01-04T09:12:32Z

@simark just fyi: I usually test an idea early with upstream, i.e. open an issue or a PR with proposed approach and ask whether it is a right way doing it

simark · 2019-01-04T18:18:27Z

@simark just fyi: I usually test an idea early with upstream, i.e. open an issue or a PR with proposed approach and ask whether it is a right way doing it

Ah I just remembered: we (at Ericsson) don't have the approval to sign the Microsoft CLA, so we can't contribute to upstream node-pty... otherwise I would have opened a PR already.

@marcdumais-work could give more details.

simark · 2019-01-09T18:49:53Z

It looks like the only errors remaining on AppVeyor are the "standard" ones, so this PR is ready to be reviewed.

simark · 2019-01-21T20:30:19Z

Ping.

akosyakov · 2019-01-22T06:34:19Z

Ah I just remembered: we (at Ericsson) don't have the approval to sign the Microsoft CLA, so we can't contribute to upstream node-pty... otherwise I would have opened a PR already.

Opening a PR does not require signing anything. You can also then say the same on the PR and ask maintainers to land it as own.

simark · 2019-01-22T14:06:55Z

Opening a PR does not require signing anything. You can also then say the same on the PR and ask maintainers to land it as own.

I don't think that is how it works. Signing a CLA usually means "I confirm I wrote this code, I confirm I own the rights on it, I confirm I have the right to license it with license X". That is used to protect the project from merging code that shouldn't be merged. The Microsoft people can't just take my code and merge it themselves if I didn't sign the CLA, because that would infringe their own CLA (they don't own that code).

marcdumais-work · 2019-01-24T11:11:57Z

Signing a CLA usually means "I confirm I wrote this code, I confirm I own the rights on it, I confirm I have the right to license it with license X"

+1. This is very similar how we require that the Eclipse ECA be signed before we merge a contribution. True, one does not need to sign the CLA to submit a PR, but it's needed before it can be ("legally"/"safely") merged.

akosyakov · 2019-01-25T09:16:18Z

@simark @marcdumais-work Can they have a look at PR, say yes it is real issue, we cannot merge your PR, but we will fix it in our way without copying your code? The point is that patching some library without bringing an issue to original maintainers is not very sustainable. They can say as well it is not an issue, you should do in this way or it should be fixed differently. At least opening an issue and hearing opinion of maintainers would be good.

simark · 2019-01-25T18:56:33Z

@simark @marcdumais-work Can they have a look at PR, say yes it is real issue, we cannot merge your PR, but we will fix it in our way without copying your code? The point is that patching some library without bringing an issue to original maintainers is not very sustainable. They can say as well it is not an issue, you should do in this way or it should be fixed differently. At least opening an issue and hearing opinion of maintainers would be good.

Ok, I opened this:

microsoft/node-pty#265

In the long term goal of handling properly shell tasks (allowing the user to specify "abc && def" as the command), we need to get rid of the findCommand method in ProcessTaskRunner. Otherwise, it will always return that the binary "abc && def" can't be found. Even for non-shell tasks, it doesn't seem ideal to implement ourselves the logic of looking through PATH. It's better to leave that to the OS. Instead, we want to rely on the underlying thing we use to run processes (either node's child_process or node-pty) to return us an appropriate error code. With node's child_process, we can do it out-of-the-box. With node-pty, we use our own fork available at [1] which adds the necessary features. On error, the Process emits an onError event. For the non-error case, we need to know when the process has been successfully started, so we can inform the client that the task has been successfully started. To do this, I added an 'onStart' event on Process. [1] https://github.com/theia-ide/node-pty/tree/theia Change-Id: Ie2db8909610129842912d312872a48aef3de23a5 Signed-off-by: Simon Marchi <[email protected]>

simark · 2019-01-28T17:09:51Z

We will try to push again internally to get permission to contribute to node-pty. Can we merge this in the mean time?

simark · 2019-02-11T16:13:05Z

Ping.

paul-marechal

LGTM, does anyone see anything?

marcdumais-work · 2019-02-11T18:16:00Z

We will try to push again internally to get permission to contribute to node-pty. Can we merge this in the mean time?

I do not have good confidence that we will get internal permission to contribute this patch to the upstream project. But we should still proceed I think, even if it's not ideal. It's possible that someone will fix the upstream issue Simon opened, at which point we could switch back to the official node-pty.

Do we need CQ for our version?

Technically yes but practically we already have the "NPM production dependencies" CQ that's under review for another PR / branch; we would need to wait until it unblocks before registering a new one for this PR here. If we skip, the new dependency here will be looked-at as part of the next such CQ, which should get opened right after the current one is approved.

paul-marechal · 2019-02-11T21:19:02Z

@akosyakov do you have anything against this change?

I mean that we still cannot contribute to Microsoft, and no one wants to pick up @simark's patch to node-pty...

Development on node-pty is rather slow, and shouldn't conflict with what we added, so it would only take a few rebase here and there. I can do it, other members of our team can do it. It can be as easy as bringing up an issue in the tracker to ask for an uplift.

marcdumais-work · 2019-02-14T14:45:32Z

@marechal-p I think we can proceed - we will need this for tasks to be compatible with vscode.

paul-marechal

LGTM, works nicely!

simark · 2019-02-14T16:32:30Z

Thanks Paul!

simark · 2019-02-14T16:49:25Z

Oops, I should have done a manual rebase and re-test, this causes is a build error. I'll take a look.

paul-marechal reviewed Dec 14, 2018

View reviewed changes

packages/task/src/common/process/task-protocol.ts Show resolved Hide resolved

simark force-pushed the simark/process-start-event branch 3 times, most recently from a0d44a4 to 8bc7593 Compare December 17, 2018 21:30

elaihau force-pushed the simark/process-start-event branch 2 times, most recently from 08b3cf2 to b18d327 Compare January 3, 2019 20:13

elaihau force-pushed the simark/process-start-event branch 2 times, most recently from f9d843b to 1738cc8 Compare January 8, 2019 16:44

simark force-pushed the simark/process-start-event branch from 1738cc8 to 3a1026c Compare January 8, 2019 22:02

marcdumais-work requested a review from azatsarynnyy January 9, 2019 20:02

simark force-pushed the simark/process-start-event branch from 3a1026c to f6fdf27 Compare January 21, 2019 19:31

simark force-pushed the simark/process-start-event branch from f6fdf27 to b174794 Compare January 28, 2019 17:08

paul-marechal reviewed Feb 11, 2019

View reviewed changes

paul-marechal approved these changes Feb 14, 2019

View reviewed changes

simark merged commit f24de1c into master Feb 14, 2019

simark deleted the simark/process-start-event branch February 14, 2019 16:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove ProcessTaskRunner.findCommand, introduce process "started" event #3843

Remove ProcessTaskRunner.findCommand, introduce process "started" event #3843

simark commented Dec 14, 2018

simark commented Dec 14, 2018

paul-marechal left a comment •

edited

Loading

paul-marechal commented Dec 21, 2018 •

edited

Loading

akosyakov commented Jan 3, 2019

simark commented Jan 3, 2019

akosyakov commented Jan 4, 2019

simark commented Jan 4, 2019 •

edited

Loading

simark commented Jan 9, 2019

simark commented Jan 21, 2019

akosyakov commented Jan 22, 2019

simark commented Jan 22, 2019

marcdumais-work commented Jan 24, 2019

akosyakov commented Jan 25, 2019

simark commented Jan 25, 2019

simark commented Jan 28, 2019

simark commented Feb 11, 2019

paul-marechal left a comment

marcdumais-work commented Feb 11, 2019

paul-marechal commented Feb 11, 2019

marcdumais-work commented Feb 14, 2019

paul-marechal left a comment

simark commented Feb 14, 2019

simark commented Feb 14, 2019

Remove ProcessTaskRunner.findCommand, introduce process "started" event #3843

Remove ProcessTaskRunner.findCommand, introduce process "started" event #3843

Conversation

simark commented Dec 14, 2018

simark commented Dec 14, 2018

paul-marechal left a comment • edited Loading

Choose a reason for hiding this comment

paul-marechal commented Dec 21, 2018 • edited Loading

akosyakov commented Jan 3, 2019

simark commented Jan 3, 2019

akosyakov commented Jan 4, 2019

simark commented Jan 4, 2019 • edited Loading

simark commented Jan 9, 2019

simark commented Jan 21, 2019

akosyakov commented Jan 22, 2019

simark commented Jan 22, 2019

marcdumais-work commented Jan 24, 2019

akosyakov commented Jan 25, 2019

simark commented Jan 25, 2019

simark commented Jan 28, 2019

simark commented Feb 11, 2019

paul-marechal left a comment

Choose a reason for hiding this comment

marcdumais-work commented Feb 11, 2019

paul-marechal commented Feb 11, 2019

marcdumais-work commented Feb 14, 2019

paul-marechal left a comment

Choose a reason for hiding this comment

simark commented Feb 14, 2019

simark commented Feb 14, 2019

paul-marechal left a comment •

edited

Loading

paul-marechal commented Dec 21, 2018 •

edited

Loading

simark commented Jan 4, 2019 •

edited

Loading