-
-
Notifications
You must be signed in to change notification settings - Fork 140
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
Remove astropy-helpers! #915
Conversation
b0a8332
to
c34ee4c
Compare
@larrybradley - I'm happy to take a look at this to see if anything can be simplified further once you rebase :) |
c34ee4c
to
be02de0
Compare
@astrofrog It's rebased. |
be02de0
to
54062ad
Compare
Hello @larrybradley! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2020-05-05 17:21:49 UTC |
54062ad
to
220dd11
Compare
425825c
to
2bb996a
Compare
666e4bb
to
f74c113
Compare
.circleci/config.yml
Outdated
@@ -3,12 +3,12 @@ version: 2 | |||
jobs: | |||
build: | |||
docker: | |||
- image: quay.io/pypa/manylinux1_i686 | |||
- image: astropy/image-tests-py37-mpl311:1.10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the removal of 32-bit testing deliberate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, definitely not. I'm just playing with CircleCI config. I had assumed astropy/image-tests-py37-mpl311:1.10
was 32-bit, but included mpl.
I changed the 32-bit tests to include alldeps
, but the issue I'm having is mpl
cannot be installed in the quay.io/pypa/manylinux1_i686
image because of a missing freetype library:
https://app.circleci.com/pipelines/github/astropy/photutils/14/workflows/f4936108-6c07-4455-8001-e1d9087499fe/jobs/1149
Unless you know how to fix this, then I'm going revert to running the 32-bit tests only with the required dependencies (which does not include mpl).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the rest of this PR look OK? Everything is ready to be merged, except for this CircleCI issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There isn't a mpl
wheel for 32-bit linux, thus CircleCI is compiling from source.
f74c113
to
77cc38c
Compare
Codecov Report
@@ Coverage Diff @@
## master #915 +/- ##
=========================================
Coverage ? 49.01%
=========================================
Files ? 61
Lines ? 6172
Branches ? 0
=========================================
Hits ? 3025
Misses ? 3147
Partials ? 0 Continue to review full report at Codecov.
|
d70c805
to
6abdefa
Compare
6abdefa
to
0ef22ae
Compare
These changes are based on astropy core and the package template, so I don't think they are controversial. I'm going to merge this PR now because it will fix the failing travis-ci and RTD builds. |
This PR is a followup to #914, simplifying the package setup by completely removing
astropy-helpers
. 🚀 Worked on during the pyastro hack day with help from @astrofrog.This PR includes a minimal
tox
configuration for 'build_docs' and 'test' (both work fine).Some notes:
I discovered that
setuptools_scm
cannot be used with packages that still rely onastropy-helpers
to build extensions. Theastropy-helpers
extension builder looks for theversion.py
module generated by helpers.I put the Cython extension builds in a separate module called
setup_extensions.py
. As discussed with @astrofrog, it would great if a small package could handle building extensions.I added dummy
test
andbuild_docs
setup commands insetup_commands.py
that print a message to the user that those commands no longer work. Again, it would be nice for that boilerplate to go into a helper package.Also discussed with @astrofrog -- having a
pytest
plugin to handle the-P
option is also needed!@astrofrog Please take a look and try it out and let me know if I missed anything.
This PR is not ready to be merged.