Generate ERFA wrappers (with modifications to astropy-helpers) - #3166

I don't really think this is necessary or desirable, though I admit I haven't taken a close look at the issues this is trying to resolve. My suspicion is that the machinery to accomplish what this is doing already exists and that this is mostly superfluous. I'm still hoping to refactor the setup too, so anything that involves adding more stuff that has to be called explicitly in setup.py will only make that work more complicated.

@embray - well, this involves very little added machinery and indeed uses much of what is present. Please do open another PR if you have ideas of a different approach. This was just intended to stimulate discussion :)

See #3170 which implements an alternative that requires no changes to setup.py or to astropy-helpers and does everything we need.

@mdboom @embray @eteq @mhvk @taldcroft - I think this is the cleanest solution. No hacks involved. The only downside is that it requires small additions to astropy-helpers.

Bonus: this enables preprocessing in any other subpackage of astropy

(note, if we want to go ahead with this, I'll open a PR on the astropy-helpers repository - also, we can make this work properly with sdist by also calling the preprocessing when sdist is run)

And Travis passes! \o/

@astrofrog - I cannot really just this well, but will say that compared to most parts of the setup_package stuff, this is remarkably clear. I feel I could actually edit it and make it work...

👍 from me, given the dead end that #3170 turned out to be.

Give me a chance to do this a little more generally in the form of pre-/post-command hooks supported via astropy-helpers, and then this would be fine as a pre-build_ext hook. If we can do it that way then I'd feel less locked in in terms of API.

@embray - ok, sounds good! I like the idea of generalized pre-/post-command hooks!

Not really related to this PR since it was already there, but this should not be here. get_package_data() means these files are installed along with the astropy package as a resource file.

If these files should be included in the source tarball they should just be added to the MANIFEST.in, for now.

👍 from me too.

@embray and @astrofrog - what are you thinking the timescale is for getting this in astropy_helpers? If it's soon, we should do this before #3173, but if it might take some time, perhaps merging #3173 now and building this from that makes sense? (#3173 does change the derived files, but not a huge amount, and it does a lot of renaming that conflicts with this. So it may be less work overall to merge it if you're already going to have to re-do this in astropy-helpers)

@eteq - from my side, if we went with my approach as a temporary measure (which is conceptually similar to what @embray is suggesting) then I could get it in to astropy-helpers on a very short timescale. I'm not sure what @embray's timescale is for implementing the more general approach.

@embray - as described in astropy/astropy-helpers#110, I can't use the latest astropy-helpers with astropy core (try it out to see what I mean).

@astrofrog - note #3206. Does that do the trick?

@embray - this now works locally, so let's see what Travis says. If everything works then once the astropy-helpers changes are merged, I suggest @eteq updates #3206 after which I can rebase this.

@eteq - I addressed your comment regarding sdist by:

• Making sure the hook gets run before sdist
• Just in case the timestamps get messed up in the tar file, I added a backup which means that if jinja2 is not installed, and the files do exist (but just don't have recent enough timestamps), it prints a warning and uses the existing files.

I also added a mention of jinja2 to the building from source section in the docs.

@eteq @embray - this is ready for final final review!

👍 from me! (assuming tests pass)

I checked this both on its own and merged with my coordinate branch that uses a lot of erfa stuff for coordinate transforms, and both work just fine. So should be good to go!

Am I right in assuming you're deferring the importlib change until later, @astrofrog ?

Oh, wait, one other thing, @astrofrog : I suggest you add a mention of the jinja2 dependency to the changelog - maybe under "general"? (Fill free to just [ci skip] that)

@eteq - I did the change for importlib here already (feel free to check). I added a changelog entry.

@embray - can you give this one more look to make sure I'm doing the hook stuff right?

Mostly unrelated to this PR other than the note added to the changelog, but I'm 👎 on the requirement for Jinja2, on the basis that these templates are simple enough that they could be generated without it. But I don't have the time to submit a change to that effect right now, so if I'm the only complaintant don't worry about it for now, I guess? Unless someone else would like to make the change...

@embray - I'm pretty sure it would be a lot of work to remove the jinja2 requirement - a large chunk of this would have to be thrown out and re-written. So I don't think that would be a productive use of anyone's time for v1.0, and we should merge this as-is.

In the future, I think removing it would probably be less flexible for potential future improvements. After all, that was the point of using an already-existing template language. And jinja2 is necessary for sphinx, so I think it's not that much of a burden to expect devs to have it. So even in future versions I'm probably 👎 on replacing jinja2 with something we roll on our own. But I could be convinced if you (or someone else) re-writes these in a way that's at least equally clear and flexible for future changes.

In any case, this build is passing (see https://travis-ci.org/astropy/astropy/builds/44260180, but the status isn't updating here, maybe due to the last [ci skip] commit) so feel free to merge this! (then we can move ahead with the other ERFA PRs)

Alright, in the intereset of moving stuff along, I will merge this now. @embray, feel free to create an issue to remove the jinja2 stuff down the road.

great! I'll start rebasing some of the other erfa PRs on this. Agreed with @astrofrog that since jinja2 is needed for building the documentation anyway, it seems fine to have it as a build dependence. The on-the-fly python generation would seem more worthy of further effort!

I concur with @mhvk that removing the jinja2 dependence is a very low priority: it's already a devel requirement due to the docs. And it's a known, rational templating system...

There's travis-ci fails because jinja is missing in other PRs like astropy/photutils#213

See https://travis-ci.org/astropy/photutils/jobs/44340514#L526

Can someone please fix this?

@cdeil - good point, we should add jinja2 to Travis for the affiliated package builds (for the 'development' build). Just add it next to Cython.

@astrofrog - I've added jinja2 in the .travis.yml files for photutils and gammapy.
But I guess this has to be done for most or all affiliated packages ...

@cdeil - that's right, so probably the template should be updated, too.

Oh, but if they're using a released version of astropy, it should be fine after 1.0 (because the generated files are generated with the tarballs)

Just ran into this after a travis failure on sncosmo (testing against astropy dev version). Might want to announce it to the mail list for other affiliated package devs.

Could we just add Jinja2 to setup_requires? I don't see why not, if we put Numpy there :)