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

Include pyx in sdist but don't install #1666

Merged
merged 6 commits into from May 22, 2014

Conversation

Projects
None yet
4 participants
@rgommers
Copy link
Member

commented May 11, 2014

Also a cleanup of setup.py included. cythonize.py adapted from scipy. See gh-1606 for discussion.

@rgommers rgommers added build and removed type-cleanup labels May 11, 2014

@@ -510,5 +439,5 @@ def get_data_files():
cmdclass = cmdclass,
packages = packages,
package_data = package_data,
include_package_data=True,
include_package_data=False, # True will install all files in repo

This comment has been minimized.

Copy link
@jseabold

jseabold May 12, 2014

Member

Does this have any effect on #907?

This comment has been minimized.

Copy link
@rgommers

rgommers May 12, 2014

Author Member

I can check a few ways by hand, but the TravisCI tests would fail if those didn't install right?

This comment has been minimized.

Copy link
@josef-pkt

josef-pkt May 12, 2014

Member

It looks like I haven't removed the workaround (moving the files to another directory), so only the license and a text file would be missing.

This comment has been minimized.

Copy link
@rgommers

rgommers May 14, 2014

Author Member

This was due to some typos in the name libqsturng, see last commit. Terrible name by the way......

This comment has been minimized.

Copy link
@josef-pkt

josef-pkt May 14, 2014

Member

Very good, thanks for finding this.
I need to check the spelling of libqsturng several times when I try to write it, and still I make typos. I remember what it's supposed to stand for when I work on it and forget it 5 minutes later
I think it's studentized range statistic
and the q stands for quantiles.

This comment has been minimized.

Copy link
@jseabold

jseabold May 14, 2014

Member

Yikes. I always thought it was someone's name...

@rgommers

This comment has been minimized.

Copy link
Member Author

commented May 14, 2014

Is there a point to the empty setup.cfg file by the way, or can that be removed?

@josef-pkt

This comment has been minimized.

Copy link
Member

commented May 14, 2014

My guess is it can be removed. I don't see any use for it. And I don't know why we have it.
Brief history check: I removed (renamed) it once, but then added it again.
I'm pretty sure this is there by accident.

@rgommers

This comment has been minimized.

Copy link
Member Author

commented May 14, 2014

Then I'll take it out. I've already done :e se<TAB> in Vim several time and each time I open the .cfg by accident:)

@jseabold

This comment has been minimized.

Copy link
Member

commented May 19, 2014

Would you mind also removing the tsa/setup.py as part of this PR? It's old cruft and is no longer used AFAICT.

@rgommers

This comment has been minimized.

Copy link
Member Author

commented May 19, 2014

Sure, done.

@coveralls

This comment has been minimized.

Copy link

commented May 19, 2014

Coverage Status

Coverage remained the same when pulling 98379ba on rgommers:sdist-build into b472807 on statsmodels:master.

include statsmodels/stats/libqsturng/tests/bootleg.csv
include statsmodels/stats/libqsturng/CH.r
include statsmodels/stats/libqsturng/LICENSE.txt
include statsmodels/regression/tests/results/leverage_influence_ols_nostars.txt

This comment has been minimized.

Copy link
@jseabold

jseabold May 21, 2014

Member

Can you explain these changes? Is it the case that files that files in MANIFEST.in are specified in addition to (not instead of) those specified in package_data? If not, don't we need to include these files in the sdist?

This comment has been minimized.

Copy link
@rgommers

rgommers May 21, 2014

Author Member

Indeed. What's in package_data gets put into sdist output as well as installed. What's only in MANIFEST.in gets into sdist output but doesn't get installed.

@jseabold

This comment has been minimized.

Copy link
Member

commented May 21, 2014

This looks good to me. Thanks. Would you mind rebasing? Then I'll hit the green button, so @ChadFulton can work off these changes.

@josef-pkt

This comment has been minimized.

Copy link
Member

commented May 22, 2014

Looks all fine to me.
I just tried with tox for python 3.3 and it works again. And spot checking the files, they look good (no pyx but all the qsturng files in the installed version, pyx are in sdist.)

Also, it didn't reuse the old .c files from the source checkout (inplace install). I only realized this because it told me my cython was too old.

@rgommers

This comment has been minimized.

Copy link
Member Author

commented May 22, 2014

rebased.

@coveralls

This comment has been minimized.

Copy link

commented May 22, 2014

Coverage Status

Coverage remained the same when pulling bf85b41 on rgommers:sdist-build into 4b94f8e on statsmodels:master.

jseabold added a commit that referenced this pull request May 22, 2014

Merge pull request #1666 from rgommers/sdist-build
BLD: Include pyx in sdist but don't install

@jseabold jseabold merged commit d27b085 into statsmodels:master May 22, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@jseabold

This comment has been minimized.

Copy link
Member

commented May 22, 2014

Thanks. That intermittent hanging test is worrisome, but unrelated to this PR.

@josef-pkt

This comment has been minimized.

Copy link
Member

commented May 22, 2014

Which intermittent hanging test? I haven't seen one in a few weeks.

@jseabold

This comment has been minimized.

Copy link
Member

commented May 23, 2014

This PR had one for 2.7 and timed out. I don't know what test it was.

Sent from my mobile

PierreBdR pushed a commit to PierreBdR/statsmodels that referenced this pull request Sep 2, 2014

Merge pull request statsmodels#1666 from rgommers/sdist-build
BLD: Include pyx in sdist but don't install
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.