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: |
Description (last modified by )
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
- Status changed from new to needs_review
comment:2 Changed 3 years ago by
- Description modified (diff)
comment:3 Changed 3 years ago by
comment:4 Changed 3 years ago by
- Branch changed from u/embray/build/configure/pre-post to public/build/configure/pre-post
- Commit changed from a2bab81a11e70bdb303ab54799516a6ae948c398 to f9c9a91dd0b142da16409f28ddb1bc476d052b15
comment:5 Changed 3 years ago by
That's a good point; yes it included #27212. Thanks for rebasing.
comment:6 follow-up: ↓ 8 Changed 3 years ago by
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: ↓ 9 Changed 3 years ago by
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
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: ↓ 10 Changed 3 years ago by
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
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
comment:12 Changed 3 years ago by
- 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
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
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
- Commit changed from f9c9a91dd0b142da16409f28ddb1bc476d052b15 to 1c24df14e480b860f83e7bbe1d4f1b86f6d42398
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
1c24df1 | Refactor 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:17 Changed 3 years ago by
- Reviewers set to Dima Pasechnik
- Status changed from needs_review to positive_review
OK, good.
comment:18 Changed 3 years ago by
- Branch changed from public/build/configure/pre-post to 1c24df14e480b860f83e7bbe1d4f1b86f6d42398
- Resolution set to fixed
- Status changed from positive_review to closed
This seems to depend, and use the branch, of #27212, no?