Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#18417 closed defect (fixed)

Tarball download fixes

Reported by: vbraun Owned by:
Priority: blocker Milestone: sage-6.7
Component: build Keywords:
Cc: Merged in:
Authors: Volker Braun Reviewers: Leif Leonhardy
Report Upstream: N/A Work issues:
Branch: 7c65f3d (Commits) Commit:
Dependencies: Stopgaps:

Description (last modified by vbraun)

  • Use own mirror (if the official mirrors haven't been updated yet)
  • Remove checksumming from sage-spkg since this is now handled in sage-download-file

Change History (16)

comment:1 Changed 4 years ago by vbraun

  • Branch set to u/vbraun/tarball_download_fixes

comment:2 Changed 4 years ago by vbraun

  • Authors set to Volker Braun
  • Commit set to 53cc3c352d150b3f2fe9cc342577b809fe60fbc3
  • Component changed from PLEASE CHANGE to build
  • Description modified (diff)
  • Priority changed from major to blocker
  • Type changed from PLEASE CHANGE to defect

New commits:

7c65f3dTry our own tarball host last
53cc3c3remove checksumming from sage-spkg

comment:3 Changed 4 years ago by vbraun

  • Status changed from new to needs_review

comment:4 Changed 4 years ago by leif

  • Status changed from needs_review to needs_work

Don't mix up completely independent things.

Using a different fallback server has absolutely nothing to do with checksumming in sage-spkg. Your change to sage-spkg just needlessly introduces another artificial dependency on Python.

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

  • Status changed from needs_work to needs_review

I'm just removing the duplicate checksumming, that does in no way introduce an additional dependency. Old-style spkgs still can't be checksummed because they don't have a checksum.

comment:6 Changed 4 years ago by tmonteil

  • Status changed from needs_review to needs_info
  • It is important to check tarballs during install, not only at download time, since there are use cases where tarballs are directly copied to the upstream/ directory, e.g.:
    • during the review process of a package upgrade,
    • when people share optional packages through external storage to save bandwidth.
  • Silently adding your personal website to the list of mirrors is not healthy.

I dont see why those two regressions justify a blocker priority.

comment:7 Changed 4 years ago by vbraun

Tarballs are still checksummed with this branch, the only difference is that they are checksummed only once.

The fallback is the mirror for the buildbot; without it there is a window of a couple of hours where a new tarball is not yet available from the mirror network.

comment:8 in reply to: ↑ 5 ; follow-up: Changed 4 years ago by leif

  • Reviewers set to Thierry Monteil, Leif Leonhardy
  • Status changed from needs_info to needs_work

Replying to vbraun:

I'm just removing the duplicate checksumming, that does in no way introduce an additional dependency. Old-style spkgs still can't be checksummed because they don't have a checksum.

I was wondering whether you didn't know what you're doing or whether you just didn't care... (and I didn't want to further comment on the second commit here in hope you'd [re]move it)

The second commit, which is completely unrelated and independent of the first one (I would have given positive review if you [re]moved the former from this ticket, where it simply doesn't belong), needlessly introduces new requirements for building Sage, even from the (self-contained) source tarball:

  • a system-wide Python installation (with a working urllib)
  • internet access (to github btw.)
  • write access to the $SAGE_DISTFILES folder (cf. my previous comment) -- ok, the build currently doesn't fail otherwise (see below), but sage-download-file keeps downloading the mirror list for each and every package subject to installation, continually re-ranking the mirrors

and all of that regardless of whether any package actually needs to get downloaded. (Thierry already mentioned silently incorporating such changes into a blocker ticket, and late in the release cycle, is even more dubious.)


That the build without a system-wide Python and/or without internet access currently still succeeds is just due to a bug in sage-spkg, namely that it doesn't at all check the exit code of sage-download-file.

So as is (with your second commit included), checksum errors wouldn't necessarily lead to (build) errors, hardly anybody would notice them. In case Python is available, and your script detects the checksum of the tarball already present in $SAGE_DISTFILES doesn't match the one in build/pkgs/..., it deletes the tarball (provided write access is granted) without asking and tries to download it. (With $SAGE_DISTFILES read-only, sage-spkg wouldn't notice checksum errors and simply proceed with the "wrong" tarball; same in case Python wasn't available -- silently no checksums would get verified, until Sage's Python has been built, but then we're back where sage-dist-files only tries to delete "wrong" tarballs...)

comment:9 in reply to: ↑ 8 Changed 4 years ago by vbraun

Replying to leif:

  • a system-wide Python installation (with a working urllib)

Not a bug, its a feature that is moving us forward.

  • write access to the $SAGE_DISTFILES folder

Not a bug.

Thierry already mentioned silently incorporating such changes into a blocker ticket, and late in the release cycle, is even more dubious.

And the reason why its this late in the release cycle? Because I worked hard to make Sage use the mirror network and then you sat on your ass until the shit hit the fan. This should have been fixed 6 months ago.

comment:10 Changed 4 years ago by vbraun

See #18187 for why corrupt tarballs must be re-downloaded automatically without user invention.

comment:11 Changed 4 years ago by git

  • Commit changed from 53cc3c352d150b3f2fe9cc342577b809fe60fbc3 to 7c65f3dc957eb5b27ad6fa2474b8ca520b83b6f4

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

comment:12 Changed 4 years ago by vbraun

  • Status changed from needs_work to positive_review

comment:13 follow-up: Changed 4 years ago by tmonteil

  • Reviewers changed from Thierry Monteil, Leif Leonhardy to Leif Leonhardy

I still don't understand:

  • why the buildbot can not use SAGE_SERVER environment variable for that purpose ?
  • why Sage is not able to afford its own infrastructure for this very particular case ?

comment:14 in reply to: ↑ 13 ; follow-up: Changed 4 years ago by vbraun

Replying to tmonteil:

  • why the buildbot can not use SAGE_SERVER environment variable for that purpose ?

The buildbot uses SAGE_SERVER, but you (having downloaded the just-released git branch) might not.

  • why Sage is not able to afford its own infrastructure for this very particular case ?

Is this a joke? Sage has no operative income and academic funding is continuously declining.

comment:15 Changed 4 years ago by vbraun

  • Branch changed from u/vbraun/tarball_download_fixes to 7c65f3dc957eb5b27ad6fa2474b8ca520b83b6f4
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:16 in reply to: ↑ 14 Changed 4 years ago by dimpase

  • Commit 7c65f3dc957eb5b27ad6fa2474b8ca520b83b6f4 deleted

Replying to vbraun:

Replying to tmonteil:

  • why the buildbot can not use SAGE_SERVER environment variable for that purpose ?

The buildbot uses SAGE_SERVER, but you (having downloaded the just-released git branch) might not.

  • why Sage is not able to afford its own infrastructure for this very particular case ?

Is this a joke? Sage has no operative income and academic funding is continuously declining.

the latter is not true, at least not for Europe: see http://gow.epsrc.ac.uk/NGBOViewGrant.aspx?GrantRef=EP/M022641/1

and, unless some very bad admin screwup happens, this will be funded too: https://github.com/sagemath/grant-europe/blob/44d81a5c2da945a70b2c0f1575b666665f591ede/H2020/ProposalEvaluation/676541-OpenDreamKit-ESR.pdf

Note: See TracTickets for help on using tickets.