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

backport optional extension building for python 2.7 #107

Merged
merged 2 commits into from
Aug 10, 2018
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 22 additions & 4 deletions setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,20 @@

try:
from setuptools import setup, Extension
from setuptools.command.build_ext import build_ext as base_build_ext
except ImportError:
from distutils.core import setup, Extension
import warnings
import sys
val = sys.exc_info()[1]

warnings.warn("import of setuptools failed %r" % val)
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 think this warning is necessary -- it falls back to distutils anyway. It seems to me this was added to debug a spelling mistake in an earlier commit. Now that we're importing it correctly, let's just leave this out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i'd like to leave it in so user see that setuptools is missing
from a packaging standpoint i'd like to completely remove the support for plain distutils if this was my project

from distutils.core import setup, Extension
from distutils.command.build_ext import build_ext as base_build_ext

import os
import re
import sys
import logging

# Get version without importing scandir because that will lock the
# .pyd file (if scandir is already installed) so it can't be
Expand All @@ -25,8 +33,18 @@
long_description = f.read()


# the extension is optional since in case of lack of c the api
# there is a ctypes fallback and a slow python fallback
class BuildExt(base_build_ext):
Copy link
Owner

Choose a reason for hiding this comment

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

Is this code copied from somewhere, the Python 3.x optional=true code or something? Just curious.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

its a small reimplementation of the python3 mechanism, which has a context managers and a few more bits to sort it out


# the extension is optional since in case of lack of c the api
# there is a ctypes fallback and a slow python fallback

def build_extension(self, ext):
try:
base_build_ext.build_extension(self, ext)
except Exception:
exception = sys.exc_info()[0]
logging.warn("building the %s failed with %s", ext.name, exception)

extension = Extension('_scandir', ['_scandir.c'], optional=True)
Copy link
Owner

Choose a reason for hiding this comment

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

Won't this still fail as we still always include the optional=True flag here? Or is it just a warning and so we're ignoring it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

its just a warning on python2, i'm ignoring it here

the `build_ext command is what's interpreting those



Expand Down Expand Up @@ -58,5 +76,5 @@
'Programming Language :: Python :: 3.5',
'Programming Language :: Python :: 3.6',
'Programming Language :: Python :: Implementation :: CPython',
]
], cmdclass={'build_ext': BuildExt},
)