Opened 4 years ago

Closed 4 years ago

#24017 closed enhancement (fixed)

Add sdh_die helper function--report an error message and exit

Reported by: embray Owned by:
Priority: major Milestone: sage-8.1
Component: build Keywords:
Cc: Merged in:
Authors: Erik Bray Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: 6f7ed49 (Commits, GitHub, GitLab) Commit: 6f7ed494ab39f2c726a4fe2808a9696a4ac40f01
Dependencies: #24092 Stopgaps:

Status badges

Description

As brought up in #23160, a lot of the spkg-install scripts define a helper function like this, so this adds one that they can share.

In many cases this won't even be needed because error messaging is already handled in the sdh_make helpers and the like. But occasionally it's still useful. This can be used like:

command || sdh_die "Error message"

much like the die command in perl.

Change History (29)

comment:1 Changed 4 years ago by embray

  • Status changed from new to needs_review

comment:2 Changed 4 years ago by embray

  • Status changed from needs_review to needs_work

Oops--accidentally pulled in some unrelated changes from #22509.

comment:3 Changed 4 years ago by git

  • Commit changed from be09dfcfe8cd8969823b3cb924ba859ae96c0514 to 18e1757cb5b875c9488dd9fe42ee72ed9d90e8a3

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

9da13d1Adds a handy sdh_die helper; rewrites existing helpers to use it
8f6ff3bAllow sdh_die, if run without arguments, to read an error message from stdin.
9085389Nicer: Automatically format the error message to the terminal width (or 80 chars by default)
f375a83With fmt, preserve existing line breaks, just wrap long lines
18e1757Use sdh_die for sdh_check_vars as well

comment:4 Changed 4 years ago by embray

  • Status changed from needs_work to needs_review

Cleaned up.

comment:5 Changed 4 years ago by embray

  • Dependencies set to #24014
  • Status changed from needs_review to needs_work

This will conflict with #24014, so better to add that as a dependency and make the required changes here to make them compatible.

comment:6 Changed 4 years ago by git

  • Commit changed from 18e1757cb5b875c9488dd9fe42ee72ed9d90e8a3 to caf2698decb2f44521922f9f04a6d494877f4be7

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

4b87c10Get rid of the $PIP_INSTALL environment variable and replace it with a
56ad241Add some docs for this function
e5bebd1Keep PIP_INSTALL for now but quietly mark it as deprecated.
c69dfdaAdd error handling and SAGE_SUDO support for sdh_pip_install
9af9c98Adds a handy sdh_die helper; rewrites existing helpers to use it
3cd5b99Allow sdh_die, if run without arguments, to read an error message from stdin.
cf67bddNicer: Automatically format the error message to the terminal width (or 80 chars by default)
56de46eWith fmt, preserve existing line breaks, just wrap long lines
caf2698Use sdh_die for sdh_check_vars as well

comment:7 Changed 4 years ago by embray

  • Status changed from needs_work to needs_review

comment:8 follow-up: Changed 4 years ago by jdemeyer

  • Status changed from needs_review to needs_work

The $(_sdh_cols) thing seems over-engineered to me. In particular, I have doubts about

echo "$msg" | fmt -s -w $(_sdh_cols)

because it means that we have to worry about the portability of the fmt program. I think it would be much better to just echo the message as-is. Also, there is a typo in a comment: 90 should be 80.

I like your sdh_die << _EOF_ trick but then that should be documented too at the top of sage-dist-helpers.

comment:9 Changed 4 years ago by jdemeyer

  • Dependencies #24014 deleted

comment:10 in reply to: ↑ 8 ; follow-up: Changed 4 years ago by embray

Replying to jdemeyer:

The $(_sdh_cols) thing seems over-engineered to me.

That's fair in general :)

In particular, I have doubts about

echo "$msg" | fmt -s -w $(_sdh_cols)

because it means that we have to worry about the portability of the fmt program.

fmt is actually very portable, moreso even than grep. It's a sadly little-known, but very widely installed program and hasn't changed much over the ages. See for yourself--it's on OSX, openbsd, freebsd, pretty much any linux, and it's the same everywhere.

I'm not married to having it here, but I think the end result is nicer-looking error messages almost everywhere.

I think it would be much better to just echo the message as-is. Also, there is a typo in a comment: 90 should be 80.

I like your sdh_die << _EOF_ trick but then that should be documented too at the top of sage-dist-helpers.

Oh you're right--I thought i did write docs about that but apparently not.

comment:11 Changed 4 years ago by git

  • Commit changed from caf2698decb2f44521922f9f04a6d494877f4be7 to c00425b6281a137c90c4ea67de5b95a8d0ee2b11

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

50c1259Error message itself should also be output to stderr!
c00425bFix typo in comment; improve documentation.

comment:12 in reply to: ↑ 10 Changed 4 years ago by jdemeyer

Replying to embray:

fmt is actually very portable, moreso even than grep. It's a sadly little-known, but very widely installed program and hasn't changed much over the ages. See for yourself--it's on OSX, openbsd, freebsd, pretty much any linux, and it's the same everywhere.

Fair enough. I give you the benefit of the doubt. It still feels over-engineered to me, but I won't block this ticket because of it.

Remember to set this back to needs_review when you are done.

comment:13 Changed 4 years ago by embray

  • Status changed from needs_work to needs_review

I don't disagree with the "over-engineered" assessment--there's more going on there than is needed for the bare-minimum functionality. But let's try it out for a bit. I think it is a nice little "quality of life" touch.

comment:14 Changed 4 years ago by jdemeyer

Concerning

If the last command run exited with an error,

Why this condition? It means that you couldn't do something like

if some_condition; then
    sdh_die "bla bla bla"
fi

comment:15 Changed 4 years ago by embray

This could be written something like

[ some_condition ] || sdh_die ...

though I realize in your example some_condition need not be the test command either.

Hmm--I think the original logic was that it was a failsafe of sorts; it would not "die" unless the previous command actually exited with an error. For example, although it's typically used with ||, if one ran command; sdh_die ... it would only "die" if the command returned a non-zero exit status.

But maybe sometimes you'd also want to "die" if some command succeeded. So I think ultimately this restriction is not well-justified. I'll remove it.

comment:16 Changed 4 years ago by git

  • Commit changed from c00425b6281a137c90c4ea67de5b95a8d0ee2b11 to 43e618a8cbd41366d457674cee37e4ba2b989b5b

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

43e618adifferent logic for handling error codes: sdh_die always returns non-zero, but if the previous exit

comment:17 follow-up: Changed 4 years ago by jdemeyer

In

if [ $ret -ne 0 ]; then

did you mean

if [ $ret -eq 0 ]; then

comment:18 in reply to: ↑ 17 Changed 4 years ago by embray

Replying to jdemeyer:

In

if [ $ret -ne 0 ]; then

did you mean

if [ $ret -eq 0 ]; then

Yes, thank you for catching that.

comment:19 Changed 4 years ago by git

  • Commit changed from 43e618a8cbd41366d457674cee37e4ba2b989b5b to 883df1141b64322b57f9b1393d455c2908fe76c3

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

883df11Use the correct comparison here

comment:20 follow-up: Changed 4 years ago by embray

Is there a good place to put miscellaneous tests that aren't for an actual module in the sage package? Under src/sage/tests, maybe?

I would like to add a test suite for these functions (in a separate ticket).

comment:21 in reply to: ↑ 20 Changed 4 years ago by jdemeyer

Replying to embray:

Under src/sage/tests, maybe?

Since it's about building Sage, it would better be in src/sage_setup.

comment:22 Changed 4 years ago by embray

There's not a sage_setup.tests package yet to speak of, but I guess there could/should be.

comment:23 Changed 4 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_review to positive_review

Works for me.

comment:24 Changed 4 years ago by embray

  • Status changed from positive_review to needs_work

FWIW the recent patchbot failure is due to the issue fixed by #24092, though it might be good the make that a dependency of this...

comment:25 Changed 4 years ago by git

  • Commit changed from 883df1141b64322b57f9b1393d455c2908fe76c3 to 6f7ed494ab39f2c726a4fe2808a9696a4ac40f01

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

6dd69fdAdd pip to brial's dependencies, replace the use of PIP_INSTALL as per #24014.
6f7ed49Merge branch 'u/fbissey/brial_pip' into u/embray/build/sdh_die

comment:26 follow-up: Changed 4 years ago by embray

  • Dependencies set to #24092
  • Status changed from needs_work to positive_review

Added #24092 as a dependency in the hopes of getting better patchbot feedback.

comment:27 in reply to: ↑ 26 Changed 4 years ago by jdemeyer

Replying to embray:

Added #24092 as a dependency in the hopes of getting better patchbot feedback.

Does the patchbot even look at the dependencies?

comment:28 Changed 4 years ago by embray

I don't think so, but it still makes sense to track doesn't it?

(I am also trying to update the trac plugin to better diff branches against their dependencies, but that's a moot point for now...)

comment:29 Changed 4 years ago by vbraun

  • Branch changed from u/embray/build/sdh_die to 6f7ed494ab39f2c726a4fe2808a9696a4ac40f01
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.