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

fix: correctly parse ps output on linux #1389

Merged
merged 6 commits into from
Sep 26, 2023

Conversation

owl-from-hogvarts
Copy link
Contributor

@owl-from-hogvarts owl-from-hogvarts commented Sep 6, 2023

Fixes #1388

The issue was that ps's output was parsed by hardcoded indexes. The parsing function had been written three years ago. Obviously, ps output format has slightly changed. So I have written regex to parse it more in more flexible fashion. Tested on mine machine. Works grate

@owl-from-hogvarts
Copy link
Contributor Author

@microsoft-github-policy-service agree

command and arguments are separated by space.
previously, if space wasn't found, function would have skipped
this process.

Now, processes processes without args yields arguments as empty string
@owl-from-hogvarts
Copy link
Contributor Author

Notice, that checks for existence of shortName and fullCommand are not required. Whole regex does not match if any of these does not.

@testforstephen
Copy link
Contributor

@owl-from-hogvarts thanks for your contribution. Since the processTree.ts implementation is originally copied from https://github.com/microsoft/vscode-node-debug/blob/main/src/node/extension/processTree.ts, I'm wondering if updating to the latest version from that source would slove your issue?

@owl-from-hogvarts
Copy link
Contributor Author

owl-from-hogvarts commented Sep 18, 2023

@testforstephen Latest version of original file works. But notice, that it uses deprecated str.substr method.

@testforstephen
Copy link
Contributor

@testforstephen Latest version of original file works. But notice, that it uses deprecated str.substr method.

We can replace the deprecated substr with str.substring(...).

@owl-from-hogvarts
Copy link
Contributor Author

Seems meaningful. So, it is now up to you to do code review and choose implementation 😉

@owl-from-hogvarts
Copy link
Contributor Author

Hey! Who's willing to review this PR?) Two weeks have passed. This still need merge. Just a friendly reminder 😉

src/processTree.ts Outdated Show resolved Hide resolved
don't use deprecated `substr` method
@owl-from-hogvarts
Copy link
Contributor Author

The file now uses original approach. Code readability enhanced

src/processTree.ts Show resolved Hide resolved
src/processTree.ts Outdated Show resolved Hide resolved
src/processTree.ts Show resolved Hide resolved
I can't imagine how this could happen, but since old code
does this, why not 🤷‍♂️
Copy link
Contributor

@testforstephen testforstephen left a comment

Choose a reason for hiding this comment

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

LGTM. thanks for contribution.

@testforstephen testforstephen merged commit ac2cfac into microsoft:main Sep 26, 2023
4 checks passed
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.

[BUG]: Cannot find any debuggable program
2 participants