Skip to content

gdbserial/gdbserver: Dynamically resolve debugserver binary#1994

Merged
derekparker merged 4 commits intogo-delve:masterfrom
xyzst:master
Apr 9, 2020
Merged

gdbserial/gdbserver: Dynamically resolve debugserver binary#1994
derekparker merged 4 commits intogo-delve:masterfrom
xyzst:master

Conversation

@xyzst
Copy link
Copy Markdown
Contributor

@xyzst xyzst commented Apr 6, 2020

Instead of hardcoding the absolute path to the Command Line
Tools (CLT) binary, will attempt to resolve the path at the
$PATH, or at the Xcode bundle. If none are available, will
fallback to the default CLT location.

Fixes #986

Instead of hardcoding the absolute path to the Command Line
Tools (CLT) binary, will attempt to resolve the path at the
$PATH, or at the Xcode bundle. If none are available, will
fallback to the default CLT location.

Fixes #986
@xyzst
Copy link
Copy Markdown
Contributor Author

xyzst commented Apr 6, 2020

I understand that unit tests are required before it's merged but I want to make sure I am on the right track before I am committed.

If I am on the right track, what's the best approach for testing this type of change in go? My plan is to mock the fs by creating temporary files and directories at test runtime with the os package functions.

@aarzilli
Copy link
Copy Markdown
Member

aarzilli commented Apr 6, 2020

I think it would be better if this was done in LLDBLaunch and LLDBAttach rather than in a initializer. Also you should add a log line so that we can find out which program ends up being launched.

@xyzst
Copy link
Copy Markdown
Contributor Author

xyzst commented Apr 6, 2020

@aarzilli thanks for reviewing my PR.

Is this more of a style concern, or is there a behavioral difference when calling the helper function eagerly vs lazily? I am new to go, so I would appreciate an explanation or a pointer to related documentation.

As for the logging, I will look into it after I finish up my daily work 😃

xyzst added 2 commits April 6, 2020 17:28
Add logging to capture the executable and associated arguments used
in LLDBLaunch and LLDBAttach

Related to #986
Define unit tests for helper function. Setup each test to temporarily make
PATH variable, and file system changes, and subsequently revert them.

Related to #986
@aarzilli
Copy link
Copy Markdown
Member

aarzilli commented Apr 7, 2020

Is this more of a style concern, or is there a behavioral difference when calling the helper function eagerly vs lazily?

It's just going to add to the load time for delve, even if it doesn't get used at all.

The test doesn't work because it doesn't have permission to go write in system folders, I don't think we need a test for this.

@xyzst
Copy link
Copy Markdown
Contributor Author

xyzst commented Apr 7, 2020

@aarzilli thanks for the explanation. Entirely reasonable, will keep that in mind going forward.

Will update the PR after work hours and remove the tests.

Lazily obtain absolute path to avoid increasing load times.

Remove flaky tests.

Related to #986
Copy link
Copy Markdown
Member

@aarzilli aarzilli left a comment

Choose a reason for hiding this comment

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

LGTM

@derekparker derekparker merged commit bc30b53 into go-delve:master Apr 9, 2020
RenovZ pushed a commit to RenovZ/delve that referenced this pull request Mar 25, 2022
…#1994)

* gdbserial/gdbserver: Dynamically resolve debugserver binary

Instead of hardcoding the absolute path to the Command Line
Tools (CLT) binary, will attempt to resolve the path at the
$PATH, or at the Xcode bundle. If none are available, will
fallback to the default CLT location.

Fixes go-delve#986

* gdbserial/gdbserver: Log outgoing executed commands

Add logging to capture the executable and associated arguments used
in LLDBLaunch and LLDBAttach

Related to go-delve#986

* gdbserial/gdbserver: Add unit tests for helper function

Define unit tests for helper function. Setup each test to temporarily make
PATH variable, and file system changes, and subsequently revert them.

Related to go-delve#986

* gdbserial/gdbserver: Lazily load function

Lazily obtain absolute path to avoid increasing load times.

Remove flaky tests.

Related to go-delve#986
abner-chenc pushed a commit to loongson/delve that referenced this pull request Mar 1, 2024
…#1994)

* gdbserial/gdbserver: Dynamically resolve debugserver binary

Instead of hardcoding the absolute path to the Command Line
Tools (CLT) binary, will attempt to resolve the path at the
$PATH, or at the Xcode bundle. If none are available, will
fallback to the default CLT location.

Fixes go-delve#986

* gdbserial/gdbserver: Log outgoing executed commands

Add logging to capture the executable and associated arguments used
in LLDBLaunch and LLDBAttach

Related to go-delve#986

* gdbserial/gdbserver: Add unit tests for helper function

Define unit tests for helper function. Setup each test to temporarily make
PATH variable, and file system changes, and subsequently revert them.

Related to go-delve#986

* gdbserial/gdbserver: Lazily load function

Lazily obtain absolute path to avoid increasing load times.

Remove flaky tests.

Related to go-delve#986
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.

lldb-server needs to be installed in $PATH

3 participants