#28224 closed enhancement (fixed)
spkgconfigure.m4 for lcalc
Reported by:  dimpase  Owned by:  

Priority:  major  Milestone:  sage8.9 
Component:  build: configure  Keywords:  spkgconfigure 
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: 
Description (last modified by )
Debian and Fedora package lcalc (Fedora calls the package Lfunctiondevel
). On Debian one needs packages liblfunctiondev
and lcalc
.
One needs to check how wellpatched 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 sagedist, 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 followup: ↓ 2 Changed 3 years ago by
comment:2 in reply to: ↑ 1 Changed 3 years ago by
Replying to dimpase:
Does Conda provide
lcalc
?
Yes and uses the patches from sage https://github.com/condaforge/lcalcfeedstock/tree/master/recipe/patches
comment:3 Changed 3 years ago by
 Dependencies set to #28242
lcalc depends on PARI, so we need to deal with pari first.
comment:4 Changed 3 years ago by
 Description modified (diff)
comment:5 Changed 3 years ago by
 Branch set to u/dimpase/packages/lcalcconf
 Commit set to 311fce3b0d7dec7b60aaa8a98f47ae45f295c43d
 Status changed from new to needs_review
New commits:
311fce3  spkgconfigure for lcalc

comment:6 Changed 3 years ago by
 Status changed from needs_review to needs_work
sagedist, 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
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
 Commit changed from 311fce3b0d7dec7b60aaa8a98f47ae45f295c43d to dbcf50601a6913abd19b5e0af2e2631ae4b0bd0b
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
dbcf506  spkgconfigure for lcalc

comment:9 Changed 3 years ago by
 Commit changed from dbcf50601a6913abd19b5e0af2e2631ae4b0bd0b to 36c5fcaaf2e17f3b5b61c637501f54c886f315c3
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
36c5fca  spkgconfigure for lcalc

comment:10 Changed 3 years ago by
 Status changed from needs_work to needs_review
OK, so the updated branch works with Debian's lcalc, currently checking with sagedist's lcalc. Any chance to test this with Conda? Thanks!
comment:11 Changed 3 years ago by
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
 Commit changed from 36c5fcaaf2e17f3b5b61c637501f54c886f315c3 to 093f706a39342393803ab0885d13ccb96bd2f360
Branch pushed to git repo; I updated commit sha1. New commits:
093f706  typo  with it the test was always passing

comment:13 Changed 3 years ago by
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
 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/66d9302300f04c128a255d6d4e486740/sagebuild/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
hmm, could you attach the output of ./configure and config.log ? Thanks!
comment:16 followup: ↓ 18 Changed 3 years ago by
Else condition of AC_CHECK_HEADERS
is run for the first header if it is not found.
comment:17 Changed 3 years ago by
 Commit changed from 093f706a39342393803ab0885d13ccb96bd2f360 to bbefc4e91ae1236e73131a345dbcc8ce90d41129
Branch pushed to git repo; I updated commit sha1. New commits:
bbefc4e  do not fall for false positive in AC_CHECK_HEADERS

comment:18 in reply to: ↑ 16 Changed 3 years ago by
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
Works for me now.
comment:20 Changed 3 years ago by
 Commit changed from bbefc4e91ae1236e73131a345dbcc8ce90d41129 to d18b88c23d303c5c2654fce6094aae916cd03fec
Branch pushed to git repo; I updated commit sha1. New commits:
d18b88c  workaoroung for gcc (?)

comment:21 Changed 3 years ago by
 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 nonempty and OK for gcc (7 and 8.3, neither worked).
Needs final review now.
comment:22 Changed 3 years ago by
 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
reviewer name missing
comment:24 Changed 3 years ago by
 Reviewers set to Isuru Fernando
comment:25 followup: ↓ 26 Changed 3 years ago by
 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
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 followup: ↓ 28 Changed 3 years ago by
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
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.
comment:29 followup: ↓ 31 Changed 3 years ago by
"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
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
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
 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:
80bef09  distutils can't deal with empty extra_compile_args

comment:33 Changed 3 years ago by
 Status changed from needs_review to positive_review
reworded the last commit message, no code changes.
comment:34 Changed 3 years ago by
 Dependencies #28242 deleted
comment:35 Changed 3 years ago by
I avoided this ticket so far because lcalc
is one of those packages that "smells" and may be better left alone.
For the record sageongentoo 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/sagetracmirror/blob/master/build/pkgs/lcalc/patches/Makefile.patch#L231
being replaced by
https://github.com/sagemath/sagetracmirror/blob/master/build/pkgs/lcalc/patches/Makefile.patch#L234
comment:36 Changed 3 years ago by
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
Being uniform about these headers locations in distributions and sage certainly would help.
comment:38 Changed 3 years ago by
Right. I just checked that on Fedora it's Lfunction
too.
I've asked on sagedevel about the reason for these naming twists...
https://groups.google.com/d/msg/sagedevel/fAPWh1G8_Qw/xMAmtUapAQAJ
comment:39 Changed 3 years ago by
 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
comment:40 Changed 3 years ago by
 Status changed from needs_review to needs_work
Needs a version bump for lcalc
comment:41 Changed 3 years ago by
 Commit changed from 567d9a0ce9375984c77a53e740524e50ded10c73 to 6aa7e76c3a5058c3427bfc0d0f1c501e78369e84
Branch pushed to git repo; I updated commit sha1. New commits:
6aa7e76  lcalc version bump due to moved INCDIR

comment:42 Changed 3 years ago by
 Status changed from needs_work to needs_review
comment:43 Changed 3 years ago by
 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
 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
 Commit 6aa7e76c3a5058c3427bfc0d0f1c501e78369e84 deleted
FYI, a more recent snapshot, for version 1.3, is available at https://github.com/agrawroh/lcalc . It has been exported from code.google.com/p/lcalc.
Does Conda provide
lcalc
?