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

[Windows] Fix for net_io_counters wrapping after 4.3GB due to MIB_IFROW #835

Merged
merged 14 commits into from Jun 12, 2016
Merged

Conversation

jomann09
Copy link
Contributor

@jomann09 jomann09 commented Jun 8, 2016

I am updating this with a new pull request. The other one was very old and didn't quite work with the current version of psutil. See more #816

… using DWORD. Updated to use MIB_IF_ROW2 which gives ULONG values instead. This causes more breaking changes for Windows XP and all Windows versions less than Vista / Server 2008 meaning that it should have no problems working on Vista / Server 2008 and beyond.
@giampaolo
Copy link
Owner

giampaolo commented Jun 8, 2016

Mmm have you tested this on Win XP? I don't see the ifdef conditionals around MIB_IF_ROW2 which I would expect. Also please update HISTORY file describing what have been fixed here.

@jomann09
Copy link
Contributor Author

jomann09 commented Jun 9, 2016

Oh, sorry, I gave some findings on XP in #816 , psutils 4.0.0+ already does not work with Windows XP so I put this PR in. Something between 3.4.2 and 4.0.0 is causing it to not work in Win XP. If you'd like me to try making psutil 4.3.0 compatible with XP I can.

@jomann09
Copy link
Contributor Author

Interesting, it seems like although the pip installs didn't work ... manually compiling it on Win XP does still work. I will need to find out a good way to do this...

@jomann09
Copy link
Contributor Author

I am so sorry for all those pushes in a row! It was the easiest way to test it across multiple systems that I had set up to test compiling with. Compiling is working in both Win XP and Win 7 (my normal workstation) though so hopefully the final tests will show it building OK.

@coveralls
Copy link

coveralls commented Jun 11, 2016

Coverage Status

Changes Unknown when pulling 7a8c268 on jhomann:master into * on giampaolo:master*.

@jomann09
Copy link
Contributor Author

jomann09 commented Jun 11, 2016

Well that's crap. I fixed the tests but it looks like the final test in AppVeyor gave an error:

======================================================================
FAIL: test_parent_ppid (test_process.TestProcess)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\projects\psutil\psutil\tests\test_process.py", line 1145, in test_parent_ppid
    self.assertNotEqual(p.ppid(), this_parent)
AssertionError: 1996 == 1996

----------------------------------------------------------------------
Ran 354 tests in 8.188s

FAILED (failures=1, skipped=198)

Which is interesting since the last few commits worked just fine and that was never edited by me.

@coveralls
Copy link

coveralls commented Jun 11, 2016

Coverage Status

Changes Unknown when pulling c105688 on jhomann:master into * on giampaolo:master*.

@jomann09
Copy link
Contributor Author

jomann09 commented Jun 11, 2016

For the record I am done with my commits, I believe everything is in order. The tests that are failing now seem like random false positives.

Please take a look at let me know if you would like anything else one for it.

#include <ws2tcpip.h>
#endif
Copy link
Owner

Choose a reason for hiding this comment

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

I don't understand: you include the same module no matter what the windows version is so why using ifdef in the first place?

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 actually doesn't need to be included. The issue was that it matters where it's located in the list of includes, but after further testing, this header is not required for Win XP.

@giampaolo
Copy link
Owner

I added some inline comments. If you reintroduced Win XP support which was broken please specify this in HISTORY. ALso please update CREDITS including your name and contribution. Thanks.

Updated HISTORY to explain better that Win XP still uses 32bit values
Reverted test code, will add in a different PR
@jomann09
Copy link
Contributor Author

I've updated with the proper changes! I definitely should have looked up the else, that was stupid of me. Again, sorry for making so many commits for this change.

As for re-introducing Win XP support, it seems that building 4.0.0-4.2.0 works however installing it from pip does not. That being said, it still had support for XP if you built it manually, which I tried and it worked, so we are definitely good there.

@giampaolo
Copy link
Owner

You can do as many pushes as you want: AppVeyor tests are for free and are designed exactly for that (make our life easier). =) Patch looks great, I'm merging it. Thanks a lot for working on this. It was a long standing issue and the solution you came up with is perfect. If you will ever feel about taking some other Windows issue from the bug tracker you're more than welcome.

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