[process][posix] Realign process.Name() with python psutil to return same value on python3 scripts processes #1488
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
These tests can't fail more than their invidiual counterparts and produce an incredibly verbose output
Lomanic
force-pushed
the
issue1485
branch
2 times, most recently
from
056d177
to
2b6de69
Compare
shirou
approved these changes
Jul 8, 2023
There was a problem hiding this comment.
Awesome! I confirmed it works on my FreeBSD 12.4-RELEASE-p1.
% go test -run Test_Process_Name_Against_Python -v
=== RUN Test_Process_Name_Against_Python
process_test.go:435: pyName python3.9
--- PASS: Test_Process_Name_Against_Python (0.16s)
PASS
ok github.com/shirou/gopsutil/v3/process 0.166s
And I am very impressed with the testing idea in this PR, which is to compare directly with psutil results. Perhaps we should extend this idea to other tests.
…same value on python3 scripts processes e2c79a1 started to blindly set the process name to the full path (instead of the basename) of the cmdline exectuable if the process name from the process comm was truncated on linux. Python psutil never did that, and this is just wrong for python (or any executable interpreted script) where the process name is not the interpreter binary but the script itself. A new test to check process name value against psutil value is added here, which would hopefully catch any potential future changes in psutil. Reverts shirou#542 Fixes shirou#1485
|
Thanks for the kind words and testing on freebsd @shirou, I was only able to test on linux. Interestingly your test shows that process comm (name) is set to the interpreter binary basename on freebsd as opposed to the script basename on linux. Is it OK to merge or do you think we should test on more platforms?
Indeed. We could have automated tests against python psutil on various platfoms, set up on CI even. |
|
merged. Thank you so much! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
e2c79a1 started to blindly set the process name to the full path (instead of the basename) of the cmdline exectuable
if the process name from the process comm was truncated on linux. Python psutil never did that, and this is just wrong
for python (or any executable interpreted script) where the process name is not the interpreter binary but the script
itself.
A new test to check process name value against psutil value is added here, which would hopefully catch any potential
future changes in psutil.
Reverts #542
Fixes #1485