Opened 5 years ago

Closed 4 years ago

#18428 closed defect (fixed)

Error checking in sage-spkg

Reported by: vbraun Owned by:
Priority: major Milestone: sage-6.8
Component: build Keywords:
Cc: Merged in:
Authors: Volker Braun Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: 69a501d (Commits) Commit: 69a501d5d4d9c26188c2e19757d091c7f7b5c729
Dependencies: Stopgaps:

Description (last modified by vbraun)

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

Change History (24)

comment:1 Changed 5 years ago by vbraun

  • Branch set to u/vbraun/error_checking_in_sage_spkg

comment:2 Changed 5 years ago by vbraun

  • Authors set to Volker Braun
  • Commit set to aa24229765773f685f943943a768736da9e999a9
  • Component changed from PLEASE CHANGE to build
  • Description modified (diff)
  • Status changed from new to needs_review
  • Type changed from PLEASE CHANGE to defect

New commits:

7c65f3dTry our own tarball host last
d5bbfc0Merge #18417 into #18428
e280943remove checksumming from sage-spkg
aa24229Do not ignor errors in sage-spkg

comment:3 Changed 5 years ago by git

  • Commit changed from aa24229765773f685f943943a768736da9e999a9 to 49aa3c5ddb0df89865383f84ab8ffa12a5a05551

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

49aa3c5Only download mirror list when needed

comment:4 Changed 5 years ago by git

  • Commit changed from 49aa3c5ddb0df89865383f84ab8ffa12a5a05551 to fd0b5ec2bb7b0a4a4713769c8e505a83a1d030a0

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

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

comment:5 Changed 5 years ago by jdemeyer

  • Status changed from needs_review to needs_work

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"

comment:6 Changed 5 years ago by jdemeyer

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

comment:7 Changed 5 years ago by jdemeyer

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

if [ $INFO -eq 0 ]; then

branches.

comment:8 Changed 5 years ago by git

  • Commit changed from fd0b5ec2bb7b0a4a4713769c8e505a83a1d030a0 to c8c260165d07b8a07d61698241f705e9f4a53fcd

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

c8c2601remove extraneous error check

comment:9 follow-up: Changed 5 years ago by 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. 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.

comment:10 in reply to: ↑ 9 Changed 5 years ago by jdemeyer

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.

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

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.

comment:12 Changed 5 years ago by jdemeyer

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

comment:13 Changed 5 years ago by vbraun

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

comment:14 in reply to: ↑ 11 Changed 5 years ago by jdemeyer

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.

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

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

comment:16 in reply to: ↑ 15 Changed 5 years ago by jdemeyer

Replying to vbraun:

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

Right indeed.

comment:17 Changed 4 years ago by jdemeyer

  • Branch changed from u/vbraun/error_checking_in_sage_spkg to u/jdemeyer/error_checking_in_sage_spkg

comment:18 Changed 4 years ago by git

  • Commit changed from c8c260165d07b8a07d61698241f705e9f4a53fcd to a5e492d50435e21d6525be194a0c7e89435ee15b

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

a5e492dRemove buffering fix

comment:19 Changed 4 years ago by git

  • Commit changed from a5e492d50435e21d6525be194a0c7e89435ee15b to 616ee13ebf90ae0bf7a48398bd4951b3a0e26620

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

616ee13Flush printed output

comment:20 Changed 4 years ago by git

  • Commit changed from 616ee13ebf90ae0bf7a48398bd4951b3a0e26620 to 69a501d5d4d9c26188c2e19757d091c7f7b5c729

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

69a501dMinor fixes to sage-spkg

comment:21 Changed 4 years ago by jdemeyer

  • Milestone changed from sage-6.7 to sage-6.8
  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_work to needs_review

comment:22 Changed 4 years ago by jdemeyer

needs review...

comment:23 Changed 4 years ago by vbraun

  • Status changed from needs_review to positive_review

comment:24 Changed 4 years ago by vbraun

  • Branch changed from u/jdemeyer/error_checking_in_sage_spkg to 69a501d5d4d9c26188c2e19757d091c7f7b5c729
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.