#24919 closed enhancement (fixed)
Generic mechanism for system package checks at configure time
Reported by: | embray | Owned by: | embray |
---|---|---|---|
Priority: | major | Milestone: | sage-8.5 |
Component: | build | Keywords: | spkg-configure |
Cc: | mmezzarobba, dimpase | Merged in: | |
Authors: | Erik Bray | Reviewers: | John Palmieri, Dima Pasechnik |
Report Upstream: | N/A | Work issues: | |
Branch: | 79de3bd (Commits, GitHub, GitLab) | Commit: | |
Dependencies: | #21524, #25011, #25208, #25188, #25198 | Stopgaps: |
Description (last modified by )
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.
Attachments (1)
Change History (141)
comment:1 Changed 3 years ago by
comment:2 Changed 3 years ago by
- Status changed from new to needs_review
comment:3 follow-up: ↓ 4 Changed 3 years ago by
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.
comment:4 in reply to: ↑ 3 ; follow-up: ↓ 5 Changed 3 years ago by
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, ...
comment:5 in reply to: ↑ 4 ; follow-up: ↓ 6 Changed 3 years ago by
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.
comment:6 in reply to: ↑ 5 ; follow-up: ↓ 9 Changed 3 years ago by
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.
comment:7 follow-up: ↓ 10 Changed 3 years ago by
- Status changed from needs_review to needs_work
The branch doesn't merge.
comment:8 Changed 3 years ago by
- Cc mmezzarobba added
comment:9 in reply to: ↑ 6 Changed 3 years ago by
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.
comment:10 in reply to: ↑ 7 Changed 3 years ago by
comment:11 Changed 3 years ago by
- Commit changed from c16060622bfc5a27b732b8d8bb819cdcf0dcc890 to 070da0bbb480307a62d8cf7cc122245da8d0912a
Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:
9ff40ba | Add the ability to move package-specific configure-time checks ( including
|
0d3e713 | As an initial demonstration of the new system, move configuration
|
f656da2 | Move the curl system package check to its own spkg-configure.m4
|
6e1ebc1 | Move the git system package check to its own spkg-configure.m4
|
32a7067 | Move the gcc/gfortran system package checks to their own spkg-configure.m4
|
a0e4078 | Slightly rewrote this macro to use some more polymorphic shell macros.
|
d66e115 | Use m4_sinclude instead--this prevents problems when switching between branches where one of these listed files no longer exists
|
d5ce869 | Move these comments inside the macro call to ensure they are actually expanded as part of the macro
|
f9ab45d | Changed a bit how SAGE_SPKG_CONFIGURE works:
|
070da0b | Implement an spkg-configure.m4 for patch, demonstrating use of the new system
|
comment:12 Changed 3 years ago by
- Dependencies changed from #21524 to #21524, #25011
Rebased, but I think some other cleanup of configure.ac is now needed as a prerequisite (e.g. #25011).
comment:13 Changed 3 years ago by
- Cc dimpase added
- Milestone changed from sage-8.2 to sage-8.3
comment:14 follow-up: ↓ 15 Changed 3 years ago by
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
.
comment:15 in reply to: ↑ 14 Changed 3 years ago by
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
.
comment:16 follow-up: ↓ 19 Changed 3 years ago by
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
comment:17 Changed 3 years ago by
- Commit changed from 070da0bbb480307a62d8cf7cc122245da8d0912a to 87d0d31e45699789d4218f2d7cee3a99d8bd63de
Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:
a1e5ea4 | Slightly rewrote this macro to use some more polymorphic shell macros.
|
135efb4 | Use m4_sinclude instead--this prevents problems when switching between branches where one of these listed files no longer exists
|
afc5572 | Move these comments inside the macro call to ensure they are actually expanded as part of the macro
|
a7125f4 | Changed a bit how SAGE_SPKG_CONFIGURE works:
|
3f64c89 | Implement an spkg-configure.m4 for patch, demonstrating use of the new system
|
7e13822 | Revert "Trac #25113: OSX is f*ed up sometimes"
|
2146433 | Fixes miscompilation by clang from XCode 6.3, see #25118
|
ab26648 | gfan version bump
|
13619ff | move configure checks for OSX compatibility into a separate macro
|
87d0d31 | Merge branch 'u/embray/build-configure/darwin-macro' into u/embray/build/autoconf-macros
|
comment:18 Changed 3 years ago by
- Dependencies changed from #21524, #25011 to #21524, #25011, #25208
comment:19 in reply to: ↑ 16 ; follow-up: ↓ 25 Changed 3 years ago by
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/includeand 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.
comment:20 Changed 3 years ago by
- Status changed from needs_work to needs_review
comment:21 Changed 3 years ago by
- Status changed from needs_review to needs_work
Seems to have a bug since the last rebase...
comment:22 Changed 3 years ago by
- Commit changed from 87d0d31e45699789d4218f2d7cee3a99d8bd63de to 862d8e4b74e54745188eb6b75fc894d90768dc53
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
5b833a2 | Move the gcc/gfortran system package checks to their own spkg-configure.m4
|
c0b2750 | Slightly rewrote this macro to use some more polymorphic shell macros.
|
751a515 | Use m4_sinclude instead--this prevents problems when switching between branches where one of these listed files no longer exists
|
9249861 | Move these comments inside the macro call to ensure they are actually expanded as part of the macro
|
4a7953a | Changed a bit how SAGE_SPKG_CONFIGURE works:
|
6433ef7 | Implement an spkg-configure.m4 for patch, demonstrating use of the new system
|
9df2996 | Revert "Trac #25113: OSX is f*ed up sometimes"
|
b6f2947 | Fixes miscompilation by clang from XCode 6.3, see #25118
|
45b4357 | gfan version bump
|
862d8e4 | move configure checks for OSX compatibility into a separate macro
|
comment:23 Changed 3 years ago by
- Status changed from needs_work to needs_review
comment:24 follow-up: ↓ 28 Changed 3 years ago by
that's better, at least make distclean
succeeds after ./bootstrap
...
comment:25 in reply to: ↑ 19 Changed 3 years ago by
Replying to embray:
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/includeand 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.0Thanks. 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"
.
no, it doesn't work this way, at least not with clang on FreeBSD. I've opened #25214 (I guess it differs from one compiler to the other whether /usr/local/include is searched by default or not)
comment:26 follow-up: ↓ 27 Changed 3 years ago by
By the way, on FreeBSD patch
is BSD's patch, with a different versioning scheme, whereas GNU patch is gpatch. I wonder whether there is a way to deal with this easily.
comment:27 in reply to: ↑ 26 Changed 3 years ago by
Replying to dimpase:
By the way, on FreeBSD
patch
is BSD's patch, with a different versioning scheme, whereas GNU patch is gpatch. I wonder whether there is a way to deal with this easily.
Right now it strictly requires GNU patch. In the past there was some reason for that in terms of some features that were needed, or possibly bugs, but I'd be surprised if that still really matters, especially since patching of spkgs was refactored...
comment:28 in reply to: ↑ 24 ; follow-up: ↓ 29 Changed 3 years ago by
Replying to dimpase:
that's better, at least
make distclean
succeeds after./bootstrap
...
Does it get any further than that? :)
Thanks for testing this out btw.
comment:29 in reply to: ↑ 28 Changed 3 years ago by
Replying to embray:
Replying to dimpase:
that's better, at least
make distclean
succeeds after./bootstrap
...Does it get any further than that? :)
Thanks for testing this out btw.
I was working on #23353 and messed things up while manually merging your branch. Namely, now
git pull trac public/gfan/spkg_check
does not bring me the branch from #23353 back, and I wonder whether there is a painless way to force this... Do I need to locally rebase this branch somehow?
comment:30 Changed 3 years ago by
never mind, it builds, and skips patch
, so this looks good so far.
(I have a soft link patch
in PATH, pointing to gpatch
, so this is a dirty way to avoid bumping into BSD patch on FreeBSD)
comment:31 Changed 3 years ago by
Would be wonderful if it can handle #25158 (cmake which is an optional package).
comment:32 follow-up: ↓ 33 Changed 3 years ago by
as well as
- autotools
- boost
- bzip2
- freetype
- gc
- gmp
- gsl
- iconv
- pcre
- r
- readline
- scons
- xz
- zlib
(this is still avoiding more python-related and maths-related packages...)
comment:33 in reply to: ↑ 32 Changed 3 years ago by
Replying to dimpase:
as well as
- autotools
- boost
- bzip2
- freetype
- gc
- gmp
- gsl
- iconv
- pcre
- r
- readline
- scons
- xz
- zlib
(this is still avoiding more python-related and maths-related packages...)
Maybe not autotools, since it's an optional package, and the main point of sage's autotools package was installing lots of different autoconf/automake versions for building patches to different package's configure scripts.
The rest though yeah--all of these kind of low level system libraries are the highest priority to add support for (add ncurses to that list--ncurses takes forever and we have to build it twice).
But first I want to get the general mechanism in place before adding support for a bunch more packages.
comment:34 Changed 3 years ago by
- Status changed from needs_review to needs_work
- Work issues set to merge conflicts
comment:35 Changed 3 years ago by
I could have sworn I rebased this recently. Maybe it was just on my todo list.
comment:36 Changed 3 years ago by
- Commit changed from 862d8e4b74e54745188eb6b75fc894d90768dc53 to e1e68ee4e81facf4114735603a40cac79ea1dcbe
Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:
b6ad44b | As an initial demonstration of the new system, move configuration
|
5ce5577 | Move the curl system package check to its own spkg-configure.m4
|
5668e2d | Move the git system package check to its own spkg-configure.m4
|
42617d3 | Move the gcc/gfortran system package checks to their own spkg-configure.m4
|
1c93c29 | Slightly rewrote this macro to use some more polymorphic shell macros.
|
bcbeddc | Use m4_sinclude instead--this prevents problems when switching between branches where one of these listed files no longer exists
|
742eaed | Move these comments inside the macro call to ensure they are actually expanded as part of the macro
|
86d87da | Changed a bit how SAGE_SPKG_CONFIGURE works:
|
0ac110b | Implement an spkg-configure.m4 for patch, demonstrating use of the new system
|
e1e68ee | move configure checks for OSX compatibility into a separate macro
|
comment:37 Changed 3 years ago by
- Status changed from needs_work to needs_review
- Work issues merge conflicts deleted
comment:38 Changed 3 years ago by
- Status changed from needs_review to needs_work
Just noticed a typo in how this builds on top of #25011.
comment:39 Changed 3 years ago by
- Dependencies changed from #21524, #25011, #25208 to #21524, #25011, #25208, #25811
comment:40 Changed 3 years ago by
- Dependencies changed from #21524, #25011, #25208, #25811 to #21524, #25011, #25208, #25188, #25198
comment:41 Changed 3 years ago by
- Milestone changed from sage-8.3 to sage-8.4
I believe this issue can reasonably be addressed for Sage 8.4.
comment:42 Changed 3 years ago by
- Summary changed from prototype generic mechanism for system package checks at configure time to Generic mechanism for system package checks at configure time
comment:43 Changed 3 years ago by
- Commit changed from e1e68ee4e81facf4114735603a40cac79ea1dcbe to f14f85b101976b6fb8a408de1f876cb40e130be9
Branch pushed to git repo; I updated commit sha1. New commits:
f14f85b | Incorporate fixes from #25188
|
comment:44 Changed 3 years ago by
This is still ready go to. Most of the dependencies are listed as dependencies just because they would conflict with this. Or rather, because this moves around code from configure.ac
, it also needs to incorporate other fixes I've made that also make changes to configure.ac
.
But if any of those tickets remain controversial or whatever I can remove them from this ticket and deal with them later.
The substance of this ticket ticket is in providing support for build/<pkg>/spkg-configure.m4
files, providing configure
-time checks for individual packages.
comment:45 Changed 3 years ago by
Why is it in "needs work" status?
comment:46 Changed 3 years ago by
- Status changed from needs_work to needs_review
comment:47 follow-up: ↓ 48 Changed 3 years ago by
Good work! What happens if the system package is updated to an incompatible version or even removed?
comment:48 in reply to: ↑ 47 Changed 3 years ago by
Replying to gh-timokau:
Good work! What happens if the system package is updated to an incompatible version or even removed?
Certainly there's nothing one can do about that; that's a problem that exists for any software. The best you can do is that if you notice something breaks to try re-running ./configure
. Depending on the exact nature of the checks for a given package, that still might not tell you anything, but the eventual result of debugging such a problem would be either a workaround for said incompatible version or changing the range of package versions that are considered compatible.
It is also still possible to force installation of the spkg. Though one thing I think this might need, either in this ticket or in a followup, is a way to automatically add --with-
flags for selecting the spkg over the system package or vice-versa.
My thinking was something like --with-curl
, where the option can take an optional argument such as --with-curl=<something>
, where <something>
could technically be anything depending on the package, and it would be up to the package's spkg-configure.ac
to determine what to do based on the argument. Without the argument I think the default should be a special value like "spkg" meaning --with-curl=spkg
or install cURL from the SPKG.
In other words, --with-curl
would mean, "configure Sage to include Sage's curl".
That's just one idea...there are other ways one could do this that might be better. That's just my current line of thinking.
comment:49 follow-up: ↓ 55 Changed 3 years ago by
after pulling and running bootstrap I get
checking for GNU patch >= 2.7.0... /usr/bin/patch checking for curl 7.22... no checking for git... /usr/bin/git ./configure: line 7948: syntax error near unexpected token `elif' ./configure: line 7948: ` elif test x$SAGE_INSTALL_GCC = xno; then'
comment:50 Changed 3 years ago by
This must be one of autoconf/m4 SNAFUs: these errors come from fragments such as
if ... # ... # true elif...
whereas true
is not commented out in original m4 files...
comment:51 follow-up: ↓ 56 Changed 3 years ago by
in fact, the following fixes configure:
--- a/build/pkgs/gcc/spkg-configure.m4 +++ b/build/pkgs/gcc/spkg-configure.m4 @@ -6,7 +6,7 @@ dnl In the latter case, a warning is given. AC_DEFUN([SAGE_SHOULD_INSTALL_GCC], [ if test x$SAGE_INSTALL_GCC = xexists; then # Already installed in Sage, but it should remain selected - # true + true elif test x$SAGE_INSTALL_GCC = xno; then AC_MSG_WARN([$1]) else
I also saw build/pkgs/curl/spkg-configure.m4
not working due to bc
not installed.
So bc
should be added to build pre-reqs.
comment:52 follow-ups: ↓ 54 ↓ 57 Changed 3 years ago by
I've tried creating more spkg-configure.m4
. I'm attaching one for xz
. Not sure it's OK to use cut
, but here it goes.
comment:53 Changed 3 years ago by
I am not sure how to deal with libraries; for some there are autoconf macros, e.g.
AX_CHECK_ZLIB.
Still, extra work for zlib would be needed to extract the version. How much can one rely
on pkgconf/pkg-config
?
comment:54 in reply to: ↑ 52 Changed 3 years ago by
Replying to dimpase:
I've tried creating more
spkg-configure.m4
. I'm attaching one forxz
. Not sure it's OK to usecut
, but here it goes.
This m4 file will also need proper check for liblzma
being installed with headers and all.
comment:55 in reply to: ↑ 49 Changed 3 years ago by
Replying to dimpase:
after pulling and running bootstrap I get
checking for GNU patch >= 2.7.0... /usr/bin/patch checking for curl 7.22... no checking for git... /usr/bin/git ./configure: line 7948: syntax error near unexpected token `elif' ./configure: line 7948: ` elif test x$SAGE_INSTALL_GCC = xno; then'
Huh, I'll have to give that a try. I definitely didn't get anything like that when I last worked on this ticket, but maybe the bug was in a code path that configure
didn't go down on my system (are you using Sage's GCC or no?)
Thank you for debugging and finding the problem.
comment:56 in reply to: ↑ 51 ; follow-up: ↓ 59 Changed 3 years ago by
Replying to dimpase:
I also saw
build/pkgs/curl/spkg-configure.m4
not working due tobc
not installed. Sobc
should be added to build pre-reqs.
Whoa, really?? Are you sure you don't have that problem without this patch? The test for curl existed in the original configure.ac
, and I just moved the existing test to build/pkgs/curl/spkg-configure.m4
pretty much unmodified. It's not clear to me where it's looking for bc
either. I thought bc
was one of those things that's pretty much always there, like grep (albeit more obscure).
Could you be more specific about ho that came up? Was there something in config.log
?
comment:57 in reply to: ↑ 52 Changed 3 years ago by
Replying to dimpase:
I've tried creating more
spkg-configure.m4
. I'm attaching one forxz
. Not sure it's OK to usecut
, but here it goes.
I can't imagine it wouldn't be OK to use cut
. Again, it's a POSIX standard program, and the flags you're using are standard as well. Are you working on OSX? I feel like if it works on OSX it should work on any other platform we're supporting. If you're worried about it we could also put in an AC_PROG_CHECK
for cut
.
If nothing else, I believe that if errors occur in dependency checks, then we should just fail on detecting the dependency and build it.
comment:58 follow-up: ↓ 62 Changed 3 years ago by
Out of curiosity
6 changequote(<,>) 7 xz_version=`$ac_path_XZ --version 2>&1 \ 8 | cut -d' ' -f4 | $SED -n 1p` 9 changequote([,])
why the changequote(<,>)
here? It's not immediately obvious that anything in that command would be interpreted as quoting. But I must be missing a subtlety--please enlighten me.
LGTM otherwise.
comment:59 in reply to: ↑ 56 Changed 3 years ago by
Replying to embray:
Replying to dimpase:
I also saw
build/pkgs/curl/spkg-configure.m4
not working due tobc
not installed. Sobc
should be added to build pre-reqs.Whoa, really?? Are you sure you don't have that problem without this patch?
I built Sage on that system, a Debian 8(?) xlc container on CromeOS laptop, before, just fine. I suppose that test for curl just failed, and as a result Sage's curl was built.
Somehow bc was not installed.
The test for curl existed in the original configure.ac
, and I just moved the existing test to build/pkgs/curl/spkg-configure.m4
pretty much unmodified. It's not clear to me where it's looking for bc
either. I thought bc
was one of those things that's pretty much always there, like grep (albeit more obscure).
Could you be more specific about ho that came up? Was there something in
config.log
?
Yes, I saw "bc not found" in config.log, in a curl-related chunk.
I'm away from the box till evening but you can try uninstalling bc yourself and observing the result.
comment:60 follow-up: ↓ 61 Changed 3 years ago by
I'll have to figure out what exactly was trying to use bc
and not finding it. That could be a bug in on of the standard autoconf macros, because in general they won't try to use a program that autoconf hasn't found first.
In any case, as long as the resulting behavior is to report curl as not found and move on, I'm mostly OK with that. The goal here is not necessarily to make all packages detectable 100% of the time* (though if there are cases where they should have been detected and weren't we certainly want to look into that).
* I should clarify: That is a goal long-term, theoretically, but for the purposes of this ticket I'm just concerned with the general mechanism.
comment:61 in reply to: ↑ 60 ; follow-ups: ↓ 63 ↓ 64 Changed 3 years ago by
Replying to embray:
I'll have to figure out what exactly was trying to use
bc
and not finding it.
without bc
, one has
$ curl-config --checkfor 7.22 /usr/bin/curl-config: 1: /usr/bin/curl-config: bc: not found /usr/bin/curl-config: 1: /usr/bin/curl-config: bc: not found /usr/bin/curl-config: 112: test: Illegal number: requested version 7.22 is newer than existing 7.52.1
As spkg-configure.m4
invokes curl-config --checkfor
, you get this error
if no bc
is installed. So this is a bug in curl
, if you like. bc
is used to
compute the "raw" version.
You see there
cpatch=`echo $checkfor | cut -d. -f3 | cut -d- -f1` checknum=`echo "$cmajor*256*256 + $cminor*256 + ${cpatch:-0}" | bc` numuppercase=`echo 073401 | tr 'a-f' 'A-F'` nownum=`echo "obase=10; ibase=16; $numuppercase" | bc`
comment:62 in reply to: ↑ 58 ; follow-up: ↓ 65 Changed 3 years ago by
Replying to embray:
Out of curiosity
6 changequote(<,>) 7 xz_version=`$ac_path_XZ --version 2>&1 \ 8 | cut -d' ' -f4 | $SED -n 1p` 9 changequote([,])why the
changequote(<,>)
here?
It's just copy/paste from build/pkgs/patch/spkg-configure.m4
It's not immediately obvious that anything in that command would be interpreted as > quoting. But I must be missing a subtlety--please enlighten me.
Puzzling to me as well.
comment:63 in reply to: ↑ 61 Changed 3 years ago by
Replying to dimpase:
As
spkg-configure.m4
invokescurl-config --checkfor
, you get this error if nobc
is installed.
Let us add the corresponding check in configure.ac
and bail out if it's not there:
AC_CHECK_PROG(found_bc, bc, yes, no) if test x$found_bc != xyes then AC_MSG_NOTICE([Sorry, the 'bc' command must be in the path to build AC_PACKAGE_NAME]) AC_MSG_NOTICE([On some systems it can be found in /usr/ccs/bin]) AC_MSG_NOTICE([See also https://www.gnu.org/software/bc/]) AC_MSG_ERROR([Exiting, as the calculator 'bc' can not be found.]) fi
How about doing something like this for cut
, as well, just as an extra sanity check?
comment:64 in reply to: ↑ 61 Changed 3 years ago by
Replying to dimpase:
Replying to embray:
I'll have to figure out what exactly was trying to use
bc
and not finding it.without
bc
, one has$ curl-config --checkfor 7.22 /usr/bin/curl-config: 1: /usr/bin/curl-config: bc: not found /usr/bin/curl-config: 1: /usr/bin/curl-config: bc: not found /usr/bin/curl-config: 112: test: Illegal number: requested version 7.22 is newer than existing 7.52.1As
spkg-configure.m4
invokescurl-config --checkfor
, you get this error if nobc
is installed. So this is a bug incurl
, if you like.bc
is used to compute the "raw" version. You see therecpatch=`echo $checkfor | cut -d. -f3 | cut -d- -f1` checknum=`echo "$cmajor*256*256 + $cminor*256 + ${cpatch:-0}" | bc` numuppercase=`echo 073401 | tr 'a-f' 'A-F'` nownum=`echo "obase=10; ibase=16; $numuppercase" | bc`
I see now. I didn't realize curl-config
was just a shell script. Then, technically, in order to run it we need to have all of its dependencies as well, hahah.
Well, that has nothing directly to do with this ticket so I'm not worried about it right now. But I will create a ticket for it.
comment:65 in reply to: ↑ 62 ; follow-up: ↓ 66 Changed 3 years ago by
Replying to dimpase:
Replying to embray:
Out of curiosity
6 changequote(<,>) 7 xz_version=`$ac_path_XZ --version 2>&1 \ 8 | cut -d' ' -f4 | $SED -n 1p` 9 changequote([,])why the
changequote(<,>)
here?It's just copy/paste from
build/pkgs/patch/spkg-configure.m4
Oh, well in the case of patch I can tell you exactly why it's needed: the regular expression I use in the sed
call uses square brackets, such as [0-9]
. These are the default quote characters used in autoconf, so you need to changequote
here (or use hideous m4 escape sequences).
It's not immediately obvious that anything in that command would be interpreted as > quoting. But I must be missing a subtlety--please enlighten me.
Puzzling to me as well.
Did you find it didn't work without it, or is it just cargo cult?
comment:66 in reply to: ↑ 65 Changed 3 years ago by
Replying to embray:
why the
changequote(<,>)
here?It's just copy/paste from
build/pkgs/patch/spkg-configure.m4
Oh, well in the case of patch I can tell you exactly why it's needed: the regular expression I use in the
sed
call uses square brackets, such as[0-9]
. These are the default quote characters used in autoconf, so you need tochangequote
here (or use hideous m4 escape sequences).It's not immediately obvious that anything in that command would be interpreted as > quoting. But I must be missing a subtlety--please enlighten me.
Puzzling to me as well.
Did you find it didn't work without it, or is it just cargo cult?
It escaped me that [] might need a special treatment, I thought it's something to do with potentially rougue numering using ,
i.p.of .
Consider this a request for comments in that code :-)
comment:67 Changed 3 years ago by
Yes, in m4 the default left and right quotes are `'
, but autoconf immediately sets the quotes to []
, because autoconf is basically a big pile of m4 macros for generating shell code where `
and '
are meaningful and common.
Of course, so are []
in the context of tests, but there the [
is just a well-known synonym for test
, which is why in autoconf you have to write if test
instead of if [
in most places.
However, there are other rare cases where you might want to use []
as literals, such as in regular expressions, so there it's best to temporarily changequotes()
. But the only reason you ever need to do that is if you really want to output some square brackets somewhere in your macro.
comment:68 Changed 2 years ago by
In addition to an obvious rebase, I also needed
--- a/configure.ac +++ b/configure.ac @@@ -754,21 -507,6 +507,9 @@@ AC_CONFIG_COMMANDS(mkdirs SAGE_INST="$SAGE_SPKG_INST" ]) +dnl A stamp file indicating that an existing, broken GCC install should be +dnl cleaned up by make. - if test x$SAGE_BROKEN_GCC = xyes; then - AC_CONFIG_COMMANDS([broken-gcc], [ - # Re-run the check just in case, such as when re-running config.status - SAGE_CHECK_BROKEN_GCC() - if test x$SAGE_BROKEN_GCC = xyes; then - touch build/make/.clean-broken-gcc - fi - ], [ - SAGE_LOCAL="$SAGE_LOCAL" - SAGE_SRC="$SAGE_SRC" - ]) - fi + AC_OUTPUT() dnl vim:syntax=m4
to avoid ./bootstrap
complaining about broken-gcc
already set in gcc/spkg-configure.m4
comment:69 Changed 2 years ago by
That stuff is supposed to already be in build/pkgs/gcc/spkg-configure.m4
so any rebase (which would include #25011 now) would indeed have to remove that block from configure.ac
(and ensure that the relevant code in build/pkgs/gcc/spkg-configure.m4
also matches).
comment:70 Changed 2 years ago by
just ran into a yasm recognition bug:
configure:3791: checking for yasm configure:3828: result: not needed for amd64
but MPIR duly needs yasm on amd64
And here
$ uname -m amd64
comment:71 follow-up: ↓ 74 Changed 2 years ago by
This isn't detecting packages properly on OS X. Without this branch, installations of curl (already there), git (already there), gcc (because it should use clang), and gfortran (already there) are skipped (config.log
says "not installed: configure check"). With this branch, they are all built. Nothing is skipped, it seems.
(I tested by running make distclean; autoreconf; make
. Did I miss a step?)
comment:72 Changed 2 years ago by
if configure.ac is changed, one should run ./bootstrap, then ./configure, and look at the list of detected/undetected packages it prints.
comment:73 Changed 2 years ago by
Dima, John, for both of these issues, do you have the same issue without this patch? Because otherwise it's not relevant to the mechanism being implemented here, as for both yasm and curl this ticket is already using the exact same configuration already in place and has just refactored things a little.
If it works without this ticket then that is a problem with the refactoring. If you have the same problem with or without this ticket then that means it's working more or less as intended.
comment:74 in reply to: ↑ 71 Changed 2 years ago by
Replying to jhpalmieri:
(I tested by running
make distclean; autoreconf; make
. Did I miss a step?)
You should run ./bootstrap
, not autoreconf
.
comment:75 Changed 2 years ago by
- Commit changed from f14f85b101976b6fb8a408de1f876cb40e130be9 to a2bedc5c944bee2c35815768cfd2ed554834429e
Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:
d9e9b4f | Move the curl system package check to its own spkg-configure.m4
|
9c35b95 | Move the git system package check to its own spkg-configure.m4
|
e9b7362 | Move the gcc/gfortran system package checks to their own spkg-configure.m4
|
8821c76 | Slightly rewrote this macro to use some more polymorphic shell macros.
|
b4630a5 | Use m4_sinclude instead--this prevents problems when switching between branches where one of these listed files no longer exists
|
3cde0d7 | Move these comments inside the macro call to ensure they are actually expanded as part of the macro
|
2f73979 | Changed a bit how SAGE_SPKG_CONFIGURE works:
|
724c459 | Implement an spkg-configure.m4 for patch, demonstrating use of the new system
|
97fec35 | Incorporate fixes from #25188
|
a2bedc5 | this check should be handled in gfortran's spkg-configure.m4
|
comment:76 Changed 2 years ago by
Still have a syntax error that crept in somewhere....
comment:77 Changed 2 years ago by
- Commit changed from a2bedc5c944bee2c35815768cfd2ed554834429e to db1bbe09714e92591369535ff751edd1be332e7b
comment:78 Changed 2 years ago by
Rebased again. Should be good. If this can get positive review along with #25188 and #25198, then I might close those tickets in favor of this one, since it incorporates both of them. If either #25188 or #25198 are too problematic but this ticket is otherwise good, I can separate it from those issues for now and deal with them later. I'm fine with any order of operations.
comment:79 Changed 2 years ago by
I made #26281 for the bc
issue with configuring curl
. Unless that issue really only impacts this ticket I propose we table it for now. IMO it's more of an upstream bug since the curl package should have whatever package includes bc
as a dependency, if it's used by curl-config
.
comment:80 Changed 2 years ago by
OK, I'm testing this now. Would you open a ticket for yasm recognition (cf comment 70)? Or perhaps just a followup ticket to add more recognition stuff and fix existing?
comment:81 Changed 2 years ago by
There is something wrong with yasm/spkg-configure.m4
. Namely, ./configure
does not report whether the result is yes or no. In config.log
I see
... configure:8328: === checking whether to install the xz SPKG === configure:8343: checking for xz >= 5.2.2 configure:8422: result: /usr/bin/xz configure:8438: === checking whether to install the yasm SPKG === configure:8553: checking SPKGs to install configure:8555: result: configure:8644: result: 4ti2-1.6.7 ...
so it looks like something is not right with it.
comment:82 Changed 2 years ago by
- Status changed from needs_review to needs_work
comment:83 Changed 2 years ago by
- Status changed from needs_work to needs_review
I noticed that, but I don't think it's a problem. The check is skipped for yasm entirely in most cases because yasm isn't even installed on all platforms.
That new notice I added just gets printed before each package that's checked for, but the final results are summarized later. Maybe there should be an additional message clarifying what's going on following each of those "checking whether to install..." messages?
comment:84 Changed 2 years ago by
- Status changed from needs_review to needs_work
On second thought, it looks like the AC_PATH_PROGS_FEATURE_CHECK
doesn't automatically output a 'checking...' message in the first place, so maybe I'll add that there.
comment:85 Changed 2 years ago by
- Commit changed from db1bbe09714e92591369535ff751edd1be332e7b to fc985cc6e2c8b951e8f7c442990f448aed9e94c1
Branch pushed to git repo; I updated commit sha1. New commits:
fc985cc | added better reporting for yasm program+feature check
|
comment:86 follow-up: ↓ 88 Changed 2 years ago by
- Status changed from needs_work to needs_review
Ok, now it shows
configure: === checking whether to install the git SPKG === checking for git... /usr/bin/git configure: === checking whether to install the yasm SPKG === checking for yasm supporting the adox instruction... no checking for Fortran flag needed to accept free-form source... -ffree-form configure: === checking whether to install the gfortran SPKG === configure: === checking whether to install the curl SPKG === checking for curl 7.22... /usr/bin/curl checking curl/curl.h usability... yes checking curl/curl.h presence... yes checking for curl/curl.h... yes checking for a sed that does not truncate output... /bin/sed configure: === checking whether to install the patch SPKG === checking for GNU patch >= 2.7.0... /usr/bin/patch
the message about the Fortran flag really belongs with the gfortran checks, but for some reason (and I noticed this long before) that check gets inserted into the configure script early, possibly something to do with diversions but I'm not sure. I'm not going to worry about it right now.
New commits:
fc985cc | added better reporting for yasm program+feature check
|
comment:87 follow-up: ↓ 89 Changed 2 years ago by
Okay, if I do make distclean; ./bootstrap; ./configure
, then I see
You are using OS X Lion (or later). You are strongly advised to install Apple's latest Xcode unless you already have it. You can install this using the App Store. Also, make sure you install Xcode's Command Line Tools -- see Sage's README.txt. checking Python version to install... 2 checking multiprecision library... MPIR checking BLAS library... openblas ./configure: line 6910: SAGE_SPKG_CONFIGURE_: command not found ./configure: line 6911: SAGE_SPKG_CONFIGURE_: command not found ./configure: line 6912: SAGE_SPKG_CONFIGURE_: command not found ./configure: line 6913: SAGE_SPKG_CONFIGURE_: command not found ./configure: line 6914: SAGE_SPKG_CONFIGURE_: command not found ./configure: line 6915: SAGE_SPKG_CONFIGURE_: command not found checking SPKGs to install... 4ti2-1.6.7 alabaster-0.7.10 appnope-0.1.0.p0 arb-2.14.0 ...
and all packages are going to be installed. As I said before, with a vanilla Sage, curl, git, gcc, and gfortran are not installed by Sage on this computer, because ./configure
correctly detects them.
The configure
script looks strange: after the line # Configure all spkgs with configure-time checks
, there are about 30 blank lines, followed by
SAGE_SPKG_CONFIGURE_ SAGE_SPKG_CONFIGURE_ SAGE_SPKG_CONFIGURE_ SAGE_SPKG_CONFIGURE_ SAGE_SPKG_CONFIGURE_ SAGE_SPKG_CONFIGURE_
The file m4/sage_spkg_configures.m4
contains this:
m4_sinclude([build/pkgs//curl/spkg-configure.m4]) m4_sinclude([build/pkgs//gcc/spkg-configure.m4]) m4_sinclude([build/pkgs//gfortran/spkg-configure.m4]) m4_sinclude([build/pkgs//git/spkg-configure.m4]) m4_sinclude([build/pkgs//patch/spkg-configure.m4]) m4_sinclude([build/pkgs//yasm/spkg-configure.m4]) SAGE_SPKG_CONFIGURE_ SAGE_SPKG_CONFIGURE_ SAGE_SPKG_CONFIGURE_ SAGE_SPKG_CONFIGURE_ SAGE_SPKG_CONFIGURE_ SAGE_SPKG_CONFIGURE_
Maybe you are using GNUisms in the bash script bootstrap
.
comment:88 in reply to: ↑ 86 Changed 2 years ago by
Replying to embray:
the message about the Fortran flag really belongs with the gfortran checks, but for some reason (and I noticed this long before) that check gets inserted into the configure script early, possibly something to do with diversions but I'm not sure. I'm not going to worry about it right now.
Ok, I couldn't help but to worry about it a little bit; I wanted to know what was going on here. It's because each SAGE_SPKG_CONFIGURE_
macro (e.g. SAGE_SPKG_CONFIGURE_GFORTRAN
) is defined with AC_DEFUN_ONCE
--these are "one shot" macros that are expanded only once. However, if you call an AC_DEFUN_ONCE
macro inside an AC_DEFUN_ONCE
macro, it orders things so that nested one-shot macros are expanded first (I think this may be to help prevent recursion).
I'm not sure if there's a good way to circumvent that for the sake of the "checking" messages. Perhaps I can move them to outside the SAGE_SPKG_CONFIGURE_
macros themselves. Like I said, it's not terribly important though, and the end result is still consistent.
comment:89 in reply to: ↑ 87 Changed 2 years ago by
Replying to jhpalmieri:
Maybe you are using GNUisms in the bash script
bootstrap
.
Thank you. This looks similar to the same silly issue that came up in #26013 with the BSD find
not stripping trailing slashes from its argument.
comment:90 Changed 2 years ago by
- Status changed from needs_review to needs_work
comment:91 follow-up: ↓ 97 Changed 2 years ago by
I still do not see a meaningful output on FreeBSD (perhaps due to a GNUism somewhere?):
configure: === checking whether to install the yasm SPKG === checking SPKGs to install...
--- this is in part due to absent amd64
case. If I add |amd64
to the corresponding list of acrs I get
configure: === checking whether to install the yasm SPKG === checking for yasm supporting the adox instruction... /usr/local/bin/yasm
comment:92 follow-up: ↓ 94 Changed 2 years ago by
- Commit changed from fc985cc6e2c8b951e8f7c442990f448aed9e94c1 to 8c8e09a2dee22dc01cecdb4f987e882c8f8555c1
Branch pushed to git repo; I updated commit sha1. New commits:
8c8e09a | BSD find puts in extra slashes if you leave a trailing slash in the argument
|
comment:93 follow-up: ↓ 95 Changed 2 years ago by
- Status changed from needs_work to needs_review
How about now?
comment:94 in reply to: ↑ 92 Changed 2 years ago by
comment:95 in reply to: ↑ 93 Changed 2 years ago by
Replying to embray:
How about now?
That helps. Now the pre-existing packages are not installed, so it behaves as it did with the devel
branch.
By the way, config.log
used to be in logs/pkgs
with a symlink to SAGE_ROOT
, and now it's just in SAGE_ROOT
. I prefer it the old way, or I suppose we could switch it around so that SAGE_ROOT/config.log
is a file and there is a symlink in logs/pkgs
. In any case, it's nice being able to access all of the logs, together with their modification times, in one place.
comment:96 Changed 2 years ago by
- Reviewers set to John Palmieri, Dima Pasechnik
- Status changed from needs_review to positive_review
Looks good to me. I've opened a follow-up ticket #26286.
Please do not forget to add a new configure tarball etc.
comment:97 in reply to: ↑ 91 Changed 2 years ago by
Replying to dimpase:
I still do not see a meaningful output on FreeBSD (perhaps due to a GNUism somewhere?):
configure: === checking whether to install the yasm SPKG === checking SPKGs to install...--- this is in part due to absent
amd64
case. If I add|amd64
to the corresponding list of acrs I getconfigure: === checking whether to install the yasm SPKG === checking for yasm supporting the adox instruction... /usr/local/bin/yasm
Please just open a separate ticket for that.
comment:98 Changed 2 years ago by
Thanks.
comment:99 Changed 2 years ago by
This is probably going to blow up at least one of the buildbots without a new configure package, but I'd rather wait and see before going through that song and dance again.
comment:100 Changed 2 years ago by
- Milestone changed from sage-8.4 to sage-8.5
comment:101 follow-up: ↓ 102 Changed 2 years ago by
This patch does seem not to work for sage -i
and sage -f
- e.g. even if I have
patch
recognised by configure
as not needed to be installed,
sage -i patch
goes and installs it from source...
comment:102 in reply to: ↑ 101 ; follow-up: ↓ 104 Changed 2 years ago by
Replying to dimpase:
This patch does seem not to work for
sage -i
andsage -f
- e.g. even if I havepatch
recognised byconfigure
as not needed to be installed,sage -i patch
goes and installs it from source...
I think that's a feature. Even if a package is installed by the system, it should still be possible to manually install the Sage package.
comment:104 in reply to: ↑ 102 Changed 2 years ago by
Replying to jdemeyer:
Replying to dimpase:
This patch does seem not to work for
sage -i
andsage -f
- e.g. even if I havepatch
recognised byconfigure
as not needed to be installed,sage -i patch
goes and installs it from source...I think that's a feature. Even if a package is installed by the system, it should still be possible to manually install the Sage package.
Yep. I said the same to Dima on the bus the other day. I can see his point and I did debate this myself, but ultimately I see sage -i
as meaning "install the Sage SPKG". It can still be uninstalled too with sage-spkg-uninstall. This could/should be made easier of course.
comment:105 Changed 2 years ago by
It's actually a very nice feature. For example, once I clean up the BLAS detection stuff a bit, it will become easier to use Sage with whatever the system's BLAS is (if indeed we do detect a usable one). But then, if need be, one can easily (in theory) use sage -i openblas
to build and install their own tuned openblas for their Sage, for example.
comment:106 Changed 2 years ago by
- Commit changed from 8c8e09a2dee22dc01cecdb4f987e882c8f8555c1 to a1a384f3206bd307a7b4429294d9b1d6878269c5
Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:
029584a | Slightly rewrote this macro to use some more polymorphic shell macros.
|
e13304c | Use m4_sinclude instead--this prevents problems when switching between branches where one of these listed files no longer exists
|
4b6c2ac | Move these comments inside the macro call to ensure they are actually expanded as part of the macro
|
31fd98b | Changed a bit how SAGE_SPKG_CONFIGURE works:
|
58acae8 | Implement an spkg-configure.m4 for patch, demonstrating use of the new system
|
273f25a | Incorporate fixes from #25188
|
7577f7a | this check should be handled in gfortran's spkg-configure.m4
|
ff087ef | add a diagnostic message that is occasionally helpful for understanding the configure output
|
94f18b1 | added better reporting for yasm program+feature check
|
a1a384f | BSD find puts in extra slashes if you leave a trailing slash in the argument
|
comment:107 Changed 2 years ago by
I rebased on current develop and there was no merge conflict. So I guess it's another mystery conflict...
comment:108 Changed 2 years ago by
- Status changed from needs_work to positive_review
tried to merge the vbraun's branch, it works now.
comment:109 follow-up: ↓ 111 Changed 2 years ago by
Merge conflict
Also if you are merging my private branch then prepare for a world of pain as I'm going to rewrite it before publishing...
comment:110 Changed 2 years ago by
- Status changed from positive_review to needs_work
comment:111 in reply to: ↑ 109 Changed 2 years ago by
Replying to vbraun:
Merge conflict
Also if you are merging my private branch then prepare for a world of pain as I'm going to rewrite it before publishing...
No, this was not done, certainly. Now we have the beta branch to rebase this against, could you please put in into the next beta?
comment:112 Changed 2 years ago by
- Branch changed from u/embray/build/autoconf-macros to u/dimpase/build/autoconf-macros
- Commit changed from a1a384f3206bd307a7b4429294d9b1d6878269c5 to 8ea6c52eb1cae9b0c5f60e2051f48c1f6efc3304
rebased
comment:113 Changed 2 years ago by
- Branch changed from u/dimpase/build/autoconf-macros to u/embray/build/autoconf-macros
- Commit changed from 8ea6c52eb1cae9b0c5f60e2051f48c1f6efc3304 to a1a384f3206bd307a7b4429294d9b1d6878269c5
hmm, something went wrong during rebase...
comment:114 Changed 2 years ago by
- Branch changed from u/embray/build/autoconf-macros to u/dimpase/build/autoconf-macros
- Commit changed from a1a384f3206bd307a7b4429294d9b1d6878269c5 to de2746fe1854b2af506f8844142880f8a108fb36
cutting/pasting from git blame build/make/deps
:
in the current develop branch (notice xz
):
15c7b88bd76 build/make/deps (Erik M. Bray 2018-07-17 15:59:31 +0000 81) TOOLCHAIN_DEPS = zlib xz $(MP_LIBRARY) mpfr mpc
in this ticket's branch (notice there is no xz
):
9c1b4f6c243 build/make/deps (Erik M. Bray 2018-07-17 15:59:31 +0000 80) TOOLCHAIN_DEPS = zlib $(MP_LIBRARY) mpfr mpc
and we have two very similar commits, but not quite:
$ git show 15c7b88bd76 | cat commit 15c7b88bd76c58b769a06c2d4c8dede4ef33d850 Author: Erik M. Bray <erik.bray@lri.fr> Date: Tue Jul 17 15:59:31 2018 +0000 move this list into a TOOLCHAIN_DEPS variable we can use to inspect this list elsewhere diff --git a/build/make/deps b/build/make/deps index 822b91b175..632ffa7c0e 100644 --- a/build/make/deps +++ b/build/make/deps @@ -78,12 +78,11 @@ toolchain: $(foreach pkgname,$(TOOLCHAIN),$(inst_$(pkgname))) # Note: This list is determined from the dependencies of the TOOLCHAIN # packages which include gcc, and optionally ccache; in principle this # list is redundant. +TOOLCHAIN_DEPS = zlib xz $(MP_LIBRARY) mpfr mpc toolchain-deps: - $(MAKE) $(inst_zlib) - $(MAKE) $(inst_xz) - $(MAKE) $(MP_LIBRARY) - $(MAKE) $(inst_mpfr) - $(MAKE) $(inst_mpc) + for pkg in $(TOOLCHAIN_DEPS); do \ + $(MAKE) $$pkg; \ + done all-toolchain: base-toolchain $(MAKE) toolchain-deps dima@hilbert /mnt/opt/Sage/sage-clang $ git show 9c1b4f6c243 | cat commit 9c1b4f6c24391f952c73783503a33eea0469fdc2 Author: Erik M. Bray <erik.bray@lri.fr> Date: Tue Jul 17 15:59:31 2018 +0000 move this list into a TOOLCHAIN_DEPS variable we can use to inspect this list elsewhere diff --git a/build/make/deps b/build/make/deps index 24508b82a4..4f3dc4588c 100644 --- a/build/make/deps +++ b/build/make/deps @@ -75,11 +75,14 @@ toolchain: $(foreach pkgname,$(TOOLCHAIN),$(inst_$(pkgname))) # unconditionally. We still use the dependency checking from $(MAKE), # so this will not trigger useless rebuilds. # See #14168 and #14232. +# Note: This list is determined from the dependencies of the TOOLCHAIN +# packages which include gcc, and optionally ccache; in principle this +# list is redundant. +TOOLCHAIN_DEPS = zlib $(MP_LIBRARY) mpfr mpc toolchain-deps: - $(MAKE) $(inst_zlib) - $(MAKE) $(MP_LIBRARY) - $(MAKE) $(inst_mpfr) - $(MAKE) $(inst_mpc) + for pkg in $(TOOLCHAIN_DEPS); do \ + $(MAKE) $$pkg; \ + done all-toolchain: base-toolchain $(MAKE) toolchain-deps
This looks very fishy, as the dates/times are the same, only hashes are different.
And merging this ticket branch into develop does not quite work, as e.g. build/make/deps
does not get updated!
Erik, Volker, any ideas what's up there?
New commits:
de2746f | Merge #24919
|
comment:115 Changed 2 years ago by
- Commit changed from de2746fe1854b2af506f8844142880f8a108fb36 to aa6932de8c8aa787089b045375e2fead582463c2
Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:
241bb4c | py3: fix doctests in ribbon tableaux
|
5dec1a7 | Resolved problem in parallel a*a
|
d07180d | Changed name
|
e16e128 | remove cephes - we do not need it anymore
|
098f249 | Trac #26510: Fix combinat.subset for python3
|
1b5fd0e | Trac #26545: Reverse 'islice' corrections and rebase ...
|
3ea41e3 | remove deprecated stuff in manifolds
|
c5d4518 | py3: fix some doctests in combinat/species
|
e319a2b | trac 26610 reviewer's request
|
aa6932d | Updated SageMath version to 8.5.beta2
|
comment:116 Changed 2 years ago by
and if instead I rebase this ticket's branch over 8.4.beta2, I get a branch with a merge conflict.
At seems that at this point something needs to be done manually, for a reason I cannot comprehend...
comment:117 Changed 2 years ago by
Please just let me....
comment:118 Changed 2 years ago by
- Owner changed from (none) to embray
comment:119 Changed 2 years ago by
- Branch changed from u/dimpase/build/autoconf-macros to u/embray/build/autoconf-macros
- Commit changed from aa6932de8c8aa787089b045375e2fead582463c2 to 5de0eb56f3dc990ac08b56a71b112a597d6efb4f
- Status changed from needs_work to needs_review
This is rebased on latest develop and cleaned up now. I think everything is good, but I want to re-check some of the gcc-related stuff just in case.
Last 10 new commits:
506ffb3 | Move the gcc/gfortran system package checks to their own spkg-configure.m4
|
e16b957 | Slightly rewrote this macro to use some more polymorphic shell macros.
|
b7ba60e | Use m4_sinclude instead--this prevents problems when switching between branches where one of these listed files no longer exists
|
8f338fb | Move these comments inside the macro call to ensure they are actually expanded as part of the macro
|
ef1edb0 | Changed a bit how SAGE_SPKG_CONFIGURE works:
|
73a6d0d | Implement an spkg-configure.m4 for patch, demonstrating use of the new system
|
c4083a6 | this check should be handled in gfortran's spkg-configure.m4
|
a512752 | add a diagnostic message that is occasionally helpful for understanding the configure output
|
3ab0529 | added better reporting for yasm program+feature check
|
5de0eb5 | BSD find puts in extra slashes if you leave a trailing slash in the argument
|
comment:120 Changed 2 years ago by
I think you ended up with the same slightly broken branch as I did in my rebase attempts - check that your TOOLCHAIN_DEPS
variable in build/make/deps
does not include xz
--- cf comment 114.
comment:121 follow-up: ↓ 123 Changed 2 years ago by
I see what you're saying now. That has nothing directly to do with this ticket actually. I think that what happened was that I merged an old version of the branch from #25857 into the branch for #25188. But then #25188 got merged into develop later, and brought in an old version of that commit (15c7b88b) into develop, superseding the correct version (9c1b4f6c) that was merged in when #25857 was merged.
That whole mess is my fault, and I apologize for the confusion it's caused for you. I'll open a separate ticket to fix it. But ultimately it doesn't impact this ticket.
comment:122 Changed 2 years ago by
E.g. you can see:
$ git diff 8.5.beta1..8.5.beta2 -- build/make/deps
-
build/make/deps
diff --git a/build/make/deps b/build/make/deps index a3abca3..bbe3277 100644
a b toolchain: $(foreach pkgname,$(TOOLCHAIN),$(inst_$(pkgname))) 75 75 # unconditionally. We still use the dependency checking from $(MAKE), 76 76 # so this will not trigger useless rebuilds. 77 77 # See #14168 and #14232. 78 # 79 # Note: This list consists of only the *runtime* dependencies of the toolchain. 80 TOOLCHAIN_DEPS = zlib $(MP_LIBRARY) mpfr mpc 81 TOOLCHAIN_DEP_INSTS = \ 82 $(foreach pkgname,$(TOOLCHAIN_DEPS),$(inst_$(pkgname))) 83 78 # Note: This list is determined from the dependencies of the TOOLCHAIN 79 # packages which include gcc, and optionally ccache; in principle this 80 # list is redundant. 81 TOOLCHAIN_DEPS = zlib xz $(MP_LIBRARY) mpfr mpc 84 82 toolchain-deps: 85 for target in $(TOOLCHAIN_DEP_INSTS); do \86 $(MAKE) $$ target; \83 for pkg in $(TOOLCHAIN_DEPS); do \ 84 $(MAKE) $$pkg; \ 87 85 done 88 86 89 87 all-toolchain: base-toolchain … … sagelib: \ 129 127 $(inst_arb) \ 130 128 $(BLAS) \ 131 129 $(inst_brial) \ 132 $(inst_cephes) \133 130 $(inst_cliquer) \ 134 131 $(inst_cypari) \ 135 132 $(inst_cysignals) \
so that just started in 8.5.beta2 since #25188 was merged in that build. It was never intended that xz should be added to TOOLCHAIN_DEPS
as part of that ticket. I just screwed up :(
comment:123 in reply to: ↑ 121 ; follow-up: ↓ 124 Changed 2 years ago by
Replying to embray:
I see what you're saying now. That has nothing directly to do with this ticket actually.
I think in Luxembourg I showed you a puzzling error that we traced to my using a branch with wrong TOOLCHAIN_DEPS
- could you fix it here, instead of delaying to another ticket?
comment:124 in reply to: ↑ 123 Changed 2 years ago by
Replying to dimpase:
Replying to embray:
I see what you're saying now. That has nothing directly to do with this ticket actually.
I think in Luxembourg I showed you a puzzling error that we traced to my using a branch with wrong
TOOLCHAIN_DEPS
- could you fix it here, instead of delaying to another ticket?
Please see above. It really has nothing to do with this ticket and doesn't impact it one way or the other. It can be fixed in a separate ticket totally independent from this one. The only impact of this mistake is that the xz
package was added to the toolchain dependencies, but Jeroen (I think) had pointed out to me that it's not actually needed there so it's sort of a superfluous dependency that gets built earlier than necessary.
This was only messing you up in Luxembourg since you were experimenting with adding a configure-time check for xz (thank you!) but since that hasn't actually been added yet there's no issue. Also IIRC the bug with that was not actually because xz was included in TOOLCHAIN_DEPS, but due to a bug that I fixed in a later version of #25188. I got confused because I was certain xz
shouldn't be there and yet it was, due to the mistake I described above (which I was unaware of until now).
comment:125 follow-up: ↓ 127 Changed 2 years ago by
If I add a build/pkgs/xz/spkg-configure.m4
, e.g. from #26286, it won't work.
This is a problem to fix on this ticket, no?
comment:126 follow-up: ↓ 128 Changed 2 years ago by
I believe there was also a discrepancy in another file in build/make
, not only deps
.
comment:127 in reply to: ↑ 125 Changed 2 years ago by
Replying to dimpase:
If I add a
build/pkgs/xz/spkg-configure.m4
, e.g. from #26286, it won't work.
But for reasons that have nothing to do with this ticket. What I mean is, even if you added a configure-time check directly to configure.ac
(e.g. ignoring the mechanisms implemented by this ticket) the same problem you were having (the SPKG being forcibly built when it wasn't needed) would occur.
If you want the specifics, there was an older version of #25857 that had this bug, which Jeroen pointed out: https://trac.sagemath.org/ticket/25857#comment:26 I fixed the bug, and #25857 was merged without any (known) bugs. I then accidentally reintroduced that bug in #25188 when I merged an old version of my branch for #25857 into that branch. I simply need to add a new ticket to re-revert that bug.
comment:128 in reply to: ↑ 126 Changed 2 years ago by
Replying to dimpase:
I believe there was also a discrepancy in another file in
build/make
, not onlydeps
.
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.
comment:129 Changed 2 years ago by
How about fixing it on #26286 ?
comment:130 Changed 2 years ago by
- Status changed from needs_review to positive_review
comment:131 Changed 2 years ago by
- Status changed from positive_review to needs_work
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.
comment:132 Changed 2 years ago by
In particular I think #25188 may have totally regressed; specifically the question of whether or not ./configure
lists the sage gcc package as installed.
comment:133 Changed 2 years ago by
Yeah, I made at least one mistake while rebasing. This should be spelled sage_spkg_install_gcc=yes
now.
comment:134 Changed 2 years ago by
- Commit changed from 5de0eb56f3dc990ac08b56a71b112a597d6efb4f to 79de3bdf4c2d09cbde306ca28d166161e5e1028a
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
8fd4dec | Move the gcc/gfortran system package checks to their own spkg-configure.m4
|
21fbddd | Slightly rewrote this macro to use some more polymorphic shell macros.
|
b4cc8eb | Use m4_sinclude instead--this prevents problems when switching between branches where one of these listed files no longer exists
|
8d01c6e | Move these comments inside the macro call to ensure they are actually expanded as part of the macro
|
cf6367a | Changed a bit how SAGE_SPKG_CONFIGURE works:
|
16c22de | Implement an spkg-configure.m4 for patch, demonstrating use of the new system
|
6526868 | this check should be handled in gfortran's spkg-configure.m4
|
a5f783e | add a diagnostic message that is occasionally helpful for understanding the configure output
|
3bedcd6 | added better reporting for yasm program+feature check
|
79de3bd | BSD find puts in extra slashes if you leave a trailing slash in the argument
|
comment:135 Changed 2 years ago by
- Status changed from needs_work to positive_review
Okay, now it should be good to go.
comment:136 Changed 2 years ago by
Specifically, the commit at https://git.sagemath.org/sage.git/commit/?id=8fd4decd9e2d2f33ec9d3fa6a488c0df23b88764 is updated with the corrected spelling of that line.
comment:137 Changed 2 years ago by
- Branch changed from u/embray/build/autoconf-macros to 79de3bdf4c2d09cbde306ca28d166161e5e1028a
- Resolution set to fixed
- Status changed from positive_review to closed
comment:138 Changed 2 years ago by
- Commit 79de3bdf4c2d09cbde306ca28d166161e5e1028a deleted
see #26715 for a gfortran-related collateral damage caused here.
comment:139 Changed 2 years ago by
- Description modified (diff)
comment:140 Changed 2 years ago by
- Keywords spkg-configure added
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 modifyingconfigure.ac
directly.