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

Refactor infrastructure as described in astropy-helpers removal APE #9726

Merged
merged 44 commits into from
Jan 24, 2020

Conversation

astrofrog
Copy link
Member

@astrofrog astrofrog commented Dec 5, 2019

This PR removes astropy-helpers in the core package and update the infrastructure as described in the APE A roadmap for package infrastructure without astropy-helpers. I am putting this up ahead of the coordination meeting in Socorro, to make it easier to see what the changes would be and discuss specific changes.

The main things this does is:

  • Remove astropy-helpers as a submodule, along with the ah_bootstrap.py
  • Add a new pyproject.toml file describing the build-time dependencies (see PEP 517 and PEP 518)
  • Uses an experimental trimmed down fork of astropy-helpers called extension-helpers (developed in a branch of astropy-helpers) which just does the task of collecting extensions
  • Move declarations of package data from setup_package.py files to setup.cfg
  • Switch to using environment variables to control whether to compile against external libraries
  • Switch to using ASTROPY_DEBUG as a way to control whether to compile extensions in debug mode
  • Use setuptools_scm to generate the version numbers
  • Add a tox configuration to make it easy to automate some of the actions (reviving [WIP] Using Tox #8248)
  • Update the CI configuration to use tox whenever possible

I've split out some of the changes into individual self-contained PRs to make the reviewing process a little easier. Specifically, the other PRs that should be reviewed first are:

I've also pushed a 4.1.dev tag to make sure that setuptools_scm works properly (how this should be done will be detailed in the packaging docs). For anyone reviewing this, I've tried to keep the commits as tidy as possible, and would recommend looking at individual commits in turn.

Things that I won't do as part of this PR and can be done in follow-ups instead:

EDIT: Close #9871

pyproject.toml Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
@mhvk
Copy link
Contributor

mhvk commented Dec 5, 2019

@astrofrog - this is very nice indeed! While I like seeing everything together (and especially that almost everything leads to code reduction rather than increase!!), I do wonder if it isn't best to actually merge roughly following the order you have your commits, so we can discuss smaller pieces in one place. For instance, it would seem that 69df655, which moves packaging information to setup.cfg is super-uncontroversial and could immediately get merged.

As another example, the --debug option, which I will admit I had never looked into, seems rather mis-named - in that it seems to change only whether warnings are visibly generated rather than set up the compiled code to be parsed by a debugger. I may well be wrong about this, but think it is easier to discuss other names if we treat it as a separate commit.

@astrofrog
Copy link
Member Author

@mhvk - I agree that some things here can be broken out into standalone PRs - some of them have to be done together though, e.g. pyproject.toml and tox. I'll take a look tomorrow at what could be split out and merged first.

@bsipocz
Copy link
Member

bsipocz commented Dec 5, 2019

Switch to Azure Pipelines

I would rather switch straight to Actions?

@astrofrog
Copy link
Member Author

I would rather switch straight to Actions?

Yes we should discuss options next week! But in any case I want to not mix that up with this PR.

pyproject.toml Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
@astrofrog astrofrog force-pushed the infrastructure-refactor branch 3 times, most recently from c15ff37 to cbb702e Compare December 18, 2019 17:13
@astrofrog astrofrog force-pushed the infrastructure-refactor branch from 90a4ba9 to 823dbbb Compare January 22, 2020 08:57
@astrofrog
Copy link
Member Author

@pllim - done!

@mhvk
Copy link
Contributor

mhvk commented Jan 24, 2020

My sense would be to get this in. Any objections?

@Cadair
Copy link
Member

Cadair commented Jan 24, 2020

Do it! I have two follow-on branches already 😛 :shipit: 🚢

@taldcroft
Copy link
Member

@bsipocz @eteq - are you each OK with this PR?

@pllim
Copy link
Member

pllim commented Jan 24, 2020

My vote is to merge now. I think @bsipocz already said she got no problem with merging this. And as for @eteq , he can open follow-up PR if he is unhappy with the content when he is available again. 😉 :shipit:

@taldcroft
Copy link
Member

OK then, I'm good with merging!

@astrofrog had previously asked the coordination committee about process for approval of this PR, so I'm speaking now with my CoCo hat on.

@pllim pllim merged commit 657c768 into astropy:master Jan 24, 2020
@pllim
Copy link
Member

pllim commented Jan 24, 2020

Merged. Thank you, @astrofrog and everyone involved! 🎉

@Cadair
Copy link
Member

Cadair commented Jan 24, 2020

🚀 🚀 🎉 🎆 👯‍♀️ 👯‍♂️

@mhvk
Copy link
Contributor

mhvk commented Jan 24, 2020

Thanks, indeed, @astrofrog and everyone else!

@astrofrog
Copy link
Member Author

Thanks for merging this! And please don't hesitate to ping me or @Cadair if you run into any issues locally or in future pull requests 🙏

@namurphy
Copy link
Contributor

Thank you to everyone who made this PR happen! I'd also like to thank everyone who has contributed to astropy-helpers over the last several years, as it has been really helpful to the community during the time that it has been around.

@mhvk
Copy link
Contributor

mhvk commented Jan 24, 2020

Seconded! I've been convinced over this PR how great it is to get rid of astropy helpers, but it made things much easier when these new nifty tools weren't around!

@eteq
Copy link
Member

eteq commented Jan 27, 2020

I had been planning to review this but it's fine that it got merged - @pllim is definitely right, I can always do my own PR with any changes, because big-picture I was already fine with it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready-for-final-review Refactoring 💤 astropy-helpers-removal PRs and issues related to the removal of astropy-helpers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

astropy.units test failures with Numpy 1.18 from conda on Windows
10 participants