Opened 6 years ago

Closed 5 years ago

#21598 closed defect (fixed)

psutil: platform cygwin not supported

Reported by: embray Owned by: embray
Priority: blocker Milestone: sage-8.0
Component: porting: Cygwin Keywords:
Cc: Merged in:
Authors: Erik Bray Reviewers: Jeroen Demeyer
Report Upstream: Reported upstream. Developers acknowledge bug. Work issues:
Branch: 3493eeb (Commits, GitHub, GitLab) Commit: 3493eeb21e5d00394ae707e4005c46c50483456f
Dependencies: #22625 Stopgaps:

Status badges

Description (last modified by embray)

Installing psutil on cygwin simply doesn't work. It actually refuses to install on cygwin. See attached log.

The relevant upstream issue for psutil is here: https://github.com/giampaolo/psutil/issues/82

My in-progress pull request is here: https://github.com/giampaolo/psutil/pull/998

The attached patch is based on the current version of my pull request, where all (relevant) tests pass on Cygwin. The code itself is a bit of a mess and needs refactoring, so it will take a little more time before I can get this accepted upstream. But in the meantime I will maintain this patch for Sage.

Attachments (1)

psutil-4.3.1.log (4.7 KB) - added by embray 6 years ago.

Download all attachments as: .zip

Change History (35)

Changed 6 years ago by embray

comment:1 Changed 6 years ago by embray

  • Description modified (diff)

comment:2 Changed 6 years ago by embray

  • Report Upstream changed from N/A to Reported upstream. Developers acknowledge bug.

I wouldn't say it's a "bug" per se, but close enough. Upstream has blessed adding Cygwin support given my offer to provide it. This is non-trivial extra work for me that I wasn't expecting to have, unfortunately, but I don't think it will be that hard to get psutil at least minimally functioning on Cygwin.

comment:3 Changed 6 years ago by jdemeyer

For the record, currently psutil is not used yet by Sage. The intent was to use it to replace src/sage/misc/memory_info.py: determine the amount of physical/swap memory and the virtual memory resource limit.

comment:4 follow-up: Changed 6 years ago by embray

Right, I noticed that it's not used yet, so for now I've simply disabled the psutil package in my build. But I still think we should use it, in which case we'll want it to work on cygwin as well (or have cygwin-specific workarounds in Sage, but that's not very nice).

comment:5 follow-up: Changed 6 years ago by embray

(Depending on how long this work takes, maybe it would be best to actually remove the psutil spkg for now, until it's actually depended on. But I could go either way.)

comment:6 in reply to: ↑ 5 Changed 6 years ago by jdemeyer

Replying to embray:

Depending on how long this work takes, maybe it would be best to actually remove the psutil spkg for now

That would make sense to do when a new stable release would come out. As long as we are in the "beta" stage, it doesn't really matter much.

comment:7 in reply to: ↑ 4 Changed 6 years ago by jdemeyer

Replying to embray:

But I still think we should use it, in which case we'll want it to work on cygwin as well (or have cygwin-specific workarounds in Sage, but that's not very nice).

I think it is pretty easy to use it. So it should not take much effort. But maybe it would be better for the Sage-on-Cygwin project to not immediately use psutil?

comment:8 Changed 6 years ago by embray

It would probably help to wait on that, yeah. I'm working on a Cygwin port of psutil though, so once that's ready it will be less hassle to move forward with using psutil. As I wrote in the upstream issue, it doesn't look like it will be that hard. Many bits from the Linux module of psutil work on Cygwin. Other bits can be pulled in from the existing Windows module, as appropriate. Most of the work is refactoring.

comment:9 Changed 6 years ago by jdemeyer

Any progress on this? I am considering moving forward on #21805.

comment:10 Changed 6 years ago by embray

I was working on that a while ago, but haven't picked it up in a few weeks. It would be good to get back to it soon. It was taking longer than I'd hoped--although there aren't any major blockers there are just a lot more interfaces in psutil than I realized (it's grown a lot in functionality since I last used it), so it's a lot of work to do a complete port. I might be able to provide, for the short term, just a partial port (applied as a patch) that works just for the interfaces Sage needs if you tell me specifically which ones those are (I know it's mainly just for memory-related stuff).

comment:11 Changed 6 years ago by embray

  • Owner changed from (none) to embray

comment:12 Changed 5 years ago by embray

  • Authors set to Erik Bray
  • Branch set to u/embray/cygwin/psutil-support
  • Commit set to 5cea3bc00d48b032c2e9319eaa20b14f4a301a5c
  • Dependencies set to #22625
  • Status changed from new to needs_review

The attached patch applies my prototype for Cygwin support in psutil. It actually supports most of psutil's interfaces on Cygwin, not just (but certainly including) the handful used by Sage. I could maybe make a pared down patch with just the bare minimum, but I'm not sure that's worth the trouble now. The patch applies to psutil 5.2.0 hence the dependency on #22625

I think that this Cygwin module for psutil still needs more refactoring work before it's ready to merge upstream, but I plan to continue working on it when I can. In the meantime, this version works (it passes psutil's test suite). This issue is a major blocker to Cygwin support so it would be helpful to add this patch now, as I don't know how much longer it will take to get a final version of it upstream.


New commits:

d2ee262Upgrade psutil to 5.2.0
3da41f4Update the checksums
5cea3bcAdd patch to psutil for supporting Cygwin

comment:13 Changed 5 years ago by embray

  • Milestone changed from sage-7.4 to sage-8.0

comment:14 Changed 5 years ago by gouezel

  • Status changed from needs_review to needs_work

Building fails for me, at the very start (on cygwin64):

Traceback (most recent call last):
  File "setup.py", line 253, in <module>
    macros.append(("_WIN32_WINNT", get_winver()))
  File "setup.py", line 249, in get_winver
    maj = int(m.group('major'))
AttributeError: 'NoneType' object has no attribute 'group'
Error: could not determine package name

comment:15 Changed 5 years ago by embray

Could you say a little more about what Windows version you're using? What does the output of cmd /c ver show for you?

comment:16 Changed 5 years ago by embray

  • Status changed from needs_work to needs_info

comment:17 Changed 5 years ago by gouezel

Windows 10 Pro on x86_64

$ cmd /c ver

Microsoft Windows [version 10.0.14393]

comment:18 Changed 5 years ago by gouezel

  • Status changed from needs_info to needs_work

Note the case mismatch between version returned above and Version in your regexp in setup.py.

comment:19 Changed 5 years ago by embray

Hah, mine is the exact same version except it prints "Version" instead of "version". Maybe it's a locale difference.

comment:20 Changed 5 years ago by git

  • Commit changed from 5cea3bc00d48b032c2e9319eaa20b14f4a301a5c to bac432890b5aedca346cdeb7374b95b3fe27daf8

Branch pushed to git repo; I updated commit sha1. New commits:

bac4328This 'Version' string appears to be locale-dependent; just ignore it

comment:21 Changed 5 years ago by embray

  • Status changed from needs_work to needs_review

I made the regular expression in the setup.py more permissive; should build now.

comment:22 follow-up: Changed 5 years ago by tscrim

Why not get have [vV]ersion? I feel that it should not be as permissive as your changes.

comment:23 Changed 5 years ago by jdemeyer

What is the upstream situation of this patch? Has it been submitted?

comment:24 in reply to: ↑ 22 ; follow-up: Changed 5 years ago by embray

Replying to tscrim:

Why not get have [vV]ersion? I feel that it should not be as permissive as your changes.

In Japanese it's "バージョン".

comment:25 in reply to: ↑ 24 Changed 5 years ago by tscrim

Replying to embray:

Replying to tscrim:

Why not get have [vV]ersion? I feel that it should not be as permissive as your changes.

In Japanese it's "バージョン".

Ah, I see.

How about Jeroen's comment:23?

comment:26 Changed 5 years ago by embray

  • Description modified (diff)

I updated the description with a link to my pull request. In short, I have submitted this patch as proof of concept, but I acknowledged in doing so that it's not ready. The code works though, it just needs refactoring. For the purposes of patching psutil in Sage it's fine though.

comment:27 Changed 5 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Please bump the package version in Sage.

comment:28 Changed 5 years ago by git

  • Commit changed from bac432890b5aedca346cdeb7374b95b3fe27daf8 to 3493eeb21e5d00394ae707e4005c46c50483456f

Branch pushed to git repo; I updated commit sha1. New commits:

3493eebBump patch level

comment:29 Changed 5 years ago by embray

  • Status changed from needs_work to needs_review

Ah yes, of course. I agree it's a significant enough patch that we want to make sure it doesn't break the existing patchbots.


New commits:

3493eebBump patch level

comment:30 Changed 5 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_review to positive_review

Did anybody besides Erik test this on Cygwin?

comment:31 Changed 5 years ago by tscrim

I haven't gotten to this point on my current Cygwin build.

comment:32 Changed 5 years ago by tscrim

I was able to get to this and it built without problems.

comment:33 Changed 5 years ago by embray

  • Priority changed from major to blocker

comment:34 Changed 5 years ago by vbraun

  • Branch changed from u/embray/cygwin/psutil-support to 3493eeb21e5d00394ae707e4005c46c50483456f
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.