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

[Solaris] Fix #1637 #1638

Merged
merged 5 commits into from Dec 17, 2019
Merged

[Solaris] Fix #1637 #1638

merged 5 commits into from Dec 17, 2019

Conversation

vser1
Copy link
Contributor

@vser1 vser1 commented Dec 10, 2019

This add support for SunOS 5.10 update prior to 4.

It shouldn't change anything for conforming targets but add support for older targets.

@vser1 vser1 changed the title Fix #1637 [Solaris] Fix #1637 Dec 11, 2019
@giampaolo
Copy link
Owner

Did you check this doesn't break anything on newer versions? I won't have a SunOS box to test against anytime soon so I'd basically merge this blindly. (also please update HISTORY.rst)

@vser1
Copy link
Contributor Author

vser1 commented Dec 13, 2019

I've checked it against the latest Solaris 5.10 box I have access to and the preprocessor code is the same as on master.

I'm asking around to get access to a 5.11 box but can't ensure I'll get it.

There is indeed one possible pitfail: if latest SunOS latest tcp.h does not anymore define _INET_MIB2_H.

History updated 👍

@vser1
Copy link
Contributor Author

vser1 commented Dec 13, 2019

OK,

Based on issue #421 and on this serverfault question I'd suggest to pass Solaris update version DC the compiler and activate NEW_MIB_COMPLIANT depending if update is greater than 1. That should clean all the pitfalls!

I'll do it on Monday!

@vser1
Copy link
Contributor Author

vser1 commented Dec 16, 2019

@giampaolo I've checked this against all of the solaris boxes I have access to (eg Initial Release, Update 2, Update 3, Update 5, Update 11). The compilation fails on master on the boxes Initial Release, Update 2, Update 3 as described in the issue. With this PR, psutil will install on all the boxes with no pb. Update5 and 11 do have the support of MIB (checked the gcc -E code manually).

@vser1 vser1 force-pushed the fix_old_solaris branch 2 times, most recently from 77bacea to e98cb6a Compare December 16, 2019 18:00
CREDITS Show resolved Hide resolved
psutil/_psutil_sunos.c Outdated Show resolved Hide resolved
psutil/_psutil_sunos.c Outdated Show resolved Hide resolved
psutil/_psutil_sunos.c Show resolved Hide resolved
@@ -6,7 +6,6 @@
* Functions specific for Process.environ().
*/

#define NEW_MIB_COMPLIANT 1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As said in the commit message, I'm pretty sure this is dead code :)

psutil/_psutil_sunos.c Show resolved Hide resolved
@@ -12,12 +12,8 @@
/* fix compilation issue on SunOS 5.10, see:
* https://github.com/giampaolo/psutil/issues/421
* https://github.com/giampaolo/psutil/issues/1077
* http://us-east.manta.joyent.com/jmc/public/opensolaris/ARChive/PSARC/2010/111/materials/s10ceval.txt
*
* Because LEGACY_MIB_SIZE defined in the same file there is no way to make autoconfiguration =\
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This does not seem relevant anymore.

*/

#define NEW_MIB_COMPLIANT 1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Defined for updates after 4 by gcc.

setup.py Outdated
@@ -268,6 +269,17 @@ def get_ethtool_macro():
if SUNOS:
posix_extension.libraries.append('socket')
if platform.release() == '5.10':
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does this mean psutil is not supported on Solaris 5.11 BTW?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it's supported, I guess it's NEW_MIB_COMPLIANT and I need to move up my macro one level!

Copy link
Owner

@giampaolo giampaolo Dec 17, 2019

Choose a reason for hiding this comment

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

No, if I am not mistaken that's in place because getifaddrs() syscall is not available on Solaris 5.10, therefore it means psutil SunOS support is supposed to start with SunOS version 5.10 (+ newer versions including 5.11).

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.

Good job. I added some comments inline. =)

CREDITS Show resolved Hide resolved
psutil/_psutil_sunos.c Show resolved Hide resolved
psutil/_psutil_sunos.c Show resolved Hide resolved
setup.py Outdated
@@ -268,6 +269,17 @@ def get_ethtool_macro():
if SUNOS:
posix_extension.libraries.append('socket')
if platform.release() == '5.10':
Copy link
Owner

@giampaolo giampaolo Dec 17, 2019

Choose a reason for hiding this comment

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

No, if I am not mistaken that's in place because getifaddrs() syscall is not available on Solaris 5.10, therefore it means psutil SunOS support is supposed to start with SunOS version 5.10 (+ newer versions including 5.11).

if update is None or (int(update.group(0)) < 4):
new_mib_compliant = False
if new_mib_compliant:
posix_extension.define_macros.append(('NEW_MIB_COMPLIANT', 1))
Copy link
Owner

Choose a reason for hiding this comment

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

How about shortening it a bit?

            # Detect Solaris 5.10, update >= 4, see: 
            # https://github.com/giampaolo/psutil/pull/1638
            # For an explanation of Solaris /etc/release see:
            # https://serverfault.com/q/524883
            with open('/etc/release') as f:
                update = re.search(r'(?<=s10s_u)[0-9]{1,2}', f.readline())
                if update is None or (int(update.group(0)) >= 4):
                    posix_extension.define_macros.append(('NEW_MIB_COMPLIANT', 1))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right! But I still need to add the macro for the other platforms.

Question, is it on purpose that you've let the condition as if update is None or … (which now means that when no update is found or the update is greater than 4, the platform is new mib compliant)? Difference with my version is that I assumed when no update is found, the platform is not mib compliant.

Copy link
Owner

Choose a reason for hiding this comment

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

Question, is it on purpose that you've let the condition as if update is None or … (which now means that when no update is found or the update is greater than 4, the platform is new mib compliant)?

No I just copy/pasted your code.

Detect Solaris MIB compliancy based on Update number

This allows setting NEW_MIB_COMPLIANT macro in the compile line.

Consider initial 5.10 release as non mib compliant
@vser1
Copy link
Contributor Author

vser1 commented Dec 17, 2019

Sorry, should have tested before pushing ^^
Error spotted!

Étienne Servais added 3 commits December 17, 2019 12:39
@vser1
Copy link
Contributor Author

vser1 commented Dec 17, 2019

Here are my tests results:

  • SunOS 5.10U11
Ran 537 tests in 24.312s
FAILED (failures=5, errors=38, skipped=276)
  • SunOS 5.10U3
Ran 542 tests in 47.379s
FAILED (failures=14, errors=20, skipped=284)

As a reference (on SunOS 5.10U11 as 5.10U3 is not supported):

Ran 537 tests in 22.933s
FAILED (failures=5, errors=39, skipped=276)

Though it does not seem really stable.

BTW, I've added a get_sunos_update as your suggestion was 82 char long ;) And it makes the code clearer AFAICT :)

@giampaolo giampaolo merged commit 5deeca9 into giampaolo:master Dec 17, 2019
@vser1 vser1 deleted the fix_old_solaris branch December 17, 2019 14:00
@giampaolo
Copy link
Owner

Merged. While we're at it, could you paste the test output of latest solaris? Just to get an idea of what is broken.

@vser1
Copy link
Contributor Author

vser1 commented Dec 18, 2019

@giampaolo Could you confirm you received the log I've sent yesterday by email?

@giampaolo
Copy link
Owner

I did not. Did you send it to g.rodola AT gmail?

@giampaolo
Copy link
Owner

Can’t you just attach it here?

@vser1
Copy link
Contributor Author

vser1 commented Dec 18, 2019

Right, hadn't seen I could simply do a drag&drop ^^
psutil-5deeca9-SunOS5.10U11.log

@giampaolo
Copy link
Owner

Except for a couple, most of these failures are related to tests themselves and not real psutil issues. I pushed 2 commits which should fix something: 508ca73 and 04bcd8d. As for the others, I'll get around them when I'll put hands on a SunOS box (or feel free to make a PR if you wish). Regardless, thanks for this PR Étienne.

@vser1
Copy link
Contributor Author

vser1 commented Dec 18, 2019

Here is my latest result:

Ran 537 tests in 23.485s
FAILED (failures=7, errors=40, skipped=284)

I'll check the plain log a bit more tomorrow.

Cheers

clrpackages pushed a commit to clearlinux-pkgs/psutil that referenced this pull request Mar 10, 2020
Adam Johnson (1):
      Reference exact CVE in HISTORY for 5.6.6 (#1653)

Anselm Kruis (1):
      Fix a compile error in _psutil_linux.c if PSUTIL_HAVE_IOPRIO is false (#1695)

Caleb Bassi (1):
      Remove links to abandoned psutil ports in readme (#1669)

Giampaolo Rodola (129):
      fix compilation err on win
      Readme tidelift update (#1636)
      SunOS, fix #1642: PID 0 raises FileNotFoundError
      HERE: use realpath() instead of abspath() because of failures seen in giampaolo/psutil#1638 (comment)
      update HISTORY for #1646
      setup.py: print instructions if C compiler is not installed
      credits for #1648
      lint
      update/fix wait_procs() doc
      bump version
      windows: move net_connections() in socks.c
      win: split code into new process_utils.c
      highlight cmd.exe warnings/errs from VS
      Drop windows XP support (#1652)
      small refactoring
      move custom exceptions in _common.py
      #1652: also drop support for Windows Server 2003
      Windows: split C modules (#1655)
      #1652: remove inet_ntop.c
      winmake / uninstall: remove installation path from easy-install.pth file
      move AF_INET6 def in global.h
      use HeapAlloc() instead of malloc() around GetAdaptersAddresses
      rename global.c -> globals.c
      fix compiler warning
      fix compiler warning + remove cruft
      win: move ObjectNameInformation in ntextapi.h
      win: provide alias for HeapAlloc()
      cleanup ntextapi.h
      fix open_files() tests broken on windows due to case sensitiveness
      winmake.py: accept builtiple targets/args
      include ntextapi.h from globals.h so that we won't have to import it ever
      windows: move get_process_info() into process_info.c to make room for Cygwin
      fix compiler warnings; move some defss into process_info.h
      move PEB structs into ntextapi.h
      rm duplicated C code
      refactor win C code: use original WinAPI functions and remove psuil_ prefix
      move send_signal() code into windows module
      refactoring + remove dead code
      Win: fix compilation err on python 32 bit
      Win: fix segfault cause by FREE/MALLOC macros
      winmake.py: use argparse
      fix #1656: [Windows] Process.memory_full_info() raises AccessDenied even for the current user and os.getpid()
      fix some win tests
      just move stuff around
      get rid of globals.c; move stuff in _psutil_common.c
      #1652 / win / XP support: remove routine to collect files on win < vista
      refactoring test_contracts.py
      #1652: ionice(), remove code checking if we're on Win Vista+
      add contract tests for IOPRIO_ win constants
      add more types tests
      #1020: remove doc part mentionin open_files() on win is only able to list files living on the C drive
      [Windows] rewrite of open_files()  (#1660)
      #1652: remove win XP code path checking avilability of GetTickCount64
      #1652 remove Windows Vista references
      update setup.py classifiers
      print/set syscall origin when raising NSP or AD
      exec make install before 2 targets
      check MALLOC_ZERO ret code
      properly cleanup C thread
      fix #1662: QueryFullProcessImageNameW may fail with error code = 0 (Win API bug?)
      AD script: print AD percentage + elapsed time
      update HISTORY
      Add *new_only* parameter for process_iter() (#1667)
      set proper NTSTATUS error code
      [Windows] psutil_handle_from_pid() refactoring (#1668)
      Add CI testing for FreeBSD (#1671)
      fix Cirrus failure
      Cirrus: enable python 2
      run py2 tests on cirrus
      OpenBSD fixes (#1673)
      Properly handle PID type in C (#1672)
      update HISTORY
      small C refactoring
      [Windows] use NtQuerySystemInformation to determine process exe()  (#1677)
      print-ad.py: double sort
      [Windows] connections() refactoring (#1678)
      #1610: clarify in the doc what cpu_count means
      [Windows] speedup connections (#1679)
      fix #1674, SunOS: disk_partitions() may raise OSError.
      update doc
      fix #1650 [Linux] sensors_temperatures() no longer emit warnings on file not found (print debug msg instead)
      [Linux] disk_io_counters() ValueError when parsing /sys/block (#1684)
      #1672, #1682: SIZEOF_INT is not available on pypy3; assume that on systems where pid_t size can't be determined at runtime pid_t is an int
      Fix test errors for PYPY.
      skip memleak tests on PyPy: they are unreliable probably because of the JIT
      fix #1681 / linux / disk_partitions: show swap
      small refact
      fix #1627: [Linux] Process.memory_maps() can raise KeyError
      fix #1576: remove code duplicate for create_time / boot_time
      try to fix travis test
      merge from master
      windows: convert pid_t to DWORD
      Add support for PyPy on Windows (#1686)
      refactor doc
      fix pypy on Linux
      refactoring
      #1672: determine pid_t size at runtime in setup.py
      fix #1688: use python3 in GIT commit hook hashbang
      small refactoring to accomodate #1691
      #1672: warning pre-processor directive don't work on win + py2; also if struct.calcsize('l') < 8 assume int
      [Windows] increase precision of boot_time() and proc create_time() (#1693)
      refactoring: get rid of duplicated code; use one function to return (user, sys, create time)
      rename method
      remove unnecessary wrap_exceptions deco
      bind cpu_times() and create_time() with oneshot()
      rename var
      remove cache for proc_times(): unnecessary because create_time() is already cached
      refactoring: get rid of duplicated code; use one function to return (user, sys, create time)
      #1681, revert 00a3398
      small refactoring
      #1659: provide error message in case of bugged PYPY2 version
      fix tests
      divide test_system.py unit tests in multiple classes
      #1693: also increase precision of users()'s login time
      update doc
      update doc
      update doc
      update history for #1695
      update doc
      refactor print colors utils
      point all shebangs to python 3
      get rid of pip_install() code for py2; move everything in runner.py
      remove deprecation test: it fails intermittently because warnings uses a global state
      #1053 fix syntax incompatible with py2.6
      fix Makefile for freebsd
      revert #1667 process_iter() new_only param
      revert process_iter() exactly how it was pre #1667
      Add C linter script (#1698)
      fix KeyError

Javad Karabi (1):
      sensors_temperatures: also search /sys/devices/platform/coretemp.* for temperatures (#1648)

Jon Dufresne (1):
      Remove use of deprecated setuptools test_suite & tests_require (#1696)

Kamil Rytarowski (1):
      Add special case for psutil_proc_cwd/NetBSD (#1538)

Mike Hommey (1):
      Future-proof disk_io_counters on Linux. (#1665)

Po-Chuan Hsieh (1):
      Fix Process on FreeBSD 12.0+ i386 (#1646)

Thomas Güttler (1):
      total physical memory (exclusive swap). (#1634)

Tim Gates (1):
      Fix simple typo: whish -> wish (#1640)

johnthagen (1):
      Enable PyPy3 on CI (#1682)

vser1 (1):
      [Solaris] Fix #1637 (#1638)
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