Opened 5 years ago

Closed 5 years ago

#21102 closed defect (fixed)

Top-level 'configure' doesn't properly exit upon errors in build/pkgs/, instead breaks the build

Reported by: leif Owned by:
Priority: blocker Milestone: sage-7.3
Component: build: configure Keywords: configure package type missing build error
Cc: jdemeyer Merged in:
Authors: Leif Leonhardy Reviewers: Volker Braun
Report Upstream: N/A Work issues:
Branch: 84c7b76 (Commits, GitHub, GitLab) Commit: 84c7b76c7a6a55eef0cdf451b6bbf8e860e40a3a
Dependencies: Stopgaps:

Status badges

Description

Which of the following exits your shell?

exit 1 | exit 0

exit 0 | exit 1

exit 1 | exit 1

exit 0 | exit 0

(The answer is "none of them".)


See this long thread on sage-devel where we tracked down an obscure build error to this bug in configure.

Change History (11)

comment:1 Changed 5 years ago by leif

  • Status changed from new to needs_review
  • Summary changed from Sage's top-level 'configure' doesn't properly exit on errors, instead breaks the build to Top-level 'configure' doesn't properly exit upon errors in build/pkgs/, instead breaks the build

comment:2 Changed 5 years ago by git

  • Commit changed from 01e99ffb71b2bcbbacfb59d5b3e669029d83b271 to 84c7b76c7a6a55eef0cdf451b6bbf8e860e40a3a

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

84c7b76Sage's top-level configure[.ac]: Really exit the script upon errors (in filtered_packages_list())

comment:3 Changed 5 years ago by leif

P.S.: I don't think we should ignore empty folders in build/pkgs/; if someone disagrees, we can change that on a follow-up ticket.

comment:4 Changed 5 years ago by leif

To test this, do for example:

mkdir build/pkgs/broken_package
./configure && echo Apparently no error.
echo broken > build/pkgs/broken_package/type
./configure && echo Apparently no error.

Optionally take a look at build/make/Makefile...

Then pull the branch here, run autoconf, and retry the above (perhaps first after having done rm -r build/pkgs/broken_package).

Last edited 5 years ago by leif (previous) (diff)

comment:5 follow-up: Changed 5 years ago by vbraun

  • Cc jdemeyer added
  • Status changed from needs_review to positive_review

Lets just say that exception handling isn't bash's strong suit...

IMHO the entire autogenerating-Makefile should be converted to Python and moved into sage_bootstrap for a) error checking sanity and b) testability and c) having a single point of truth for if and when a directory is a package. But not on this ticket...

comment:6 Changed 5 years ago by vbraun

  • Reviewers set to Volker Braun

comment:7 in reply to: ↑ 5 ; follow-up: Changed 5 years ago by leif

Replying to vbraun:

Lets just say that exception handling isn't bash's strong suit...

Well, that's standard(ized) (Bourne) shell behavior.


IMHO the entire autogenerating-Makefile should be converted to Python and moved into sage_bootstrap for a) error checking sanity and b) testability and c) having a single point of truth for if and when a directory is a package. But not on this ticket...

Even if we keep it in configure[.ac], the shell functions could be improved (e.g. not rereading the folders and files five times). But we could of course use Python there as well, where it (creating the Makefile) IMHO belongs.

Note that while we meanwhile require a system python, AFAICS there's not a single test for python in configureyet.

comment:8 in reply to: ↑ 7 Changed 5 years ago by leif

Replying to leif:

Replying to vbraun:

Lets just say that exception handling isn't bash's strong suit...

Well, that's standard(ized) (Bourne) shell behavior.

P.S.: You have similar problems in functional languages or attribute grammars, without global variables (Pfui anyway) and without passing pointers or references for results... ;-)

So the shell's behavoir appears to be good, the problem being that not everybody's aware of when subshells (with their own environment) are invoked.

comment:9 Changed 5 years ago by leif

Opinions on configure dumping a list of all packages (one per line!) near the end?

I think this makes it even more unlikely people will notice warnings etc. from configure.

comment:10 Changed 5 years ago by leif

I've opened a follow-up for a couple of improvements to configure.

Not intended for rewriting it in Python though. ;-)

(I'd prefer you doing that on a separate ticket.)

comment:11 Changed 5 years ago by vbraun

  • Branch changed from u/leif/really_exit_configure_on_errors to 84c7b76c7a6a55eef0cdf451b6bbf8e860e40a3a
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.