Error checking in sage-spkg

[vbraun]

• Remove duplicate checksumming
• Do not silently ignore errors in sage-spkg.

Component: build

Author: Volker Braun

Branch/Commit: 69a501d

Reviewer: Jeroen Demeyer

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

[vbraun]

Branch: u/vbraun/error_checking_in_sage_spkg

[vbraun]

Author: Volker Braun

[vbraun]

Commit: aa24229

[vbraun]

New commits:

7c65f3d	Try our own tarball host last
d5bbfc0	Merge #18417 into #18428
e280943	remove checksumming from sage-spkg
aa24229	Do not ignor errors in sage-spkg

[sagetrac-git]

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

49aa3c5

Only download mirror list when needed

[sagetrac-git]

Changed commit from aa24229 to 49aa3c5

[sagetrac-git]

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

fd0b5ec

sage-spkg has to ignore some errors when building from scratch

[sagetrac-git]

Changed commit from 49aa3c5 to fd0b5ec

[jdemeyer]

comment:5

Surely, this is wrong:

sage-download-file "$PKG_URL" "$PKG_TMP"
if [ $? -ne 0 ]; then
    exit 1
fi
if [ $? -ne 0 ]; then
    # Delete failed download
    rm -f "$PKG_TMP"

[jdemeyer]

comment:6

Why this?

export PYTHONUNBUFFERED=yes

Why is it a problem if output is buffered?

(I am a bit worried about performance implications, since this is passed to every Python script run during the installation of Sage).

[jdemeyer]

comment:7

And I also don't understand why you remove some of the

if [ $INFO -eq 0 ]; then

branches.

[sagetrac-git]

Changed commit from fd0b5ec to c8c2601

[sagetrac-git]

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

c8c2601

remove extraneous error check

[vbraun]

comment:9

The problem with buffering is that network connections might take a while, and the last message on the screen should be flushed to indicate what we are doing. PYTHONUNBUFFERED only makes Python stdin/out unbuffered, and we don't print() that much. So I don't see where there could be a performance impact. Even in the worst case (passing the entire build log through Python unbuffered, which we don't do) its unlikely to take a long time compared to actually producing the build log.

The first INFO branch is about checking whether the tarball is already downloaded, which is now done by sage-download file. The second INFO branch is just printing logs, and I moved that to the relevant place.

[jdemeyer]

comment:10

Replying to @vbraun:

The problem with buffering is that network connections might take a while, and the last message on the screen should be flushed to indicate what we are doing.

Of course, that can be done with sys.stdout.flush() also...

Regarding this, I also don't think that sage-spkg is the best place to put this. I would rather like to put it in build/deps, so it also applies to building the Sage library.

[vbraun]

comment:11

Not sure I follow, build/deps is a Makefile but just setting a make variable is not good enough. You mean use the "export" gnu make extension to change the environment?

Probably the best solution would be to subclass the stdout stream to a version that always flushes. But sage-download-file is already too many loc. I'm thinking to refactor it into a python package (say, src/sage_bootstrap) that deals with downloading and potentially other pre-installation stuff. That then could also be tested nicely with tox to see that it runs on different Python versions. But we should probably fix more urgent issues first.

[jdemeyer]

comment:12

Can we just move the buffering stuff to a new ticket and focus on the "more urgent issues first" then?

[vbraun]

comment:13

Sure, it just simplifies analyzing problems if the last log message is actually on the screen.

[jdemeyer]

comment:14

Replying to @vbraun:

You mean use the "export" gnu make extension to change the environment?

Well, I guess we are already assuming GNU make. Alternatively, put it in build/install.

[vbraun]

comment:15

But that is only used during the initial build, and not when running sage -i <package>, right?

[jdemeyer]

comment:16

Replying to @vbraun:

But that is only used during the initial build, and not when running sage -i <package>, right?

Right indeed.

[jdemeyer]

Changed branch from u/vbraun/error_checking_in_sage_spkg to u/jdemeyer/error_checking_in_sage_spkg

[sagetrac-git]

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

a5e492d

Remove buffering fix

[sagetrac-git]

Changed commit from c8c2601 to a5e492d

[sagetrac-git]

Changed commit from a5e492d to 616ee13

[sagetrac-git]

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

616ee13

Flush printed output

[sagetrac-git]

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

69a501d

Minor fixes to sage-spkg

[sagetrac-git]

Changed commit from 616ee13 to 69a501d

[jdemeyer]

Reviewer: Jeroen Demeyer

[jdemeyer]

comment:22

needs review...

[vbraun]

Changed branch from u/jdemeyer/error_checking_in_sage_spkg to 69a501d