Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#29345 closed enhancement (fixed)

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

Reported by: Michael Orlitzky Owned by:
Priority: critical Milestone: sage-9.2
Component: build: configure Keywords:
Cc: Dima Pasechnik, Matthias Köppe, Erik Bray, Volker Braun Merged in:
Authors: Michael Orlitzky Reviewers: Dima Pasechnik
Report Upstream: Fixed upstream, in a later stable release. Work issues:
Branch: 0e66a0a (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by Matthias Köppe)

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.

Change History (53)

comment:1 Changed 3 years ago by Matthias Köppe

I agree in general - do you actually run this with a CONFIG_SHELL other than bash? Is the other configure code clean?

comment:2 Changed 3 years ago by Matthias Köppe

This should probably be done on top of #29287, which touches the same file

comment:3 Changed 3 years ago by Michael Orlitzky

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.

comment:4 Changed 3 years ago by Matthias Köppe

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.

comment:5 Changed 3 years ago by Matthias Köppe

Milestone: sage-9.1sage-9.2

comment:6 Changed 3 years ago by Michael Orlitzky

Authors: Michael Orlitzky
Branch: u/mjo/ticket/29345
Commit: 4a09db546ace687c0e466b2cd5e8b6fef8a52cee
Status: newneeds_review

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.

comment:7 Changed 3 years ago by git

Commit: 4a09db546ace687c0e466b2cd5e8b6fef8a52cee489d8ccd27bd03b4e3771062b042de6f7092e5df

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

26bf7afTrac #29345: replace a few obsolete "-a" tests with "-e".
fea7ad9Trac #29345: replace a bash array with something portable.
4f0a56aTrac #29345: replace a few uses of "source" with "."
42a63fdTrac #29345: fix some bashisms in sage-env's resolvelinks() function.
489d8ccTrac #29345: don't force SHELL=bash any longer.

comment:8 Changed 3 years ago by Matthias Köppe

Nice on first glance. I'll be happy to do a full review after 9.1 is out

comment:9 Changed 3 years ago by Dima Pasechnik

it's a bit in vain, as there are bashisms or just "bash required" in several Sage packages.

comment:10 Changed 3 years ago by Dima Pasechnik

comment:11 in reply to:  9 Changed 3 years ago by Michael Orlitzky

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.

comment:12 in reply to:  10 Changed 3 years ago by Michael Orlitzky

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.

comment:13 Changed 3 years ago by Dima Pasechnik

needs rebase.

comment:14 Changed 3 years ago by git

Commit: 489d8ccd27bd03b4e3771062b042de6f7092e5df279765f38a9ae291313055520d115c23ca4d5b9c

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

165d554Trac #29345: fix most autotools bashisms.
1b5dafbTrac #29345: fix one more newline-constant bashism in SAGE_SPKG_ENABLE.
c4a74d8Trac #29345: replace a few obsolete "-a" tests with "-e".
fcd9cefTrac #29345: replace a bash array with something portable.
839623aTrac #29345: replace a few uses of "source" with "."
0717ec4Trac #29345: fix some bashisms in sage-env's resolvelinks() function.
279765fTrac #29345: don't force SHELL=bash any longer.

comment:15 Changed 3 years ago by Michael Orlitzky

fixed, thanks

comment:16 Changed 3 years ago by Dima Pasechnik

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

comment:17 Changed 3 years ago by Michael Orlitzky

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

comment:18 Changed 3 years ago by Dima Pasechnik

ksh also notices a stray break and prints a warning, no error.

comment:19 Changed 3 years ago by Dima Pasechnik

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

comment:20 Changed 3 years ago by Dima Pasechnik

ditto (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  
\-------------------------------------------------------

comment:21 in reply to:  19 Changed 3 years ago by Michael Orlitzky

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 print not found etc. 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.

comment:22 Changed 3 years ago by git

Commit: 279765f38a9ae291313055520d115c23ca4d5b9c65ee1fb28d27aa8d31cee492b0fbfbb6a4e205e1

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

e0cd5d7Trac #29345: remove "break" statements from AC_SEARCH_LIBS.
65ee1fbTrac #29345: don't use sage's config.status for the lrcalc build.

comment:23 Changed 3 years ago by Michael Orlitzky

New commits fix the break statements and probably lrcalc.

comment:24 Changed 3 years ago by Dima Pasechnik

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.

comment:25 Changed 3 years ago by Dima Pasechnik

The following does the job to fix the latter

  • build/pkgs/cvxopt/spkg-install.in

    a b cd src 
    22
    33# Stolen from Gentoo
    44pkg_libs() {
    5        pkg-config --libs-only-l $* | \
     5       pkg-config --libs-only-l $* | sed -e 's/^ *//g' | \
    66               sed -e 's:[ ]-l*\(pthread\|m\)\([ ]\|$\)::g' -e 's:[ ]*$::' | \
    77               tr ' ' '\n' | sort -u | sed -e "s:^-l\(.*\):\1:g" | \
    88               tr '\n' ';' | sed -e 's:;$::'

comment:26 Changed 3 years ago by Dima Pasechnik

Report Upstream: N/AFixed upstream, in a later stable release.

comment:27 Changed 3 years ago by Michael Orlitzky

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 list of "-lfoo" flags as would be output
# by pkg-config, and 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#;}"
}
Version 0, edited 3 years ago by Michael Orlitzky (next)

comment:28 Changed 3 years ago by Dima Pasechnik

care to add the latter to the branch?

comment:29 Changed 3 years ago by Michael Orlitzky

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

comment:30 Changed 3 years ago by Dima Pasechnik

I opened #29677 to track OpenBSD quirks.

comment:31 Changed 3 years ago by Michael Orlitzky

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

comment:32 Changed 3 years ago by git

Commit: 65ee1fb28d27aa8d31cee492b0fbfbb6a4e205e121cbae89235a44ec83a062f8415c6fb6ae29adbc

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

21cbae8Trac #29345: replace the function that populates the CVXOPT_* variables.

comment:33 Changed 3 years ago by Michael Orlitzky

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.

comment:34 Changed 3 years ago by Dima Pasechnik

Is this ready for review? Just checking.

comment:35 Changed 3 years ago by git

Commit: 21cbae89235a44ec83a062f8415c6fb6ae29adbc00de6b33336d253703aac11866ced81431d1477d

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

e778266Trac #29345: fix one more newline-constant bashism in SAGE_SPKG_ENABLE.
c18eda8Trac #29345: replace a few obsolete "-a" tests with "-e".
2ba5fe9Trac #29345: replace a bash array with something portable.
f86546eTrac #29345: replace a few uses of "source" with "."
76311fdTrac #29345: fix some bashisms in sage-env's resolvelinks() function.
f72178bTrac #29345: don't force SHELL=bash any longer.
f040377Trac #29345: remove "break" statements from AC_SEARCH_LIBS.
ef2888eTrac #29345: don't use sage's config.status for the lrcalc build.
3e2d6f3Trac #29345: replace the function that populates the CVXOPT_* variables.
00de6b3Trac #29345: add Dima's SPKG patches for ksh compatibility.

comment:36 in reply to:  34 Changed 3 years ago by Michael Orlitzky

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.

comment:37 Changed 3 years ago by Matthias Köppe

OK to rebase this on top of #29098 (Merge build/make/deps into build/make/Makefile.in)?

comment:38 in reply to:  37 Changed 3 years ago by Michael Orlitzky

Replying to mkoeppe:

OK to rebase this on top of #29098 (Merge build/make/deps into build/make/Makefile.in)?

Sure. I'm running a full build/ptestlong today to ensure that my new sympow package works with #29673. If you don't beat me to it, I'll rebase on Monday most likely.

comment:39 Changed 3 years ago by git

Commit: 00de6b33336d253703aac11866ced81431d1477d2f5f383c2be1eec62b7c7ea72ae2de25c5d34cb8

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

11eba29Trac #29345: fix one more newline-constant bashism in SAGE_SPKG_ENABLE.
f6746ddTrac #29345: replace a few obsolete "-a" tests with "-e".
31a5a18Trac #29345: replace a bash array with something portable.
e0d0f6cTrac #29345: replace a few uses of "source" with "."
47ff65cTrac #29345: fix some bashisms in sage-env's resolvelinks() function.
59b8cccTrac #29345: don't force SHELL=bash any longer.
43a81c3Trac #29345: remove "break" statements from AC_SEARCH_LIBS.
76a931dTrac #29345: don't use sage's config.status for the lrcalc build.
e0d871aTrac #29345: replace the function that populates the CVXOPT_* variables.
2f5f383Trac #29345: add Dima's SPKG patches for ksh compatibility.

comment:40 Changed 3 years ago by Michael Orlitzky

Dependencies: 29098

comment:41 Changed 3 years ago by git

Commit: 2f5f383c2be1eec62b7c7ea72ae2de25c5d34cb8b3f9f5595c5443571cbe38dea141f9246d62f271

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

647e64cTrac #29345: fix one more newline-constant bashism in SAGE_SPKG_ENABLE.
4e0b8ddTrac #29345: replace a few obsolete "-a" tests with "-e".
955da4dTrac #29345: replace a bash array with something portable.
76bdb66Trac #29345: replace a few uses of "source" with "."
7f46faeTrac #29345: fix some bashisms in sage-env's resolvelinks() function.
791194aTrac #29345: don't force SHELL=bash any longer.
c22ef41Trac #29345: remove "break" statements from AC_SEARCH_LIBS.
20a1cdbTrac #29345: don't use sage's config.status for the lrcalc build.
7077febTrac #29345: replace the function that populates the CVXOPT_* variables.
b3f9f55Trac #29345: add Dima's SPKG patches for ksh compatibility.

comment:42 Changed 3 years ago by Michael Orlitzky

That last one is just a rebase onto the rebase in #29098.

comment:43 Changed 3 years ago by Dima Pasechnik

Dependencies: 29098#29098

comment:44 Changed 3 years ago by Dima Pasechnik

oops, needs another rebase, over the beta0 that just came out.

comment:45 Changed 3 years ago by git

Commit: b3f9f5595c5443571cbe38dea141f9246d62f2710e66a0abc00d5bf5ac1496e13f4d2f4ef7fe29dc

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

bc3ea8eTrac #29345: fix one more newline-constant bashism in SAGE_SPKG_ENABLE.
fb475f3Trac #29345: replace a few obsolete "-a" tests with "-e".
305d8cfTrac #29345: replace a bash array with something portable.
d0dff56Trac #29345: replace a few uses of "source" with "."
5ac420bTrac #29345: fix some bashisms in sage-env's resolvelinks() function.
0a61795Trac #29345: don't force SHELL=bash any longer.
5db5318Trac #29345: remove "break" statements from AC_SEARCH_LIBS.
e810ad1Trac #29345: don't use sage's config.status for the lrcalc build.
93c9921Trac #29345: replace the function that populates the CVXOPT_* variables.
0e66a0aTrac #29345: add Dima's SPKG patches for ksh compatibility.

comment:46 Changed 3 years ago by Michael Orlitzky

Dependencies: #29098

Done again.

comment:47 Changed 3 years ago by Dima Pasechnik

Reviewers: Dima Pasechnik
Status: needs_reviewpositive_review

lgtm

comment:48 Changed 2 years ago by Matthias Köppe

Description: modified (diff)
Summary: replace bashisms in m4/sage_spkg_collect.m4 and m4/sage_spkg_enable.m4replace 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

comment:49 Changed 2 years ago by Dima Pasechnik

Branch: u/mjo/ticket/29345public/build/29345rebased
Cc: Volker Braun added
Commit: 0e66a0abc00d5bf5ac1496e13f4d2f4ef7fe29dc604657ac876c0ab6c6ba96f151db03eec3e8ad2e
Priority: majorcritical

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:

bbf5265Trac #29345: fix one more newline-constant bashism in SAGE_SPKG_ENABLE.
beccf24Trac #29345: replace a few obsolete "-a" tests with "-e".
ff63210Trac #29345: replace a bash array with something portable.
bb69c7eTrac #29345: replace a few uses of "source" with "."
c549738Trac #29345: fix some bashisms in sage-env's resolvelinks() function.
b445b6eTrac #29345: don't force SHELL=bash any longer.
18dd124Trac #29345: remove "break" statements from AC_SEARCH_LIBS.
6a5d73fTrac #29345: don't use sage's config.status for the lrcalc build.
3d8ba26Trac #29345: replace the function that populates the CVXOPT_* variables.
604657aTrac #29345: add Dima's SPKG patches for ksh compatibility.

comment:50 Changed 2 years ago by Matthias Köppe

0e66a0a is already merged on Volker's branch.

comment:51 Changed 2 years ago by Matthias Köppe

Branch: public/build/29345rebasedu/mjo/ticket/29345
Commit: 604657ac876c0ab6c6ba96f151db03eec3e8ad2e0e66a0abc00d5bf5ac1496e13f4d2f4ef7fe29dc

comment:52 Changed 2 years ago by Volker Braun

Branch: u/mjo/ticket/293450e66a0abc00d5bf5ac1496e13f4d2f4ef7fe29dc
Resolution: fixed
Status: positive_reviewclosed

comment:53 in reply to:  23 Changed 2 years ago by Dima Pasechnik

Commit: 0e66a0abc00d5bf5ac1496e13f4d2f4ef7fe29dc

Replying to mjo:

New commits fix the break statements and probably lrcalc.

no, lrcalc on OpenBSD is still broken due to this print issue. Probably we really need to run a modern autoconf on it and replace the resulting scripts.

Note: See TracTickets for help on using tickets.