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

Cythonized files now deleted after setup. (closes #667 / #629) #692

Merged
merged 1 commit into from
Feb 9, 2016

Conversation

mnmelo
Copy link
Member

@mnmelo mnmelo commented Feb 2, 2016

Default behavior now depends on release or dev status. When in dev mode
default is to use Cython and delete cythonized files. When in release
mode default is to skip Cython but keep existing cythonized files.

setup.py now reads the version string directly from version.py.

Environment variable handling in setup.py enhanced to recognize strings
with boolean meaning.

I tried to keep the burden on the release manager to a minimum:

  • The version string now only needs to be set in version.py;
  • For a release build, generating the cythonized files for shipping is as easy as issuing:
MDA_USE_CYTHON=1 python setup.py build
  • For a development build cythonized files are generated by default, and deleted after compilation. To keep them just run:
MDA_KEEP_CYTHONIZED=1 python setup.py build

@@ -67,6 +67,14 @@
cython_found = False
cmdclass = {}

# Find our own release code (by shamelessly running MDAnalysis/version.py,
# which brings the __version__ variable into the current namespace.)
with open("MDAnalysis/version.py") as f:
Copy link
Member

Choose a reason for hiding this comment

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

Does this always work even when the package is installed in weird ways? Ie is MDAnalysis/version.py always there? Or would it be better to use resource_filename like we do with the test files?

Copy link
Member Author

Choose a reason for hiding this comment

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

After I submitted I realized that this is relatively fragile (the tests' setup.py can't do the same, for instance). It was mostly a convenience so that you'd only need to change the version string in one place. It's not even directly related to the issue at hand.
I'll revert it.

@richardjgowers
Copy link
Member

If we're removing the pyx->c files from develop, doesn't that need to be done in this PR?

@jbarnoud
Copy link
Contributor

jbarnoud commented Feb 3, 2016

@richardjgowers It would make sense indeed.

@kain88-de kain88-de changed the title Cythonized files now deleted after setup. (closes #667) Cythonized files now deleted after setup. (closes #667 / #629) Feb 3, 2016
@mnmelo
Copy link
Member Author

mnmelo commented Feb 6, 2016

Duh... git add no longer adds deletions by default and I didn't pay attention to the warning notice.

One important aspect:
From what I could gather, the release process that @richardjgowers follows involves updating version strings and documentation on develop and then merging into master. This means that there are brief points in develop history with non-dev version strings (see tag release-0.13.0, for instance).

Any work branched from one of these release points carries the non-dev version string. Since this PR changes behavior based on those strings, devs might run into non-obvious inconsistencies.
I don't know if this is a showstopper to my changes, or if we should become more consistent about the version strings (perhaps by only updating them in master, after merging develop).

@richardjgowers
Copy link
Member

@mnmelo so that's correct, but I only ever push a dev suffixed version to develop, so you'd have to put effort in to branch from there.

No worries about the version number, 2 places is ok

@mnmelo mnmelo force-pushed the issue-667-untrack-cythonized branch 2 times, most recently from 36ff92d to 00e3c04 Compare February 8, 2016 02:18
@mnmelo
Copy link
Member Author

mnmelo commented Feb 8, 2016

Ok, with that out of the way this should be done (I didn't add anything to CHANGELOG, as this is a dev-side change. Let me know if you think I should).

@jbarnoud
Copy link
Contributor

jbarnoud commented Feb 8, 2016

There something to do with the travis build. It make no sense anymore to have a no-cython build. Having two build continue to make sense, though, to have a full and a minimal dependency builds.

@mnmelo mnmelo force-pushed the issue-667-untrack-cythonized branch from 00e3c04 to 243dffb Compare February 9, 2016 01:56
Default behavior now depends on release or dev status. When in dev mode
default is to use Cython and delete cythonized files. When in release
mode default is to skip Cython but keep existing cythonized files.

setup.py now reads the version string directly from version.py.

Environment variable handling in setup.py enhanced to recognize strings
with boolean meaning (fixes #629).

Travis' minimal build now also rebuilds cythonized files.
@mnmelo mnmelo force-pushed the issue-667-untrack-cythonized branch from 243dffb to 9b577f1 Compare February 9, 2016 02:20
richardjgowers added a commit that referenced this pull request Feb 9, 2016
Cythonized files now deleted after setup. (closes #667 / #629)
@richardjgowers richardjgowers merged commit eee9932 into develop Feb 9, 2016
@richardjgowers richardjgowers deleted the issue-667-untrack-cythonized branch February 9, 2016 10:35
@richardjgowers richardjgowers added this to the 0.14 milestone Feb 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants