xmake-io / xmake-vscode

🍩 A XMake integration in Visual Studio Code
https://xmake.io
Apache License 2.0
229 stars 54 forks source link

Debug refactor #151

Closed A2va closed 1 year ago

A2va commented 1 year ago

This PR refactor the debugging part of this extension. It adds an xmake debug type configuration, so you can have a configuration like this:

{
    "configurations": [
    {
       "name": "XMake Debug",
        "type": "xmake",
        "request": "launch",
        "target": "example",
        "args":["-args"],
        "stopAtEntry": true
    }
  ]
}

Changelog

Config changes

waruqi commented 1 year ago

I'm glad you're contributing to xmake, but I still don't see what this patch actually improves. And it breaks a lot of existing configurations, which may affect the experience of many users now.

Remove runningTargetsArguments, debuggingTargetsArguments and customDebugConfig Rename debugConfigType to debuggerBackend

We should not remove them. Even if there are new configuration interfaces, they should be compatible with the existing old configuration. Otherwise a large number of users will be affected.

It adds an xmake debug type configuration, so you can have a configuration like this:

customDebugConfig does the same thing, so why add an extra configuration?

A2va commented 1 year ago

We should not remove them. Even if there are new configuration interfaces, they should be compatible with the existing old configuration. Otherwise a large number of users will be affected.

I think it is sometimes necessary to make breaking changes, so as not to maintain the same interface twice.

A2va commented 1 year ago

customDebugConfig does the same thing, so why add an extra configuration?

You can have multiple configurations for one target, it's not a problem. With the old implementation, that was not possible.

waruqi commented 1 year ago

We should not remove them. Even if there are new configuration interfaces, they should be compatible with the existing old configuration. Otherwise a large number of users will be affected.

I think it is sometimes necessary to make breaking changes, so as not to maintain the same interface twice.

I don't think so, every pr should make minimal changes, not massive destructive ones.

Also, I don't see any points of improvement in the new configuration interface.

waruqi commented 1 year ago

and debugger does not work for me now.

A2va commented 1 year ago

Also, I don't see any points of improvement in the new configuration interface.

This is much more in line with the way other vscode debugging extensions work.

and debugger does not work for me now.

It's weird, I tested it on Windows with cpptools and codelldb and both of them worked.

waruqi commented 1 year ago

Also, now onRun has gone and loaded the debugger too, which it shouldn't have done. We just need to run xmake run, which will better support the runtime configuration in xmake.lua.

waruqi commented 1 year ago

This is much more in line with the way other vscode debugging extensions work.

But it doesn't work, and we shouldn't have to break existing configuration behaviour.

image image
waruqi commented 1 year ago

This is much more in line with the way other vscode debugging extensions work.

I accept patches for native debugging support, but only if they don't break existing configurations and tasks, and provide the smallest changes possible.

Even extraneous code formatting and changes to generic implementations such as bytes2string. Not that they can't be changed completely, of course, but they should be done in a separate pr. The smaller the change, the easier it is to review and merge the patch.

If the patch is too large and heavily broken, it's hard for me to read it and accept the merge.

waruqi commented 1 year ago

It also introduces more bugs and doesn't make it easier for me to do testing.

A previous major explorer-related patch introduced a number of bugs that affected a lot of users and is still not fully stable.

A2va commented 1 year ago

I understand your concern, and I propose to leave this PR as it is. I will split the modifications into smaller commit/PR, and take into account your comments/criticism.

waruqi commented 1 year ago

sure, you can open a new pr and provide a smaller patch.