Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allows launch config to override launch target connection type #625

Merged
merged 1 commit into from
Dec 27, 2023

Conversation

betamaxbandit
Copy link
Contributor

When launching a GDB debug session using the Launch Bar and a Launch Target, now the launch takes into account a REMOTE_TCP value in the launch configuration before considering the type of the Launch Target.

Improves the change submitted in:
Bug 547881 - Allow targets to set ATTR_REMOTE_TCP

Fixes issue #622

@betamaxbandit
Copy link
Contributor Author

Hi @jonahgraham ,

I'm not sure how to test this!
Either a manual UI test could be done or a JUnit added.
Manual test: I tried a manual test but didn't get past trying to launch it [1]

JUnit: I've investigated this a bit and it looks doable, but not sure whether it's worth it [2]

Do you have any advice please?

[1] Manual test
Using CDT New C/C++ Project wizard, created a CMake Project.

In Preferences > Core Build Toolchains, the auto detected GCC is shown:
Core Build Toolchains
Available Toolchains
Type Name OS Arch
GCC win32 x86_64: C:\msys64\mingw64\bin\gcc.exe win32 x86_64

In Preferences > CMake, I downloaded an example CMake toolchain file (MSYS2.cmake) from internet and added it to the toolchain above.

CMake
Toolchain Files
Toolchain File Toolchain
MSYS2.cmake win32 x86_64: C:\msys64\mingw64\bin\gcc.exe

I created a new Generic Target launch target:
Name: gen
Operating System: win32
CPU Architecture: x86_64

I built the project. An executable was successfully generated.

Click Launch Bar Run (Launch in Run mode) button
Console: No action specified. Skipping.

[2] Junit
I noticed org.eclipse.cdt.tests.dsf.gdb.launching.TestLaunchDelegate. I can extend this class and override

GdbLaunchDelegate.getLaunch(ILaunchConfiguration, String)

And then set the launch target using GdbLaunch.setLaunchTarget(ILaunchTarget)

Then setup a launch configuration which includes IGDBLaunchConfigurationConstants.ATTR_REMOTE_TCP attr set to true.

Then launch and inspect whether the FinalLaunchSequence.stepRemoteConnection(RequestMonitor) indeed attempts a isTcpConnection rather than a serial connect. [Not sure how to detect this]

Copy link
Member

@jonahgraham jonahgraham left a comment

Choose a reason for hiding this comment

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

This change lgtm - I can merge it after I branch which is in the new few days.

My feeling for a test here is that a lot of scaffolding is probably needed to automate this test. I suspect the dev effort required to do it for this change doesn't warrant it. But I think we should make sure that at least a manual test runs, even if it is just for the default case where the attribute is not in the launch already. From your description it sounds like this has exposed other problems with the cmake integration. Some of them may already be reported in this query

Copy link
Member

@jonahgraham jonahgraham left a comment

Choose a reason for hiding this comment

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

@betamaxbandit and I had a call about this and we verified that the behavior wasn't regressing as requested. So this is good to go. I am going to rebase the change to make sure there are no conflicts/issues

When launching a GDB debug session using the Launch Bar and a Launch
Target, now the launch takes into account a REMOTE_TCP value in the
launch configuration before considering the type of the Launch Target.

Improves the change submitted in:
Bug 547881 - Allow targets to set ATTR_REMOTE_TCP

Fixes issue eclipse-cdt#622
@jonahgraham jonahgraham merged commit 959c027 into eclipse-cdt:main Dec 27, 2023
5 checks passed
@jonahgraham jonahgraham added this to the 11.5.0 milestone Dec 27, 2023
@jonahgraham
Copy link
Member

Thanks @betamaxbandit for this fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants