This is an archive of the discontinued LLVM Phabricator instance.

[LNT] Python 3 support: Set up (client) setup requirements
ClosedPublic

Authored by hubert.reinterpretcast on Sep 12 2019, 8:54 PM.

Details

Summary

Changes required to allow setup of a LNT client with Python 3.

  • Use PyModule_Create instead of Py_InitModule
  • Drop argparse and wsgiref install requirements
  • Require Python 2.7 or higher in setup.py; Python 2.7 is needed for some Python 3 support idioms

Diff Detail

Repository
rL LLVM

Event Timeline

hubert.reinterpretcast marked 2 inline comments as done.Sep 12 2019, 8:57 PM
hubert.reinterpretcast added inline comments.
lnt/testing/profile/cPerf.cpp
819 ↗(On Diff #220038)

I think this could be 0 (no space required for module state), but that might be asking for unnecessary trouble.

setup.py
34 ↗(On Diff #220038)

This is unfortunate, but this setup.py is built off of pip internal interfaces (which I understand to be a departure from current best practices), and environment markers are not supported by older versions of pip anyway.

thopre added inline comments.Sep 13 2019, 2:17 AM
requirements.client.txt
15 ↗(On Diff #220038)

I believe this is part of Python 2.7 already. IMO the first thing to do in porting LNT would be to mandate Python 2.7 or later and add the 'Programming Language :: Python :: 2 :: Only' tag to setup.py

setup.py
34 ↗(On Diff #220038)

Indeed but this can be fixed after LNT is migrated to Python3 so I appreciate this immediate solution.

hubert.reinterpretcast marked an inline comment as done.Sep 13 2019, 5:03 AM
hubert.reinterpretcast added inline comments.
requirements.client.txt
15 ↗(On Diff #220038)

I believe so too (re: availability of the subject packages in Python 2.7), but I'm not too keen on increasing the possible effects of the patch set. There are more patches necessary to address changes to the behaviour of import, text encoding assumptions, etc.

As for the 'Programming Language :: Python :: 2 :: Only' tag, I'm loath to add it because my use of LNT is limited, meaning that after adding it now, I won't be confident on removing it.

I'll look into adding python_requires='>=2.7' to the setup call in this patch.

requirements.client.txt
15 ↗(On Diff #220038)

I missed the implication: We can just remove the two package requirements after mandating Python 2.7 and not maintain different lists for 2.7 and 3. I guess that is enough of a win to make that change.

This revision is now accepted and ready to land.Sep 13 2019, 10:37 AM
  • Require 2.7 or higher; remove argparse and wsgiref
hubert.reinterpretcast marked 3 inline comments as done.Sep 14 2019, 2:35 PM
hubert.reinterpretcast edited the summary of this revision. (Show Details)Sep 14 2019, 2:39 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 15 2019, 3:29 PM