Skip to content

Fix the non cython build for cpp extensions #19707

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Feb 19, 2018
Merged

Fix the non cython build for cpp extensions #19707

Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7666765
Fix the non cython build for cpp extensions
hexgnu Feb 15, 2018
0b0f318
sourcefile should be here not cfile
hexgnu Feb 15, 2018
ea7c9e2
Fix linting error
hexgnu Feb 15, 2018
d1e1a2e
Build msgpack in the same extension loop
hexgnu Feb 17, 2018
d3a8757
add comment
hexgnu Feb 17, 2018
641d0ac
Merge branch 'master' into PR_TOOL_MERGE_PR_19707
jreback Feb 19, 2018
42a80c3
lint
jreback Feb 19, 2018
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
25 changes: 16 additions & 9 deletions setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -311,7 +311,6 @@ class CheckSDist(sdist_class):
'pandas/_libs/missing.pyx',
'pandas/_libs/reduction.pyx',
'pandas/_libs/testing.pyx',
'pandas/_libs/window.pyx',
'pandas/_libs/skiplist.pyx',
'pandas/_libs/sparse.pyx',
'pandas/_libs/parsers.pyx',
Expand All @@ -331,19 +330,24 @@ class CheckSDist(sdist_class):
'pandas/_libs/writers.pyx',
'pandas/io/sas/sas.pyx']

_cpp_pyxfiles = ['pandas/_libs/window.pyx']

def initialize_options(self):
sdist_class.initialize_options(self)

def run(self):
if 'cython' in cmdclass:
self.run_command('cython')
else:
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add some comments on what is going on here

for pyxfile in self._pyxfiles:
cfile = pyxfile[:-3] + 'c'
msg = ("C-source file '{source}' not found.\n"
"Run 'setup.py cython' before sdist.".format(
source=cfile))
assert os.path.isfile(cfile), msg
pyx_files = [(self._pyxfiles, 'c'), (self._cpp_pyxfiles, 'cpp')]

for pyxfiles, extension in pyx_files:
for pyxfile in pyxfiles:
sourcefile = pyxfile[:-3] + extension
msg = ("{extension}-source file '{source}' not found.\n"
"Run 'setup.py cython' before sdist.".format(
source=sourcefile, extension=extension))
assert os.path.isfile(sourcefile), msg
sdist_class.run(self)


Expand Down Expand Up @@ -618,7 +622,8 @@ def pxd(name):
'_libs.window': {
'pyxfile': '_libs/window',
'pxdfiles': ['_libs/skiplist', '_libs/src/util'],
'language': 'c++'},
'language': 'c++',
'suffix': '.cpp'},
Copy link
Contributor

Choose a reason for hiding this comment

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

rather than adding 'suffix' any reason not to base the suffix on language='c++'?

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 guess I just saw it as more of a configuration piece that belonged in a dictionary over branches in the code.

I could do something like:

    if suffix == '.pyx':
        source_suffix = '.pyx'
    elif data.get('language', 'c') == 'c++':
        source_suffix = '.cpp'
    else:
        source_suffix = '.c'

But that feels a bit verbose. Thoughts?

Copy link
Contributor

@chris-b1 chris-b1 Feb 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Not a huge deal, but that feels cleaner to me. Reading the config I would probably guess 'suffix' controls the suffix of the output of cython, which it woudln't.

'_libs.writers': {
'pyxfile': '_libs/writers',
'pxdfiles': ['_libs/src/util']},
Expand All @@ -628,7 +633,9 @@ def pxd(name):
extensions = []

for name, data in ext_data.items():
sources = [srcpath(data['pyxfile'], suffix=suffix, subdir='')]
source_suffix = suffix if suffix == '.pyx' else data.get('suffix', '.c')

sources = [srcpath(data['pyxfile'], suffix=source_suffix, subdir='')]
pxds = [pxd(x) for x in data.get('pxdfiles', [])]
if suffix == '.pyx' and pxds:
sources.extend(pxds)
Expand Down