Fix the non cython build for cpp extensions #19707
Conversation
|
Hello @hexgnu! Thanks for updating the PR. Cheers ! There are no PEP8 issues in this Pull Request. 🍻 Comment last updated on February 19, 2018 at 23:38 Hours UTC |
setup.py
Outdated
| sourcefile = pyxfile[:-3] + extension | ||
| msg = ("{extension}-source file '{source}' not found.\n" | ||
| "Run 'setup.py cython' before sdist.".format( | ||
| source=cfile, extension=extension)) |
There was a problem hiding this comment.
this is a mistake fixing now
Codecov Report
@@ Coverage Diff @@
## master #19707 +/- ##
==========================================
+ Coverage 91.6% 91.6% +<.01%
==========================================
Files 150 150
Lines 48864 48862 -2
==========================================
- Hits 44763 44762 -1
+ Misses 4101 4100 -1
Continue to review full report at Codecov.
|
|
@chris-b1 do you have any experience with Cython and cpp? Does this look OK to you? |
|
The pip build stuff looks OK here: https://travis-ci.org/pandas-dev/pandas/jobs/341700234#L2422 |
| @@ -618,7 +622,8 @@ def pxd(name): | |||
| '_libs.window': { | |||
| 'pyxfile': '_libs/window', | |||
| 'pxdfiles': ['_libs/skiplist', '_libs/src/util'], | |||
| 'language': 'c++'}, | |||
| 'language': 'c++', | |||
| 'suffix': '.cpp'}, | |||
There was a problem hiding this comment.
rather than adding 'suffix' any reason not to base the suffix on language='c++'?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
|
One question, but this looks good to me. @hexgnu, certainly welcome any refactoring of |
gfyoung
added
Build
|
This seems OK. Any objections @jreback? It'd be good to get that build going on Travis again. |
| @@ -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: | |||
There was a problem hiding this comment.
can you add some comments on what is going on here
|
K I moved msgpack building into the same loop and added a comment. |
|
thanks. hopefully this fixes the pip build. |
git diff upstream/master -u -- "*.py" | flake8 --diffcc @TomAugspurger @jreback
This should fix the problem with building with a non Cython environment. The problem is that we assume extensions are C everywhere and I didn't realize that this was a test path.
Also setup.py might be in need of a rethink, there's a lot of special conditions that I think could be cleaned up with some better formatting.