Remove dependence on PSUtil #182
Conversation
techalchemy
force-pushed
the
feature/remove-psutil
branch
from
c7a8e5c
to
631041d
Compare
|
Wow, thank you... looks good (even if it feels icky to add such low level code in our project) Why was pipenv vendoring psutil? I understand the the current integration tests cover decently the functionality you implemented, though adding some more specific tests would be better ( I don't want to block the PR on this, though) I'll either merge this tonight, or fork this out in a separate project, release that on Pypi and add that as Pew's dependency (I'd obviously add you as collaborator and maintainer, but I think you might prefer to be the main owner, since you wrote this yourself) |
|
We censored psutil to keep up with the strict pin to 5.3.1 which pew had. I initially cracked pew open to see if I could unpin it (seemed likely) and then just unvendor it, but basically I remembered there was a native way to crawl the process tree with ctypes. Honestly there isn’t much to test here, it’s just an explicit call to the windows API for the process list + an iteration over that list for details. Pew already tests whether the grandparent process call is correct which implicitly tests the process map (not sure how you’d test that anyway). I don’t want to take too much credit, as I mentioned in the code it’s ported from a 2012 celery implementation which ported it from another library — all that is to say this code really won’t require much maintenance... I’m happy to debug or troubleshoot but at the end of the day it may look like a lot, but it’s not doing much. It’s quite small and it does one task (much more quickly than psutil) and uses APIs which haven’t changed in ages. I would probably say it doesn’t make sense to make a new project or anything, it literally only gets the grandparent process |
techalchemy
force-pushed
the
feature/remove-psutil
branch
from
5608d13
to
c53b5ec
Compare
|
We changed |
pew/_win_utils.py
Outdated
| pe = Process32First(h_process) | ||
| while pe: | ||
| pids[pe.th32ProcessID] = { | ||
| 'executable': str(pe.szExeFile.decode('utf-8')) |
There was a problem hiding this comment.
rather than str, I'd use pew._utils.to_unicode
(which relies on the system locale, I'm not sure which way would be the best between that and assuming utf-8... I'd lean to assume utf-8, but
1 - we're not on a unix-like, windows traditionally uses utf-16
2 - those bytes are not under our control)
There was a problem hiding this comment.
Any code that runs here will only run on windows. I think I ran into a bug trying to coax this which is how it wound up like this, which is admittedly not pretty. I’ll follow up when I’m at a pc but it seems reasonable. I have a sense that I had an issue representing this as Unicode...
pew/_win_utils.py
Outdated
| def _get_executable(process_dict): | ||
| if hasattr(process_dict, 'keys'): | ||
| executable = process_dict.get('executable') | ||
| if isinstance(executable, six.string_types): |
There was a problem hiding this comment.
We fill in that dictionary ourselves, this cannot be anything else but a string (and ideally it should just be a unicode string) right?
There was a problem hiding this comment.
I am again stricken with the sense that we made such an assumption and were wrong. Will find out
|
Now there's only a Windows failure, apparently you'd need to add btw, why did you want to evaluate WORKON_HOME lazily? (I reckon that's one less global, but that global is set once at the start of the program and then never modified, so I wouldn't be concerned about it) |
|
I actually have no idea, @uranusjr introduced that as a bugfix but I can’t remember why. I just wanted to make sure to keep this up to date. Will follow up on those comments |
|
OK-
|
- Use native ctypes for parent process traversal - Addresses issues raised in pypa/pipenv#1587 and microsoft/vscode-python#978 - Improves speed on windows - Allows pipenv to remove vendored psutil which sometimes fails to find linked python dlls
- Change function to `get_shell` - Allow `get_shell` to take `max_depth` as an arg
- More robust native cmd.exe and powershell support using native windows C extensions - Error handling when exiting the shell - Lazy workon_home resolution - Improved error handling when crawling the process tree
Signed-off-by: Dan Ryan <dan@danryan.co>
techalchemy
force-pushed
the
feature/remove-psutil
branch
3 times, most recently
from
495f684
to
d4d974f
Compare
- New shell finding method finds accurate shell process irrespective of hierarchical depth - Test update accounts for this change Signed-off-by: Dan Ryan <dan@danryan.co>
techalchemy
force-pushed
the
feature/remove-psutil
branch
8 times, most recently
from
a171cd0
to
0cb04a3
Compare
Signed-off-by: Dan Ryan <dan@danryan.co>
- Account for bytes and bytearrays - Decode those objects before stringifying them Signed-off-by: Dan Ryan <dan@danryan.co>
techalchemy
force-pushed
the
feature/remove-psutil
branch
from
0cb04a3
to
6a4baaa
Compare
techalchemy
force-pushed
the
feature/remove-psutil
branch
2 times, most recently
from
20f2f4a
to
0b98591
Compare
|
@berdario sorry this took so long to get working properly but I think it's basically set, the only failures now seem to be related to the fact that Nix tests are using both pip 9 and pip 10 simultaneously somehow (I know nothing about nix so can't really offer much insight) |
|
@techalchemy Are you still interested in this PR? If so, could you rebase it on latest master? We've just committed a change (#196) to unpin |
|
Pipenv ended up pulling this implementation out to a separate package. It should be quite simple to port shutil usage to use that instead. |
|
@uranusjr If you'd prefer to rework this PR to use that project as an external dependency, that seems fine to me. |
|
I’d definitely recommend that. Happy to take care of it. |
|
I will likely be occupied this weekend, so take it on if you can make time. I can probably do this next week if you couldn’t. |
and be able to specify the path to pipenv microsoft/vscode-python#978
linked python dlls