Syskit integration

[doudou]

Depends on:

• [x] https://github.com/rock-core/tools-syskit/pull/150
• [x] https://github.com/rock-core/tools-syskit/pull/152

This is the first step towards integrating Syskit as the runtime backend for the extension.

It implements the general infrastructure, discovers orogen deployments and resolves debug configurations using syskit.

[doudou]

@g-arjones this is just the beginning, in case you already have comments

[doudou]

Long term, when we get to getting rid of the manual package type selection, I'd move the Package resolution into Workspace or Workspaces. It seems to be the better place. But not for now ;-)

[g-arjones]

Cool.. No comments as of now. I was going to refactor Autoproj.Workspaces to add the events myself... Luckily, I didn't. 👍

[doudou]

Updated after the merge of #39.

I want to point out the change in the assertThrowsAsync helper (https://github.com/rock-core/vscode-rock/pull/38/commits/52e25402d285dc7ecfc201be90e0e6530bbb12ac), for future code ;-)

[g-arjones]

I saw that.. Looks better.

[doudou]

Hey ... I'm finally getting somewhere. The current state basically provides the same functionality than you had, but using the new Syskit/Roby REST API instead of custom ruby scripts.

Depends on:

• [x] rock-core/tools-syskit#150

There's a few issues still that I want to fix before we merge this:

• [x] fix current tests
• [x] complete the test suite
• [x] fix syskit instances ending up lying around
• [x] show the output of the syskit instance in the tasks output
• [ ] start the Syskit instance at workspace creation (for more reactivity) if the user sets a configuration (false by default)
• [x] start the Syskit instance on a random port / use a Unix socket
• [x] reset the bundle's environment variables

Apart from the comments you probably will have, that would be good enough to release 0.1 for me. The rest (attaching/debugging syskit-started deployments, integration vscode-started deployments in running Syskit systems) would come later.

[g-arjones]

It will take sometime to review this (I will also have a look into the other PRs)..

FYI: to add new dependencies in package.json, one should use npm install <package> rather than add it manually and then npm install. This way package-lock.json gets updated as well

[doudou]

It will take sometime to review this (I will also have a look into the other PRs)..

Was expecting as much. The "bottom" PRs (orogen, orocos.rb and Roby) are independent of each other.

This way package-lock.json gets updated as well

I wasn't sure whether we wanted to commit package-lock.json. I never could make up my mind on this...

[g-arjones]

Is there another client for the rest api other than the vscode extension I can use to test? The extension keeps getting error 500 (Internal Error) from syskit but this is really hard to debug as there are no logs/output either from the extension itself or from syskit

[doudou]

but this is really hard to debug as there are no logs/output either from the extension itself or from syskit

I'm working on that ...

I usually use firefox and the RESTED extension

[doudou]

Also, the REST API has tests.

• in roby: test/interface/test_rest.rb
• in syskit: test/roby_app/test_rest_deployment_manager.rb and test/roby_app/test_rest_api.rb

[doudou]

The extension keeps getting error 500 (Internal Error) from syskit

Damn me. I really wanted to warn you about something and forgot: the 'task' entry in launch.json is renamed to 'deploy' (because it can be a deployment name). You must have a deployAs for plain orogen tasks (optional for deployments). This is probably the problem.

Syskit (actually, grape) should properly warn there, though. I have to test it out.

[doudou]

Something else: the syskit instances end up lying around :(. Just added it to the list of things that need to be fixed. Not ready to remove the wip label :P

[doudou]

Something else: the syskit instances end up lying around

If you have any clue: if I try to stop a Syskit instance that has been spawned by Syskit using syskit quit, it does something (the interface gets closed), but the process does not quit. If I start the same syskit instance in a console (under autoproj exec and all), it quits normally.

[g-arjones]

start the Syskit instance at workspace creation (for more reactivity)

I wouldn't like that.. Personally, I would dislike if the extension kept running syskit every time I open vscode just to edit some random markdown file.. The "lazy loading" approach seems better...

start the Syskit instance on a random port

How about communicating over a unix socket rather? This requires zero effort on syskit's/roby's side. Here, only syskit.callBase would have to change...

if I try to stop a Syskit instance that has been spawned by Syskit using syskit quit, it does something (the interface gets closed), but the process does not quit.

Assuming you meant "spawned by the extension", it seems to me that it is just not quitting (aka syskit quit is simply not called and I can still connect to the restful port). Maybe dispose() doesn't get called when vscode terminates? You could try to listen to the node's host exit event and quit syskit from there:

process.on('exit', function () {
    syskit_process.kill();
});

Still, there's no guarantee that it will always be called (i.e: when VSCode freezes or is killed). Could we have something like --rest-timeout in roby to make it stop itself when idle?

[g-arjones]

Forgot to mention: in order to make this work I had to bundle-sel <...>/rock-default-bundle

[doudou]

Forgot to mention: in order to make this work I had to bundle-sel <...>/rock-default-bundle

This is probably due to you pre-loading env.sh. I'm going for ripping Rock's bundle system out of Syskit, until then I'll reset whatever environment variable the bundles set.

[doudou]

I wouldn't like that.. Personally, I would dislike if the extension kept running syskit every time I open vscode just to edit some random markdown file.. The "lazy loading" approach seems better...

Fair enough. I'll hide it behind a configuration flag (for what it's worth, I locally had a typelib commit that made Syskit loading horribly slow, that's why I noticed. Fixed it, so maybe I won't see the problem anymore)

[g-arjones]

due to you pre-loading env.sh

I didn't do this.

until then I'll reset whatever environment variable the bundles set

I don't think it will make any difference since this makes autoproj exec set it again. Or doesn't it?

[doudou]

Thanks a lot for your comments, I'm integrating most of them. I'll have a look at the bundle stuff.

On the subject of typing, I'd rather avoid endless picking and bikeshedding on the subject. We are all compentent software engineer, and should understand that dynamic and static typing have both pros and cons. You're obviously consider that the cost/benefit analysis is in favor of static typing, I think of myself as much more "in the middle". Let's leave it at that, and not pick on each other's choice of where to add types and where not to (especially given that I do add explicit types in most places).

[doudou]

until then I'll reset whatever environment variable the bundles set

I don't think it will make any difference since this makes autoproj exec set it again. Or doesn't it?

As far as I can see, it doesn't. It is the ROCK_BUNDLE variable, and it's set only in the shell (look at the .bundle_env.sh file in the workspace root).

[g-arjones]

Thanks a lot for your comments

You're very welcome. Glad to help.

bikeshedding

New expression for me.. I am always learning something here :)

Let's leave it at that, and not pick on each other's choice of where to add types and where not to (especially given that I do add explicit types in most places).

You are absolutely right. I shouldn't be insisting on that and I may not have chosen my words properly. Apologies for that

[doudou]

You are absolutely right. I shouldn't be insisting on that and I may not have chosen my words properly. Apologies for that

No hard feelings ... I'm more often than not the one that insist on these kind of things.

[doudou]

due to you pre-loading env.sh

I didn't do this.

... I'm really at a loss to explain why you would have had the behavior you've had if not for ROCK_BUNDLE being defined within vscode. Could you try again ?

[g-arjones]

Could you try again ?

Definitely. It's been a while since I ran into this so I can't say for sure if I experienced that when running syskit through the shell (in which case it's quite possible that I've pre loaded env.sh) or if I let the extension do it (I'm pretty positive I didn't load env.sh before running vscode though). I will try to reproduce

[g-arjones]

Could you try again ?

Couldn't reproduce.. I guess I was probably running it from the terminal

[doudou]

I'm OK with the state it is, there's still the --rest-timeout thingy and the pre-starting of the syskit app, but those are things that can IMO be added later.

Let's test this for a few days, and then release 0.1 ?

[g-arjones]

those are things that can IMO be added later

I agree.

Let's test this for a few days, and then release 0.1 ?

Sounds good to me. I will try to rebase and finish #40 before then

A couple of things are not clear to me:

1. Is deployAs required or not?
2. You kept setupTask() and the setup ruby script (which is fine by me) but you removed start and confDir properties from the orogen debug provider definition in the manifest?

Other than that, this looks great. Awesome work... 👍

[doudou]

Sounds good to me. I will try to rebase and finish #40 before then

OK !

1. Is deployAs required or not?

Not always. If 'deploy' is a task model, it is required as the extension cannot know under which name you want to start the task. If 'deploy' is a deployment, it is optional. The REST API validates this, so the user should get an error.

2. You kept setupTask() and the setup ruby script (which is fine by me) but you removed start and confDir properties from the orogen debug provider definition in the manifest?

I meant to keep them, will fix package.json.

[doudou]

Fixed ... may I merge this ?

[g-arjones]

Yep.. Works nicely! 👍