Cleanup version

[mhvk]

Arrgghh, after releasing and testing with pyerfa, I find out that I forgot to also update the version returned by the C functions. So, we need to do a bug-fix 1.7.2.

This PR also updates the release instructions, although really we should try to ensure that a given release number only has to be entered once!!

[sergiopasra]

Actually, you don't need this. The macros PACKAGE_VERSION and the like are defined in config.h

We can safely remove the defines before #ifdef HAVE_CONFIG_H

[mhvk]

@sergiopasra - ah, yes that makes sense. I started this since over at pyerfa, I am getting failures, but indeed if I just download the 1.7.1 source that I created, compile that following README.rst, and link with the library in src/.libs, it works fine, so we must be doing something a bit fishy over at pyerfa. Will have a look there.

p.s. While compiling, I also noticed that with my changes for the leap seconds, one cannot just include "erfaextra.h" anymore, so this will need a follow-up regardless. Can then also remove the version definitions in erfaversion.c

[mhvk]

Indeed, checking what exactly we do in pyerfa, I see we just take all the C code and compile it directly, without creating a config file, etc... See https://github.com/liberfa/pyerfa/blob/822d095c96e8203d222be23450beb83dc792b876/setup.py#L60-L66

So, perhaps the best way forward is still to merge this one, and then think a bit more about how to ensure we check that the version information is kept up to date.

[sergiopasra]

@mhvk

p.s. While compiling, I also noticed that with my changes for the leap seconds, one cannot just include "erfaextra.h" anymore, so this will need a follow-up regardless. Can then also remove the version definitions in erfaversion.c

You have to include erfam.h in erfaextra.h, because eraLEAPSECOND is defined there

[sergiopasra]

Indeed, checking what exactly we do in pyerfa, I see we just take all the C code and compile it directly, without creating a config file, etc... See https://github.com/liberfa/pyerfa/blob/822d095c96e8203d222be23450beb83dc792b876/setup.py#L60-L66

pyerfa's setup.py should run ./configure to generate config.h. After that you can compile manually. Probably.

I admit I have no experience embedding configure scripts inside Python build system

[bnavigator]

Related: liberfa/pyerfa#52

[avalentino]

pyerfa's setup.py should run ./configure to generate config.h. After that you can compile manually. Probably.

@sergiopasra actually one would need to run ./bootstrap.sh (which needs autotools).
This is not too complex to implement but IMHO it would be a pain on the user side e.g. for users not having autotools installed of windows users not having sh.

AFAIK erfaversion.c uses config.h only if available

erfa/src/erfaversion.c

Line 24 in 8e9b08e

#ifdef HAVE_CONFIG_H

which makes sense to me.

Probably a good idea would be to raise a warning if config.h is missing:

#ifdef HAVE_CONFIG_H
#include <config.h>
#else
#warning ...
/* Define to the major version of this package. */
#define PACKAGE_VERSION_MAJOR 1
...
#endif /* HAVE_CONFIG_H */

[avalentino]

Anyway I'm (slowly) working on a change in pyerfa to run bootstrap/configure if possible at build time.

[sergiopasra]

pyerfa's setup.py should run ./configure to generate config.h. After that you can compile manually. Probably.

@sergiopasra actually one would need to run ./bootstrap.sh (which needs autotools).
This is not too complex to implement but IMHO it would be a pain on the user side e.g. for users not having autotools installed of windows users not having sh.

You only need autotools if you are a developer and you need to generate configure from configure.ac and Makefile.in from Makefile.am
If you work with the released tarball you only need /bin/sh, make and the compiler. Nothing more, because the
configure script has been already created for you.

AFAIK erfaversion.c uses config.h only if available

erfa/src/erfaversion.c

Line 24 in 8e9b08e

#ifdef HAVE_CONFIG_H

which makes sense to me.

Probably a good idea would be to raise a warning if config.h is missing:

#ifdef HAVE_CONFIG_H
#include <config.h>
#else
#warning ...
/* Define to the major version of this package. */
#define PACKAGE_VERSION_MAJOR 1
...
#endif /* HAVE_CONFIG_H */

Yes, this is the typical way of working with config.h

It is used only if it has been created. The compilation line in the Makefile is something like:

gcc -D HAVE_CONFIG_H ....

If the macro is not defined, you still can compile, but you have to define the macros yourself, with -D in the compiler command line:

gcc -D PACKAGE_VERSION_MAJOR=1 -D PACKAGE_VERSION_MINOR=7 ...

In my opinion, we should remove the macro definitions and let config.h do its thing.
In that case, the compilation will fail unless you define the macros in the command line. That's enough warning

[avalentino]

You only need autotools if you are a developer and you need to generate configure from configure.ac and Makefile.in from Makefile.am

@sergiopasra yes, so in any case we need to improve the pyerfa setup.py (or whatever) to generate and bundle the configure script together with liberfa sources in the pyerfa source distribution. Correct?

In my opinion, we should remove the macro definitions and let config.h do its thing.
In that case, the compilation will fail unless you define the macros in the command line. That's enough warning

IMHO it is a good solution for liberfa as soon as we take care a writing a note in the docs.

From a pyerfa perspective IMO the user expects that pip install pyerfa just works, so we shall find a way to detect the missing config.h and provide fallback values for required C defines in the pyerfa setup.py.
Does it make sense for you?

[mhvk]

Maybe we can simply read bootstrap.sh from python and auto-generate config.h with just the few needed defines (or even erfaversion.c itself) before compiling?

[sergiopasra]

@avalentino In theory, you could generate config.h in python, but then you are reinventing autotools.

Furthermore, you would be adding a new dependency in pyerfa, the macro definitions inside config.h, that should be completely hidden from you (they are a detail of the implementation, not part of the public interface).

config.h is not even installed, it's only used during the compilation.

In my opinion, the best approach is to embed a released version of erfa inside pyerfa and delegate the work of building the library to ./configure and make

@mhvk bootstrap.sh doesn't have the info you need. It's inside configure.ac or configure. But you have to parse them.

[avalentino]

@avalentino In theory, you could generate config.h in python, but then you are reinventing autotools.

@sergiopasra actually the Idea was simply to run bootstrap.sh from the setup.py script before calling setuptools.setup(...) when one have to generate the sdist.

The point is that we are bundling a copy of liberfa and hence we should also bundle a proper configure script.

I agree with you that replicating the logic that is already in the liberfa configuration machinery does not make sense.

Running the configure script will be part of the build process on the user side (this can also be done with a subprocess call in setup.py).

The problem is that we shall also ensure that the build process completes successfully on platform that are not able to run the liberfa configure script.

In this specific case one could simply add define_macros for PACKAGE_VERSION_MAJOR, PACKAGE_VERSION_MINOR, etc. hand the setuptools.Extension is declared.
This is necessary if we decide to drop the version macro definition form erfaversion.c.

[sergiopasra]

@sergiopasra actually the Idea was simply to run bootstrap.sh from the setup.py script before calling setuptools.setup(...) when one have to generate the sdist.

@avalentino You have to run ./configure in setup.py. To run bootstrap.sh you need the complete autools machinery. The users are not expected to have autotools to compile erfa: just make, the shell and a compiler.

The problem is that we shall also ensure that the build process completes successfully on platform that are not able to run the liberfa configure script.

What platforms are those?

[sergiopasra]

@sergiopasra actually the Idea was simply to run bootstrap.sh from the setup.py script before calling setuptools.setup(...) when one have to generate the sdist.

@avalentino I see that you are using a submodule, so your version of liberfa doesn't have ./configure This file is generated before creating the distributed tarball, by running make dist

Let's say that you are not using the source tarball that is created for the users, instead you have a snapshot of the code that we use to generate the tarball. The tarball contains the ./configure, the git submodule doesn't

[avalentino]

@avalentino I see that you are using a submodule, so your version of liberfa doesn't have ./configure This file is generated before creating the distributed tarball, by running make dist

yes, this is the point of confusion IMHO

@sergiopasra actually the Idea was simply to run bootstrap.sh from the setup.py script before calling setuptools.setup(...) when one have to generate the sdist.
@avalentino You have to run ./configure in setup.py. To run bootstrap.sh you need the complete autools machinery. The users are not expected to have autotools to compile erfa: just make, the shell and a compiler.

yes, the idea is to run the bootstrap.sh only in the sdist command.
In principle, in my opinion, python3 setup.py sdist is something that only developers need to run.

Once the source distribution is on PyPi the user should only need to use the build, build_ext to install commands.
Is it correct?

[avalentino]

The problem is that we shall also ensure that the build process completes successfully on platform that are not able to run the liberfa configure script.
What platforms are those?

windows?
I'm not sure I don't use windows

[bnavigator]

Please keep in mind that config.h won't be installed with a system liberfa and its dev package when PYERFA_USE_SYSTEM_LIBERFA=1 is desired.

[sergiopasra]

@avalentino

Possible solutions:

1. We (liberfa) could store the complete tarball that we release in our git, so you (pyerfa) could still use a submodule, but with all the files
2. You (pyerfa) could still use the submodule, but performing part of the steps we (liberfa) do to create a release. Basically, running boostrap.sh to create configure. Then you distribute the files inside pyerfa. This is basically the same thing as copying the tarball of liberfa inside pyerfa before release.
3. You (pyerfa) forget about the submodule and you just embed a released tarball. Just copying a released tarball of liberfa inside pyerfa

In my opinion, 3 is the way to go...

[avalentino]

@sergiopasra I agree that option 3 is probably the safest one.
Personally I also like option 2, once we manage to fix the setup.py script it should work well.
But mine is not a strong preference.
@mhvk do you have comments?

Please keep in mind that config.h won't be installed with a system liberfa and its dev package when PYERFA_USE_SYSTEM_LIBERFA=1 is desired.

@bnavigator using system liberfa should be ok IMHO for what discussed here.
That use case is more affected by liberfa/pyerfa#52 IMO

[bnavigator]

Ok, I did not follow the complete discussion. I just wanted to make sure that you do not rely on a present config.h even when the liberfa/erfa subdir in pyerfa is not relevant to the installation.

[avalentino]

@sergiopasra , @bnavigator changes have been implemented in liberfa/pyerfa#53 in case you want to review them.

[mhvk]

@avalentino - with your changes at pyerfa, do you think it is still useful to do the corrections here?

Alternatively, I think with your changes we can now follow @sergiopasra's advice above and just remove the backup version definitions - that would seem cleaner (one way only to do it, etc.). If so, no new version would seem needed, right?

[mhvk]

p.s. I ask in part since if no new version here is needed, we might as well do a release of pyerfa.

[avalentino]

@avalentino - with your changes at pyerfa, do you think it is still useful to do the corrections here?

Alternatively, I think with your changes we can now follow @sergiopasra's advice above and just remove the backup version definitions - that would seem cleaner (one way only to do it, etc.). If so, no new version would seem needed, right?

IMHO we should implement the solution proposed by @sergiopasra, having the fallback version demonstrated to be quite problematic to manage. An I agree with you there is no need to make a liberfa release now.

p.s. I ask in part since if no new version here is needed, we might as well do a release of pyerfa.

Regarding the pyerfa I would like to add a couple of changes before the release:

• test for Python 3.9
• improved version management like in Never use setuptools_scm except when importing astropy from a git repository astropy/astropy#10774
  I'm going to open PRs soon.

[mhvk]

OK, I'm going to go with the suggestion to just remove the version information in erfaversion.c, so closing this PR.

[mhvk]

See #74.