Opened 21 months ago

Closed 13 months ago

Last modified 10 months ago

#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) Commit:
Dependencies: #21524, #25011, #25208, #25188, #25198 Stopgaps:

Description (last modified by dimpase)

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)

spkg-configure.m4 (582 bytes) - added by dimpase 16 months ago.
xz spkg-configure.m4

Download all attachments as: .zip

Change History (141)

comment:1 Changed 21 months ago by embray

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.

comment:2 Changed 21 months ago by embray

  • Status changed from new to needs_review

comment:3 follow-up: Changed 21 months ago by embray

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: Changed 21 months ago by 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, ...

comment:5 in reply to: ↑ 4 ; follow-up: Changed 21 months ago by embray

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: Changed 21 months ago by 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.

comment:7 follow-up: Changed 21 months ago by jdemeyer

  • Status changed from needs_review to needs_work

The branch doesn't merge.

comment:8 Changed 21 months ago by mmezzarobba

  • Cc mmezzarobba added

comment:9 in reply to: ↑ 6 Changed 21 months ago by embray

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 21 months ago by embray

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.

comment:11 Changed 20 months ago by git

  • Commit changed from c16060622bfc5a27b732b8d8bb819cdcf0dcc890 to 070da0bbb480307a62d8cf7cc122245da8d0912a

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

comment:12 Changed 20 months ago by embray

  • 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 20 months ago by dimpase

  • Cc dimpase added
  • Milestone changed from sage-8.2 to sage-8.3

comment:14 follow-up: Changed 20 months ago by 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.

comment:15 in reply to: ↑ 14 Changed 20 months ago by embray

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: Changed 20 months ago by 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

comment:17 Changed 20 months ago by git

  • Commit changed from 070da0bbb480307a62d8cf7cc122245da8d0912a to 87d0d31e45699789d4218f2d7cee3a99d8bd63de

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

comment:18 Changed 20 months ago by embray

  • Dependencies changed from #21524, #25011 to #21524, #25011, #25208

comment:19 in reply to: ↑ 16 ; follow-up: Changed 20 months ago by 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/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.

comment:20 Changed 20 months ago by embray

  • Status changed from needs_work to needs_review

comment:21 Changed 20 months ago by embray

  • Status changed from needs_review to needs_work

Seems to have a bug since the last rebase...

comment:22 Changed 20 months ago by git

  • Commit changed from 87d0d31e45699789d4218f2d7cee3a99d8bd63de to 862d8e4b74e54745188eb6b75fc894d90768dc53

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

5b833a2Move the gcc/gfortran system package checks to their own spkg-configure.m4
c0b2750Slightly rewrote this macro to use some more polymorphic shell macros.
751a515Use m4_sinclude instead--this prevents problems when switching between branches where one of these listed files no longer exists
9249861Move these comments inside the macro call to ensure they are actually expanded as part of the macro
4a7953aChanged a bit how SAGE_SPKG_CONFIGURE works:
6433ef7Implement an spkg-configure.m4 for patch, demonstrating use of the new system
9df2996Revert "Trac #25113: OSX is f*ed up sometimes"
b6f2947Fixes miscompilation by clang from XCode 6.3, see #25118
45b4357gfan version bump
862d8e4move configure checks for OSX compatibility into a separate macro

comment:23 Changed 20 months ago by embray

  • Status changed from needs_work to needs_review

comment:24 follow-up: Changed 20 months ago by dimpase

that's better, at least make distclean succeeds after ./bootstrap...

comment:25 in reply to: ↑ 19 Changed 20 months ago by dimpase

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/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".

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: Changed 20 months ago by 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.

comment:27 in reply to: ↑ 26 Changed 20 months ago by embray

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: Changed 20 months ago by 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.

comment:29 in reply to: ↑ 28 Changed 20 months ago by dimpase

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 20 months ago by dimpase

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 19 months ago by vdelecroix

Would be wonderful if it can handle #25158 (cmake which is an optional package).

comment:32 follow-up: Changed 19 months ago by 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...)

comment:33 in reply to: ↑ 32 Changed 19 months ago by embray

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 17 months ago by saraedum

  • Status changed from needs_review to needs_work
  • Work issues set to merge conflicts

comment:35 Changed 17 months ago by embray

I could have sworn I rebased this recently. Maybe it was just on my todo list.

comment:36 Changed 17 months ago by git

  • Commit changed from 862d8e4b74e54745188eb6b75fc894d90768dc53 to e1e68ee4e81facf4114735603a40cac79ea1dcbe

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

b6ad44bAs an initial demonstration of the new system, move configuration
5ce5577Move the curl system package check to its own spkg-configure.m4
5668e2dMove the git system package check to its own spkg-configure.m4
42617d3Move the gcc/gfortran system package checks to their own spkg-configure.m4
1c93c29Slightly rewrote this macro to use some more polymorphic shell macros.
bcbeddcUse m4_sinclude instead--this prevents problems when switching between branches where one of these listed files no longer exists
742eaedMove these comments inside the macro call to ensure they are actually expanded as part of the macro
86d87daChanged a bit how SAGE_SPKG_CONFIGURE works:
0ac110bImplement an spkg-configure.m4 for patch, demonstrating use of the new system
e1e68eemove configure checks for OSX compatibility into a separate macro

comment:37 Changed 17 months ago by embray

  • Status changed from needs_work to needs_review
  • Work issues merge conflicts deleted

comment:38 Changed 17 months ago by embray

  • Status changed from needs_review to needs_work

Just noticed a typo in how this builds on top of #25011.

comment:39 Changed 17 months ago by embray

  • Dependencies changed from #21524, #25011, #25208 to #21524, #25011, #25208, #25811

comment:40 Changed 17 months ago by embray

  • Dependencies changed from #21524, #25011, #25208, #25811 to #21524, #25011, #25208, #25188, #25198

comment:41 Changed 17 months ago by embray

  • 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 16 months ago by embray

  • 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 16 months ago by git

  • Commit changed from e1e68ee4e81facf4114735603a40cac79ea1dcbe to f14f85b101976b6fb8a408de1f876cb40e130be9

Branch pushed to git repo; I updated commit sha1. New commits:

f14f85bIncorporate fixes from #25188

comment:44 Changed 16 months ago by embray

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 16 months ago by dimpase

Why is it in "needs work" status?

comment:46 Changed 16 months ago by embray

  • Status changed from needs_work to needs_review

comment:47 follow-up: Changed 16 months ago by gh-timokau

Good work! What happens if the system package is updated to an incompatible version or even removed?

comment:48 in reply to: ↑ 47 Changed 16 months ago by embray

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: Changed 16 months ago by 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'

comment:50 Changed 16 months ago by dimpase

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: Changed 16 months ago by dimpase

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: Changed 16 months ago by dimpase

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.

Changed 16 months ago by dimpase

xz spkg-configure.m4

comment:53 Changed 16 months ago by dimpase

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 16 months ago by dimpase

Replying to dimpase:

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.

This m4 file will also need proper check for liblzma being installed with headers and all.

comment:55 in reply to: ↑ 49 Changed 16 months ago by embray

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: Changed 16 months ago by embray

Replying to dimpase:

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.

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 16 months ago by embray

Replying to dimpase:

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.

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.

Last edited 16 months ago by embray (previous) (diff)

comment:58 follow-up: Changed 16 months ago by 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 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.

Last edited 16 months ago by embray (previous) (diff)

comment:59 in reply to: ↑ 56 Changed 16 months ago by dimpase

Replying to embray:

Replying to dimpase:

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.

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: Changed 16 months ago by embray

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.

Last edited 16 months ago by embray (previous) (diff)

comment:61 in reply to: ↑ 60 ; follow-ups: Changed 16 months ago by 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.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: Changed 16 months ago by 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

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 16 months ago by dimpase

Replying to dimpase:

As spkg-configure.m4 invokes curl-config --checkfor, you get this error if no bc 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?

Last edited 16 months ago by dimpase (previous) (diff)

comment:64 in reply to: ↑ 61 Changed 16 months ago by embray

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.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`

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: Changed 16 months ago by embray

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 16 months ago by dimpase

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 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?

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 16 months ago by embray

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 15 months ago by dimpase

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

Last edited 15 months ago by dimpase (previous) (diff)

comment:69 Changed 15 months ago by embray

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 15 months ago by dimpase

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: Changed 15 months ago by jhpalmieri

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 15 months ago by dimpase

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 15 months ago by embray

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 15 months ago by embray

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 15 months ago by git

  • Commit changed from f14f85b101976b6fb8a408de1f876cb40e130be9 to a2bedc5c944bee2c35815768cfd2ed554834429e

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

d9e9b4fMove the curl system package check to its own spkg-configure.m4
9c35b95Move the git system package check to its own spkg-configure.m4
e9b7362Move the gcc/gfortran system package checks to their own spkg-configure.m4
8821c76Slightly rewrote this macro to use some more polymorphic shell macros.
b4630a5Use m4_sinclude instead--this prevents problems when switching between branches where one of these listed files no longer exists
3cde0d7Move these comments inside the macro call to ensure they are actually expanded as part of the macro
2f73979Changed a bit how SAGE_SPKG_CONFIGURE works:
724c459Implement an spkg-configure.m4 for patch, demonstrating use of the new system
97fec35Incorporate fixes from #25188
a2bedc5this check should be handled in gfortran's spkg-configure.m4

comment:76 Changed 15 months ago by embray

Still have a syntax error that crept in somewhere....

comment:77 Changed 15 months ago by git

  • Commit changed from a2bedc5c944bee2c35815768cfd2ed554834429e to db1bbe09714e92591369535ff751edd1be332e7b

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

f01eb8aIncorporate fixes from #25188
d89f08athis check should be handled in gfortran's spkg-configure.m4
db1bbe0add a diagnostic message that is occasionally helpful for understanding the configure output

comment:78 Changed 15 months ago by embray

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 15 months ago by embray

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 15 months ago by dimpase

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 15 months ago by dimpase

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 15 months ago by dimpase

  • Status changed from needs_review to needs_work

comment:83 Changed 15 months ago by embray

  • 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 15 months ago by embray

  • 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 15 months ago by git

  • Commit changed from db1bbe09714e92591369535ff751edd1be332e7b to fc985cc6e2c8b951e8f7c442990f448aed9e94c1

Branch pushed to git repo; I updated commit sha1. New commits:

fc985ccadded better reporting for yasm program+feature check

comment:86 follow-up: Changed 15 months ago by embray

  • 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:

fc985ccadded better reporting for yasm program+feature check

comment:87 follow-up: Changed 15 months ago by jhpalmieri

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 15 months ago by embray

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 15 months ago by embray

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 15 months ago by embray

  • Status changed from needs_review to needs_work

comment:91 follow-up: Changed 15 months ago by 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 get

configure: === checking whether to install the yasm SPKG ===
checking for yasm supporting the adox instruction... /usr/local/bin/yasm
Last edited 15 months ago by dimpase (previous) (diff)

comment:92 follow-up: Changed 15 months ago by git

  • Commit changed from fc985cc6e2c8b951e8f7c442990f448aed9e94c1 to 8c8e09a2dee22dc01cecdb4f987e882c8f8555c1

Branch pushed to git repo; I updated commit sha1. New commits:

8c8e09aBSD find puts in extra slashes if you leave a trailing slash in the argument

comment:93 follow-up: Changed 15 months ago by embray

  • Status changed from needs_work to needs_review

How about now?

comment:94 in reply to: ↑ 92 Changed 15 months ago by dimpase

Replying to git:

Branch pushed to git repo; I updated commit sha1. New commits:

8c8e09aBSD find puts in extra slashes if you leave a trailing slash in the argument

this does not change anything in respect to comment 91

comment:95 in reply to: ↑ 93 Changed 15 months ago by jhpalmieri

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 15 months ago by dimpase

  • 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 15 months ago by embray

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 get

configure: === 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 15 months ago by embray

Thanks.

comment:99 Changed 15 months ago by embray

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 13 months ago by embray

  • Milestone changed from sage-8.4 to sage-8.5

comment:101 follow-up: Changed 13 months ago by dimpase

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: Changed 13 months ago by jdemeyer

Replying to dimpase:

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...

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:103 Changed 13 months ago by vbraun

  • Status changed from positive_review to needs_work

Merge conflict

comment:104 in reply to: ↑ 102 Changed 13 months ago by embray

Replying to jdemeyer:

Replying to dimpase:

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...

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 13 months ago by embray

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 13 months ago by git

  • Commit changed from 8c8e09a2dee22dc01cecdb4f987e882c8f8555c1 to a1a384f3206bd307a7b4429294d9b1d6878269c5

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

029584aSlightly rewrote this macro to use some more polymorphic shell macros.
e13304cUse m4_sinclude instead--this prevents problems when switching between branches where one of these listed files no longer exists
4b6c2acMove these comments inside the macro call to ensure they are actually expanded as part of the macro
31fd98bChanged a bit how SAGE_SPKG_CONFIGURE works:
58acae8Implement an spkg-configure.m4 for patch, demonstrating use of the new system
273f25aIncorporate fixes from #25188
7577f7athis check should be handled in gfortran's spkg-configure.m4
ff087efadd a diagnostic message that is occasionally helpful for understanding the configure output
94f18b1added better reporting for yasm program+feature check
a1a384fBSD find puts in extra slashes if you leave a trailing slash in the argument

comment:107 Changed 13 months ago by embray

I rebased on current develop and there was no merge conflict. So I guess it's another mystery conflict...

comment:108 Changed 13 months ago by dimpase

  • Status changed from needs_work to positive_review

tried to merge the vbraun's branch, it works now.

comment:109 follow-up: Changed 13 months ago by 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...

comment:110 Changed 13 months ago by vbraun

  • Status changed from positive_review to needs_work

comment:111 in reply to: ↑ 109 Changed 13 months ago by dimpase

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 13 months ago by dimpase

  • Branch changed from u/embray/build/autoconf-macros to u/dimpase/build/autoconf-macros
  • Commit changed from a1a384f3206bd307a7b4429294d9b1d6878269c5 to 8ea6c52eb1cae9b0c5f60e2051f48c1f6efc3304

rebased

comment:113 Changed 13 months ago by dimpase

  • 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 13 months ago by dimpase

  • 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:

de2746fMerge #24919

comment:115 Changed 13 months ago by git

  • Commit changed from de2746fe1854b2af506f8844142880f8a108fb36 to aa6932de8c8aa787089b045375e2fead582463c2

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

241bb4cpy3: fix doctests in ribbon tableaux
5dec1a7Resolved problem in parallel a*a
d07180dChanged name
e16e128remove cephes - we do not need it anymore
098f249Trac #26510: Fix combinat.subset for python3
1b5fd0eTrac #26545: Reverse 'islice' corrections and rebase ...
3ea41e3remove deprecated stuff in manifolds
c5d4518py3: fix some doctests in combinat/species
e319a2btrac 26610 reviewer's request
aa6932dUpdated SageMath version to 8.5.beta2

comment:116 Changed 13 months ago by dimpase

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 13 months ago by embray

Please just let me....

comment:118 Changed 13 months ago by embray

  • Owner changed from (none) to embray

comment:119 Changed 13 months ago by embray

  • 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:

506ffb3Move the gcc/gfortran system package checks to their own spkg-configure.m4
e16b957Slightly rewrote this macro to use some more polymorphic shell macros.
b7ba60eUse m4_sinclude instead--this prevents problems when switching between branches where one of these listed files no longer exists
8f338fbMove these comments inside the macro call to ensure they are actually expanded as part of the macro
ef1edb0Changed a bit how SAGE_SPKG_CONFIGURE works:
73a6d0dImplement an spkg-configure.m4 for patch, demonstrating use of the new system
c4083a6this check should be handled in gfortran's spkg-configure.m4
a512752add a diagnostic message that is occasionally helpful for understanding the configure output
3ab0529added better reporting for yasm program+feature check
5de0eb5BSD find puts in extra slashes if you leave a trailing slash in the argument

comment:120 Changed 13 months ago by dimpase

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: Changed 13 months ago by embray

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 13 months ago by embray

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))) 
    7575# unconditionally. We still use the dependency checking from $(MAKE),
    7676# so this will not trigger useless rebuilds.
    7777# 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.
     81TOOLCHAIN_DEPS = zlib xz $(MP_LIBRARY) mpfr mpc
    8482toolchain-deps:
    85        for target in $(TOOLCHAIN_DEP_INSTS); do \
    86            $(MAKE) $$target; \
     83       for pkg in $(TOOLCHAIN_DEPS); do \
     84           $(MAKE) $$pkg; \
    8785       done
    8886
    8987all-toolchain: base-toolchain
    sagelib: \ 
    129127               $(inst_arb) \
    130128               $(BLAS) \
    131129               $(inst_brial) \
    132                $(inst_cephes) \
    133130               $(inst_cliquer) \
    134131               $(inst_cypari) \
    135132               $(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: Changed 13 months ago by 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?

comment:124 in reply to: ↑ 123 Changed 13 months ago by embray

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).

Last edited 13 months ago by embray (previous) (diff)

comment:125 follow-up: Changed 13 months ago by dimpase

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: Changed 13 months ago by dimpase

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

comment:127 in reply to: ↑ 125 Changed 13 months ago by embray

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.

Last edited 13 months ago by embray (previous) (diff)

comment:128 in reply to: ↑ 126 Changed 13 months ago by embray

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.

comment:129 Changed 13 months ago by dimpase

How about fixing it on #26286 ?

comment:130 Changed 13 months ago by dimpase

  • Status changed from needs_review to positive_review

comment:131 Changed 13 months ago by embray

  • 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 13 months ago by embray

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 13 months ago by embray

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

comment:134 Changed 13 months ago by git

  • Commit changed from 5de0eb56f3dc990ac08b56a71b112a597d6efb4f to 79de3bdf4c2d09cbde306ca28d166161e5e1028a

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

comment:135 Changed 13 months ago by embray

  • Status changed from needs_work to positive_review

Okay, now it should be good to go.

comment:136 Changed 13 months ago by embray

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 13 months ago by vbraun

  • Branch changed from u/embray/build/autoconf-macros to 79de3bdf4c2d09cbde306ca28d166161e5e1028a
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:138 Changed 13 months ago by dimpase

  • Commit 79de3bdf4c2d09cbde306ca28d166161e5e1028a deleted

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

comment:139 Changed 12 months ago by dimpase

  • Description modified (diff)

comment:140 Changed 10 months ago by dimpase

  • Keywords spkg-configure added
Note: See TracTickets for help on using tickets.