Opened 4 years ago
Closed 4 years ago
#18428 closed defect (fixed)
Error checking in sagespkg
Reported by:  vbraun  Owned by:  

Priority:  major  Milestone:  sage6.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 )
 Remove duplicate checksumming
 Do not silently ignore errors in sagespkg.
Change History (24)
comment:1 Changed 4 years ago by
 Branch set to u/vbraun/error_checking_in_sage_spkg
comment:2 Changed 4 years ago by
 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
comment:3 Changed 4 years ago by
 Commit changed from aa24229765773f685f943943a768736da9e999a9 to 49aa3c5ddb0df89865383f84ab8ffa12a5a05551
Branch pushed to git repo; I updated commit sha1. New commits:
49aa3c5  Only download mirror list when needed

comment:4 Changed 4 years ago by
 Commit changed from 49aa3c5ddb0df89865383f84ab8ffa12a5a05551 to fd0b5ec2bb7b0a4a4713769c8e505a83a1d030a0
Branch pushed to git repo; I updated commit sha1. New commits:
fd0b5ec  sagespkg has to ignore some errors when building from scratch

comment:5 Changed 4 years ago by
 Status changed from needs_review to needs_work
Surely, this is wrong:
sagedownloadfile "$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 4 years ago by
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 4 years ago by
And I also don't understand why you remove some of the
if [ $INFO eq 0 ]; then
branches.
comment:8 Changed 4 years ago by
 Commit changed from fd0b5ec2bb7b0a4a4713769c8e505a83a1d030a0 to c8c260165d07b8a07d61698241f705e9f4a53fcd
Branch pushed to git repo; I updated commit sha1. New commits:
c8c2601  remove extraneous error check

comment:9 followup: ↓ 10 Changed 4 years ago by
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 sagedownload file. The second INFO branch is just printing logs, and I moved that to the relevant place.
comment:10 in reply to: ↑ 9 Changed 4 years ago by
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 sagespkg
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 followup: ↓ 14 Changed 4 years ago by
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 sagedownloadfile 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 preinstallation 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 4 years ago by
Can we just move the buffering stuff to a new ticket and focus on the "more urgent issues first" then?
comment:13 Changed 4 years ago by
Sure, it just simplifies analyzing problems if the last log message is actually on the screen.
comment:14 in reply to: ↑ 11 Changed 4 years ago by
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 followup: ↓ 16 Changed 4 years ago by
But that is only used during the initial build, and not when running sage i <package>
, right?
comment:16 in reply to: ↑ 15 Changed 4 years ago by
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
 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
 Commit changed from c8c260165d07b8a07d61698241f705e9f4a53fcd to a5e492d50435e21d6525be194a0c7e89435ee15b
Branch pushed to git repo; I updated commit sha1. New commits:
a5e492d  Remove buffering fix

comment:19 Changed 4 years ago by
 Commit changed from a5e492d50435e21d6525be194a0c7e89435ee15b to 616ee13ebf90ae0bf7a48398bd4951b3a0e26620
Branch pushed to git repo; I updated commit sha1. New commits:
616ee13  Flush printed output

comment:20 Changed 4 years ago by
 Commit changed from 616ee13ebf90ae0bf7a48398bd4951b3a0e26620 to 69a501d5d4d9c26188c2e19757d091c7f7b5c729
Branch pushed to git repo; I updated commit sha1. New commits:
69a501d  Minor fixes to sagespkg

comment:21 Changed 4 years ago by
 Milestone changed from sage6.7 to sage6.8
 Reviewers set to Jeroen Demeyer
 Status changed from needs_work to needs_review
comment:22 Changed 4 years ago by
needs review...
comment:23 Changed 4 years ago by
 Status changed from needs_review to positive_review
comment:24 Changed 4 years ago by
 Branch changed from u/jdemeyer/error_checking_in_sage_spkg to 69a501d5d4d9c26188c2e19757d091c7f7b5c729
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
Try our own tarball host last
Merge #18417 into #18428
remove checksumming from sagespkg
Do not ignor errors in sagespkg