-
-
Notifications
You must be signed in to change notification settings - Fork 557
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
cygwin: get_memory_usage isn't implemented #9170
Comments
comment:1
Another test failure caused by this:
|
comment:2
I assume this means that
will not work either. I know on Solaris, I called
to
Dave |
comment:3
Replying to @sagetrac-drkirkby:
I'm not at home so I can't test on my machine, but
looks like the line that's causing the first error to be throwing. The offending line(s) should be
I've double checked, and cygwin does ship with a top, so I suspect all that needs to be done is add a few instances along the line of "or U == 'cygwin'" and these issue would be resolved. I'll try to test this over the weekend and have a patch prepared. |
comment:4
Irrespective of whether Cygwin comes with top or not, using it can't be the best way to get memory usage. top is not used on Linux exept as a last resort. There are system calls to get the memory usage, which is the most sensible way to do this. If those system calls work on Cygwin, then it would be
Dave |
comment:5
Very true. I should have thought about what the code was doing, not just how to fix the breakage. As for top not being POSIX, I wasn't aware of that. I had always assumed it was. Since it's not POSIX it seems fine to let top() calls fail on Windows if top() is not installed, leaving an appropriately worded explanation. The only time top is called on linux is via a top() call. To get the memory usage under linux, the /proc//status is inspected. While they don't seem to document how complete it is, cygwin does populate a /proc directory. I'll poke around to see if the cygwin /proc system has what is needed. As far as I can see this the closest Python has to a memory usage call without using external libraries. Geoff |
comment:6
Replying to @sagetrac-gbe:
I don't know, but I thought the plan was to make an installer for Cygwin which installed the perquisites, which would include top. So it should not really fail. As much as I don't like the idea of using 'top', I think in the short term it is a OK. There are more significant issues causing problems on Cygwin. I would have thought this one of the lower priority ones, but that's just my opinion. Dave |
comment:7
Unsurprisingly, this still doesn't work, though most stuff now does/can on Cygwin. |
comment:9
I guess we could prereq for the Cygwin procps package but is this really necessary? |
comment:10
Well, |
comment:11
After fighting with Cygwin to get enough memory to run heegner.py test (did not manage yet... not sure that I changed anything but I can allocate 500000000 bytes, but not 512000000), I'm not really convinced that get_memory_usage is that useful on Cygwin as you won't really be able to allocate as much memory as you want (without fighting with Cygwin...). |
comment:12
Whatsoever, with Cygwin procps package which provides top, the doctests do not pass. |
comment:13
So I still think the best solution is not to test this on Cygwin or to expect a different result... Or if you insist we add procps as a prereq and modify the Sage's top function implem. Alternatively, we could modify Sage's top so that it raises an Error iff the system top is not available. |
comment:15
Yes ps is included in the "cygwin" package itself so always available. |
comment:16
But that won't help, I guess we are stuck with top. |
comment:17
Or we could just directly look into /proc/meminfo, not sure why we don't do that on Linuces. |
comment:18
Or rather things in /proc/$PID/ |
comment:19
Surely that is not portable across a large range of Linuces. |
comment:20
I do have top on my Cygwin install and its output is very much Linux-like. So I propose just to hack |
Attachment: trac_9170.patch.gz enabling it on Cygwin |
This comment has been minimized.
This comment has been minimized.
comment:22
please test the patch (on Cygwin you might need to install |
This comment has been minimized.
This comment has been minimized.
Reviewer: Jean-Pierre Flori |
Author: Dmitrii Pasechnik |
Merged: sage-5.7.beta4 |
Cygwin has
top
now, so it's straightforward to fix.apply
CC: @jpflori
Component: porting: Cygwin
Author: Dmitrii Pasechnik
Reviewer: Jean-Pierre Flori
Merged: sage-5.7.beta4
Issue created by migration from https://trac.sagemath.org/ticket/9170
The text was updated successfully, but these errors were encountered: