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

Syntax error in autotools C++ HEALPix install #461

Closed
zonca opened this issue Jun 29, 2018 · 13 comments
Closed

Syntax error in autotools C++ HEALPix install #461

zonca opened this issue Jun 29, 2018 · 13 comments
Assignees

Comments

@zonca
Copy link
Member

zonca commented Jun 29, 2018

@mreineck not sure if the autotools installation is officially supported in HEALPix so I opened an issue here instead of the HEALPix bug-tracker.

HEALPix C++ fails to install with autotools (on Cori at NERSC). I am trying to install the version bundled with healpy, which is https://github.com/healpy/healpixmirror/tree/bd2ddf629d2d75d65205b2bc38f3940a3f9eeefb

autoreconf (GNU Autoconf) 2.69
> pkg-config --version
0.28

I run:

> autoreconf --install
libtoolize: putting auxiliary files in '.'.
libtoolize: copying file './ltmain.sh'
libtoolize: putting macros in AC_CONFIG_MACRO_DIRS, 'm4'.
libtoolize: copying file 'm4/libtool.m4'
libtoolize: copying file 'm4/ltoptions.m4'
libtoolize: copying file 'm4/ltsugar.m4'
libtoolize: copying file 'm4/ltversion.m4'
libtoolize: copying file 'm4/lt~obsolete.m4'
configure.ac:11: installing './ar-lib'
configure.ac:11: installing './compile'
configure.ac:13: installing './config.guess'
configure.ac:13: installing './config.sub'
configure.ac:2: installing './install-sh'
configure.ac:2: installing './missing'
Makefile.am: installing './depcomp'
parallel-tests: installing './test-driver'
> ./configure
[....]
./configure: line 17986: syntax error near unexpected token `CFITSIO,'
./configure: line 17986: `PKG_CHECK_MODULES(CFITSIO, cfitsio, , CFITSIO_LIBS=-lcfitsio)' 

do you know what might be the problem?

@mreineck
Copy link
Member

No idea. Things work fine on my laptop, and the only difference I see at the moment is that I have pkg-config 0.29 installed.

@lpsinger, have you seen this before?

@mreineck
Copy link
Member

Maybe something like stlink-org/stlink#83 ?

@zonca
Copy link
Member Author

zonca commented Jun 29, 2018

Thanks, I gave it a try using the path to aclocal at NERSC but didn't work.

@tskisner would you have any suggestion?

@tskisner
Copy link

Autotools usually works under the model where "developers" have (recent) autotools versions installed. Developers build a distribution tarball (make dist). This distribution tarball can be installed by "users" who only have a Bourne shell and Make installed. This way the installation does not depend on the age / version of autotools on the user system.

Recall my comment on the other issue:

#432 (comment)

I believe that healpy should be installing healpix C++ from a distribution tarball and NOT calling autoreconf on a user system. Doing this makes the build dependent on the autotools versions that may or may not be installed on the user system.

When I install healpix / healpy at NERSC, I first create a distribution tarball like this:

https://github.com/tskisner/healpix-autotools/releases/tag/v3.31.4

and I build this tarball on my laptop with all the latest autotools. Then I go install it on the NERSC machines. That tarball includes a copy of healpy with a modified setup.py that forces it to use the healpix C++ that is built as part of the package. (I did not yet update that repo to that recently releases healpix versions).

Obviously a "unified healpix installation" is challenging because the upstream packages are completely different source trees created by different people.

If you really want to bundle the healpix C++ source tree (rather than a distribution tarball), have you considered other build solutions? For example, you could explicitly build a shared library directly with setup.py and just ignore the autotools files. Or you could use pybind11 and cmake to build the extension (this would require a conda / pip dependency on the cmake package). Currently your package has an implicit dependency on autotools...

@lpsinger
Copy link
Member

@lpsinger, have you seen this before?

Yes. You're normally expected to distribute all of the required autoconf macros in your package. This is a defect in the healpix-cxx package. I'll provide you with a patch.

@lpsinger
Copy link
Member

lpsinger commented Jun 29, 2018

If you really want to bundle the healpix C++ source tree (rather than a distribution tarball), have you considered other build solutions? For example, you could explicitly build a shared library directly with setup.py and just ignore the autotools files. Or you could use pybind11 and cmake to build the extension (this would require a conda / pip dependency on the cmake package). Currently your package has an implicit dependency on autotools...

Autotools is only a dependency for building a distribution tarball (e.g. setup.py sdist). After that, the generated configure script is baked into the source. So you don't need autotools if you are installing from a release source tarball.

I agree that the autotools dependency for development is weird for a Python package. But it is this way for a good reason: the C++ package is very sensitive to correct GCC versions and compiler flags; the autoconf-generated configure script does all of these checks and it was going to be a lot of work to duplicate these checks in a pure Python build system.

@lpsinger
Copy link
Member

When I install healpix / healpy at NERSC, I first create a distribution tarball like this:

https://github.com/tskisner/healpix-autotools/releases/tag/v3.31.4

@tskisner, are you aware that there are vanilla autotools build systems for healpix-cxx and chealpix? @mreineck and I maintain them and I build the Debian and MacPorts packages from them.

The autotools packages are distributed on HEALPix's SourceForge project page (e.g. healpix_cxx-3.40.0.tar.gz), but unfortunately they are less well advertised than the unorthodox, monolithic, multi-language tarball with its interactive configure script (e.g. Healpix_3.40_2018Jun22.tar.gz).

From a Debian and MacPorts maintainer and a HEALPix user, I would really like to see the monolithic tarball go away. In an ideal world, I would like to see the HEALPix project make the following changes:

  1. Move to GitHub or GitLab (or at least git).
  2. Split the language variants (Python, C, C++, Fortran, Java, IDL) into separate repositories.
  3. For each language variant, use the most orthodox possible build system (setuptools for Python; autotools along the lines of what Martin and I have done for C, C++, and Fortran; ant for Java; and probably just a bare tarball of sources for IDL).
  4. Release each language variant in a separate tarball, using standard version numbers (probably server).

@tskisner
Copy link

Hi @lpsinger, thanks for pointing out some of the details of which I was not aware. I agree with your goals above, and appreciate all the work you and @mreineck have done. @zonca, was the build above that you tried on cori using a healpy git checkout or installing from a release tarball? If it was from a git checkout and you just want a newer version of autotools, you can get that from the toast-deps module (or obviously can install yourself).

@zonca
Copy link
Member Author

zonca commented Jun 29, 2018

@tskisner, I was already using toast-deps

@lpsinger
Copy link
Member

@mreineck, please take the attached file pkg.m4.txt, rename it to pkg.m4, and put it in the m4 directory of your autotools package. This is the autoconf macro file from pkgconfig 0.29.2.

Once you do that and @zonca updates the embedded healpix-cxx source, that should fix the issue.

pkg.m4.txt

@lpsinger
Copy link
Member

@zonca, @mreineck has updated the healpix-cxx source to include that m4 file. Would you please update the bundled sources and verify that this fixes the issue?

@zonca
Copy link
Member Author

zonca commented Jun 30, 2018

Yes it worked, thanks! Do you think this is worth a release?

@zonca zonca closed this as completed in 96ddea1 Jun 30, 2018
@lpsinger
Copy link
Member

Yes it worked, thanks! Do you think this is worth a release?

No, I don't think that is necessary, because it only affects building from git.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants