[Python-Dev] dear core-devs

[Michael Felt]

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 test_uuid 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 test_uuid is making platform specific
> assumptions that are not true on AIX and possibly other platforms.
> So, do something like:
>
>     
>     _IS_AIX = sys.platform.startswith("aix")
better name.
>
>     _HAVE_MACADDR = (os.name == 'posix' and not _IS_AIX)
AIX has MACADDR, but formatted with '.' rather than ':' and uses a
single hex-digit when value between dots is < 16 (decimal)
>
>     @unittest.skipUnless(_HAVE_MACADDR, 'requires Posix with macaddr')
>     def test_arp_getnode(self):
>         ...
>
> The _HAVE_MACADDR test is relatively simple and clear, does this
> platform have this capability.  Later in the code, a check for
> _HAVE_MACADDR 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:
>
> _HAVE_MACADDR = False
> if os.name == 'posix':
>     if <check arp to see if it supports macaddr>:
>         _HAVE_MACADDR = True
>
> Hope that is helpful.
All feedback and constructive criticism is helpful. Thank you for taking
the time to share!
>
>   Neil