Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#27114 closed enhancement (fixed)

Further libffi fixes

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-8.7
Component: packages: standard Keywords: spkg-configure
Cc: embray, strogdon Merged in:
Authors: Erik Bray Reviewers: Erik Bray, Dima Pasechnik
Report Upstream: N/A Work issues:
Branch: 07e5561 (Commits, GitHub, GitLab) Commit:
Dependencies: #27109, #27219 Stopgaps:

Status badges

Description (last modified by dimpase)

with #27219 done, all it remains is just to fix spkg-configure.m4

Change History (33)

comment:1 Changed 3 years ago by jdemeyer

  • Branch set to u/embray/build/ticket-27109
  • Commit set to 4d9ccb4a31892374bc8eb1b16a7f9b9a2d7cc1d1

New commits:

bfa7c90Trac #27109: Include upstream patch to libffi for --disable-multi-os-directory
4d9ccb4Trac #27109: Use pkg-config if possible to detect libffi

comment:2 Changed 3 years ago by jdemeyer

  • Dependencies set to #27109

comment:3 Changed 3 years ago by jdemeyer

  • Cc embray strogdon added
  • Status changed from new to needs_review

comment:4 follow-up: Changed 3 years ago by embray

Since Steven already said this worked, I'm inclined to go ahead and set this to positive review. Though I might like to try on OSX since it doesn't have pkg-config and I'm not 100% sure what it will do in this case (I think it will "fail correctly" but I'm not certain).

comment:5 in reply to: ↑ 4 Changed 3 years ago by strogdon

Replying to embray:

Since Steven already said this worked, I'm inclined to go ahead and set this to positive review. Though I might like to try on OSX since it doesn't have pkg-config and I'm not 100% sure what it will do in this case (I think it will "fail correctly" but I'm not certain).

I'm fine with this. I too think it should be checked on other distros.

Last edited 3 years ago by strogdon (previous) (diff)

comment:6 Changed 3 years ago by strogdon

Perhaps we should try to get this in as soon as possible, else there will be Linuxes with system libffi installed where the Sage version will be built and used. A system libffi will then not be used until an upgrade or a manual removal of the Sage installed version.

comment:8 Changed 3 years ago by strogdon

Is there any reason why the patchbot emmits:

configure:8323: error: possibly undefined macro: AC_SEARCH_LIBS
configure:8324: error: possibly undefined macro: AC_CHECK_HEADERS

Shouldn't it have autotools installed?

comment:9 Changed 3 years ago by embray

On OSX this "works" okay, or doesn't work as the case may be, in that it (correctly) fails to find pkg-config, and thus does not try to use pkg-config.

I am seeing some other troubling problems failing to detect a system libffi on OSX, but I think whatever that problem is predates this ticket.

comment:10 Changed 3 years ago by embray

I have no idea about the patchbot. It is probably missing some dependency for running autoconf. Maybe this ticket needs a configure.tar.gz package, though it really should't...

comment:11 Changed 3 years ago by embray

  • Status changed from needs_review to positive_review

comment:12 Changed 3 years ago by embray

  • Reviewers set to Erik Bray

Let's just ask Volker and see.

comment:13 Changed 3 years ago by git

  • Commit changed from 4d9ccb4a31892374bc8eb1b16a7f9b9a2d7cc1d1 to 3d2eb36cc0ff7bdc9cb5447858aa15413cd07da4
  • Status changed from positive_review to needs_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

761c978AC_CHECK_HEADERS should come before AC_SEARCH_LIBS
3d2eb36move ffi/ffi.h first since it seems to be more common too

comment:14 Changed 3 years ago by embray

With apologies for being a bit sloppy about it, here are a couple more fixes I found while testing this on OSX.

comment:15 Changed 3 years ago by embray

  • Status changed from needs_review to positive_review

comment:16 Changed 3 years ago by dimpase

just to confirm that libffi's spkg-configure.m4 on this branch works with the pretty non-standard settings on Gentoo, cf. #27175.

comment:17 Changed 3 years ago by dimpase

However, supporting this properly on Homebrew really needs a call to pkg-config. (as there ffi.h really goes to a weird location).

comment:18 Changed 3 years ago by vbraun

  • Status changed from positive_review to needs_work

Fails ./bootstrap on kucalc (autoconf-2.13), maybe the script could be made more backward compatible?

make[1]: warning: -jN forced in submake: disabling jobserver mode.
configure.ac:308: installing 'config/compile'
configure.ac:105: installing 'config/config.guess'
configure.ac:105: installing 'config/config.sub'
configure.ac:68: installing 'config/install-sh'
configure.ac:68: installing 'config/missing'
configure.ac:13: error: possibly undefined macro: dnl
      If this token and others are legitimate, please use m4_pattern_allow.
      See the Autoconf documentation.
configure:9028: error: possibly undefined macro: AC_CHECK_HEADERS
configure:9029: error: possibly undefined macro: AC_SEARCH_LIBS
Makefile:197: recipe for target 'configure' failed
make[1]: Leaving directory '/var/lib/buildbot/slave/sage_git/build'
make[1]: *** [configure] Error 1
Makefile:31: recipe for target 'base-toolchain' failed
make: *** [base-toolchain] Error 2
make: Target 'start' not remade because of errors.

comment:19 Changed 3 years ago by dimpase

  • Status changed from needs_work to positive_review

configure.ac:13: error: possibly undefined macro: dnl indicates that this is waaaay to old for anything useful, really. It is from 1999 (sic!)

I think we should move on here, and whoever runs that bot should update...

comment:20 follow-up: Changed 3 years ago by vbraun

  • Status changed from positive_review to needs_work

This is Ubuntu 16.04.5 LTS. Sorry I had the wrong autoconf version (multiple are installed)

vbraun@kucalc:~$ autoconf --version
autoconf (GNU Autoconf) 2.69

comment:21 in reply to: ↑ 20 Changed 3 years ago by dimpase

Replying to vbraun:

This is Ubuntu 16.04.5 LTS. Sorry I had the wrong autoconf version (multiple are installed)

vbraun@kucalc:~$ autoconf --version
autoconf (GNU Autoconf) 2.69

But the error messages are from a very old autoconf... What is the problem then, what "needs_work"?

Are you saying you want a check that autoconf is new enough, and better error messages?

comment:22 Changed 3 years ago by embray

Volker, please clarify: Should this just have a new configure tarball? There's no reason for this to fail unless that machine is missing some dependencies to build the configure script, or some of the other tooling is misconfigured. I cannot reproduce the same issue.

comment:23 Changed 3 years ago by embray

Okay, I was able to reproduce in a container. Probably something is missing but I'm not sure what...

comment:24 follow-up: Changed 3 years ago by embray

  • Status changed from needs_work to needs_info

I see. These errors indicate that pkg-config is not installed, since it is required to have the PKG_CHECK_MODULES macro.

I could put in a fallback so that it works when PKG_CHECK_MODULES is not defined, but that would still build to a partially-crippled configure script. So can we just require pkg-config to be installed to build the configure script?

Alternatively, it might work if I include a copy of pkg.m4 in our m4/ directory, but I would rather not have to.

comment:25 in reply to: ↑ 24 Changed 3 years ago by embray

Replying to embray:

Alternatively, it might work if I include a copy of pkg.m4 in our m4/ directory, but I would rather not have to.

Update: this does work, but it's not ideal. I would rather just have pkg-config installed.

comment:26 Changed 3 years ago by dimpase

If this dependence is really prebuilt- only, then why not, I see no problem in requiring it along with autotools

comment:27 Changed 3 years ago by vbraun

I don't mind requiring pkgconfig to actually run autoconf, but having bootstrap error out and not download the confball is not an option ;-)

comment:28 Changed 3 years ago by dimpase

Please see #27219 to deal with the bootstrap issue.

comment:29 Changed 3 years ago by vbraun

  • Dependencies changed from #27109 to #27109, #27219

comment:30 Changed 3 years ago by dimpase

  • Branch changed from u/embray/build/ticket-27109 to public/build/better_libffi_config
  • Commit changed from 3d2eb36cc0ff7bdc9cb5447858aa15413cd07da4 to 07e5561184d6520f049334cf1e1619c1428e5433
  • Description modified (diff)
  • Reviewers changed from Erik Bray to Erik Bray, Dima Pasechnik
  • Status changed from needs_info to needs_review

Erik, are you fine with this?

comment:31 Changed 3 years ago by embray

  • Status changed from needs_review to positive_review

Yep, per my comment on the other ticket. Thanks!

comment:32 Changed 3 years ago by vbraun

  • Branch changed from public/build/better_libffi_config to 07e5561184d6520f049334cf1e1619c1428e5433
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:33 Changed 3 years ago by dimpase

  • Commit 07e5561184d6520f049334cf1e1619c1428e5433 deleted
  • Keywords spkg-configure added
Note: See TracTickets for help on using tickets.