#23451 closed defect (fixed)

compiling linbox fails when /bin/sh is a recent dash

Reported by: tornaria Owned by:
Priority: critical Milestone: sage-8.0
Component: build: configure Keywords:
Cc: jdemeyer Merged in:
Authors: Gonzalo Tornaría Reviewers: Dima Pasechnik
Report Upstream: N/A Work issues:
Branch: 0446feb (Commits) Commit: 0446feb04e8ce222a35016cbdde7bcdd45e34526
Dependencies: #24491 Stopgaps:

Description

(see #23448 for a different issue when /bin/sh is a recent dash)

Both fflas-ffpack and linbox packages seem to have issues.

  • fflas-ffpack: this one doesn't give an error message, but in fact it installs a broken version of local/include/fflas-ffpack/config.h which produces a failure later on when compiling linbox
  • linbox: creates a broken version of linbox/config.h and then the compilation fails

The symptom in both cases is that spurious characters (A and B) appear in the files fflas-ffpack/config.h and linbox/config.h, thus causing errors in the compiler.

Sample line:

#define __FFLASFFPACK_^A ^B 

The root cause seems to be bad escaping of substitutions in sed (i.e. \1, \2, etc) which instead of substituting the captured text they become a literal \1 (A) or \2 (B), etc.

Since sage requires bash anyway, the easiest fix seems to be to force CONFIG_SHELL=bash when running configure for those two packages.

Change History (19)

comment:1 Changed 22 months ago by tornaria

  • Branch set to u/tornaria/23451
  • Cc jdemeyer added
  • Commit set to bcb666372def5d110367a5e4e329411fa27c75b9
  • Status changed from new to needs_review

New commits:

60ac900(#23448) fix configure when run with dash
e77704b(#23448) fix configure when run with old dash
bcb6663Trac #23451: fix linbox when /bin/sh is a recent dash

comment:2 Changed 22 months ago by tornaria

I wrote the patch on top of #23448 since without that one compilation fails earlier anyway, but they are independent.

comment:3 follow-up: Changed 22 months ago by jdemeyer

  • Status changed from needs_review to needs_work

I have no idea what CONFIG_SHELL does, but wouldn't it be simpler to just run bash configure?

Also: author name on Trac

comment:4 Changed 22 months ago by jdemeyer

And I don't understand the changes to configure.ac.

comment:5 follow-up: Changed 22 months ago by jdemeyer

I guess the changes to configure.ac belong to #23448 and should be removed here.

comment:6 Changed 22 months ago by tornaria

  • Dependencies set to #23448

comment:7 in reply to: ↑ 3 Changed 22 months ago by tornaria

Replying to jdemeyer:

I have no idea what CONFIG_SHELL does, but wouldn't it be simpler to just run bash configure?

I think either way is ok.

One advantage of using the environment variable is: we could actually set CONFIG_SHELL globally so that the configure scripts for all packages are run using bash (maybe as provided by the user).

As a matter of fact, for my particular set up those two were the only two packages that needed to be run under bash.

comment:8 in reply to: ↑ 5 ; follow-up: Changed 22 months ago by tornaria

Replying to jdemeyer:

I guess the changes to configure.ac belong to #23448 and should be removed here.

I don't know how to remove them from here. The first two commits are the same as the ones in #23448. The third commit is the only relevant one here.

comment:9 Changed 22 months ago by tornaria

  • Authors set to tornaria
  • Status changed from needs_work to needs_review

comment:10 Changed 22 months ago by jdemeyer

  • Status changed from needs_review to needs_info

Author name

comment:11 in reply to: ↑ 8 Changed 22 months ago by jdemeyer

Replying to tornaria:

I don't know how to remove them from here. The first two commits are the same as the ones in #23448. The third commit is the only relevant one here.

Thanks for the info, but there is no way that I could have figured it out without you telling me. You don't need to change anything, it is good like this.

comment:12 Changed 22 months ago by tornaria

  • Authors changed from tornaria to Gonzalo Tornaría
  • Branch changed from u/tornaria/23451 to u/tornaria/23451-v2
  • Commit changed from bcb666372def5d110367a5e4e329411fa27c75b9 to 0446feb04e8ce222a35016cbdde7bcdd45e34526
  • Dependencies #23448 deleted
  • Status changed from needs_info to needs_review

I rewrote the commit to use command -v bash instead of just bash (see ticket:23448#comment:18).


Final commit for this ticket, on branch u/tornaria/23451-v2:

0446febTrac #23451: fix linbox configure when run with dash
Last edited 22 months ago by tornaria (previous) (diff)

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

I still feel like this is over-complicating things. Just run bash configure instead of ./configure.

comment:14 Changed 21 months ago by tornaria

I agree using autoconf is over-complicating things, but that was not my choice.

In this case your suggestion is probably equivalent except what I mentioned in comment:7.

comment:15 follow-ups: Changed 17 months ago by dimpase

Does this do the right thing with the sdh_* scripts, or was it only tested before the moment they came in?

comment:16 in reply to: ↑ 15 Changed 17 months ago by jdemeyer

Replying to dimpase:

Does this do the right thing with the sdh_* scripts

Good point, see #24491.

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

  • Dependencies set to #24491
  • Reviewers set to Dima Pasechnik
  • Status changed from needs_review to positive_review

Replying to jdemeyer:

I still feel like this is over-complicating things. Just run bash configure instead of ./configure.

It seems that you have taken this back on #24491. Thus, positive review.

comment:18 in reply to: ↑ 15 Changed 17 months ago by tornaria

Replying to dimpase:

Does this do the right thing with the sdh_* scripts, or was it only tested before the moment they came in?

I didn't test with the sdh_* scripts, but I would expect

export CONFIG_SHELL=whatever
./configure --prefix="$SAGE_LOCAL" --libdir="$SAGE_LOCAL/lib" blah

to be equivalent to

export CONFIG_SHELL=whatever
sdh_configure blah

as the "export" makes sure the variable is recursively inherited by subshells.

comment:19 Changed 16 months ago by vbraun

  • Branch changed from u/tornaria/23451-v2 to 0446feb04e8ce222a35016cbdde7bcdd45e34526
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.