Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
embray opened this issue Apr 11, 2019 · 21 comments
Closed

Comments

@embray
Copy link
Contributor

embray commented Apr 11, 2019

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.

CC: @dimpase

Component: build: configure

Author: Erik Bray

Branch/Commit: 1c24df1

Reviewer: Dima Pasechnik

Issue created by migration from https://trac.sagemath.org/ticket/27641

@embray embray added this to the sage-8.8 milestone Apr 11, 2019
@embray

This comment has been minimized.

@dimpase
Copy link
Member

dimpase commented Apr 17, 2019

comment:3

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

@dimpase
Copy link
Member

dimpase commented Apr 19, 2019

@dimpase
Copy link
Member

dimpase commented Apr 19, 2019

Changed commit from a2bab81 to f9c9a91

@dimpase
Copy link
Member

dimpase commented Apr 19, 2019

comment:4

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

@embray
Copy link
Contributor Author

embray commented Apr 19, 2019

comment:5

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

@dimpase
Copy link
Member

dimpase commented Apr 19, 2019

comment:6

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?

@dimpase
Copy link
Member

dimpase commented Apr 23, 2019

comment:7

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?

@embray
Copy link
Contributor Author

embray commented Apr 23, 2019

comment:8

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.

@embray
Copy link
Contributor Author

embray commented Apr 23, 2019

comment:9

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.

@dimpase
Copy link
Member

dimpase commented Apr 23, 2019

comment:10

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?

@embray
Copy link
Contributor Author

embray commented Apr 24, 2019

comment:11

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.

@dimpase
Copy link
Member

dimpase commented May 3, 2019

comment:12

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

@embray
Copy link
Contributor Author

embray commented May 3, 2019

comment:13

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

@embray
Copy link
Contributor Author

embray commented May 3, 2019

comment:14

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 3, 2019

Changed commit from f9c9a91 to 1c24df1

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 3, 2019

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

@embray
Copy link
Contributor Author

embray commented May 3, 2019

comment:16

Try now.

@dimpase
Copy link
Member

dimpase commented May 3, 2019

Reviewer: Dima Pasechnik

@dimpase
Copy link
Member

dimpase commented May 3, 2019

comment:17

OK, good.

@vbraun
Copy link
Member

vbraun commented May 6, 2019

Changed branch from public/build/configure/pre-post to 1c24df1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants