Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tarball download fixes #18417

Closed
vbraun opened this issue May 14, 2015 · 21 comments
Closed

Tarball download fixes #18417

vbraun opened this issue May 14, 2015 · 21 comments

Comments

@vbraun
Copy link
Member

vbraun commented May 14, 2015

  • 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

Component: build

Author: Volker Braun

Branch: 7c65f3d

Reviewer: Leif Leonhardy

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

@vbraun vbraun added this to the sage-6.7 milestone May 14, 2015
@vbraun
Copy link
Member Author

vbraun commented May 14, 2015

Branch: u/vbraun/tarball_download_fixes

@vbraun

This comment has been minimized.

@vbraun
Copy link
Member Author

vbraun commented May 14, 2015

Commit: 53cc3c3

@vbraun
Copy link
Member Author

vbraun commented May 14, 2015

New commits:

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

@vbraun
Copy link
Member Author

vbraun commented May 14, 2015

Author: Volker Braun

@nexttime
Copy link
Mannequin

nexttime mannequin commented May 15, 2015

comment:4

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.

@nexttime nexttime mannequin added s: needs work and removed s: needs review labels May 15, 2015
@vbraun
Copy link
Member Author

vbraun commented May 15, 2015

comment:5

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.

@sagetrac-tmonteil
Copy link
Mannequin

sagetrac-tmonteil mannequin commented May 15, 2015

comment:6
  • 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.

@vbraun
Copy link
Member Author

vbraun commented May 15, 2015

comment:7

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.

@nexttime
Copy link
Mannequin

nexttime mannequin commented May 15, 2015

comment:8

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

@nexttime
Copy link
Mannequin

nexttime mannequin commented May 15, 2015

Reviewer: Thierry Monteil, Leif Leonhardy

@nexttime nexttime mannequin added s: needs work and removed s: needs info labels May 15, 2015
@vbraun
Copy link
Member Author

vbraun commented May 16, 2015

comment:9

Replying to @nexttime:

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

@vbraun
Copy link
Member Author

vbraun commented May 16, 2015

comment:10

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 16, 2015

Changed commit from 53cc3c3 to 7c65f3d

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 16, 2015

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

@sagetrac-tmonteil
Copy link
Mannequin

sagetrac-tmonteil mannequin commented May 16, 2015

comment:13

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 ?

@sagetrac-tmonteil
Copy link
Mannequin

sagetrac-tmonteil mannequin commented May 16, 2015

Changed reviewer from Thierry Monteil, Leif Leonhardy to Leif Leonhardy

@vbraun
Copy link
Member Author

vbraun commented May 16, 2015

comment:14

Replying to @sagetrac-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.

@vbraun
Copy link
Member Author

vbraun commented May 17, 2015

Changed branch from u/vbraun/tarball_download_fixes to 7c65f3d

@dimpase
Copy link
Member

dimpase commented May 17, 2015

Changed commit from 7c65f3d to none

@dimpase
Copy link
Member

dimpase commented May 17, 2015

comment:16

Replying to @vbraun:

Replying to @sagetrac-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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants