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

Add support for new platform (IBM i) #1564

Closed
wants to merge 33 commits into from

Conversation

ThePrez
Copy link

@ThePrez ThePrez commented Aug 5, 2019

For reference, see lengthy discussions in #1415 and #1444. I found it eventually much more sensible to have IBM i as a completely new module than sharing a codestream with AIX.
I'll attach the test output shortly. While there is still plenty of test cases that should be cleaned up, the basic functions seem to work well. This fork has been used by several applications for nearly six months, so there is some assurance of quality.

This new module relies in an external package, but IBM will distribute the package in RPM form as to ensure that does not pose a problem.

======================================================================
FAIL: psutil.tests.test_contracts.TestFetchAllProcesses.test_fetch_all
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/JGORZINS/psutil/psutil/tests/test_contracts.py", line 356,
in test_fetch_all
meth(ret, p)
File "/home/JGORZINS/psutil/psutil/tests/test_contracts.py", line 399,
in create_time
self.assertIsInstance(ret, float)
AssertionError: 1546917581 is not an instance of <class 'float'>
@ThePrez
Copy link
Author

ThePrez commented Aug 6, 2019

testOutput.txt

Here is the test output. Like I said, it isn't pretty, but if you agree to merging in the new IBM i module, I'll gladly circle around to clean up the test cases (which I haven't yet figured out how to do), perhaps with the help of @jwoehr.

@ThePrez
Copy link
Author

ThePrez commented Jun 5, 2020

@giampaolo , would you be amenable to merging this in once we further clean up these test cases?

@giampaolo
Copy link
Owner

What's the test status?

@ThePrez
Copy link
Author

ThePrez commented Jun 7, 2020

The test status is still in pretty sad shape (no work has been done since my posted test results, above).
If you can verify that you're ok with landing this new platform support, I'll work to clean them up further.
Thanks!

@giampaolo
Copy link
Owner

Just let me have a look at the output first to have an idea of what's implemented and what is not. =)

@ThePrez
Copy link
Author

ThePrez commented Jun 8, 2020

@giampaolo , I've done zero work on this since the posted results on Aug 5th.

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.

Hello Jesse. Here's an initial review. Other than the inline comments, it looks like this platform is quite different than AIX (it has it's own py and c modules) so I suggest to add a IBMI constant in _common.py and treat it differently, and also replace os.uname().sysname != 'OS400' all over the place.

Other than that, there are some errors such as this one that need to be fixed:

  File "/QOpenSys/pkgs/lib/python3.6/site-packages/psutil-5.5.1-py3.6-os400-powerpc64.egg/psutil/_psibmi.py", line 420, in _proc_name_and_args
    return cext.proc_name_and_args(self.pid)
OSError: [Errno 14] Bad address

But let's get to tests later (there's more).

psutil/__init__.py Show resolved Hide resolved
psutil/_psibmi.py Show resolved Hide resolved
psutil/_psibmi.py Show resolved Hide resolved
psutil/_psutil_ibmi.c Show resolved Hide resolved
psutil/_psutil_ibmi.c Show resolved Hide resolved
psutil/_psibmi.py Show resolved Hide resolved
psutil/_psibmi.py Show resolved Hide resolved
psutil/_psibmi.py Show resolved Hide resolved
psutil/_psibmi.py Show resolved Hide resolved
psutil/_psibmi.py Show resolved Hide resolved
@giampaolo
Copy link
Owner

Me and Jesse already discussed this privately a while ago, but forgot to update this ticket - doing it now.
Despite I was originally OK-ish pushing this forward, I think it makes more sense to make this work develop and mature in its own fork (link to psutil IBM i fork), essentially for the same reasons I gave in #1690 (comment). Current psutil monthly download stats:

$ pypinfo --days 30 psutil system
Served from cache: True
Data processed: 0.00 B
Data billed: 0.00 B
Estimated cost: $0.00

| system_name          | download_count |
| -------------------- | -------------- |
| Linux                |     11,084,549 |
| Windows              |        790,397 |
| Darwin               |        232,315 |
| FreeBSD              |          3,499 |
| OS/390               |             88 |
| AIX                  |             84 |
| SunOS                |             74 |
| OpenBSD              |             39 |
| Total                |     12,111,089 |

Given that monthly downloads for AIX and OS/390 (both IBM platforms) are < 100 (0.0007% of total), I expect IBM i numbers would be similar. Going forward, if support for more exotic OSes is started and completed, it may make sense to list them somewhere in mainstream psutil (doc or README).

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

2 participants