Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#28342 closed enhancement (fixed)

spkg-configure.m4 for m4ri, m4rie, givaro

Reported by: dimpase Owned by:
Priority: major Milestone: sage-8.9
Component: build: configure Keywords:
Cc: embray, fbissey, isuruf Merged in:
Authors: Dima Pasechnik Reviewers: Isuru Fernando, Erik Bray
Report Upstream: Fixed upstream, in a later stable release. Work issues:
Branch: 21d6aa5 (Commits, GitHub, GitLab) Commit:
Dependencies: #28352 Stopgaps:

Status badges

Description

these are very straightforward, basically just doing dependencies and version checks.

m4ri(e) are quite stable, and it appears impossibe to distinguish the present version of m4rie from the older one. We just rely on the version of m4ri, it's dependence, which thankfully has pkg-config support.

Change History (19)

comment:1 Changed 3 years ago by dimpase

  • Report Upstream changed from N/A to Reported upstream. No feedback yet.

comment:2 Changed 3 years ago by dimpase

  • Branch set to u/dimpase/packages/m4ri-e-givaro
  • Cc embray fbissey isuruf added
  • Commit set to 283e64a5018a726fa92e36cc7e143b228600fcb4
  • Status changed from new to needs_review

this is quite straighforward w.r.t. to m4ri(e) and givaro spkg-configure files, less so with a macro to check dependencies that I wrote and used on these examples. Please review.


New commits:

283e64amacro SAGE_SPKG_DEPCHECK, to test for dependencies:

comment:3 Changed 3 years ago by git

  • Commit changed from 283e64a5018a726fa92e36cc7e143b228600fcb4 to 4e1d00eb74017fac3eea1106a5ee9b08d24d83cf

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

4e1d00eSAGE_SPKG_DEPCHECK, spkg-configure for m4ri(e), givaro

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

One improvement for SAGE_SPKG_DEPCHECK would be to get rid of the 1st argument, the package name. After all it is called from within SAGE_SPKG_CONFIGURE, and the package name should be "known", but I have no clue how to get it.

comment:5 Changed 3 years ago by isuruf

  • Reviewers set to Isuru Fernando

This works for me on conda. Somebody else should review the macro.

comment:6 Changed 3 years ago by embray

  • Dependencies set to #28352

comment:7 in reply to: ↑ 4 Changed 3 years ago by embray

Replying to dimpase:

One improvement for SAGE_SPKG_DEPCHECK would be to get rid of the 1st argument, the package name. After all it is called from within SAGE_SPKG_CONFIGURE, and the package name should be "known", but I have no clue how to get it.

If it's only used within SAGE_SPKG_CONFIGURE it can probably use the SPKG_NAME macro. But I think it's maybe a bit cleaner actually to keep it more independent.

comment:8 Changed 3 years ago by dimpase

Already working on this ticket I made an error involving the 1st parameter of SAGE_SPKG_DEPCHECK, as I copy-pasted it and didn't adjust is :-)

As we should eventually rewrite already done 20 or 30 spkg-configure.4 files using this macro, it's better to minimise the number of places one can make this sort of error...

comment:9 Changed 3 years ago by embray

I think a typo in SAGE_SPKG_DEPCHECK:

AC_DEFUN([SAGE_SPKG_DEPCHECK], [
+    m4_foreach_w([DEP], $2, [
+       AC_REQUIRE([SAGE_SPKG_CONFIGURE_]m4_toupper(DEP))])
+    AC_MSG_CHECKING([installing $2? ])
+    AS_IF([test x = y m4_foreach_w([DEP], $2, [ -o [x$sage_spkg_install_]DEP = x$yes])], }}}

At the end of the last line I think you just meant `xyes` not `x$yes`.
Version 0, edited 3 years ago by embray (next)

comment:10 Changed 3 years ago by embray

  • Status changed from needs_review to needs_work

comment:11 Changed 3 years ago by git

  • Commit changed from 4e1d00eb74017fac3eea1106a5ee9b08d24d83cf to afacd43081bb1bbcc91c8a8e8ee8f02933dfabdc

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

3bf2069remove spurious '$'
0fa8a4dupdate lists of packages which can be useful for installation
afacd43remove unneeded parameter, use SPKG_NAME instead

comment:12 Changed 3 years ago by dimpase

  • Report Upstream changed from Reported upstream. No feedback yet. to Fixed upstream, in a later stable release.
  • Reviewers changed from Isuru Fernando to Isuru Fernando, Erik Bray
  • Status changed from needs_work to needs_review

OK, fixed that embarassing extra $, and further improvements (see commits). Needs review.

comment:13 Changed 3 years ago by git

  • Commit changed from afacd43081bb1bbcc91c8a8e8ee8f02933dfabdc to 21d6aa514a5b918ebfa5c1ff1b91ccd63fa33f2d

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

21d6aa5mention lcalc system packages (prepare for #28224)

comment:14 Changed 3 years ago by embray

I don't really think afacd43 is much of an improvement. To me it looks fragile and potentially confusing. But I won't fight it for now, as long as it works, if you really think it's better...

comment:15 Changed 3 years ago by embray

  • Status changed from needs_review to positive_review

The m4ri in my (old-ish) Ubuntu is too old to otherwise test this.

I had a container with a newer Ubuntu but it seems to have gone away. I'll set that up again, but in the meantime I think this looks good assuming it works at least on your system.

comment:16 Changed 3 years ago by dimpase

you may try installing m4ri(e) in /usr/local/, for testing.

certainly the new macro should have been a part of sage_spkg_configure() all along, getting rid of one parameter is a step towards this refactoring...

comment:17 Changed 3 years ago by embray

I got a new Ubuntu 18.04 container set up and it worked! :)

comment:18 Changed 3 years ago by vbraun

  • Branch changed from u/dimpase/packages/m4ri-e-givaro to 21d6aa514a5b918ebfa5c1ff1b91ccd63fa33f2d
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:19 Changed 3 years ago by dimpase

  • Commit 21d6aa514a5b918ebfa5c1ff1b91ccd63fa33f2d deleted

It seems something wasn't fully tested w.r.t. #26932, and in fact this needs

  • build/pkgs/givaro/spkg-configure.m4

    a b  
    11SAGE_SPKG_CONFIGURE([givaro], [
    2     m4_pushdef([SAGE_GIVARO_MINVER],["40004"])
     2    m4_pushdef([SAGE_GIVARO_MINVER],["40101"])
    33    SAGE_SPKG_DEPCHECK([gmp mpir], [
    44        AC_PATH_PROG([GIVAROCONFIG], [givaro-config])
    55        AS_IF([test x$GIVAROCONFIG = x], [

I've opened #28380 to fix this.

Note: See TracTickets for help on using tickets.