replace bashisms in m4/sage_spkg_collect.m4, m4/sage_spkg_enable.m4, build/pkgs/*/spkg-configure.m4, src/bin/sage-env, build/make/Makefile.in
orlitzky opened this issue · 70 comments
Autoconf scripts should be POSIX sh rather than bash, but there are a few bashisms in m4/sage_spkg_collect.m4 and m4/sage_spkg_enable.m4:
- The quoted newlines
$'\n' - The use of
VAR+=VALUE
Some of these should be straightforward to fix. The format
SAGE_BUILT_PACKAGES+=" $SPKG_NAME \\"$'\n'
can be changed to
SAGE_BUILT_PACKAGES="$SAGE_BUILT_PACKAGES $SPKG_NAME"
since the newline is only used to make the BUILT_PACKAGES rule in the resulting Makefile look nice. Changing them to spaces doesn't change what the rule does. I'm not sure about
SAGE_PACKAGE_VERSIONS+="vers_$SPKG_NAME = $SPKG_VERSION"$'\n
and
SAGE_PACKAGE_DEPENDENCIES+="deps_$SPKG_NAME = $DEPS"$'\n'
though, because those are inserted verbatim into the Makefile as rules, and the newlines probably matter.
Upstream: Fixed upstream, in a later stable release.
CC: @dimpase @mkoeppe @embray @vbraun
Component: build: configure
Author: Michael Orlitzky
Branch: 0e66a0a
Reviewer: Dima Pasechnik
Issue created by migration from https://trac.sagemath.org/ticket/29345
I agree in general - do you actually run this with a CONFIG_SHELL other than bash? Is the other configure code clean?
This should probably be done on top of #29287, which touches the same file
Things seem to run OK with dash if I comment out the part of configure.ac that forces bash. Of course I can't be sure that it works until I have a suggestion for how to fix SAGE_PACKAGE_VERSIONS and SAGE_PACKAGE_DEPENDENCIES.
One way to address this is to make things a liitle more static (see #29114 - "Only ./bootstrap should glob build/pkgs"), also getting rid of GNU-make-isms and macrology in build/make/Makefile.
bootstrap already emits macro calls for many packages into m4/sage_spkg_configures.m4, of three types.
- m4_sinclude([build/pkgs/arb/spkg-configure.m4])
- SAGE_SPKG_CONFIGURE_ARB
- SAGE_SPKG_ENABLE([ninja_build], [optional])
Might as well emit another line for every package that creates whatever is needed in the Makefile.
Author: Michael Orlitzky
Branch: u/mjo/ticket/29345
The printf stuff is going to make anyone reading the code think we're stupid, but it works and didn't involve changing much code. I just completed a full build using /bin/dash as my shell.
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
26bf7af | Trac #29345: replace a few obsolete "-a" tests with "-e". |
fea7ad9 | Trac #29345: replace a bash array with something portable. |
4f0a56a | Trac #29345: replace a few uses of "source" with "." |
42a63fd | Trac #29345: fix some bashisms in sage-env's resolvelinks() function. |
489d8cc | Trac #29345: don't force SHELL=bash any longer. |
Nice on first glance. I'll be happy to do a full review after 9.1 is out
it's a bit in vain, as there are bashisms or just "bash required" in several Sage packages.
regarding newline, perhaps https://stackoverflow.com/questions/21880891/newline-character-in-posix-shell says something.
Replying to @dimpase:
it's a bit in vain, as there are bashisms or just "bash required" in several Sage packages.
Ultimately my goal is to not install any sage packages =)
I'll always need bash installed for other stuff, but ./configure is noticeably faster with dash. It may be a "micro-optimization" but half of what I do lately is re-run ./configure.
Replying to @dimpase:
regarding newline, perhaps https://stackoverflow.com/questions/21880891/newline-character-in-posix-shell says something.
The suggestion there is to print a newline and a space at the end of each line, and then remove the space afterwards. Since all of the lines in question are indented, what I did instead was switch to printing a newline and some spaces at the beginning of each line -- that way the newline is still followed by spaces (and therefore doesn't get dropped), but the spaces aren't "extra" and don't need to be removed.
needs rebase.
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
165d554 | Trac #29345: fix most autotools bashisms. |
1b5dafb | Trac #29345: fix one more newline-constant bashism in SAGE_SPKG_ENABLE. |
c4a74d8 | Trac #29345: replace a few obsolete "-a" tests with "-e". |
fcd9cef | Trac #29345: replace a bash array with something portable. |
839623a | Trac #29345: replace a few uses of "source" with "." |
0717ec4 | Trac #29345: fix some bashisms in sage-env's resolvelinks() function. |
279765f | Trac #29345: don't force SHELL=bash any longer. |
fixed, thanks
A interesting test (apart from using dash) would be to use ksh (on OpenBSD - trying this now for lolz) or zsh (the latter is the default on recent macOS, I am told).
./configure fails with /bin/sh -> /bin/zsh at,
Checking whether SageMath should install SPKG mpir...
checking gmp.h usability... yes
checking gmp.h presence... yes
checking for gmp.h... yes
checking gmpxx.h usability... yes
checking gmpxx.h presence... yes
checking for gmpxx.h... yes
checking for library containing __gmpq_cmp_z... -lgmp
./configure:break:10258: not in while, until, select, or repeat loop
Since it was executed via /bin/sh, I think zsh is already in bourne-shell compatibility mode.
ksh also notices a stray break and prints a warning, no error.
with ksh/openbsd, the ugly hack in spkg-install of lrcalc does not work, as copied over config.status gets really confused, and complains about print not found etc.
The following (needs autotools isntalled, obviously) fixes this:
-cp "$SAGE_ROOT"/config/config.* .
+autoreconf -ivfditto (ksh/openbsd) - this is how givaro outputs its givaro.pc - which is of course broken this way
/------------------ givaro.pc ------------------------
prefix=/home/dima/sagetrac-mirror/local
exec_prefix=/home/dima/sagetrac-mirror/local
libdir=/home/dima/sagetrac-mirror/local/lib
includedir=/home/dima/sagetrac-mirror/local/include
Name: Givaro
Description: C++ library for arithmetic and algebraic computations
URL: https://casys.gricad-pages.univ-grenoble-alpes.fr/givaro
Version: 4.1.1
Requires:
Libs: -L/home/dima/sagetrac-mirror/local/lib -lgivaro -lgmp -lgmpxx
Cflags: -I${prefix}/include
\-------------------------------------------------------
Replying to @dimpase:
with ksh/openbsd, the ugly hack in spkg-install of lrcalc does not work, as copied over config.status gets really confused, and complains about
The following (needs autotools isntalled, obviously) fixes this:-cp "$SAGE_ROOT"/config/config.* . +autoreconf -ivf
That commit mentions only config.guess, but it doesn't say what problem it was solving. Maybe copying config.status was unintentional.
New commits fix the break statements and probably lrcalc.
The is also a bug in build/pkgs/cvxopt/spkg-install.in, in
# Stolen from Gentoo
pkg_libs() {
pkg-config --libs-only-l $* | \
sed -e 's:[ ]-l*\(pthread\|m\)\([ ]\|$\)::g' -e 's:[ ]*$::' | \
tr ' ' '\n' | sort -u | sed -e "s:^-l\(.*\):\1:g" | \
tr '\n' ';' | sed -e 's:;$::'
}
so that on ksh/openbsd I get CVXOPT_BLAS_LIB="$(pkg_libs blas)"
gets ;openblas rather than openblas.
Note that
(sage-buildsh) dima@sagemath:sagetrac-mirror$ pkg-config --libs-only-l blas
-lopenblas
outputs a leading space.
$ pkg-config --libs-only-l blas | sed -e 's:[ ]-l*\(pthread\|m\)\([ ]\|$\)::g' -e 's:[ ]*$::' | tr ' ' '\n'
-lopenblas
And so the latter outputs an extra blank line, leading to erroneous ;.
It would suffice to remove the leading spaces from the output of pkg-config, say.
The following does the job to fix the latter
--- a/build/pkgs/cvxopt/spkg-install.in
+++ b/build/pkgs/cvxopt/spkg-install.in
@@ -2,7 +2,7 @@ cd src
# Stolen from Gentoo
pkg_libs() {
- pkg-config --libs-only-l $* | \
+ pkg-config --libs-only-l $* | sed -e 's/^ *//g' | \
sed -e 's:[ ]-l*\(pthread\|m\)\([ ]\|$\)::g' -e 's:[ ]*$::' | \
tr ' ' '\n' | sort -u | sed -e "s:^-l\(.*\):\1:g" | \
tr '\n' ';' | sed -e 's:;$::'And here are already merged upstream fixes for comment:20 problem
Upstream: Fixed upstream, in a later stable release.
Lol that's like ten pipelines just to parse a space delimited list and replace -l with ;. I think this does the same thing, and works with bash, dash, ksh, zsh, and posh:
# The BLAS_LIB and LAPACK_LIB variables (among others) in cvxopt's setup.py
# are passed in as colon-delimited strings. So, for example, if your blas
# lib flags are "-lblas -lcblas", then cvxopt wants "blas;cblas". The
# following function takes a package name, feeds it to pkg-config, and then
# converts it to the proper format.
cvxopt_libs() {
CVXOPT_LIBS=""
SKIP_LIBS="pthread m"
for PKGCONFIG_LIB in $(pkg-config --libs-only-l "${1}"); do
for SKIP_LIB in ${SKIP_LIBS}; do
if [ "${PKGCONFIG_LIB}" = "-l${SKIP_LIB}" ]; then
# break out of the big loop if we're skipping this lib
continue 2
fi
done
# replace leading "-l" with ";"
CVXOPT_LIBS="${CVXOPT_LIBS};${PKGCONFIG_LIB#-l}"
done
# strip the leading ";" from ";foo;bar" before output
echo "${CVXOPT_LIBS#;}"
}
care to add the latter to the branch?
Yeah I will after I'm sure that it works. I'll probably also update the Gentoo ebuild. Some ad-hoc testing shows differences between the two, but it looks like they're all mistakes in the existing version; some things aren't handled correctly after -lpthread or -lm are removed, for example:
$ pkg-config --libs-only-l librsvg-2.0
-lrsvg-2 -lm -lgio-2.0 -lgdk_pixbuf-2.0 -lgobject-2.0 -lglib-2.0 -lcairo
# sage/gentoo version
$ ./orig.sh librsvg-2.0
cairo;gdk_pixbuf-2.0;glib-2.0;gobject-2.0;rsvg-2-lgio-2.0
# the new one
$ ./cvxopt_libs.sh librsvg-2.0
rsvg-2;gio-2.0;gdk_pixbuf-2.0;gobject-2.0;glib-2.0;cairo
There are more issues with cvxopt in both Gentoo and Sage... for example, if you don't set one of the *_LIB variables, then it defaults to adding /usr/lib to your search path which will pick up the wrong libraries on most Gentoo systems (you want /usr/lib64 instead). Passing in the empty string to override sort-of works, but not for the *_INC_DIR variables, meaning that we need special cases everywhere to do it right. I've opened an issue with cvxopt to see if they can make our lives easier by treating the empty string as "no extra libs/paths" rather than "one extra lib/path consisting of the empty string."
Branch pushed to git repo; I updated commit sha1. New commits:
21cbae8 | Trac #29345: replace the function that populates the CVXOPT_* variables. |
The latest commit lets me build cvxopt against the suitesparse SPKG using ksh.
Now that I'm familiar, I have a feeling that some more work is going to be needed on the suitesparse spkg-configure.m4 ticket to keep cvxopt working if suitesparse comes from the system. The new function I added should help if that proves true.
Is this ready for review? Just checking.
Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:
e778266 | Trac #29345: fix one more newline-constant bashism in SAGE_SPKG_ENABLE. |
c18eda8 | Trac #29345: replace a few obsolete "-a" tests with "-e". |
2ba5fe9 | Trac #29345: replace a bash array with something portable. |
f86546e | Trac #29345: replace a few uses of "source" with "." |
76311fd | Trac #29345: fix some bashisms in sage-env's resolvelinks() function. |
f72178b | Trac #29345: don't force SHELL=bash any longer. |
f040377 | Trac #29345: remove "break" statements from AC_SEARCH_LIBS. |
ef2888e | Trac #29345: don't use sage's config.status for the lrcalc build. |
3e2d6f3 | Trac #29345: replace the function that populates the CVXOPT_* variables. |
00de6b3 | Trac #29345: add Dima's SPKG patches for ksh compatibility. |
Replying to @dimpase:
Is this ready for review? Just checking.
Yes, I've added one more commit with your pc-file patches for givaro, linbox, and fflas-ffpack. There are no more (known) problems with non-bash shells.
OK to rebase this on top of #29098 (Merge build/make/deps into build/make/Makefile.in)?
Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:
11eba29 | Trac #29345: fix one more newline-constant bashism in SAGE_SPKG_ENABLE. |
f6746dd | Trac #29345: replace a few obsolete "-a" tests with "-e". |
31a5a18 | Trac #29345: replace a bash array with something portable. |
e0d0f6c | Trac #29345: replace a few uses of "source" with "." |
47ff65c | Trac #29345: fix some bashisms in sage-env's resolvelinks() function. |
59b8ccc | Trac #29345: don't force SHELL=bash any longer. |
43a81c3 | Trac #29345: remove "break" statements from AC_SEARCH_LIBS. |
76a931d | Trac #29345: don't use sage's config.status for the lrcalc build. |
e0d871a | Trac #29345: replace the function that populates the CVXOPT_* variables. |
2f5f383 | Trac #29345: add Dima's SPKG patches for ksh compatibility. |
Dependencies: 29098
Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:
647e64c | Trac #29345: fix one more newline-constant bashism in SAGE_SPKG_ENABLE. |
4e0b8dd | Trac #29345: replace a few obsolete "-a" tests with "-e". |
955da4d | Trac #29345: replace a bash array with something portable. |
76bdb66 | Trac #29345: replace a few uses of "source" with "." |
7f46fae | Trac #29345: fix some bashisms in sage-env's resolvelinks() function. |
791194a | Trac #29345: don't force SHELL=bash any longer. |
c22ef41 | Trac #29345: remove "break" statements from AC_SEARCH_LIBS. |
20a1cdb | Trac #29345: don't use sage's config.status for the lrcalc build. |
7077feb | Trac #29345: replace the function that populates the CVXOPT_* variables. |
b3f9f55 | Trac #29345: add Dima's SPKG patches for ksh compatibility. |
oops, needs another rebase, over the beta0 that just came out.
Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:
bc3ea8e | Trac #29345: fix one more newline-constant bashism in SAGE_SPKG_ENABLE. |
fb475f3 | Trac #29345: replace a few obsolete "-a" tests with "-e". |
305d8cf | Trac #29345: replace a bash array with something portable. |
d0dff56 | Trac #29345: replace a few uses of "source" with "." |
5ac420b | Trac #29345: fix some bashisms in sage-env's resolvelinks() function. |
0a61795 | Trac #29345: don't force SHELL=bash any longer. |
5db5318 | Trac #29345: remove "break" statements from AC_SEARCH_LIBS. |
e810ad1 | Trac #29345: don't use sage's config.status for the lrcalc build. |
93c9921 | Trac #29345: replace the function that populates the CVXOPT_* variables. |
0e66a0a | Trac #29345: add Dima's SPKG patches for ksh compatibility. |
Done again.
lgtm
Reviewer: Dima Pasechnik
Description changed:
---
+++
@@ -29,3 +29,4 @@
though, because those are inserted verbatim into the Makefile as rules, and the newlines probably matter.
+rebased over 9.2.beta1 - making this critical, as we're approaching a rebase hell state with this ticket not merged for 4 weeks.
Last 10 new commits:
bbf5265 | Trac #29345: fix one more newline-constant bashism in SAGE_SPKG_ENABLE. |
beccf24 | Trac #29345: replace a few obsolete "-a" tests with "-e". |
ff63210 | Trac #29345: replace a bash array with something portable. |
bb69c7e | Trac #29345: replace a few uses of "source" with "." |
c549738 | Trac #29345: fix some bashisms in sage-env's resolvelinks() function. |
b445b6e | Trac #29345: don't force SHELL=bash any longer. |
18dd124 | Trac #29345: remove "break" statements from AC_SEARCH_LIBS. |
6a5d73f | Trac #29345: don't use sage's config.status for the lrcalc build. |
3d8ba26 | Trac #29345: replace the function that populates the CVXOPT_* variables. |
604657a | Trac #29345: add Dima's SPKG patches for ksh compatibility. |
Changed branch from u/mjo/ticket/29345 to public/build/29345rebased
Changed branch from public/build/29345rebased to u/mjo/ticket/29345
Changed branch from u/mjo/ticket/29345 to 0e66a0a