Skip to content
This repository has been archived by the owner on Nov 19, 2024. It is now read-only.

Use the debug adapter from cdt-gdb-vscode/cdt-gdb-adapter #10

Merged
merged 1 commit into from
Jan 9, 2019

Conversation

simark
Copy link

@simark simark commented Dec 13, 2018

This patch swaps the debug adapter we use from code-debug to
cdt-gdb-vscode. We depend on a Theia-specific build, because the
cdt-gdb-vscode package needs some tweak to be import-able as a node
package (which is preferable, IMO, to downloading in a (post)install
script).

Signed-off-by: Simon Marchi simon.marchi@ericsson.com

@simark
Copy link
Author

simark commented Dec 13, 2018

@marechal-p @thegecko

The @theia/cdt-gdb-vscode this patch depends on is at

https://github.com/theia-ide/cdt-gdb-vscode

I tested using a local npm registry (using verdaccio), but using yarn link would probably work too. If you agree with the approach, I can see with @marcdumais-work so we can publish @theia/cdt-gdb-vscode to the global npm registry.

@thegecko
Copy link
Member

Hmm, I'm not sure how I feel about patching adapters specifically for Theia. It introduces overhead to continually maintain a fork (which may fall behind the original without committed support).

I'd also like to see all adapters imported in the same way (whether as node modules, downloaded, git modules or otherwise) to promote consistency.

@simark
Copy link
Author

simark commented Dec 13, 2018

Ok. Since there are no releases of cdt-gdb-vscode, where should we host it?

@simark
Copy link
Author

simark commented Dec 13, 2018

Ok, I've built the adapter and hosted it in the https://github.com/theia-ide/cdt-gdb-vscode repo (I find it preferable to have it under the theia-ide organization rather than under a personal account). I have updated the patch to make use of it.

This patch swaps the debug adapter we use from code-debug to
cdt-gdb-vscode.  The adapter was built using

    npm install && npm run build && npm pack

and hosted as a release at [1].

The file cpp-debug-configuration-schema.json seems unused, so I've
removed it.  The schema for configurations comes from the package.json
of the debug adapter/extension anyway.

[1] https://github.com/theia-ide/cdt-gdb-vscode

Signed-off-by: Simon Marchi <simon.marchi@ericsson.com>
@thegecko
Copy link
Member

Ok. Since there are no releases of cdt-gdb-vscode, where should we host it?

That's a very good question. It should be hosted along with the source IMO. So it depends whether we are going to work on a fork or persuade the original authors to host it.

Ok, I've built the adapter and hosted it in the https://github.com/theia-ide/cdt-gdb-vscode repo

Having it on cdt-gdb-vscode is probably OK for now and allows us to work on it separately.

Copy link
Member

@thegecko thegecko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good to me. Not tested however.

@marcdumais-work
Copy link
Contributor

Ok, I've built the adapter and hosted it in the https://github.com/theia-ide/cdt-gdb-vscode repo (I find it preferable to have it under the theia-ide organization rather than under a personal account). I have updated the patch to make use of it.

If we eventually use Travis CI in this repo, I think the adapter download during CI will then be subject to the GitHub API rate limit, like we have for ripgrep in the main repo: eclipse-theia/theia#3787
I guess we could probably use something else than Travis. Thoughts?

@thegecko
Copy link
Member

You may be able to add a GH token to avoid limits. Other CI systems allow you to cache downloads which don't change, too (e.g. CircleCI).

Copy link
Member

@paul-marechal paul-marechal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@paul-marechal paul-marechal merged commit 28ebb45 into eclipse-theia:master Jan 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants