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

Build wheel #1758

Merged
merged 7 commits into from
May 16, 2020
Merged

Build wheel #1758

merged 7 commits into from
May 16, 2020

Conversation

Czaki
Copy link
Contributor

@Czaki Czaki commented May 13, 2020

I would like to add build wheel for psutil to allow easy install on non gcc macos and linux system.

All wheels are build without problem, but when I add tests it fails. I found that part of test need to be disabled (because they are disabled on travis).

I choose github workflows, because they allow to create artefacts (on travis there is need to upload to remote server) and not need to create additional account like on AzurePipelines.

cibuildwheel is really nice tool and reduce maintainty of building wheel process. most often to upgrade version of cibuildwheel.

I see discussion in #824 and think that I can help with fast wheel creation, but I do not know psutil enough weel to understnad if some test should be blacklisted if run on Macos 10.15 or inside Docker container.

@giampaolo
Copy link
Owner

giampaolo commented May 13, 2020

Nice, this looks promising. On what Linux distro is this built?
Without going back to re-read the whole #824 (I forgot details) I seem to remember we have 2 problems:

  • wheels must be built on CentOS 6 in order to be more compatible
  • that means we'll have to change some stuff in psutil/_psutil_linux.c first

What Linux distro are these wheels built on?
Also, where are the artifacts stored and how do I get to them? (I suppose I'll have to download them locally, and the upload them on PYPI)

@Czaki
Copy link
Contributor Author

Czaki commented May 13, 2020

Currently it is build on Centos 7 but I will switch it to Centos6 in next commit.
These are manylinux2010 image which is Centos 7 based.

On Centos 6 I register few more erors.

Wheels will be avaliable in actions. You can see here https://github.com/Czaki/psutil/actions
After merge it will be avaliable in actions for this repository.

@Czaki
Copy link
Contributor Author

Czaki commented May 13, 2020

pypy wheels will be build on centos7 because they not offer centos6 docker based images.

@giampaolo
Copy link
Owner

giampaolo commented May 14, 2020

On Centos 6 I register few more erors.

I suppose that's because of _psutil_linux.c: the parts relying on the pre-processor checks (#ifdefs) will either have to be replaced with a counterpart detecting the kernel / glibc version at runtime in C, or rewritten in Python by using ctypes (still not sure what's best). It's one of the blockers. For now I suggest to leave that alone (if it compiles) and ignore the test failures related to that.

@Czaki
Copy link
Contributor Author

Czaki commented May 14, 2020

As far as I know I try to response on your answer to manylinux.

There are three manylinux tags

  • manylinux1
  • manylinux2010 (most distro released after 2010 supported)
  • manylinux2014 (most distro released after 2014 supported)

and pip know host system glibc version and then check if there is available wheel with enough low version glibc. So if hos system has to low version of glibc then it will build from source.

Main weekness of manylinux2010 platform tag is that it need pip in version 19.0.

The idea of building manylinux wheel is to build them inside docker container. manylinux team create docker images manylinux1 - centos 6, manylinux2010 - centos 7 , manylinux2014 - centos 8. So there is no problem that travis support centos 7, because everything is build inside docker container.

I'm contributor to cibuildwheel package and can try explain more technical details.

@giampaolo
Copy link
Owner

giampaolo commented May 14, 2020

and pip know host system glibc version and then check if there is available wheel with enough low version glibc. So if hos system has to low version of glibc then it will build from source.

Sweet. In that case I think we should stick with CentOS 7. For the record, I've been using a similar policy for Windows XP (supported from sources only, but now it's gone for good).

Question, in case you know: what happens if the wheel does not support a recent distro? Is pip able to know that in advance or retry by using the tarball? My doubt is if by distributing wheels there's a chance we break some installations that now work fine because the tarball is the only thing currently available on PYPI.

@Czaki
Copy link
Contributor Author

Czaki commented May 14, 2020

Question, in case you know: what happens if the wheel does not support a recent distro? Is pip able to know that in advance or retry by using the tarball? My doubt is if by distributing wheels there's a chance we break some installations that now work fine because the tarball is the only thing currently available on PYPI.

What did you men by not support? wheels are checked with auditwheel that verify that they depend only on few libraries and should run on every more modern system.

I do not meet case when wheel works only with old system and does not work with modern one.

Win wheel name there is platform tag. win macos_versionormanylinux*` pip know which is maximum platform tag that is supported.

@Czaki
Copy link
Contributor Author

Czaki commented May 15, 2020

@giampaolo I I should manually mark some test as not use in build, or wait on signal from you that I should use more modern version?

And maybe it is beter to check CIBUILDWHEEL=1 variable rather than check if running on github (CIBUILDWHEEL is defined by tool)

@giampaolo
Copy link
Owner

Is it using CentOS 6 or 7?
From this failure:
https://github.com/Czaki/psutil/runs/671305932?check_suite_focus=true

======================================================================
FAIL: psutil.tests.test_contracts.TestAvailConstantsAPIs.test_linux_rlimit
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/project/psutil/tests/test_contracts.py", line 91, in test_linux_rlimit
    ae(hasattr(psutil.Process, "rlimit"), hasit)
AssertionError: False != True

I guess we're on 6?

@Czaki
Copy link
Contributor Author

Czaki commented May 15, 2020

This build is on centos 7 https://github.com/Czaki/psutil/actions/runs/103672866
This is on centos 6 https://github.com/Czaki/psutil/actions/runs/103718944

This is two line switch. Go back to centos 7?

giampaolo added a commit that referenced this pull request May 15, 2020
Rely on "__NR_prlimit64" availability and check GLIBC version only.
Take kernel version out of the picture.
This way it works on both CentOS 6 and 7.
Also, have test_contracts.py tests assume prlimit() is always available,
so that we will be notified (by failure).
Ref: #1758
@Czaki
Copy link
Contributor Author

Czaki commented May 15, 2020

What to do with error OSError: AF_UNIX path too long ?

giampaolo added a commit that referenced this pull request May 15, 2020
Copy link
Owner

@giampaolo giampaolo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is two line switch. Go back to centos 7?

Yes. I pushed a couple of commits which should fix some failures so you should rebase.

setup.py Outdated
"ipaddress; python_version < '3.0'",
"mock; python_version < '3.0'",
"pypiwin32; sys.platform == 'win32'",
"wmi; sys.platform == 'win32'"]}
if sys.version_info[:2] <= (3, 3):
extras_require.update(dict(enum='enum34'))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add this to the above list.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@@ -123,7 +123,8 @@
TRAVIS = bool(os.environ.get('TRAVIS'))
APPVEYOR = bool(os.environ.get('APPVEYOR'))
CIRRUS = bool(os.environ.get('CIRRUS'))
CI_TESTING = TRAVIS or APPVEYOR or CIRRUS
GITHUB = bool(os.environ.get('CI', False))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'CI' is a bit generic and perhaps it can collide with something else;
Maybe we can set an env var from .github/workflows/build_wheel.yml. Something like GITHUB_WHEELS.
https://help.github.com/en/actions/configuring-and-managing-workflows/using-environment-variables#about-environment-variables

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

Copy link
Contributor Author

@Czaki Czaki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still few errors (2 on mac 4 on linux) and unknown reason of windows fail

@giampaolo
Copy link
Owner

According to this failure:

FAIL: psutil.tests.test_contracts.TestAvailProcessAPIs.test_rlimit
----------------------------------------------------------------------
Traceback (most recent call last):
File "/project/psutil/tests/test_contracts.py", line 153, in test_rlimit
self.assertEqual(hasattr(psutil.Process, "rlimit"), LINUX)
AssertionError: False != True

...Process.prlimit() is not available, and that's because of this check:

// Linux >= 2.6.36 (supposedly) and glibc >= 2.13
#define PSUTIL_HAVE_PRLIMIT \
defined(__NR_prlimit64) && \
(__GLIBC__ >= 2 && __GLIBC_MINOR__ >= 13)

...which should be "true" on CentOS 7. Are you sure we're compiling against CentOS 7? The resulting wheels have manylinux2010 in their name. Doesn't that mean CentOS 6?

@giampaolo giampaolo self-requested a review May 15, 2020 23:37
@Czaki
Copy link
Contributor Author

Czaki commented May 16, 2020

...which should be "true" on CentOS 7. Are you sure we're compiling against CentOS 7? The resulting wheels have manylinux2010 in their name. Doesn't that mean CentOS 6?

argh...

I do wrong counting manylinux1 is from centos 5 so manylinux2010 is form centos 6. so for centos 7 there is need to use manylinux2014.

I add step with manylinux2014 build and it still fail.

@giampaolo
Copy link
Owner

giampaolo commented May 16, 2020

OK, good. There are still some failures but the important one was to make Process.rlimit() available and that is fixed. manylinux2010 wheels are still produced though. We should not run anything at all on CentOS6. We should only run tests on CentOS7 and produce wheels on CentOS7, so basically I think we should only keep this section:
39b5a0b
...and remove/readapt/adjust the rest.

@Czaki
Copy link
Contributor Author

Czaki commented May 16, 2020

Done.
If I should rebase on current master?

It meanas that there will be no wheel for pypy for linux (they provide only manylinux 2010 docker image)

@giampaolo
Copy link
Owner

It means that there will be no wheel for pypy for linux (they provide only manylinux 2010 docker image)

Mmmm. OK, I just downloaded the zip and indeed I see some wheels produced for PYPY with 2010/CentOS-6:

  • psutil-5.7.1-pp36-pypy36_pp73-win32.whl
  • psutil-5.7.1-pp36-pypy36_pp73-manylinux2010_x86_64.whl
  • psutil-5.7.1-pp36-pypy36_pp73-macosx_10_9_x86_64.whl
  • psutil-5.7.1-pp27-pypy_73-manylinux2010_x86_64.whl
  • psutil-5.7.1-pp27-pypy_73-macosx_10_9_x86_64.whl

I have mixed feelings about PYPY. I consider it low priority, but since this basically comes for free maybe we can decide to either 1) accept that Process.prlimit() won't be available on PYPY + Linux 2) provide an additional pure-Python implementation of prlimit() which uses ctypes. I will think about that later. Let's not worry about that for now.

Also I have a couple of questions about OSX:

  • a lot of python versions are missing on OSX. I think it's because this script depends on .travis.yml right? In that case I think we should extend .travis.yml to also include python 2.7, 3.5, 3.6, 3.7, 3.8 on OSX.
  • It looks like the wheels on OSX are produced on macosx 10.9. Will they work on more recent OSX versions?

As for this failure:

2020-05-16T10:18:31.7778978Z psutil.tests.test_connections.TestSystemWideConnections.test_multi_sockets_procs ... Traceback (most recent call last):
2020-05-16T10:18:31.7780948Z   File "/tmp/@psutil-100-rgd3uhff", line 2, in <module>
2020-05-16T10:18:31.7781195Z     from psutil.tests import create_sockets
2020-05-16T10:18:31.7781707Z ImportError: No module named 'psutil'
2020-05-16T10:18:31.7797725Z Traceback (most recent call last):
2020-05-16T10:18:31.7798318Z   File "/tmp/@psutil-100-s2bbb_6w", line 2, in <module>
2020-05-16T10:18:31.7798447Z     from psutil.tests import create_sockets

I think you can try this change:

diff --git a/psutil/tests/__init__.py b/psutil/tests/__init__.py
index 9667b5d5..33d5d2fd 100644
--- a/psutil/tests/__init__.py
+++ b/psutil/tests/__init__.py
@@ -199,7 +199,9 @@ def _get_py_exe():
         else:
             return exe
 
-    if MACOS:
+    if GITHUB_WHEELS:
+        return 'python'
+    elif MACOS:
         exe = \
             attempt(sys.executable) or \
             attempt(os.path.realpath(sys.executable)) or \
~/svn/psutil {master}$ 

If it doesn't work don't worry and I'll investigate it once we merge this. The few failures we have are issues with tests themselves and not psutil, so they are not important.

Czaki added a commit to Czaki/psutil that referenced this pull request May 16, 2020
@Czaki
Copy link
Contributor Author

Czaki commented May 16, 2020

Also I have a couple of questions about OSX:

  • a lot of python versions are missing on OSX. I think it's because this script depends on .travis.yml right? In that case I think we should extend .travis.yml to also include python 2.7, 3.5, 3.6, 3.7, 3.8 on OSX.

I just download and see this files:

psutil-5.7.1-cp27-cp27m-macosx_10_9_x86_64.whl
psutil-5.7.1-cp35-cp35m-macosx_10_9_x86_64.whl
psutil-5.7.1-cp36-cp36m-macosx_10_9_x86_64.whl
psutil-5.7.1-cp37-cp37m-macosx_10_9_x86_64.whl
psutil-5.7.1-cp38-cp38-macosx_10_9_x86_64.whl
psutil-5.7.1-pp27-pypy_73-macosx_10_9_x86_64.whl
psutil-5.7.1-pp36-pypy36_pp73-macosx_10_9_x86_64.whl

which version is missed?

  • It looks like the wheels on OSX are produced on macosx 10.9. Will they work on more recent OSX versions?

macos 10.9 means that minimal version of system is macosx 10.9. Wheel should run on all newer version (from 10.15 only x86_64, but this wheel is already 64-bits)

@giampaolo
Copy link
Owner

Maybe this:

+    if GITHUB_WHEELS:
+        return os.path.realpath('python')

find path to python executable
@Czaki
Copy link
Contributor Author

Czaki commented May 16, 2020

cibuildwheel guarantee that python executable will be in path under python name.

One problem which I see that if any test fail then wheel building fail. Because of this I add second group which build wheels without test.

So I prefer to mark this test with x-fail to run see if there are no problem with other versions.

@giampaolo
Copy link
Owner

giampaolo commented May 16, 2020

Yeah, I agree it is best to ignore test failures. I am already busy enough taking a look at "normal" (non-wheel) builds. =)
Anyway, this is great work, thanks. I didn't know github was capable of this. I'm going to merge this PR and try to adjust those minor failures on my own. I'll also have to see how to write a script to download the zip (by using github APIs I guess) for when I'll have to make a new release.

@giampaolo giampaolo merged commit 1356286 into giampaolo:master May 16, 2020
@giampaolo
Copy link
Owner

Python 2.7 wheels for Linux are missing due to this directive:

 CIBW_SKIP: "pp27-*win* cp27-*manylinux* pp-*manylinux*"

When I remove it I get this error:

    + echo 'cibuildwheel: python available on PATH doesn'\''t match our installed instance. If you have modified PATH, ensure that you don'\''t overwrite cibuildwheel'\''s entry or insert python above it.'

The issue disappears if I use CentOS 6 / manylinux2010. I've been struggling with this the whole day. :(

@Czaki
Copy link
Contributor Author

Czaki commented May 17, 2020

Yes. manylinux2014 does not provide python 2.7 executable.

You supprise me with this merge I have add few commit to branch but I plan to create next PR when I got working version.
There. Pypy on macos add specific variable to environment for example.

@giampaolo
Copy link
Owner

Oh sorry, I thought you were done. And it was probably too soon anyway (the Windows error is very obscure). I have a PR I am going to merge soon which fixes some of the tests which were/are failing here.

Yes. manylinux2014 does not provide python 2.7 executable.

Then we're stuck with 2010. That'll probably mean ctypes for prlimit() (not fun).

@giampaolo
Copy link
Owner

Merged #1761.

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

Successfully merging this pull request may close these issues.

2 participants