[Python-Dev] dear core-devs (original) (raw)

Michael Felt aixtools at felt.demon.nl
Wed Oct 3 13:22:26 EDT 2018

On 10/3/2018 1:46 AM, Neil Schemenauer wrote:

On 2018-10-02, Michael Felt wrote:

I am sorry, for myself obviously - but also for Python. Obviously, I am doing it all wrong - as I see lots of other issues being picked up immediately. I'm not sure that's the case. There are a lot of PRs or bugs that sit there without getting reviews. The problem is that few (or no) core developers get paid to work on Python. So, the time they spend is motivated by their specific "itch". Getting reviews on any PR is difficult, even for core developers. In their case, they have to option of forcing the issue, I guess. This is a problem we should try to deal with somehow. Turning off valuable contributors like you is bad. I'm not sure how to do it though. At the core Python sprint in September there was some talk about how CPython developers might get funding. Maybe that could help deal with the backlog of reviews required.

And, while you may not give a damn about anything other than Windows, macos and/or Linux - there are other platforms that would like a stable Python. There is probably some truth in not caring about other platforms. The problem from the reviewer perspective is the question of "what is the potential downsides of this PR vs what are the benefits?". The safest thing is to not approve the PR. No core developer wants to be the person who broke CPython. You must admit, AIX is an extremely niche platform at this point. I bet if you picked 1000 software developers at random, it would be likely that zero of them have ever used AIX. So, it's not that we don't care at all about AIX but that the cost/benefit equation makes accepting AIX specific changes more difficult. Nods. However - this is a chicken/egg issue (imho). AIX is seen a weak platform because noone has ever tackled these. When I started on this I had never expected to have found a resolution to them all.

Platforms have differences and when the tests miss that difference that the tests give a false result. e.g., one accepted PR was because AIX libc printf() output for printf(NULL) is "" while other platforms output "(null)".

One specific suggestion I have about your PR is to try to make your changes not AIX specific. Or at least, make the AIX checking as localized as possible. So, as an example, in testuuid you have: notAIX = not sys.platform.startswith("aix") a) I thought/hoped this was better practice and performance - calling  sys.platform.startswith("aix")only once, rather than X times. b) more maintainable (e.g., change to not platform.system() c) iirc - this got changed to AIX = ...., and throughout the test is "if not AIX"... then later in the module you check that flag. While that is the most direct approach to fixing the issue and making the test pass, it is not good for the long term maintainability of the code. You end up with boolean flags like notAIX spread about the logic. Over time, code like that becomes a nightmare to maintain. Instead, I would suggest testuuid is making platform specific assumptions that are not true on AIX and possibly other platforms. So, do something like:

ISAIX = sys.platform.startswith("aix") better name. HAVEMACADDR = (os.name == 'posix' and not ISAIX) AIX has MACADDR, but formatted with '.' rather than ':' and uses a single hex-digit when value between dots is < 16 (decimal) @unittest.skipUnless(HAVEMACADDR, 'requires Posix with macaddr') def testarpgetnode(self): ... The HAVEMACADDR test is relatively simple and clear, does this platform have this capability. Later in the code, a check for HAVEMACADDR is also quite clear. If someone comes along with another platform that doesn't support macaddr, they only have to change one line of code. This kind of capability checking is similar to what happened with web browsers. In that case, people discovered that checking the User Agent header was a bad idea. Instead, you should probe for specific functionality and not assume based on browser IDs. For the macaddr case, is there some way to you probe the arp command to see if supports macaddr? I suppose if someone had written the original test with "check program to see if ..." it would have worked already. I am trying to get current tests to work with minimal changes.

I am certainly not "blaming" anyone for not knowing this unique behavior of this platform. Before debugging this I did not know of the difference either. I agree that wherever a generic resolution is possible - it should be done. However, as an "outsider" I do not feel empowered enough to propose such a change to existing (and well proven for other platforms) tests.

That way your test doesn't have to include any AIX specific check at all. Further, it would have some hope of working on platforms other than AIX that also don't support macaddr but are POSIX and have 'arp'. The code could be something like:

HAVEMACADDR = False if os.name == 'posix': if : HAVEMACADDR = True Hope that is helpful. All feedback and constructive criticism is helpful. Thank you for taking the time to share! Neil

More information about the Python-Dev mailing list