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

bootstrap should fail gracefully, if there is no pkg-config available #27219

Closed
dimpase opened this issue Feb 4, 2019 · 42 comments
Closed

bootstrap should fail gracefully, if there is no pkg-config available #27219

dimpase opened this issue Feb 4, 2019 · 42 comments

Comments

@dimpase
Copy link
Member

dimpase commented Feb 4, 2019

When #27114 is merged, ./bootstrap requires pkgconf to run properly. It should switch to downloading the configure spkg if the pkgconf m4 macros are not installed. Otherwise one gets

checking whether g++ supports C++11 features with -std=gnu++11... yes
checking for gcc option to accept ISO C99... none needed
checking if gcc accepts -dumpversion option... yes
checking gcc version... 5.4.0
checking if g++ accepts -dumpversion option... yes
checking g++ version... 5.4.0
configure: === checking whether to install the libffi SPKG ===
./configure: line 9026: syntax error near unexpected token `LIBFFI,'
./configure: line 9026: `        PKG_CHECK_MODULES(LIBFFI, libffi, ,'
If you would like to try to build Sage anyway (to help porting),
export the variable 'SAGE_PORT' to something non-empty.
Makefile:39: recipe for target 'build/make/Makefile' failed
make[1]: Leaving directory '/var/lib/buildbot/slave/sage_git/build'
make[1]: *** [build/make/Makefile] Error 1
Makefile:31: recipe for target 'build-clean' failed
make: *** [build-clean] Error 2
program finished with exit code 2

In order to solve this issue, we check for presence of PKG_PROG_PKG_CONFIG at bootstrap/autoconf run. We need renaming of PKG_ -> SPKG_ in m4/sage_spkg_collect.m4, which we take from #27114 in order not to cause conflicts.

#27114 probably will need a rebase on top of this branch.

CC: @embray @vbraun @jdemeyer

Component: build

Author: Dima Pasechnik

Branch/Commit: 187de34

Reviewer: Jeroen Demeyer

Issue created by migration from https://trac.sagemath.org/ticket/27219

@dimpase dimpase added this to the sage-8.7 milestone Feb 4, 2019
@dimpase
Copy link
Member Author

dimpase commented Feb 4, 2019

Branch: u/dimpase/build/bootstrap_check

@dimpase
Copy link
Member Author

dimpase commented Feb 4, 2019

Commit: af57d5b

@dimpase
Copy link
Member Author

dimpase commented Feb 4, 2019

comment:1

OK, Volker, how about this?

@vbraun
Copy link
Member

vbraun commented Feb 6, 2019

comment:2

IMHO whether or not there is a pkg-config binary isn't the same as autoconf understanding PKG_CHECK_MODULES.

@dimpase
Copy link
Member Author

dimpase commented Feb 7, 2019

comment:3

Replying to @vbraun:

IMHO whether or not there is a pkg-config binary isn't the same as autoconf understanding PKG_CHECK_MODULES.

PKG.m4 - macros are a part of pkg-config. (a sufficiently new one - we can add a version check in bootstrap...)

We can also add a call to PKG_PROG_PKG_CONFIG to configure.ac.

It is of course perfectly possible to have a mangled install of pkg-config, without the needed m4 macros.

@dimpase
Copy link
Member Author

dimpase commented Feb 7, 2019

Author: Dima Pasechnik

@jdemeyer
Copy link

jdemeyer commented Feb 7, 2019

comment:4

Replying to @vbraun:

IMHO whether or not there is a pkg-config binary isn't the same as autoconf understanding PKG_CHECK_MODULES.

I agree. The right thing to check then is whether the macro PKG_CHECK_MODULES is expanded.

@jdemeyer
Copy link

jdemeyer commented Feb 7, 2019

comment:5

Also, what goes wrong when pkg-config is not installed? The bug report should say that.

@jdemeyer
Copy link

jdemeyer commented Feb 7, 2019

comment:6

m4_pattern_forbid may be the correct solution here. See https://www.gnu.org/software/autoconf/manual/autoconf.html#Forbidden-Patterns

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 7, 2019

Changed commit from af57d5b to 6e9f902

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 7, 2019

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

6e9f902check that pkg-config and its m4 macros are installed

@dimpase
Copy link
Member Author

dimpase commented Feb 7, 2019

comment:8

from the latest commit message:

    check that pkg-config and its m4 macros are installed
    
    use m4_ifndef() in configure.ac to check that PKG_PROG_PKG_CONFIG
    is provided.
    
    just in case, also check that pkg-config is present
    
    calling PKG_PROG_PKG_CONFIG in configure.ac
    
    renaming of clashing with PKG_* m4 names in m4/*
    (otherwise PKG_PROG_PKG_CONFIG cannot be mentioned at all)

took a while to figure out weird errors caused by m4 vars name clashes.

@dimpase
Copy link
Member Author

dimpase commented Feb 7, 2019

comment:9

Not 100% sure whether we ought to call PKG_PROG_PKG_CONFIG. I do this in the current branch,
I suppose it cannot hurt.

@vbraun
Copy link
Member

vbraun commented Feb 7, 2019

comment:10

What currently happens is

checking whether g++ supports C++11 features with -std=gnu++11... yes
checking for gcc option to accept ISO C99... none needed
checking if gcc accepts -dumpversion option... yes
checking gcc version... 5.4.0
checking if g++ accepts -dumpversion option... yes
checking g++ version... 5.4.0
configure: === checking whether to install the libffi SPKG ===
./configure: line 9026: syntax error near unexpected token `LIBFFI,'
./configure: line 9026: `        PKG_CHECK_MODULES(LIBFFI, libffi, ,'
If you would like to try to build Sage anyway (to help porting),
export the variable 'SAGE_PORT' to something non-empty.
Makefile:39: recipe for target 'build/make/Makefile' failed
make[1]: Leaving directory '/var/lib/buildbot/slave/sage_git/build'
make[1]: *** [build/make/Makefile] Error 1
Makefile:31: recipe for target 'build-clean' failed
make: *** [build-clean] Error 2
program finished with exit code 2

@jdemeyer

This comment has been minimized.

@jdemeyer
Copy link

jdemeyer commented Feb 7, 2019

comment:11

Right, so all that you need is something like m4_pattern_forbid([^PKG_]).

What's the point of checking for pkgconf in bootstrap? Generating the ./configure script should be orthogonal to whether or not pkgconf is installed.

@jdemeyer

This comment has been minimized.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 7, 2019

Branch pushed to git repo; I updated commit sha1. New commits:

371cd8euse Eriks's m4 fixes
4d9ccb4Trac #27109: Use pkg-config if possible to detect libffi
761c978AC_CHECK_HEADERS should come before AC_SEARCH_LIBS
3d2eb36move ffi/ffi.h first since it seems to be more common too
cc37228Merge remote-tracking branch 'trac/u/embray/build/ticket-27109' into booboo

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 7, 2019

Changed commit from 6e9f902 to cc37228

@dimpase
Copy link
Member Author

dimpase commented Feb 7, 2019

comment:18

oops, sorry, I should have put in the old version of libffi/spkg-configure.m4...

@jdemeyer
Copy link

jdemeyer commented Feb 7, 2019

comment:19

Replying to @dimpase:

Generating configure needs pkg-config since #27219

You need to distinguish between generating configure and running configure. In this ticket, we are only concerned with the bootstrap script which runs autoconf to generate configure.

Now autoconf is a macro processor, so it would be surprising that it would run pkg-config. Even if it does, what happens if pkg-config is not installed? I would assume that it fails gracefully, so I still don't see the point of the extra pkg-config --version check.

@jdemeyer
Copy link

jdemeyer commented Feb 7, 2019

comment:20

By the way, if you want to use m4_ifndef, at least make sure that error message is correct:

m4_ifndef([PKG_PROG_PKG_CONFIG],
    [m4_fatal([Could not locate the pkg-config autoconf macros.
    These are usually located in /usr/share/aclocal/pkg.m4. If your
    macros are in a different location, try setting the environment
    variable ACLOCAL="aclocal -I/other/macro/dir" before running
    autoreconf/autogen.sh.])])

The script in Sage is called bootstrap.

@jdemeyer
Copy link

jdemeyer commented Feb 7, 2019

comment:21

Replying to @dimpase:

oops, sorry, I should have put in the old version of libffi/spkg-configure.m4...

In general, your commit history on this ticket is a mess, so I suggest to rebase and squash.

@dimpase
Copy link
Member Author

dimpase commented Feb 7, 2019

comment:22

I am certainly distinguishing between generating and running configure. Generating configure only needs m4 macros, this is correct.

Would you like me to take out from bootstrap the test for pkg-config presence?

@dimpase
Copy link
Member Author

dimpase commented Feb 7, 2019

comment:23

Replying to @jdemeyer:

Replying to @dimpase:

oops, sorry, I should have put in the old version of libffi/spkg-configure.m4...

In general, your commit history on this ticket is a mess, so I suggest to rebase and squash.

the problem is that for this to work, I need changes to m4/sage_spkg_collect.m4
akin to ones done on #27114. (I first did my own, without using #27114, only to find out that merging #27114 gets painful with them; so I decided to use a part of #27114, but haven't done it quite right...).

After I'm done here, #27114 will have to be rebased on top of this one.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 7, 2019

Changed commit from 89a402f to 87621c7

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 7, 2019

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

453033drenaming PKG_* -> SPKG_* in m4
87621c7check for pkg-config's m4 macros at bootstrap

@dimpase

This comment has been minimized.

@dimpase
Copy link
Member Author

dimpase commented Feb 7, 2019

comment:25

I hope I have addressed all the issues raised, including the messy branches. Sorry, it took a while to get right.

@jdemeyer
Copy link

jdemeyer commented Feb 7, 2019

comment:26

Indeed, this is more or less what I meant.

For the record, the message when the pkg-config macros are not installed:

configure.ac:219: error: Could not locate the pkg-config autoconf macros.
    These are usually located in /usr/share/aclocal/pkg.m4. If your
    macros are in a different location, try setting the environment
    variable ACLOCAL="aclocal -I/other/macro/dir" before running
    ./bootstrap
configure.ac:219: the top level
autom4te: /usr/bin/m4 failed with exit status: 1
aclocal-1.15: error: echo failed with exit status: 1
Bootstrap failed. Either install pkg-config m4 macros for autotools or run bootstrap with                                                                               
the -d option to download the auto-generated files instead.                                                                                                             

So it fails correctly.

@jdemeyer
Copy link

jdemeyer commented Feb 7, 2019

comment:27

One more detail: it would be better to reserve a specific exit code for that case. I'll prepare a review commit.

@jdemeyer
Copy link

jdemeyer commented Feb 7, 2019

@jdemeyer
Copy link

jdemeyer commented Feb 7, 2019

Changed commit from 87621c7 to 187de34

@jdemeyer
Copy link

jdemeyer commented Feb 7, 2019

comment:29

I'm happy with this now. Feel free to set to positive review if you agree.


New commits:

dcf2ab6renaming PKG_* -> SPKG_* in m4
13c87f7check for pkg-config's m4 macros at bootstrap
187de34Use exit status 16 for missing pkg-config macros

@jdemeyer
Copy link

jdemeyer commented Feb 7, 2019

Reviewer: Jeroen Demeyer

@dimpase
Copy link
Member Author

dimpase commented Feb 7, 2019

comment:30

looks good to me, thanks!

@embray
Copy link
Contributor

embray commented Feb 7, 2019

comment:31

I've been AFK, else I would've chimed in sooner. But this looks good to me now.

Indeed, there's no need to actually have pkg-config to build anything, just the PKG_ macros that are typically installed alongside pkg-config. I had also just suggested adding a copy of pkg.m4 to our m4/ directory but I like this even better (I see no reason a build machine, except maybe on OSX, shouldn't have pkg-config installed anyways).

@vbraun
Copy link
Member

vbraun commented Feb 8, 2019

Changed branch from u/jdemeyer/build/bootstrap_check to 187de34

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

No branches or pull requests

4 participants