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

Search debugger first in selected CBS toolchain. #1033

Merged
merged 1 commit into from
Jan 16, 2025

Conversation

ewaterlander
Copy link
Contributor

For Core Build System local debug target. If there is no absolute path set in the Debugger tab of the launch configuration, try to find the debugger first in the selected toolchain. If the debugger is not found in the toolchain, let GdbLaunch search in PATH.
If an absolute path is set, GdbLaunch will use that.

Fixes #1008

For Core Build System local debug target. If there is no absolute path
set in the Debugger tab of the launch configuration, try to find the
debugger first in the selected toolchain. If the debugger is not found
in the toolchain, let GdbLaunch search in PATH.
If an absolute path is set, GdbLaunch will use that.

Fixes eclipse-cdt#1008
@ewaterlander
Copy link
Contributor Author

Hi @jonahgraham and @betamaxbandit ,

Here is my fix for #1008

Copy link

github-actions bot commented Jan 16, 2025

Test Results

   590 files   -  11     590 suites   - 11   13m 8s ⏱️ +20s
10 079 tests  - 121  10 056 ✅  - 121  23 💤 ±0  0 ❌ ±0 
10 117 runs   - 121  10 094 ✅  - 121  23 💤 ±0  0 ❌ ±0 

Results for commit 4086e29. ± Comparison against base commit 751b031.

This pull request removes 121 tests.
org.eclipse.cdt.debug.core.memory.tests.PlainTextTransportTest ‑ transport0ffff
org.eclipse.cdt.debug.core.memory.tests.PlainTextTransportTest ‑ transport10000
org.eclipse.cdt.debug.core.memory.tests.PlainTextTransportTest ‑ transport10001
org.eclipse.cdt.debug.core.memory.tests.RAWBinaryTransportTest ‑ transport0ffff
org.eclipse.cdt.debug.core.memory.tests.RAWBinaryTransportTest ‑ transport10000
org.eclipse.cdt.debug.core.memory.tests.RAWBinaryTransportTest ‑ transport10001
org.eclipse.cdt.debug.core.memory.tests.SRecordTransportTest ‑ transport0ffff
org.eclipse.cdt.debug.core.memory.tests.SRecordTransportTest ‑ transport10000
org.eclipse.cdt.debug.core.memory.tests.SRecordTransportTest ‑ transport10001
org.eclipse.cdt.dsf.gdb.multicorevisualizer.ui.test.PersistentSettingsManagerTest ‑ testGlobalParamsWithMultipleInstances
…

♻️ This comment has been updated with latest results.

@jonahgraham
Copy link
Member

This pull request removes 121 tests.

Please see #1002 (comment) for discussion about this new feature that comments tests results. I don't know why less tests were run, but the change doesn't seem to be the cause.

@jonahgraham
Copy link
Member

jonahgraham commented Jan 16, 2025

This pull request removes 121 tests.

Please see #1002 (comment) for discussion about this new feature that comments tests results. I don't know why less tests were run, but the change doesn't seem to be the cause.

Never mind - the difference is explained in the log, the maven run did not complete. I have restarted it.

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 looks reasonable to me. @betamaxbandit can you confirm if this fixes the regression you identified in #1008?

Copy link
Contributor

@betamaxbandit betamaxbandit left a comment

Choose a reason for hiding this comment

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

LGTM.

@betamaxbandit
Copy link
Contributor

This looks reasonable to me. @betamaxbandit can you confirm if this fixes the regression you identified in #1008?

I haven't tested this.

@ewaterlander , did you have chance to test this fix on Windows?

@jonahgraham
Copy link
Member

@betamaxbandit and I did a screen share and tested this on Windows

@jonahgraham
Copy link
Member

Tests are failing, catastrophically. I don't understand why and can't see that it is related to this change. But I need to investigate a bit before I merge to make sure this isn't somehow exposing some weird bug.

@jonahgraham
Copy link
Member

Tests are failing, catastrophically. I don't understand why and can't see that it is related to this change. But I need to investigate a bit before I merge to make sure this isn't somehow exposing some weird bug.

#1035 has the same issue, so I am confident not related to this change now.

@jonahgraham jonahgraham merged commit c4f22cd into eclipse-cdt:main Jan 16, 2025
4 of 5 checks passed
@ewaterlander
Copy link
Contributor Author

@betamaxbandit and I did a screen share and tested this on Windows

I did test it, but on Linux. I could see that the debugger was taken from the toolchain.

@ewaterlander ewaterlander deleted the gdbpath branch January 17, 2025 15:14
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.

Cannot launch CMake debug session using Local launch target
3 participants