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

Add support for PyPy to 'python' tool. #23

Closed
wants to merge 2 commits into from
Closed

Conversation

stefanseefeld
Copy link
Owner

No description provided.

@stefanseefeld stefanseefeld self-assigned this Oct 14, 2020
@stefanseefeld stefanseefeld mentioned this pull request Oct 14, 2020
@codecov-io
Copy link

codecov-io commented Oct 14, 2020

Codecov Report

Merging #23 into develop will decrease coverage by 0.06%.
The diff coverage is 63.15%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop      #23      +/-   ##
===========================================
- Coverage    72.91%   72.84%   -0.07%     
===========================================
  Files           71       71              
  Lines         4065     4081      +16     
===========================================
+ Hits          2964     2973       +9     
- Misses        1101     1108       +7     
Impacted Files Coverage Δ
src/faber/tools/python.py 76.40% <61.11%> (-4.16%) ⬇️
src/faber/artefacts/python.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ff4f894...8b83e6b. Read the comment docs.

self.libpath = join(prefix, 'libs')
self.lib = 'python' + version
if impl == 'CPython':
version = self.check_python('import sys; print("%d%d"%sys.version_info[0:2])')
Copy link

Choose a reason for hiding this comment

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

I think you are looking for sysconfig.get_config_var('py_version_nodot') since on python 3.10 this will become 3_10 not 310 (at least that is the direction from python/cypthon#20333)

Copy link
Owner Author

Choose a reason for hiding this comment

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

Thanks for the tip !

flags = self.check_sysconfig('get_config_var("LINKFORSHARED")')
if flags is not None:
flags=flags.split()
self.ldflags = ldflags(*flags) # TODO: use them !
Copy link

Choose a reason for hiding this comment

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

does this need an else clause?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Perhaps, but as this code is merely shifting these lines around, I'd rather not touch the logic within this PR.

@mattip
Copy link

mattip commented Oct 14, 2020

You may want to think about changing

    def check_python(self, cmd):
        return subprocess.check_output([self.command, '-c', cmd]).decode().strip()

    def check_sysconfig(self, cmd):
        r = self.check_python('import distutils.sysconfig as c; print(c.{})'.format(cmd))
        return r if r != 'None' else ''
  • to use check_output(... , universal_newlines=True) to get strings as output directly with no decode needed
  • to use import sysconfig rather than import distutils.sysconfig since it should be cheaper and more future-proof (import distutils can trigger an import of setuptools)
  • to use f-strings f'import ... print(c.{cmd})' since they are all the rage and all the cool kids use them (but not on the EOL python3.5)

@stefanseefeld
Copy link
Owner Author

Thanks for the suggestions. As to the difference between sysconfig and distutils.sysconfig, there are unfortunately a few crucial additions in the latter that I need. E.g. `https://github.com/python/cpython/blob/master/Lib/distutils/sysconfig.py#L87-L124)

(Python's packaging and distribution support is a big mess !)

The other two suggestions are fine. I'm a fan of f-strings, too, and as I just recently decided to no longer support Python 2.7, will happily clean up the code incrementally.

@mattip
Copy link

mattip commented Oct 14, 2020

E.g. `https://github.com/python/cpython/blob/master/Lib/distutils/sysconfig.py#L87-L124)

sysconfig.get_config_var('INCLUDEPY') ?

@stefanseefeld
Copy link
Owner Author

Merged manually.

@stefanseefeld
Copy link
Owner Author

I just noticed that the sysconfig.get_config_var("EXT_SUFFIX") trick works on Python3, but not Python2 (where it returns None. @mattip any idea what I may have to substitute it with there ?

@mattip
Copy link

mattip commented Oct 16, 2020

On CPython2 you can do

import imp  # python2 only, will emit deprecation warning on python3
[x[0] for x in imp.get_suffixes() if x[2] == imp.C_EXTENSION]

which returns

['.x86_64-linux-gnu.so', '.so', 'module.so']

for me, so take ret[0]. I think PyPy2 does export sysconfig.get_config_var("EXT_SUFFIX"), so you might want to do some if None kind of check.

@stefanseefeld stefanseefeld deleted the pypy branch February 9, 2021 01:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants