Opened 4 years ago

Closed 4 years ago

#27219 closed defect (fixed)

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

Reported by: Dima Pasechnik Owned by:
Priority: major Milestone: sage-8.7
Component: build Keywords:
Cc: Erik Bray, Volker Braun, Jeroen Demeyer Merged in:
Authors: Dima Pasechnik Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: 187de34 (Commits, GitHub, GitLab) Commit: 187de3457be410072694ea4a69583ba15edde09a
Dependencies: Stopgaps:

Status badges

Description (last modified by Dima Pasechnik)

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.

Change History (32)

comment:1 Changed 4 years ago by Dima Pasechnik

Branch: u/dimpase/build/bootstrap_check
Cc: Erik Bray Volker Braun added
Commit: af57d5b8852f43fd1878382d08d0fc43c6bb9c62
Status: newneeds_review

OK, Volker, how about this?

comment:2 Changed 4 years ago by Volker Braun

Cc: Jeroen Demeyer added

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

comment:3 in reply to:  2 Changed 4 years ago by Dima Pasechnik

Authors: Dima Pasechnik

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.

comment:4 in reply to:  2 Changed 4 years ago by Jeroen Demeyer

Status: needs_reviewneeds_work

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.

comment:5 Changed 4 years ago by Jeroen Demeyer

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

comment:6 Changed 4 years ago by Jeroen Demeyer

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

comment:7 Changed 4 years ago by git

Commit: af57d5b8852f43fd1878382d08d0fc43c6bb9c626e9f902941d25c10bc97bd106ecbcd9fe2dc5cd8

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

comment:8 Changed 4 years ago by Dima Pasechnik

Status: needs_workneeds_review

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.

comment:9 Changed 4 years ago by Dima Pasechnik

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

comment:10 Changed 4 years ago by Volker Braun

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

comment:11 Changed 4 years ago by Jeroen Demeyer

Description: modified (diff)
Status: needs_reviewneeds_work

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.

comment:12 Changed 4 years ago by Jeroen Demeyer

Description: modified (diff)

comment:13 Changed 4 years ago by git

Commit: 6e9f902941d25c10bc97bd106ecbcd9fe2dc5cd8cc372289db0b8cd0753220b65162afdaf24481b9

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

comment:14 Changed 4 years ago by git

Commit: cc372289db0b8cd0753220b65162afdaf24481b989a402f1e848652a1845d8baa07b8f16ec063db0

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

89a402fdo not put the libffi/spkg-configure.m4 here

comment:15 Changed 4 years ago by Dima Pasechnik

OK, so this should be the correct fix, merging cleanly with #27114, too.

Last edited 4 years ago by Dima Pasechnik (previous) (diff)

comment:16 in reply to:  11 ; Changed 4 years ago by Dima Pasechnik

Replying to jdemeyer:

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.

Generating configure needs pkg-config since #27114, which uses PKG_* m4 macros, why not? The resulting ./configure does not need pkg-config, it can do without just fine.

Last edited 4 years ago by Dima Pasechnik (previous) (diff)

comment:17 Changed 4 years ago by Dima Pasechnik

Status: needs_workneeds_review

comment:18 Changed 4 years ago by Dima Pasechnik

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

comment:19 in reply to:  16 Changed 4 years ago by Jeroen Demeyer

Status: needs_reviewneeds_work

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.

comment:20 Changed 4 years ago by Jeroen Demeyer

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.

comment:21 in reply to:  18 ; Changed 4 years ago by Jeroen Demeyer

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.

comment:22 Changed 4 years ago by Dima Pasechnik

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?

comment:23 in reply to:  21 Changed 4 years ago by Dima Pasechnik

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.

comment:24 Changed 4 years ago by git

Commit: 89a402f1e848652a1845d8baa07b8f16ec063db087621c7b1d84df427ef10eaada47cb054080f6b6

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

comment:25 Changed 4 years ago by Dima Pasechnik

Description: modified (diff)
Status: needs_workneeds_review

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

comment:26 in reply to:  22 Changed 4 years ago by Jeroen Demeyer

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.

comment:27 Changed 4 years ago by Jeroen Demeyer

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

comment:28 Changed 4 years ago by Jeroen Demeyer

Branch: u/dimpase/build/bootstrap_checku/jdemeyer/build/bootstrap_check

comment:29 Changed 4 years ago by Jeroen Demeyer

Commit: 87621c7b1d84df427ef10eaada47cb054080f6b6187de3457be410072694ea4a69583ba15edde09a
Reviewers: Jeroen Demeyer

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

comment:30 Changed 4 years ago by Dima Pasechnik

Status: needs_reviewpositive_review

looks good to me, thanks!

comment:31 Changed 4 years ago by Erik Bray

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).

comment:32 Changed 4 years ago by Volker Braun

Branch: u/jdemeyer/build/bootstrap_check187de3457be410072694ea4a69583ba15edde09a
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.