Opened 7 years ago
Closed 7 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, GitHub, GitLab) | Commit: | 69a501d5d4d9c26188c2e19757d091c7f7b5c729 |
Dependencies: | Stopgaps: |
Description (last modified by )
- Remove duplicate checksumming
- Do not silently ignore errors in sage-spkg.
Change History (24)
comment:1 Changed 7 years ago by
- Branch set to u/vbraun/error_checking_in_sage_spkg
comment:2 Changed 7 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 7 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 7 years ago by
- Commit changed from 49aa3c5ddb0df89865383f84ab8ffa12a5a05551 to fd0b5ec2bb7b0a4a4713769c8e505a83a1d030a0
Branch pushed to git repo; I updated commit sha1. New commits:
fd0b5ec | sage-spkg has to ignore some errors when building from scratch
|
comment:5 Changed 7 years ago by
- 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 7 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 7 years ago by
And I also don't understand why you remove some of the
if [ $INFO -eq 0 ]; then
branches.
comment:8 Changed 7 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 follow-up: ↓ 10 Changed 7 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 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 7 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 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: ↓ 14 Changed 7 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 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 7 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 7 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 7 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 follow-up: ↓ 16 Changed 7 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 7 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 7 years ago by
- Branch changed from u/vbraun/error_checking_in_sage_spkg to u/jdemeyer/error_checking_in_sage_spkg
comment:18 Changed 7 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 7 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 7 years ago by
- Commit changed from 616ee13ebf90ae0bf7a48398bd4951b3a0e26620 to 69a501d5d4d9c26188c2e19757d091c7f7b5c729
Branch pushed to git repo; I updated commit sha1. New commits:
69a501d | Minor fixes to sage-spkg
|
comment:21 Changed 7 years ago by
- 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 7 years ago by
needs review...
comment:23 Changed 7 years ago by
- Status changed from needs_review to positive_review
comment:24 Changed 7 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 sage-spkg
Do not ignor errors in sage-spkg