Opened 3 years ago

Closed 3 years ago

Last modified 2 years ago

#28224 closed enhancement (fixed)

spkg-configure.m4 for lcalc

Reported by: dimpase Owned by:
Priority: major Milestone: sage-8.9
Component: build: configure Keywords: spkg-configure
Cc: embray, isuruf, arojas, fbissey Merged in:
Authors: Dima Pasechnik Reviewers: Isuru Fernando
Report Upstream: N/A Work issues:
Branch: 6aa7e76 (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by dimpase)

Debian and Fedora package lcalc (Fedora calls the package L-function-devel). On Debian one needs packages liblfunction-dev and lcalc.

One needs to check how well-patched they are.

We had to introduce an env. var. SAGE_LCALC_INCDIR_NOLIBPREFIX to deal with the discrepancy in the include names, which is libLfunction/ on sage-dist, but Lfunction/ on Debian.

if SAGE_LCALC_INCDIR_NOLIBPREFIX is not '' then we assume that the include prefix is Lfunction/, and otherwise it is libLfunction/.

Change History (45)

comment:1 follow-up: Changed 3 years ago by dimpase

Does Conda provide lcalc?

comment:2 in reply to: ↑ 1 Changed 3 years ago by isuruf

Replying to dimpase:

Does Conda provide lcalc?

Yes and uses the patches from sage https://github.com/conda-forge/lcalc-feedstock/tree/master/recipe/patches

comment:3 Changed 3 years ago by dimpase

  • Dependencies set to #28242

lcalc depends on PARI, so we need to deal with pari first.

comment:4 Changed 3 years ago by dimpase

  • Description modified (diff)

comment:5 Changed 3 years ago by dimpase

  • Authors set to Dima Pasechnik
  • Branch set to u/dimpase/packages/lcalcconf
  • Commit set to 311fce3b0d7dec7b60aaa8a98f47ae45f295c43d
  • Status changed from new to needs_review

New commits:

311fce3spkg-configure for lcalc

comment:6 Changed 3 years ago by isuruf

  • Status changed from needs_review to needs_work

sage-dist, conda, arch installs the header as <prefix>/include/libLfunction/L.h while debian installs it as <prefix>/include/Lfunction/L.h.

You'll need more changes to make lcalc work with Debian way. For eg: https://github.com/sagemath/sage/blob/master/src/sage/libs/lcalc/lcalc_sage.h#L1

comment:7 Changed 3 years ago by dimpase

oops, right, one would in the very least deal with hardcoded

src/sage/libs/lcalc/lcalc_sage.h:#include "libLfunction/L.h"

comment:8 Changed 3 years ago by git

  • Commit changed from 311fce3b0d7dec7b60aaa8a98f47ae45f295c43d to dbcf50601a6913abd19b5e0af2e2631ae4b0bd0b

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

dbcf506spkg-configure for lcalc

comment:9 Changed 3 years ago by git

  • Commit changed from dbcf50601a6913abd19b5e0af2e2631ae4b0bd0b to 36c5fcaaf2e17f3b5b61c637501f54c886f315c3

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

36c5fcaspkg-configure for lcalc

comment:10 Changed 3 years ago by dimpase

  • Status changed from needs_work to needs_review

OK, so the updated branch works with Debian's lcalc, currently checking with sage-dist's lcalc. Any chance to test this with Conda? Thanks!

comment:11 Changed 3 years ago by isuruf

Works for me on conda. I see that you have test x$ac_cv_header_libLfunction = x. Isn't that supposed to be test x$ac_cv_header_libLfunction_L_h = x ?

comment:12 Changed 3 years ago by git

  • Commit changed from 36c5fcaaf2e17f3b5b61c637501f54c886f315c3 to 093f706a39342393803ab0885d13ccb96bd2f360

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

093f706typo - with it the test was always passing

comment:13 Changed 3 years ago by dimpase

good catch. I guess you won't be able to build lcalc cython extension without the latest commit...

comment:14 Changed 3 years ago by isuruf

  • Status changed from needs_review to needs_work

With the last commit and the one before that, I get

configure: === checking whether to install the lcalc SPKG ===
checking installing any of pari mpfr? ... no
checking for lcalc... /projects/66d93023-00f0-4c12-8a25-5d6d4e486740/sage-build/bin/lcalc
checking is lcalc's version good enough? ... yes.
checking Lfunction/L.h usability... no
checking Lfunction/L.h presence... no
checking for Lfunction/L.h... no
checking libLfunction/L.h usability... yes
checking libLfunction/L.h presence... yes
checking for libLfunction/L.h... yes
checking whether we can link and run a program using libLfunction... yes; use lcalc from the system

but lcalc spkg is being built.

comment:15 Changed 3 years ago by dimpase

hmm, could you attach the output of ./configure and config.log ? Thanks!

comment:16 follow-up: Changed 3 years ago by isuruf

Else condition of AC_CHECK_HEADERS is run for the first header if it is not found.

comment:17 Changed 3 years ago by git

  • Commit changed from 093f706a39342393803ab0885d13ccb96bd2f360 to bbefc4e91ae1236e73131a345dbcc8ce90d41129

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

bbefc4edo not fall for false positive in AC_CHECK_HEADERS

comment:18 in reply to: ↑ 16 Changed 3 years ago by dimpase

Replying to isuruf:

Else condition of AC_CHECK_HEADERS is run for the first header if it is not found.

fixed by the last commit, good catch. I never used AC_CHECK_HEADER*S* before...

comment:19 Changed 3 years ago by isuruf

Works for me now.

comment:20 Changed 3 years ago by git

  • Commit changed from bbefc4e91ae1236e73131a345dbcc8ce90d41129 to d18b88c23d303c5c2654fce6094aae916cd03fec

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

d18b88cworkaoroung for gcc (?)

comment:21 Changed 3 years ago by dimpase

  • Status changed from needs_work to needs_review

The latest commit fixes a weird build bug I was getting. Namely, if LCALC_INCDIR_NOLIBPREFIX=='' the build was failing. So I made it to always be something non-empty and OK for gcc (7 and 8.3, neither worked).

Needs final review now.

comment:22 Changed 3 years ago by isuruf

  • Status changed from needs_review to positive_review

I think distutils was sending an empty argument. Empty arguments will give that file not found error.

comment:23 Changed 3 years ago by chapoton

reviewer name missing

comment:24 Changed 3 years ago by isuruf

  • Reviewers set to Isuru Fernando

comment:25 follow-up: Changed 3 years ago by dimpase

  • Cc arojas fbissey added
  • Description modified (diff)

Distributions - new env. var., please take a look.

comment:26 in reply to: ↑ 25 Changed 3 years ago by arojas

Replying to dimpase:

Distributions - new env. var., please take a look.

As long as there's a fallback that allows not having to set the variable I'm fine with it - I can change the lcalc package headers accordingly.

comment:27 follow-up: Changed 3 years ago by dimpase

I am confused now. I thought that you are fine with variables, but not happy with running configure. Otherwise a much easier solution would have been to have something like src/sage/libs/lcalc/lcalc_sage.h.in and fill it in by ./configure.

comment:28 in reply to: ↑ 27 Changed 3 years ago by arojas

Replying to dimpase:

I am confused now. I thought that you are fine with variables, but not happy with running configure. Otherwise a much easier solution would have been to have something like src/sage/libs/lcalc/lcalc_sage.h.in and fill it in by ./configure.

Running configure < setting env variables < not having to do anything

So using an env variable with a sane fallback is best.

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

comment:29 follow-up: Changed 3 years ago by dimpase

"not having to do anything" would only be possible if Debian et. al reverted their clever idea to rename the include location to Lfunction.

I don't know what you mean by a fallback here.

comment:30 Changed 3 years ago by isuruf

I don't think any distribution shipping headers in libLfunction has to do anything here as there's a fallback in src/sage/env.py.

comment:31 in reply to: ↑ 29 Changed 3 years ago by arojas

Replying to dimpase:

"not having to do anything" would only be possible if Debian et. al reverted their clever idea to rename the include location to Lfunction.

I mean "not having to do anything when building sagelib on Arch"

I don't know what you mean by a fallback here.

I mean a reasonable behavior out of the box when the variable is unset. (ie. if some distro renames the header dir or the lib name, not setting the variable should use the upstream name)

comment:32 Changed 3 years ago by git

  • Commit changed from d18b88c23d303c5c2654fce6094aae916cd03fec to 80bef09bb4d2f6408bf8b1dd7316d6fa91c9dd70
  • Status changed from positive_review to needs_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. This was a forced push. New commits:

80bef09distutils can't deal with empty extra_compile_args

comment:33 Changed 3 years ago by dimpase

  • Status changed from needs_review to positive_review

reworded the last commit message, no code changes.

comment:34 Changed 3 years ago by dimpase

  • Dependencies #28242 deleted

comment:35 Changed 3 years ago by fbissey

I avoided this ticket so far because lcalc is one of those packages that "smells" and may be better left alone.

For the record sage-on-gentoo installs headers in Lfunction which is closer to the original upstream than what libLfunction is. The latter being a sage packaging special. https://github.com/sagemath/sagetrac-mirror/blob/master/build/pkgs/lcalc/patches/Makefile.patch#L231 being replaced by https://github.com/sagemath/sagetrac-mirror/blob/master/build/pkgs/lcalc/patches/Makefile.patch#L234

comment:36 Changed 3 years ago by isuruf

Shall we change the default and also the patch to use Lfunction then?

I can change it in conda.

comment:37 Changed 3 years ago by fbissey

Being uniform about these headers locations in distributions and sage certainly would help.

comment:38 Changed 3 years ago by dimpase

Right. I just checked that on Fedora it's Lfunction too. I've asked on sage-devel about the reason for these naming twists... https://groups.google.com/d/msg/sage-devel/fAPWh1G8_Qw/xMAmtUapAQAJ

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

comment:39 Changed 3 years ago by dimpase

  • Branch changed from u/dimpase/packages/lcalcconf to u/dimpase/packages/lcalcchangeinclude
  • Commit changed from 80bef09bb4d2f6408bf8b1dd7316d6fa91c9dd70 to 567d9a0ce9375984c77a53e740524e50ded10c73
  • Status changed from positive_review to needs_review

I've changed the branch so that INCDIR of libLfunction is Lfunction/.

this is much simpler, please review.


New commits:

ec86516Updated SageMath version to 8.9.beta8
567d9a0spkg-configure.m4 for lcalc, and INCDIR rename

comment:40 Changed 3 years ago by isuruf

  • Status changed from needs_review to needs_work

Needs a version bump for lcalc

comment:41 Changed 3 years ago by git

  • Commit changed from 567d9a0ce9375984c77a53e740524e50ded10c73 to 6aa7e76c3a5058c3427bfc0d0f1c501e78369e84

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

6aa7e76lcalc version bump due to moved INCDIR

comment:42 Changed 3 years ago by dimpase

  • Status changed from needs_work to needs_review

comment:43 Changed 3 years ago by fbissey

  • Status changed from needs_review to positive_review

Love the fact that I won't have to patch that little bit anymore - I am sure my colleagues in other distros are happy about that too.

comment:44 Changed 3 years ago by vbraun

  • Branch changed from u/dimpase/packages/lcalcchangeinclude to 6aa7e76c3a5058c3427bfc0d0f1c501e78369e84
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:45 Changed 2 years ago by gh-thierry-FreeBSD

  • Commit 6aa7e76c3a5058c3427bfc0d0f1c501e78369e84 deleted

FYI, a more recent snapshot, for version 1.3, is available at https://github.com/agrawroh/l-calc . It has been exported from code.google.com/p/l-calc.

Note: See TracTickets for help on using tickets.