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

Generic mechanism for system package checks at configure time #24919

Closed
embray opened this issue Mar 6, 2018 · 150 comments
Closed

Generic mechanism for system package checks at configure time #24919

embray opened this issue Mar 6, 2018 · 150 comments

Comments

@embray
Copy link
Contributor

embray commented Mar 6, 2018

Here's a look at the work I've been doing on top of (albeit not completely dependent on) #21524 in order to better support use of system packages for any of Sage's SPKGs.

This takes the workarounds that are already hard-coded in Sage's configure.ac for detecting certain packages--notably gcc/gfortran, yasm, git, and and curl--and converts those to a more generic mechanism.

In the new mechanism each SPKG may have a file in its SPKG directory named spkg-configure.m4. The ./bootstrap script gathers these all up and includes them into configure.

Each spkg-configure.m4 should call a macro called SAGE_SPKG_CONFIGURE which is passed the package name, and some configure-time checks it should perform for that package. See the documentation in spkg-configure.m4 for more details on that macro. Inside these checks, they should set either sage_spkg_install_<pkgname>=yes|no in order to track whether or not Sage should install that package, or if we can just the version from the system (or we don't require the package at all for the current platform).

We then move the hard-coded bits for yasm, gcc, etc. out of configure.ac and into their individual spkg-configure.m4 files. The end result is the same in terms of the checks that are performed.

As proof of concept we also add a configure-time check for the system's patch, demonstrating how this might be used to enable support for more system packages.

See #26286 for a follow-up.

Depends on #21524
Depends on #25011
Depends on #25208
Depends on #25188
Depends on #25198

CC: @mezzarobba @dimpase

Component: build

Author: Erik Bray

Branch: 79de3bd

Reviewer: John Palmieri, Dima Pasechnik

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

@embray embray added this to the sage-8.2 milestone Mar 6, 2018
@embray
Copy link
Contributor Author

embray commented Mar 6, 2018

comment:1

It should be noted that, of course, each time a package's spkg-configure.m4 is added or modified we must re-run ./bootstrap, same as if we were just modifying configure.ac directly.

@embray
Copy link
Contributor Author

embray commented Mar 6, 2018

comment:3

One other aspect of this that probably needs work before it's ready, is to add flags for explicitly selecting whether or not to use the system's version of some package.

I would definitely prefer to not install SPKGs by default we don't have to. But we could also add a global flag like "always use SPKGs regardless of configure checks" or something.

@jdemeyer
Copy link

comment:4

Replying to @embray:

I would definitely prefer to not install SPKGs by default we don't have to.

I agree. That's also the current behaviour for gcc, git, ...

@embray
Copy link
Contributor Author

embray commented Mar 19, 2018

comment:5

Replying to @jdemeyer:

Replying to @embray:

I would definitely prefer to not install SPKGs by default we don't have to.

I agree. That's also the current behaviour for gcc, git, ...

Indeed. I don't know if anyone would have a problem with this or not. I think most people would consider it an enhancement if Sage installed fewer dependencies by default :) I guess it mostly comes down to how reliable the system versions of those dependencies are and how well they integrate with Sage.

I'm concerned about using GMP/MPIR from the system--not because it wouldn't work, but because of how Sage currently installs MPIR as a synonym for GMP. Many platforms don't have an MPIR package in the first place, so I guess they would just use GMP. I'm not sure how best to handle that one.

@jdemeyer
Copy link

comment:6

Replying to @embray:

I'm concerned about using GMP/MPIR from the system--not because it wouldn't work, but because of how Sage currently installs MPIR as a synonym for GMP. Many platforms don't have an MPIR package in the first place, so I guess they would just use GMP. I'm not sure how best to handle that one.

Sage can use both MPIR and GMP. Of course, you'll need to check whether the MPIR/GMP version is sufficiently recent.

@jdemeyer
Copy link

comment:7

The branch doesn't merge.

@embray
Copy link
Contributor Author

embray commented Mar 20, 2018

comment:9

Replying to @jdemeyer:

Replying to @embray:

I'm concerned about using GMP/MPIR from the system--not because it wouldn't work, but because of how Sage currently installs MPIR as a synonym for GMP. Many platforms don't have an MPIR package in the first place, so I guess they would just use GMP. I'm not sure how best to handle that one.

Sage can use both MPIR and GMP. Of course, you'll need to check whether the MPIR/GMP version is sufficiently recent.

I know. I think my concern here was just that Sage builds MPIR with its --enable-gmpcompat flag. So if some system has both GMP and MPIR installed side-by-side, Sage can only use the GMP on that system. And if some system has only MPIR and not GMP somehow, then it won't work because we use <include gmp.h>, etc. I'm not sure if that's really a problem though so maybe it's premature to worry about.

@embray
Copy link
Contributor Author

embray commented Mar 20, 2018

comment:10

Replying to @jdemeyer:

The branch doesn't merge.

I know--at the very least #24907 conflicted with this, which is why I'd like to come up with a different solution to that issue that is more compatible.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 18, 2018

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

9ff40baAdd the ability to move package-specific configure-time checks ( including
0d3e713As an initial demonstration of the new system, move configuration
f656da2Move the curl system package check to its own spkg-configure.m4
6e1ebc1Move the git system package check to its own spkg-configure.m4
32a7067Move the gcc/gfortran system package checks to their own spkg-configure.m4
a0e4078Slightly rewrote this macro to use some more polymorphic shell macros.
d66e115Use m4_sinclude instead--this prevents problems when switching between branches where one of these listed files no longer exists
d5ce869Move these comments inside the macro call to ensure they are actually expanded as part of the macro
f9ab45dChanged a bit how SAGE_SPKG_CONFIGURE works:
070da0bImplement an spkg-configure.m4 for patch, demonstrating use of the new system

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 18, 2018

Changed commit from c160606 to 070da0b

@embray
Copy link
Contributor Author

embray commented Apr 18, 2018

comment:12

Rebased, but I think some other cleanup of configure.ac is now needed as a prerequisite (e.g. #25011).

@embray
Copy link
Contributor Author

embray commented Apr 18, 2018

Changed dependencies from #21524 to #21524, #25011

@dimpase dimpase modified the milestones: sage-8.2, sage-8.3 Apr 18, 2018
@dimpase
Copy link
Member

dimpase commented Apr 18, 2018

comment:14

By the way, I have curl headers installed in /usr/local/include/curl/curl.h, and
their recognition is broken, even if I set CFLAGS to have -I/usr/local/include and try to pass them to ./configure.

@embray
Copy link
Contributor Author

embray commented Apr 18, 2018

comment:15

Replying to @dimpase:

By the way, I have curl headers installed in /usr/local/include/curl/curl.h, and
their recognition is broken, even if I set CFLAGS to have -I/usr/local/include and try to pass them to ./configure.

Dima, does that work without this ticket, or did this break it somehow?

The way this ticket does the feature test for curl is not substantively different from how it was done previously--it's just been moved to a different file. But if it did work before then I'm sure there is a real bug. Note, the correct curl-config also needs to be on the PATH, and needs to support curl-config --checkfor 7.22.

@dimpase
Copy link
Member

dimpase commented Apr 18, 2018

comment:16

It didn't work before either, I think, but in fact curl-config is on the PATH,
and even tells you about the compiler flags:

$ curl-config --cflags
-I/usr/local/include

and the version seems good too:

$ curl-config --checkfor 7.22
$ curl-config --checkfor 999
requested version 999 is newer than existing 7.59.0
$ curl-config --version      
libcurl 7.59.0

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 18, 2018

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

a1e5ea4Slightly rewrote this macro to use some more polymorphic shell macros.
135efb4Use m4_sinclude instead--this prevents problems when switching between branches where one of these listed files no longer exists
afc5572Move these comments inside the macro call to ensure they are actually expanded as part of the macro
a7125f4Changed a bit how SAGE_SPKG_CONFIGURE works:
3f64c89Implement an spkg-configure.m4 for patch, demonstrating use of the new system
7e13822Revert "Trac #25113: OSX is f*ed up sometimes"
2146433Fixes miscompilation by clang from XCode 6.3, see #25118
ab26648gfan version bump
13619ffmove configure checks for OSX compatibility into a separate macro
87d0d31Merge branch 'u/embray/build-configure/darwin-macro' into u/embray/build/autoconf-macros

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 18, 2018

Changed commit from 070da0b to 87d0d31

@embray
Copy link
Contributor Author

embray commented Apr 18, 2018

Changed dependencies from #21524, #25011 to #21524, #25011, #25208

@embray
Copy link
Contributor Author

embray commented Apr 18, 2018

comment:19

Replying to @dimpase:

It didn't work before either, I think, but in fact curl-config is on the PATH,
and even tells you about the compiler flags:

$ curl-config --cflags
-I/usr/local/include

and the version seems good too:

$ curl-config --checkfor 7.22
$ curl-config --checkfor 999
requested version 999 is newer than existing 7.59.0
$ curl-config --version      
libcurl 7.59.0

Thanks. If it wasn't working before this ticket either then I wouldn't be inclined to address it here, since this is just moving around the existing code. I would open a separate issue for that. Though it seems like it should work if you run it like ./configure CFLAGS="-I/usr/local/include".

I'll see if I can set up a way to test that myself. Also if there are problems with the C compiler itself many other configure-time checks won't succeed at first.

@embray
Copy link
Contributor Author

embray commented Nov 6, 2018

comment:128

Replying to @dimpase:

I believe there was also a discrepancy in another file in build/make, not only deps.

Yes, it looks like a change to build/make/Makefile.in was also accidentally reverted as part of the same bogus merge, though that one is merely cosmetic.

@dimpase
Copy link
Member

dimpase commented Nov 6, 2018

comment:129

How about fixing it on #26286 ?

@embray
Copy link
Contributor Author

embray commented Nov 7, 2018

comment:131

I still have some doubts about whether the gcc-related checks are working as intended. I need to re-review my original fixes for those and ensure that this isn't causing a regression (I think it might be).

Previously I had incorporated all those fixes into this ticket but everything's been through so many revisions/rebases it's not clear that it's working as I expect.

@embray
Copy link
Contributor Author

embray commented Nov 7, 2018

comment:132

In particular I think #25188 may have totally regressed; specifically the question of whether or not ./configure lists the sage gcc package as installed.

@embray
Copy link
Contributor Author

embray commented Nov 7, 2018

comment:133

Yeah, I made at least one mistake while rebasing. This should be spelled sage_spkg_install_gcc=yes now.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 7, 2018

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

8fd4decMove the gcc/gfortran system package checks to their own spkg-configure.m4
21fbdddSlightly rewrote this macro to use some more polymorphic shell macros.
b4cc8ebUse m4_sinclude instead--this prevents problems when switching between branches where one of these listed files no longer exists
8d01c6eMove these comments inside the macro call to ensure they are actually expanded as part of the macro
cf6367aChanged a bit how SAGE_SPKG_CONFIGURE works:
16c22deImplement an spkg-configure.m4 for patch, demonstrating use of the new system
6526868this check should be handled in gfortran's spkg-configure.m4
a5f783eadd a diagnostic message that is occasionally helpful for understanding the configure output
3bedcd6added better reporting for yasm program+feature check
79de3bdBSD find puts in extra slashes if you leave a trailing slash in the argument

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 7, 2018

Changed commit from 5de0eb5 to 79de3bd

@embray
Copy link
Contributor Author

embray commented Nov 7, 2018

comment:135

Okay, now it should be good to go.

@embray
Copy link
Contributor Author

embray commented Nov 7, 2018

comment:136

Specifically, the commit at sagemath/sagetrac-mirror@8fd4dec is updated with the corrected spelling of that line.

@vbraun
Copy link
Member

vbraun commented Nov 9, 2018

Changed branch from u/embray/build/autoconf-macros to 79de3bd

@dimpase
Copy link
Member

dimpase commented Nov 18, 2018

Changed commit from 79de3bd to none

@dimpase
Copy link
Member

dimpase commented Nov 18, 2018

comment:138

see #26715 for a gfortran-related collateral damage caused here.

@dimpase

This comment has been minimized.

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

8 participants