Skip to content
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

msys2 (mingw) support #845

Closed
wants to merge 2 commits into from
Closed

Conversation

manuelnaranjo
Copy link

Guys,

I fixed the tool to build and run in msys2[1].

If needed I can provide with the full list of packages I have installed as msys2 provides pacman :D.

-Manuel

[1] http://msys2.github.io

* Adding a check for testing for msys2
* disabling inet_ntop.c on msys2 as it's provided by mingw
Fixes for building the lib in an msys environment with mingw32
mingw64 should work as well.
@giampaolo
Copy link
Owner

This is cool.

  • Did you make sure you didn't break Visual Studio (well the CI will tell us)?
  • Does this work with 64 bit versions of python? I remember one of the reasons I dropped mingw support at some point was because I couldn't make mingw 64 bit version work.
  • Which python versions did you test?
    ! As for the pull request: please update INSTALL and/or DEVGUIDE in order to include instructions on how to install mingw (also 64 bit version if possible) and compile psutil by using mingw.

@manuelnaranjo
Copy link
Author

Ok I'll check the flake8 errors on setup.py and possibly others, then I will update the docs.

I tested on python 2.7.11 for mingw32 provided by msys.

I have access to a 64b Windows machine, I'll setup mingw64 on it and give it a try.

@mati865
Copy link

mati865 commented Jul 9, 2016

Tested with MSYS2 native MinGW-w64 32 bit and 64 bit, python 2.7.11 and 3.5.0 and it is working fine.
I created draft PKGBUILD that you can use to test it here

Testing procedure:
I. Save PKGBUILD to any directory
II. In MSYS2 shell

cd <directory/with/PKGBUILD>
makepkg-mingw -sfi --noconfirm

III. In MINGW shell

python3 # or python2
import psutil
psutil.cpu_times()
scputimes(user=501.453125, system=381.703125, idle=4440.75, interrupt=25.921876907348633, dpc=24.078125)

EDIT: Forgot to mention it will create packages for both architectures and both Python versions

@manuelnaranjo
Copy link
Author

Some tests are failing to success when I execute make test. So the pull request is not ready for merging.

Anyhow I packed and built this for mingw, it's available on my repository at https://github.com/manuelnaranjo/mnaranjo-mingw-repository

I think we should close the PR

@mati865
Copy link

mati865 commented Aug 29, 2016

@manuelnaranjo Fixes for packages should go to the upstream if possible so everyone could benefit from it.
Alternatively you could create PR to (https://github.com/Alexpux/MINGW-packages)[MINGW-packages] with PKGBUILD and patches.

@manuelnaranjo
Copy link
Author

@mati865 Yes I'm aware of all that, I'll create a PR to mingw later today.

Before getting the changes back into upstream I think we need to make sure the tests passes. memory-leaks and some network interface related ones are failing.

@giampaolo
Copy link
Owner

Any update about this?

@manuelnaranjo
Copy link
Author

@giampaolo no sorry, I don't have time to work on that. I have created a mingw32 repository, and I'm maintaining it. We can tell people to use it, I don't think there's need to merge this changes back into upstream.

We can close the bug report if you feel that's right.

@giampaolo
Copy link
Owner

I see little sense in redirecting users to another repo for something this specific, which is also almost done AFAIU. If you don't have time that's fine of course. I'll keep this PR open and will try to take a look at it when I find some time.

@manuelnaranjo
Copy link
Author

Actually there's a reason for that, pip install doesn't work out of the box
in the platform. I agree it's easier to keep this changes in psutil than in
msys project
El El lun, 12 de sept de 2016 a las 3:21 p.m., giampaolo <
notifications@github.com> escribió:

I see little sense in redirecting users to another repo for something this
specific, which is also almost done AFAIU. If you don't have time that's
fine of course. I'll keep this PR open and will try to take a look at it
when I find some time.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#845 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAhUW8_KioBzZuoJLVA6WjmUtd9Wg1Ozks5qpZgUgaJpZM4I9QJn
.

@giampaolo
Copy link
Owner

Actually there's a reason for that, pip install doesn't work out of the box

what do you mean exactly?

@manuelnaranjo
Copy link
Author

If the package is pure python then pip install <package> is fine on mingw32, but if the package needs some C/C++ extension to be built, then it may fail with the following, unless it's called with the right arguments, or the package is patched.

I created a PR with the mingw package maintainers, so users would be able to do: pacman -S mingw-w64-i686-python2-psutil # or mingw-w64-i686-python3-psutil, mingw-w64-x86_64-python2-psutil..... Which is the preferred way on msys for installing packages.

@manuelnaranjo
Copy link
Author

@giampaolo
Copy link
Owner

Given that currently I distribute exes/wheels by compiling them with Visual Studio this should not represent a problem even if this is merged though, correct?

@giampaolo
Copy link
Owner

As far as I understand, mingw support would be good for people who want to compile psutil from sources.

@manuelnaranjo
Copy link
Author

Yes that's right, you would be fine merging. And would make mingw team life easier, if you ever merge this and make a new release let me know and I update mingw package.

And yes having the source build on mignw would make people willing to build from source life easier. Otherwise they would need to learn how to repackage for mingw which is pretty damn simple, pacman is way simpler than dpkg.

@giampaolo
Copy link
Owner

OK, this brings us back to my original question: is this ready to be merged? Is there something missing? =)

@manuelnaranjo
Copy link
Author

Not AFAIK, it's ready to merge, but now this build failed [1] and I'm clue less. That function is defined by ws2_32 according to [2]

[1] https://tea-ci.org/Alexpux/MINGW-packages/1373/2
[2] https://msdn.microsoft.com/es-es/library/windows/desktop/cc805843%28v=vs.85%29.aspx

@manuelnaranjo
Copy link
Author

The patch is missing an include, and gcc is complaining that the function is been implicit defined, I'll fix that tomorrow and update both PR

@mati865
Copy link

mati865 commented Sep 12, 2016

Tea-Ci is based on Wine Staging and sometimes it's unstable or missing some Windows functionality.

@giampaolo
Copy link
Owner

Any news update this? The PR is now old and no longer applies cleanly. This is still something I would welcome.

@manuelnaranjo
Copy link
Author

manuelnaranjo commented Apr 25, 2017 via email

@giampaolo
Copy link
Owner

giampaolo commented May 8, 2019

Looking back at this old PR, and on a second thought: despite being able to compile psutil with it would be advantageous for the user, I'm gonna reject this proposal as the extra burden needed in order to maintain MinGW32 support is not worth it from a maintenance standpoint (e.g. the extra Appveyor CI/build). FWIW, cPython also only support VS, I believe for similar reasons.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants