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

BUILD from sdist broken if cython available #1606

Closed
josef-pkt opened this issue Apr 19, 2014 · 10 comments

Comments

Projects
None yet
2 participants
@josef-pkt
Copy link
Member

commented Apr 19, 2014

I think there is a mistake in setup.py

the sdist doesn't distribute .pyx files anymore
However, if the install python has cython available, then it checks for .pyx files not for .c files.

File "c:\users\josef\appdata\local\temp\pip-cak2dv-build\setup.py", line 323, in check_cython_extensions
    """ % src)

Exception: Cython-generated file 'statsmodels\nonparametric/_smoothers_lowess.pyx' not found.

        Cython is required to compile statsmodels from a development branch.
        Please install Cython or download a source release of statsmodels.
@rgommers

This comment has been minimized.

Copy link
Member

commented Apr 19, 2014

The .pyx files should still be included in the sdist output; source releases should contain all sources. The generated C should be shipped in addition to, not instead of pyx.

I suggest to replace the custom Cython invokation in setup.py by tools/cythonize from scipy.

@josef-pkt

This comment has been minimized.

Copy link
Member Author

commented Apr 19, 2014

we removed the .pyx files from the sdist in #1200

@rgommers

This comment has been minimized.

Copy link
Member

commented Apr 19, 2014

Ah, missed that PR. That was just the quickest way right, not a principled decision? I'd think you want to keep them in there.

@josef-pkt

This comment has been minimized.

Copy link
Member Author

commented Apr 19, 2014

Thinking about it, we should revisit #1200 and #1195
we don't want to install the .pyx, but we can still include them in the sdist.
keep them out of manifest.in, but add them to package_data
I don't see why they are not included as sources

@rgommers

This comment has been minimized.

Copy link
Member

commented Apr 19, 2014

There's no need to add them to package_data, just don't exclude them in MANIFEST.in and use tools/cythonize.py to use C files unless the pyx timestamp says it has changes in it. That's also useful for the dev version.

@josef-pkt

This comment has been minimized.

Copy link
Member Author

commented Apr 19, 2014

If they are in MANIFEST.in, then they will also be installed. If they are only in package data, then they will be in the sdist but will not be installed, IIRC.

@rgommers

This comment has been minimized.

Copy link
Member

commented Apr 19, 2014

That's the wrong way around. package_data includes files in MANIFEST plus installs them: https://docs.python.org/3/distutils/setupscript.html#distutils-installing-package-data

@rgommers

This comment has been minimized.

Copy link
Member

commented Apr 21, 2014

I'm a big fan of initiatives like "bug week"! You want me to have a go at this one? I'll make sure to double check the install behavior of what I said above.

@josef-pkt

This comment has been minimized.

Copy link
Member Author

commented Apr 21, 2014

Please do

The way I see it:
fix the pyx check
add .pyx to sdist but not to installed directories
or both

I guess I mixed up MANIFEST.in and package_data. I never understood the python documentation for this, and still don't, and always only succeeded with lots of trial end error.

rgommers added a commit to rgommers/statsmodels that referenced this issue May 11, 2014

@rgommers

This comment has been minimized.

Copy link
Member

commented May 11, 2014

OK bug week is over but I got something done. Issue was some setuptools-specific behavior described in https://pythonhosted.org/setuptools/setuptools.html#including-data-files.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.