Update bundled ERFA to 1.3.0 - #5418

closes #5395
closes #5234

@eteq - does this look OK? (the coverage failure is presumably just because there are now more *.c files.)

@eteq, @StuartLittlefair - as noted in #5234, with this PR in principle the workarounds for aticq and atciqz could be removed. I was about to do that as part of this PR, but then realized it may need some thought, since in many distributions, erfa is not bundled with astropy, but rather packaged separately. I think the easiest solution is to set the minimum version that erfa needs to have to 1.3 (if only to avoid people being hit by the leap second). But I don't really know how to do that...

Is it possible to test for the erfa version and call the home-rolled versions of aticq and atciqz if the version is less than 1.3.0? We could also add a deprecation message to get users to nag packagers of distributions where erfa is distributed separately?

Is a decrease in coverage by 0.4% expected?

@pllim Coveralls (https://coveralls.io/builds/8394184) states that in astropy/_erfa/core.py 286 lines were added but the covered lines only increased by 12. That means that 274 relevant new lines are uncovered.

I think they just added some more wrappers or functions and these are not covered by our test suite: So yes, if they add new functionality our coverage must drop. Could be worthwile to remove extern packages from coverage.

@MSeifert04 , are we supposed to add new tests for those, or is it out of scope?

Sorry I was a bit too late. I've edit-added a sentence to my previous comment:

Could be worthwile to remove extern packages from coverage.

I don't think it's our job to test extern packages. If we use their functionality the coverage report is just a side effect.

I think the coverage of the external packages is only useful in that it gives us a sense of what fraction of a package we actually use, but really, it should not be included in the coverage fraction for astropy.

@StuartLittlefair - yes, the version would work -- except, I don't know how to get the version out! (i.e., astropy.erfa doesn't seem to give any indication of its version...)

@mhvk - I'm just guessing here, since I don't really understand how the erfa machinery works. Can't we add a version number to core.py.templ?

I think it has to be in erfa_generator -- which could get the version out of cextern/erfa/README.rst (which has been around only since erfa 1.2.0, but I guess that is good enough).

Actually, I think the above is too specific to how we bundle things ourselves. In debian and redhat, astropy simply depends on liberfa and so we should get the version out of the library, not out of the source code.

Anyway, I think we should try to solve this independently of the present PR, since we do need to get the leap second in at least! (and this doesn't break anything).

I think the coverage of the external packages is only useful in that it gives us a sense of what fraction of a package we actually use, but really

@mhvk I don't think coveralls actually shows which parts of generated files are covered it just shows how many lines are present and covered (but not which) .

p.s. Indeed, in Debian testing at least, liberfa is already at version 1.3, so I do have the leap second correct already... But still not sure how to get a version number out...

@mhvk agreed - this should go in before we find a fix to the version question.

@mhvk - I'm happy with this except for one thing: this PR adds cextern/erfa/t_erfa_c.c . In the past we've not included that because it's the c-level "tests" for erfa, which are not used by astropy. Did you intentionally add it or just copy it over not realizing it shouldn't be included? If the latter, I'd say just remove it (either with a rebase to keep it out of the history or not - whatever you prefer), and then this is good to merge.

re: getting the erfa version, I agree it's important that it not be machinery special to the astropy-packaged version because of the need to accept the system copy on debian. The problem is, I don't think there's any way to get that information... As it stands right now the versioning all comes from the standard c autoconf, which doesn't inject anything into the source code itself. But I guess that means erfa should include that somehow?

@eteq - OK, removed the file (indeed, just copied by mistake) and rebased. Will merge if this passes.

As for the version number, I agree it should be exposed by erfa itself, so raised liberfa/erfa#38 there.

Now why is appveyor failing to build astropy/table/_np_utils.pyx???

I've heard a lot of people complaining about cython today.

I haven't found an issue at cython but I've noticed the last passing build used 0.24.1 and this failing test used 0.25.0. So I guess and hope that's not related to erfa. 😄

@MSeifert04 - thanks for looking into this so quickly! Since this passed appveyor before, I'll assume all is OK if travis passes.

@mhvk That's where it becomes sortof funny: Travis hasn't picked up any PR today (https://travis-ci.org/astropy/astropy/pull_requests)

Looks like Travis caught up but this is afflicted with the same as #5437 ... Hopefully conda will update to the Cython that isn't breaking things soon and then you can re-run and hopefully merge.

OK, appveyor and travis now both are back in business and the tests passed, so merging...

👏