Initial Prototype for Cygwin support module (take 2)

[embray]

This is an easier to digest alternative to #998 .

Rather than try to implement the kitchen sink, this provides a barely-more-than bare-minimum implementation for a Cygwin support module. Most top-level functions and Process methods either work, at least with the default arguments, or raise a NotImplementedError for now.

You can get a sense for what works and what doesn't work by looking at what tests are skipped.

The major bit of refactoring needed for this to work is the introduction of a psutil._pslinux_common module. It implements a Process base class and several functions that work on Linux and Cygwin, but mostly avoids very specialized Linux-specific functionality. In principle this could also be useful for implementing support on other platforms (if they exist?) that aim for partial Linux-compatibility without being fully Linux.

I think some version of this would be good to start with and is much better than nothing (it still does quite a bit!) and I will reintroduce additional features a bit at a time, making any additional prerequisite refactoring in the process.

TODO:

• Add CI on AppVeyor
• Add some mention of prototype support for Cygwin; for now I don't want to document in detail what does or does not work on Cygwin, but just mention that prototype support exists and is incomplete.

[embray]

I didn't mean to commit this bit yet. Going to remove it and rebase.

[giampaolo]

Good job @embray. I added some initial comments. On top of that, I still don't like the separate _linux_common.py file. Please keep the main logic in _pslinux.py and do this instead:

• from _pscygwin.py import _pslinux
• in _pslinux.py exclude only the Linux-specific code which causes error on import by using:

# _pslinux.py

if not CYGWIN:
    ...

• in _pscygwin.py do this depending on whether the implementation is the same or not. Example:

# _pscygwin.py

pids = _pslinux.pids  # same as Linux

def virtual_memory():  # not the same
    ...

• in the Process class just override the methods that differ:

# _pscygwin.py

class Process(_pslinux.Process):
    
    @memoize_when_activated
    def _parse_stat_file(self):
         ...

Also, generally speaking instead of merging a partial implementation I'm aiming at implementing everything, unless it's really difficult or not possible (in which case we'll document it). For the functionality which need to use MSDN APIs in C we can figure out how to refactor _psutil_windows.c. I have no idea how things are on that front yet but it seems we have two choices:

• use #ifdef CYGWIN to include / exclude things which do not compile
• extract only the things we need and put them in psutil/arch/windows/cygwin_common.c file

...but let's get to that later.

Also I noticed something interesting. ps aux on cygwin shows both a PID and a WINPID column. I don't know how it does that but it makes me think that maybe it would be nice to have a brand new, cygwin-only Process.winpid method (this can also be done later after this is merged).

I'd have further comments, like trying to reuse tests from test_linux.py but those can be adjusted later (and I can help you with that).

Extra: I notices something weird. While playing with this on Windows 10 + VirtualBox the VM suddenly crashed a couple of times. Scary... =)

[embray]

Also, generally speaking instead of merging a partial implementation I'm aiming at implementing everything, unless it's really difficult or not possible (in which case we'll document it).

That's going to require significant refactoring, which I do intend to do, but it's going to have to be done separately. The complexity will grow significantly à la #998, it will become difficult to keep up with changes in master, and just generally become an unmaintainable mess again. Better to start with something small, note in the documentation (as I have started to do) that it is partial and/or experimental, and then add on functionality as able. All the core functionality is already there.

[embray]

Also I noticed something interesting. ps aux on cygwin shows both a PID and a WINPID column. I don't know how it does that but it makes me think that maybe it would be nice to have a brand new, cygwin-only Process.winpid method (this can also be done later after this is merged).

My original implementation in #998 does have this, and I agree it's useful. I plan to add it back in later, as you say.

[embray]

I'm open to other ideas on how to handle this

In fact a better way would be to just go ahead and duplicate the common parts; it's unfortunate but also clear. I don't know how many other ways to explain that Cygwin is not Linux. It just takes inspiration from Linux in more areas than most other platforms being that it's the dominant UNIX-like OS. This is for practical reasons in porting software primarily written on Linux, it's necessary to copy some Linux-like functionality, in particular in the procfs implementation.

Other areas hew more closely to plain POSIX. There is no /sys FS on Cygwin. Etc., etc.

[embray]

sigh @giampaolo I owe you an apology. I went ahead and tried it your way just to see how bad it actually was, and this is all it took to at least make _pslinux.py importable on Cygwin. And all that matters as far as reusing bits of it, I think, is that it can be imported.

I tried to keep the changes from even being Cygwin-specific and this is all it took:

diff --git a/psutil/_pslinux.py b/psutil/_pslinux.py
index a1eaa3cd..f303ed53 100644
--- a/psutil/_pslinux.py
+++ b/psutil/_pslinux.py
@@ -23,7 +23,12 @@ from collections import namedtuple
 from . import _common
 from . import _psposix
 from . import _pslinux_common
-from . import _psutil_linux as cext
+
+try:
+    from . import _psutil_linux as cext
+except ImportError:
+    cext = object()
+
 from . import _psutil_posix as cext_posix
 from ._common import isfile_strict
 from ._common import memoize
@@ -102,11 +107,16 @@ LITTLE_ENDIAN = sys.byteorder == 'little'
 # * https://lkml.org/lkml/2015/8/17/234
 DISK_SECTOR_SIZE = 512

+if hasattr(socket, 'AF_PACKET'):
+    AF_PACKET = socket.AF_PACKET
+else:
+    AF_PACKET = -1
+
 if enum is None:
-    AF_LINK = socket.AF_PACKET
+    AF_LINK = AF_PACKET
 else:
     AddressFamily = enum.IntEnum('AddressFamily',
-                                 {'AF_LINK': int(socket.AF_PACKET)})
+                                 {'AF_LINK': int(AF_PACKET)})
     AF_LINK = AddressFamily.AF_LINK

 # ioprio_* constants http://linux.die.net/man/2/ioprio_get

On principle I still don't like it, per my comment that Cygwin as not Linux. But we can try things your way for now, given that at least practically speaking it took very little to at least make the _pslinux module importable. I'm just frustrated with this is all.

[embray]

How's this as a middle ground? I did away with the _pslinux_common module and instead have _pscygwin import _pslinux as you prefer.

However, it does not make _pscygwin.Process a subclass of _pslinux.Process. So doing would have a few problems: It gives the false impression that Cygwin is somehow a superset of Linux in terms of functionality, which is false. If anything, by that logic, it should be the other way around, with Linux inheriting from Cygwin, though that would come with its own problems. This is why I wanted a module of common functionality between them--they are in a sense divergent common ancestors of, say, an older version of Linux. As a technical problem, having _pscygwin.Process inherit _pslinux.Process would also mean it would inherit methods which just don't make sense on Cygwin (such as _read_smaps_file).

Instead, _pscgwin.Process stands on its own, but borrows judiciously from _pslinux.Process where it makes sense to. This appears to work quite well, modulo fine details.

[embray]

I don't like this. Didn't there used to be a psutil._exceptions module? Why did that go away? Having something like that would seem a bit less crazy than stuff like this...

[giampaolo]

Look @embray, I'm not willing to debate over design decisions anymore. I've basically been saying the same thing over the course of 2 years already:

• Prototype for Cygwin support module (not ready to merge) #998 (comment)
• Prototype for Cygwin support module (not ready to merge) #998 (comment)
• Prototype for Cygwin support module (not ready to merge) #998 (comment)
• Some refactoring of string encoding/decoding utilities #1342 (comment)
• Initial Prototype for Cygwin support module (take 2) #1483 (comment)

There must be some communication issue at play here because the same goes for the str/unicode/bytes utilities which I rejected in the other PR some time ago already, but that you decided to include into this one anyway (they could have made some sense if _pswindows.py was involved in this PR, but it's not).

Also, comments like these ones:

I don't know how many other ways to explain that Cygwin is not Linux

on principle I still don't like it

I strongly disagree with this

I say this based on significant experience with dealing with code just like this.

this is why I wanted a module of common functionality between them [again]

better to start with something small, note in the documentation (as I have started to do) that it is partial and/or experimental, and then add on functionality as able. All the core functionality is already there. [I already said I'm not willing to deliver a partial implementation]

we can try things your way for now

In principle this could also be useful for implementing support on other platforms (if they exist?) [they almost certainly don't + we shouldn't make assumptions for something which doesn't even exist]

[referred to code unrelated with this PR] I don't like this. Didn't there used to be a psutil._exceptions module? Why did that go away? Having something like that would seem a bit less crazy than stuff like this... [come on - are you serious?]

...are not constructive and keep the both of us in this never ending loop in which we just disagree with each other and nothing happens.

I believe we can easily move this forward. This is even a relatively easy task since most of the stuff is in psutil already, the differences re. the parsing of /proc on Linux vs. Cygwin are minimal, and for the rest we'll just reuse or adjust the existing _psutil_windows.c implementation. In summary, it's mostly just a matter of moving stuff around and refactor it, and I have a pretty clear idea on how to do it. But I think it's fair that you get rewarded for the fine work and passion you've put into this so far, not me, and that's why I'm trying to steer direction instead of doing it myself. But in order to do that I'm not willing to overturn a 10-years-old Linux implementation just to make room for Cygwin the way you intend to (which is what this PR does, it even mixes Linux and Cygwin logic (1, 2), which is a firm no-no). I fully understood your refactoring counter-proposal and I simply do not agree with it (sorry). Instead of bashing me and convince me to do otherwise just give me the benefit of the doubt (I do this for free). I hope this clarifies things for the better. Let me know if you're still willing to move this forward. I'll be happy if you will but please stop complaining.

[embray]

I'm a little confused by your last comment. I think you might have missed something since I initially replied: You're quoting partly from a comment I made out of frustration and later deleted (nevermind, apparently not), because I agree it wasn't productive.

I even went ahead and reworked things I think closer to your way and I'm not sure if you actually looked at the code because I did almost exactly as you asked.

I fully understood your refactoring counter-proposal and I simply do not agree with it

Like I said, I'm not sure what you mean here. Are you referring to the _pslinux_common module? Because I removed that as you asked.

Relatedly, you keep saying you disagree without giving any sort of justification. I've at least explained my reasoning and I'm not satisfied often that you have.

mixes Linux and Cygwin

I don't understand the second link. The logic there is nothing specifically to do with Cygwin, and is just reworked a bit to be a bit clearer and easier to extend if needed.

[referred to code unrelated with this PR]

I'm not sure what you mean by this. The comment you're mentioning here is exactly related to this PR, it's on code I added.

As for the bytes / str stuff as I explained last time we discussed it I wasn't satisfied with your reasoning behind rejecting the PR as-is because I foresaw future usage that you didn't. That's fair enough though, so I also explained that I would make a new PR explicitly incorporating it to make clearer the use I had in mind. So while you're still free to disagree with it, when you write

you decided to include into this one anyway

You make it sound like I'm trying to make an end run around this, when in fact I'm doing exactly what I (think) I previously communicated was my plan in the first place.

I believe we can easily move this forward.

Maybe but having done the actual work I don't think it's as easy as you think. You're welcome to either do it yourself or show some willingness to compromise as I have.

[embray]

BTW I'm sorry again for being cranky about this. I think it's mostly not your fault. But I do wish you'd explain your rationale behind things better, as then it'd be easier to find understanding and compromises. Instead I just get absolutisms like

[I already said I'm not willing to deliver a partial implementation]

[embray]

Actually that last comment leaves me especially confused because there are many features in this package that are platform-dependent and some that are available on some platforms if not others, so I really don't see what the problem is, especially if it's documented as provisional and or an initial version.

Furthermore, I had just assumed you would be happier with a smaller, more minimal PR that would be easier to review. Again, I communicated clearly that that was my plan and you approved it. If you didn't like that idea I wish you had said so.

Of course, I'm happy to build further on top of this one, though I figured it would be best to merge the initial work first (hence the care I took in stubbing out those tests that don't work yet; all other tests pass on my system). This way it can start being used and tested immediately in the development version while continuing the work of adding additional functionality. Adding more will take some time in refactoring the Windows code. I'm particularly wary of using Windows API at all because Cygwin is not Windows. Regardless it's possible in some cases as a workaround to missing Cygwin features. Just not always without danger. It will take some additional care and I fear tast stalling for too long will once again prevent getting anything at all done. Why let something that works mostly be the enemy of something that works 100%? I don't think this is an unreasonable question.

[giampaolo]

You're quoting partly from a comment I made out of frustration and later deleted (nevermind, apparently not), because I agree it wasn't productive.
I even went ahead and reworked things I think closer to your way and I'm not sure if you actually looked at the code because I did almost exactly as you asked.

My last comment was about the (not so nice) message you deleted, in which you kept insisting to use _linux_common.py, despite I previously said I disagree with it. I saw (later, since github apparently didn't email me about it) that you force-pushed a change which got rid of it and which deleted history. Because of that it took me a bit to figure out where _linux_common.py ended up. I noticed you removed it but I decided to make my point clear anyway, because you'll admit you've been quite insistent about this, and I was hoping we could stop the chit-chat and finally get this running.

I don't understand the second link. The logic there is nothing specifically to do with Cygwin

You're right, my bad.

I'm not sure what you mean by this. The comment you're mentioning here is exactly related to this PR

The reason this is the way it is is because of #1402, which is unrelated with Cygwin.

Maybe but having done the actual work I don't think it's as easy as you think. You're welcome to either do it yourself or show some willingness to compromise as I have.

I'm happy to compromise when I agree with your proposal and think it's reasonable, not because I necessarily have to.

Furthermore, I had just assumed you would be happier with a smaller, more minimal PR that would be easier to review. Again, I communicated clearly that that was my plan and you approved it. If you didn't like that idea I wish you had said so.

You misunderstood me. I proposed to proceed step by step, which is different than committing a draft PR. Compared to #998 this PR is surely a step in the right direction, but it's still a draft, useful to discuss how to proceed (together) and add functionality as we go.

Crucial things (because interconnected with other functionality) like Process create_time(), ppid(), pids(), memory_info(), name(), are either not implemented or skipped in unit-tests, meaning they're broken, and you ignored my initial inline comments to this PR. I have and would have more of those as we proceed, because this is how the code review workflow is supposed to work, and for good reasons, not because I don't like you or something.

There is no point in committing this as-is and it is inappropriate of you to tell me what should be part of a new release so insistently. My aim at delivering a close-to-complete implementation is because I believe it can be achieved. And even in case we decide to do otherwise (minimal functionality) as we go, it would at the very least pass the quality standards, meaning engaging in a code review first and progressively go from "draft" to "minimal" (but stable).

Instead I just get absolutisms like [I already said I'm not willing to deliver a partial implementation]
Why let something that works mostly be the enemy of something that works 100%? I don't think this is an unreasonable question.

The question is not unreasonable but if I already expressed my view on this twice in this very thread, why do you keep insisting?

As for the bytes / str stuff as I explained last time we discussed it I wasn't satisfied with your reasoning behind rejecting the PR as-is because I foresaw future usage that you didn't.

I explained why I didn't foresee future usage: #1342 (comment)

Relatedly, you keep saying you disagree without giving any sort of justification.
I've at least explained my reasoning and I'm not satisfied often that you have.

In the 10 years I've received contributions to this project for which I gave most of my spare time for free (including to you) it's the first time I've read something like this. I don't think you can delete these 2 PRs so if you can please do me a favor and do a "blank" push-force similarly to what you did above in order to remove your changes. I will eventually implement this myself from scratch sometime over the course of this year.