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

[embray]

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.

Depends on #24092

Component: build

Author: Erik Bray

Branch/Commit: 6f7ed49

Reviewer: Jeroen Demeyer

Issue created by migration from https://trac.sagemath.org/ticket/24017

[embray]

comment:2

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

[sagetrac-git]

Changed commit from be09dfc to 18e1757

[sagetrac-git]

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

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

[embray]

comment:4

Cleaned up.

[embray]

Dependencies: #24014

[embray]

comment:5

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

[sagetrac-git]

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

4b87c10	Get rid of the $PIP_INSTALL environment variable and replace it with a
56ad241	Add some docs for this function
e5bebd1	Keep PIP_INSTALL for now but quietly mark it as deprecated.
c69dfda	Add error handling and SAGE_SUDO support for sdh_pip_install
9af9c98	Adds a handy sdh_die helper; rewrites existing helpers to use it
3cd5b99	Allow sdh_die, if run without arguments, to read an error message from stdin.
cf67bdd	Nicer: Automatically format the error message to the terminal width (or 80 chars by default)
56de46e	With fmt, preserve existing line breaks, just wrap long lines
caf2698	Use sdh_die for sdh_check_vars as well

[sagetrac-git]

Changed commit from 18e1757 to caf2698

[jdemeyer]

comment:8

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.

[jdemeyer]

Changed dependencies from #24014 to none

[embray]

comment:10

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.

[sagetrac-git]

Changed commit from caf2698 to c00425b

[sagetrac-git]

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

50c1259	Error message itself should also be output to stderr!
c00425b	Fix typo in comment; improve documentation.

[jdemeyer]

comment:12

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.

[embray]

comment:13

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.

[jdemeyer]

comment:14

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

[embray]

comment:15

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.

[sagetrac-git]

Changed commit from c00425b to 43e618a

[sagetrac-git]

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

43e618a

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

[jdemeyer]

comment:17

In

if [ $ret -ne 0 ]; then

did you mean

if [ $ret -eq 0 ]; then

[embray]

comment:18

Replying to @jdemeyer:

In

if [ $ret -ne 0 ]; then

did you mean

if [ $ret -eq 0 ]; then

Yes, thank you for catching that.

[sagetrac-git]

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

883df11

Use the correct comparison here

[sagetrac-git]

Changed commit from 43e618a to 883df11

[embray]

comment:20

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

[jdemeyer]

comment:21

Replying to @embray:

Under src/sage/tests, maybe?

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

[embray]

comment:22

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

[jdemeyer]

comment:23

Works for me.

[jdemeyer]

Reviewer: Jeroen Demeyer

[embray]

comment:24

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

[sagetrac-git]

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

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

[sagetrac-git]

Changed commit from 883df11 to 6f7ed49

[embray]

Dependencies: #24092

[embray]

comment:26

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

[jdemeyer]

comment:27

Replying to @embray:

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

Does the patchbot even look at the dependencies?

[embray]

comment:28

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

[vbraun]

Changed branch from u/embray/build/sdh_die to 6f7ed49