Opened 3 years ago

Closed 3 years ago

#27641 closed enhancement (fixed)

SAGE_SPKG_CONFIGURE macro: Add new pre-check and post-check optional arguments

Reported by: embray Owned by:
Priority: major Milestone: sage-8.8
Component: build: configure Keywords:
Cc: dimpase Merged in:
Authors: Erik Bray Reviewers: Dima Pasechnik
Report Upstream: N/A Work issues:
Branch: 1c24df1 (Commits, GitHub, GitLab) Commit: 1c24df14e480b860f83e7bbe1d4f1b86f6d42398
Dependencies: Stopgaps:

Status badges

Description (last modified by embray)

This adds two new optional arguments to the SAGE_SPKG_CONFIGURE. Both are sets of actions that should be run unconditionally when running the resulting configure script, regardless of the outcome of checks for the package.

This demonstrates a use case for this in mpir, where no matter what else it is imperative to set a value for SAGE_MP_LIBRARY (even if we are not installing the mpir SPKG, or the SPKG is already installed).

The follow-up ticket #27642 might make more clear why this is useful: If some SPKG is already installed we shouldn't bother checking for it on the system, so we skip the dependency checks. But we may still want to perform other package-specific configure actions.

One thing I don't like about this entirely is that it would seem clearer and more logical to make the "pre" argument come before the other arguments. However, since it's optional, and is not used for many packages, I thought it would be better to add it to the end of the argument list and not have to update every spkg-configure.m4 to pass an empty value for the first argument.

Change History (18)

comment:1 Changed 3 years ago by embray

  • Status changed from new to needs_review

comment:2 Changed 3 years ago by embray

  • Description modified (diff)

comment:3 Changed 3 years ago by dimpase

This seems to depend, and use the branch, of #27212, no?

comment:4 Changed 3 years ago by dimpase

  • Branch changed from u/embray/build/configure/pre-post to public/build/configure/pre-post
  • Commit changed from a2bab81a11e70bdb303ab54799516a6ae948c398 to f9c9a91dd0b142da16409f28ddb1bc476d052b15

rebased over 8.8.beta3


New commits:

191269eAdd support for unconditional pre- and post-check actions in spkg-configure.m4
f9c9a91Refactor the mpir/spkg-configure.m4 (and to a lesser extent the one for gmp) to use the new pre and post arguments to SAGE_SPKG_CONFIGURE

comment:5 Changed 3 years ago by embray

That's a good point; yes it included #27212. Thanks for rebasing.

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

Do you have an example use of the macro where the 3rd argument is not empty? (Perhaps from one of the tickets on the list #27330 ?)

And, by the way, docs explaining them are hard to read, could you please instead write: "1st argument does this and this", "2nd argument does...", etc?

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

It looks as if it basically adds a bit of structure into spkg-configure.m4 files (i.e. logically different parts are inside []), no more than that, right? Can this wait until it gets used for actual refactoring?

comment:8 in reply to: ↑ 6 Changed 3 years ago by embray

Replying to dimpase:

Do you have an example use of the macro where the 3rd argument is not empty? (Perhaps from one of the tickets on the list #27330 ?)

The (currently) third argument is used in a few packages. At least one example I can think of off the top of my head is yasm. The point of the third argument is to check whether that dependency is needed at all for anything, as opposed to the second argument which assumes the package is required but does the check for whether it's provided already by the system.

And, by the way, docs explaining them are hard to read, could you please instead write: "1st argument does this and this", "2nd argument does...", etc?

I can try to reword it.

comment:9 in reply to: ↑ 7 ; follow-up: Changed 3 years ago by embray

Replying to dimpase:

It looks as if it basically adds a bit of structure into spkg-configure.m4 files (i.e. logically different parts are inside []), no more than that, right?

Essentially, yes. It just allows insertion of commands into a couple places where it wasn't previously possible to insert them.

Can this wait until it gets used for actual refactoring?

I'm not sure what you mean here. It is used for #27642 which has this ticket as a dependency.

comment:10 in reply to: ↑ 9 Changed 3 years ago by dimpase

Replying to embray:

Replying to dimpase:

It looks as if it basically adds a bit of structure into spkg-configure.m4 files (i.e. logically different parts are inside []), no more than that, right?

Essentially, yes. It just allows insertion of commands into a couple places where it wasn't previously possible to insert them.

Can this wait until it gets used for actual refactoring?

I'm not sure what you mean here. It is used for #27642 which has this ticket as a dependency.

I mean to say, I'd like to see this ticket "in action", by itself it does not make much sense. How about reviewing once #27642 is done?

comment:11 Changed 3 years ago by embray

I think #27642 already demonstrates in "in action". The only bits missing from #27642, if any (that I'm aware of) is ensuring that configure is re-run with its previous arguments. I would go ahead and start playing with #27642.

comment:12 Changed 3 years ago by dimpase

  • Status changed from needs_review to needs_work

Trying this with ./configure leaves gmp not marked as an not to be installed, while mpir is marked as not to be installed.

comment:13 Changed 3 years ago by embray

I had a problem like that much earlier when I was working on this, but I thought I fixed that.

comment:14 Changed 3 years ago by embray

Yes, I see the problem. The change in build/pkgs/gmp/spkg-configure.m4 is not correct.

Nothing in the "PRE" argument should set the sage_spkg_install_<pkgname>, and if the second argument (the one that's supposed to actually check whether or not to install the SPKG) is empty then it will force sage_spkg_install_gmp=yes:

m4_ifval(
 [$2],
 [AS_VAR_SET_IF(SPKG_INSTALL_VAR, [], SPKG_INSTALL_VAR[=no])],
 [AS_VAR_SET_IF(SPKG_INSTALL_VAR, [], SPKG_INSTALL_VAR[=yes])])

That's what's going on in the above lines in SAGE_SPKG_CONFIGURE

comment:15 Changed 3 years ago by git

  • Commit changed from f9c9a91dd0b142da16409f28ddb1bc476d052b15 to 1c24df14e480b860f83e7bbe1d4f1b86f6d42398

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

1c24df1Refactor the mpir/spkg-configure.m4 (and to a lesser extent the one for gmp) to use the new pre and post arguments to SAGE_SPKG_CONFIGURE

comment:16 Changed 3 years ago by embray

  • Status changed from needs_work to needs_review

Try now.

comment:17 Changed 3 years ago by dimpase

  • Reviewers set to Dima Pasechnik
  • Status changed from needs_review to positive_review

OK, good.

comment:18 Changed 3 years ago by vbraun

  • Branch changed from public/build/configure/pre-post to 1c24df14e480b860f83e7bbe1d4f1b86f6d42398
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.