Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#25900 closed enhancement (fixed)

Package libffi

Reported by: jdemeyer Owned by: embray
Priority: major Milestone: sage-8.7
Component: packages: standard Keywords: spkg-configure
Cc: embray Merged in:
Authors: Jeroen Demeyer Reviewers: Frédéric Chapoton, Erik Bray
Report Upstream: N/A Work issues:
Branch: e45cf27 (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by jdemeyer)

This is required for compiling Python 3.7

Tarball: ftp://sourceware.org/pub/libffi/libffi-3.2.1.tar.gz

Change History (24)

comment:1 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:2 Changed 4 years ago by jdemeyer

  • Branch set to u/jdemeyer/package_libffi

comment:3 Changed 4 years ago by jdemeyer

  • Commit set to 8f6d8304420e0894d5e8221238adcb09ab7edbed
  • Status changed from new to needs_review

New commits:

8f6d830Add libffi package

comment:4 Changed 4 years ago by chapoton

  • Reviewers set to Frédéric Chapoton
  • Status changed from needs_review to positive_review

ok, should be good. Tested on my machine with py2 (and py3 also). Let's ask the buildbots.

comment:5 follow-up: Changed 4 years ago by vbraun

  • Cc embray added

Afaik libffi has been a de facto dependency of ecl (and, therefore, Sage) for a long time. We could just document said dependency...

On a side note, the cygwin patchbot is unhappy with it

comment:6 Changed 4 years ago by embray

The only problem with the patchbot is that it was unable to find the tarball. Normally, with tickets that add new packages, it's supposed to be able to parse out the path to the source tarball from the ticket description, but that seems to have failed in this case. I'll look into why.

AFAIK libffi works fine on Cygwin--I have used it there plenty myself (and have even fixed some bugs in it :)

comment:7 Changed 4 years ago by embray

  • Status changed from positive_review to needs_info

Let me test on Cygwin though before setting this to positive review.

comment:8 Changed 4 years ago by embray

  • Owner changed from (none) to embray

comment:9 in reply to: ↑ 5 Changed 4 years ago by embray

Replying to vbraun:

Afaik libffi has been a de facto dependency of ecl (and, therefore, Sage) for a long time. We could just document said dependency...

I'm fine with adding an spkg for it. I think there should be one as some libffi versions do have bugs that could theoretically affect us.

But I do think there should be a configure-time check for it to avoid needlessly installing the spkg. I would like to start adding this for all new packages. For now it could go directly in configure.ac, but if some version of #24919 ever gets merged then the check might be moved.

comment:10 Changed 4 years ago by jdemeyer

If ECL really required libffi, I'm sure that we would have noticed by now. For example, I have a system where Python 3.7 failed to build because libffi wasn't installed. But this problem did not occur for ECL.

Erik: this ticket had positive_review and it's a dependency for Python 3.7. So it's fine if you want to add the logic to check for a system-wide install, but please don't stall this ticket.

comment:11 Changed 4 years ago by embray

I'm only stalling it to check that it actually works on Cygwin. I'm happy to add the system check either now, or as a separate ticket. I'd like to discuss making this required for adding any new spkgs, though since we haven't actually done that yet I won't hold this up over that.

comment:12 Changed 4 years ago by jdemeyer

Any news?

comment:13 Changed 4 years ago by embray

I'll try it today or tomorrow--soon as I'm done testing 8.4.beta1 on my Windows build.

comment:14 Changed 4 years ago by embray

Odd that libffi puts its headers in lib/libffi-3.2.1/include/. I found this to be the case on Cygwin and on Linux. I guess it's not really a problem since pkg-config reports that location correctly, and I bet there's probably a good reason for it. Just...odd.

comment:15 Changed 4 years ago by embray

Could you add something like:

  • configure.ac

    diff --git a/configure.ac b/configure.ac
    index 5a830e5..a31b53b 100644
    a b AC_CACHE_CHECK([for curl 7.22], [ac_cv_path_CURL], [ 
    435435    [need_to_install_curl=yes; ac_cv_path_CURL=no])])
    436436
    437437
     438# Only install libffi if needed; if found, we also check for its headers
     439# below
     440need_to_install_libffi=no
     441AC_SEARCH_LIBS([ffi_call], [ffi], [], [need_to_install_libffi=yes])
     442
     443
    438444###############################################################################
    439445# Check header files
    440446###############################################################################
    then 
    446452    AC_CHECK_HEADER([curl/curl.h], [], [need_to_install_curl=yes])
    447453fi
    448454
     455# If libffi is installed, we need to check for the ffi.h header file too.
     456if test $need_to_install_libffi = no;
     457then
     458    AC_CHECK_HEADERS([ffi.h ffi/ffi.h], [break], [need_to_install_libffi=yes])
     459fi
     460
    449461# complex.h is one that might not exist on older systems.
    450462AC_LANG(C++)
    451463AC_CHECK_HEADER([complex.h],[],[

?

Otherwise, seems fine to me. I haven't tested this against Python 3.7 itself yet, but that has other build problems on Cygwin I need to work on. If any further libffi-related problems come up in the context of building Python we can deal with it there.

comment:16 Changed 4 years ago by embray

  • Milestone changed from sage-8.4 to sage-8.5

comment:17 Changed 3 years ago by git

  • Commit changed from 8f6d8304420e0894d5e8221238adcb09ab7edbed to faad2c9730f11809c6ce052f41d7a74a7df1d11f

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

faad2c9Add libffi package

comment:18 follow-up: Changed 3 years ago by jdemeyer

  • Status changed from needs_info to needs_review

Erik, I added the new-style configure check. Does this look correct to you?

comment:19 Changed 3 years ago by git

  • Commit changed from faad2c9730f11809c6ce052f41d7a74a7df1d11f to e45cf27f5d21275c0fd69296a51e97ffdf543493

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

e45cf27Add libffi package

comment:20 Changed 3 years ago by embray

  • Milestone changed from sage-8.5 to sage-8.7

Retargeting some of my tickets.

comment:21 in reply to: ↑ 18 Changed 3 years ago by embray

  • Reviewers changed from Frédéric Chapoton to Frédéric Chapoton, Erik Bray
  • Status changed from needs_review to positive_review

Replying to jdemeyer:

Erik, I added the new-style configure check. Does this look correct to you?

Sorry, I didn't see your update before. I haven't tested, but it looks good to me in principle. If we need to make the check any more specific we can do that later (e.g. there could be bugs with older libffi versions, but I'm not worried about that unless it is found to actually affect someone's system).

comment:22 Changed 3 years ago by vbraun

  • Branch changed from u/jdemeyer/package_libffi to e45cf27f5d21275c0fd69296a51e97ffdf543493
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:23 Changed 3 years ago by chapoton

  • Commit e45cf27f5d21275c0fd69296a51e97ffdf543493 deleted

some issue arise from this ticket both on gentoo and ubuntu:

cp: cannot overwrite non-directory '/64bitdev/storage/sage-git_develop/sage/local/./lib64' with directory '/64bitdev/storage/sage-git_develop/sage/local/var/tmp/sage/build/libffi-3.2.1/inst/64bitdev/storage/sage-git_develop/sage/local/./lib64'

comment:24 Changed 3 years ago by embray

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