Opened 7 years ago

Closed 6 years ago

#16629 closed defect (wontfix)

Improve sage-fix-pkg-checksums

Reported by: charpent Owned by:
Priority: minor Milestone: sage-duplicate/invalid/wontfix
Component: build Keywords:
Cc: Merged in:
Authors: Reviewers: Emmanuel Charpentier, Volker Braun, J. H. Palmieri, Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: u/charpent/sage_fix_pkg_checksums_munches_build_pkgs__packagename__checksums (Commits, GitHub, GitLab) Commit: a0469055a4f293df53e6f6ae42ac9dc0e761f7c8
Dependencies: Stopgaps:

Status badges

Description (last modified by jdemeyer)

The current version of sage-fix-pkg-checksums incorrectly sets the package version to the literal "VERSION" in checksums.ini, which is confusing and not helpful.

This patch correcly computes the version string and puts it at the right place in checksums.ini.

Change History (34)

comment:1 Changed 7 years ago by charpent

  • Branch set to u/charpent/sage_fix_pkg_checksums_munches_build_pkgs__packagename__checksums

comment:2 Changed 7 years ago by charpent

  • Commit set to b607718bd7be120232547f34549750d557e35c4c
  • Status changed from new to needs_review

The proposed solution needs to be reviewed by someone more knowledgeable in the sage building system than I am.


New commits:

08c39b2Point d'étape avant test.
b607718Fixed package checksums munching by /src/bin/sage-fix-pkg-checksums

comment:3 Changed 7 years ago by vbraun

French commit messages are verboten ;-)

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

  • Status changed from needs_review to needs_work

Hmm "fubar"?

Having old tarballs in upstream/ is normal and shouldn't lead to endless warnings.

IMHO there shouldn't be any warnings, anything that is not correct ought to be a hard error. If we aren't strict about it we are bound to get track tickets with invalid checksums.ini.

comment:5 in reply to: ↑ 4 Changed 7 years ago by charpent

Replying to vbraun:

Hmm "fubar"?

Wups. Oversight. Fixed...

Having old tarballs in upstream/ is normal and shouldn't lead to endless warnings.

What do you mean ?

IMHO there shouldn't be any warnings, anything that is not correct ought to be a hard error. If we aren't strict about it we are bound to get track tickets with invalid checksums.ini.

Fix underway : orders rather than suggestions.

Last edited 7 years ago by charpent (previous) (diff)

comment:6 Changed 7 years ago by git

  • Commit changed from b607718bd7be120232547f34549750d557e35c4c to d941afd9150164e979b7df98b2e574cdacd78053

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

d941afdAll discrepancies ==> error and abort (+ cleanup of oversight).

comment:7 Changed 7 years ago by charpent

  • Status changed from needs_work to needs_review

"Tous vos ordres, Seigneur, seront exécutés" (Jean Racine) [ Hey, it's not a commit message... ]

Followed Volker's suggestions.

comment:8 follow-ups: Changed 7 years ago by vbraun

I've got various tarball versions of most packages. I don't want to delete them because I might want to go back to an older ticket. Yet I get:

(sage-sh) vbraun@localhost:upstream$ sage-fix-pkg-checksums 
atlas-3.10.1.20140128.tar.bz2
Version mismatch between tarball 3.10.1.20140128 and directory build/pkgs/atlas.
Newer version ? You must put 3.10.1.20140128.p0 in package-version.txt.

comment:9 in reply to: ↑ 8 Changed 7 years ago by charpent

Replying to vbraun:

I've got various tarball versions of most packages. I don't want to delete them because I might want to go back to an older ticket. Yet I get:

(sage-sh) vbraun@localhost:upstream$ sage-fix-pkg-checksums 
atlas-3.10.1.20140128.tar.bz2
Version mismatch between tarball 3.10.1.20140128 and directory build/pkgs/atlas.
Newer version ? You must put 3.10.1.20140128.p0 in package-version.txt.

Hmmm... Now we have a conflict on intended usage(s) (and therefore designs) :

  • The builg/pkgs/<pkgname>/... structure implies at most one version per package name.
  • The relevant snippet of the Developer's guide hints at a "batch" usage of src/bin/sage-fix-pkg-checksums.

So far, that's consistent with upstream/ as the set of active (i. e. built by Sage's build system) Sage's external packages source tarballs.

  • Your intended usage amounts to use upstream/ as a cache of potential external packages, the set of active Sage external packages being determined otherwise.
    1. This might be the build/pkgs/*/package-version.txt set, in which case the sage-fix-pkg-checksums script should loop over this set and try to find a relevant tarball. At most, it might spit a warning (or fail) when no such tarball exists. No automatic rewriting possible.
    2. It might be an external file listing each package and the relevant version. That makes build/pkgs/<pkgname>/package-version.txt redundant (and a possible source of conflicts).

The case 1. is consistent, but needs a script like sage-fix-one-pkg-checksum-Volkers-style to generate the right checksums. As said before, the current batch-style script should loop on the build/pkgs/... tree.

In any case, it appears that the consistency of the proposed change (use upstream/ as a cache and not as a reference set) needs modifying the use of this script (and, quite possibly, the rest of the build subsystem for external packages). Since I do not yet know my way in this system, fixing this is, for now, out of my reach. At a minimum, I need advice from someone knowing what it does.

By the way, what is wrong with an external (.gitignored) directory and mv ing the right tarball at the right time ? An even lazier solution is, of course, to symlink the right tarball in upstream/, but I doubt one can get git to follow this transparently.

Since we are playing redesigning this subsystem, may I ask why we have to have package-version.txt and checksums.ini to describe different attributes of the same tarball ? Why not one file ?

Last edited 7 years ago by charpent (previous) (diff)

comment:10 follow-up: Changed 7 years ago by vbraun

The upstream/ directory has always been meant as a cache where possibly multiple versions of tarballs are stored, potentially even shared between different sage installations. This is also how the sage-fix-pkg-checksums script works right now, it looks for a tarball matching build/pkgs/*/package-version.txt and ignores the rest.

The split package-version.txt and checksums.ini is to make reading the version easier from scripts. I didn't vote for it...

comment:11 in reply to: ↑ 8 Changed 7 years ago by charpent

Replying to vbraun with a bit of "esprit de l'escalier"...:

I've got various tarball versions of most packages. I don't want to delete them because I might want to go back to an older ticket.

Different branches, I suppose ? If so, why not let git track the change of tarball ?

Yet I get:

(sage-sh) vbraun@localhost:upstream$ sage-fix-pkg-checksums 
atlas-3.10.1.20140128.tar.bz2
Version mismatch between tarball 3.10.1.20140128 and directory build/pkgs/atlas.
Newer version ? You must put 3.10.1.20140128.p0 in package-version.txt.

comment:12 follow-up: Changed 7 years ago by vbraun

I think I'm not clear, so let me try again. Git does track the package version, because package-version.txt is checked into the repository. For any given Sage commit, build/pkgs/*/package-version.txt is the only relevant set of tarball versions. The upstream/ directory content is irrelevant, and only used to cache downloaded tarballs.

comment:13 Changed 7 years ago by jhpalmieri

I agree with Volker that nothing in upstream should be tracked; we should be free to have tarballs for many versions of each package there. I mean, it might be nice to have a script which cleaned this directory up, deleting all out of date versions, but that would be (1) a very low priority (since you can just delete the whole directory if you have a good internet connection) and (2) for another ticket.

comment:14 in reply to: ↑ 10 ; follow-up: Changed 7 years ago by rws

Replying to vbraun:

... it looks for a tarball matching build/pkgs/*/package-version.txt and ignores the rest.

Minus the patchlevel, to be exact.

comment:15 in reply to: ↑ 12 Changed 7 years ago by charpent

Replying to vbraun:

I think I'm not clear, so let me try again. Git does track the package version, because package-version.txt is checked into the repository. For any given Sage commit, build/pkgs/*/package-version.txt is the only relevant set of tarball versions. The upstream/ directory content is irrelevant, and only used to cache downloaded tarballs.

I beg to differ : From the current sage-fix-pkg-checksum, (as in the develop branch) :

#!/usr/bin/env bash

# If any command-line arguments are present, treat them as files to be
# checksummed. If no arguments are present, all tarballs in
# $SAGE_ROOT/upstream are checksummed.
if [ $# -eq 0 ]; then
    cd "$SAGE_ROOT"
    set upstream/*.tar*
fi

In other words, the current version is driven by the content of upstream/ if no argument is given. If an (some) argument(s) is|are given, it uses the beginning of each argument up to the first dash as a package name and whatever follows tar in this argument as the compression method. Therefore, this will produce the expected behaviour IF AND ONLY IF the script is passed tarball filenames.

It will also produce the expected result if given arguments containing the package version. But not with arguments containing the package name only : there is no information available to choose which version is to be documented in build/pkgs/<pkgname>/package-version.txt|checksums.ini.

A simple fix is to ignore package version mismatchs (i. e. removing the error message an the abort). In this case, the tarball ultimately documented in build/pkgs/<pkgname>/checksums.ini will be the one passed in argument, or the last matching the package name in the upstream/ file list (lexicographic order).

Another possibility is to use build/pkgs ad the corresponding package-version.txt to get the necessary information, and search upstream/ for a relevant tarball. In this case, one might find one tarball (and process it as before), none (one type of error) or more than one (another type of error).

Which one do you want ?

comment:16 in reply to: ↑ 14 Changed 7 years ago by vbraun

Replying to rws:

Minus the patchlevel, to be exact.

Exactly, which is another reason for why the package-version.txt is the authorative version. Also, the Sage build process uses the package-version.txt timestamp to determine whether a package needs to be rebuilt, which is why you need to add the patch level here.

The sage-fix-pkg-checksum iterates over upstream/ and then ignores any entries for which there is no corresponding build/pkgs/*/package-version.txt. So that it can auto-generate the entire checksums.ini (though it trips over cases). E.g. hashdist has a much more sensible system of caching upstream sources.

comment:17 Changed 7 years ago by git

  • Commit changed from d941afd9150164e979b7df98b2e574cdacd78053 to 50697bf4b64ce0cf0a185c15ffce539d1392a80f

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

50697bfBack to original behaviour (minus tolerance for mixed-case tarball names).

comment:18 Changed 7 years ago by charpent

  • Authors set to Emmanuel Charpentier, Volker Braun (with input from J. H. Palmieri)

Okay. New commit ==> back to previous behaviour.

I still kept the ability to accept mixed-case tarball names (with no warning).

When called without arguments, the script will treat all tarballs. Therefore, for a given package, the last package treated (i. e. last in the lexicographic order) will be the one used for the final checksums.ini for that package. Is that what you wanted ?

When called with explicit tarball names, these tarballs will generate checksums.ini.

Status still needs_review.

comment:19 Changed 7 years ago by vbraun

No, the last in lex order is wrong. Version 3.9 is older than 3.10. Really, you need to get the version from the package-version.txt file.

comment:20 Changed 7 years ago by git

  • Commit changed from 50697bf4b64ce0cf0a185c15ffce539d1392a80f to 214941f6443a352a00f6b84acf6f029bdba67d85

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

214941fProcess if versions match.

comment:21 Changed 7 years ago by charpent

Done. I hope I've guessed the specs right this time...

comment:22 follow-up: Changed 7 years ago by vbraun

The "fubar" file is still in the branch...

comment:23 Changed 7 years ago by git

  • Commit changed from 214941f6443a352a00f6b84acf6f029bdba67d85 to 7a2c9da063f353392d4b6629a1172b162de4c2f5

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

7a2c9daFinal cleanup of my distractions.

comment:24 in reply to: ↑ 22 Changed 7 years ago by charpent

Replying to vbraun:

The "fubar" file is still in the branch...

Re-wups ! I didn't saw that when I checked before committing/pushing. Should be good now.

comment:25 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:26 follow-up: Changed 7 years ago by jdemeyer

  • Description modified (diff)
  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_review to needs_work
  • Summary changed from sage-fix-pkg-checksums munches build/pkgs/<packagename>/checksums to Improve sage-fix-pkg-checksums

Authors: please describe what this patch is supposed to do, because the previous description was wrong (package-version.txt isn't changed by sage-fix-pkg-checksums) and confusing! It's easier to review when I know what this patch is supposed to achieve.

And fix the indentation please: no TABS, use indents of 4 spaces

And I would like to add an additional change:

upstream_pkg_name=${tarball%-*}

should become

upstream_pkg_name=${tarball%%-*}

to allow upstream versions with dashes (package-abc-1 should have package name "package" and version "abc-1").

comment:27 Changed 7 years ago by git

  • Commit changed from 7a2c9da063f353392d4b6629a1172b162de4c2f5 to a0469055a4f293df53e6f6ae42ac9dc0e761f7c8

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

a046905Trac #16629: improve sage-fix-pkg-checksums

comment:28 in reply to: ↑ 26 Changed 7 years ago by charpent

  • Description modified (diff)
  • Status changed from needs_work to needs_review

Replying to jdemeyer:

Authors: please describe what this patch is supposed to do, because the previous description was wrong (package-version.txt isn't changed by sage-fix-pkg-checksums) and confusing! It's easier to review when I know what this patch is supposed to achieve.

Done

And fix the indentation please: no TABS, use indents of 4 spaces

Also done. Although I wonder why this recommendation, which is sensible for python (and sage) programs, should apply to shell scripts, which are a horse of a totally different color...

And I would like to add an additional change:

upstream_pkg_name=${tarball%-*}

should become

upstream_pkg_name=${tarball%%-*}

to allow upstream versions with dashes (package-abc-1 should have package name "package" and version "abc-1").

Also done (although I'm not sure of the pertinence...).

HTH,

comment:29 in reply to: ↑ description ; follow-up: Changed 7 years ago by jdemeyer

  • Description modified (diff)

Replying to charpent:

The current version of sage-fix-pkg-checksums incorrectly sets the package version to the literal "VERSION" in checksums.ini, which is confusing and not helpful.

It's a feature, not a bug. It ensures that version info is stored in exactly one place, namely package-version.txt.

comment:30 Changed 7 years ago by jdemeyer

  • Status changed from needs_review to needs_work

comment:31 Changed 7 years ago by jdemeyer

  • Authors changed from Emmanuel Charpentier, Volker Braun (with input from J. H. Palmieri) to Emmanuel Charpentier, Volker Braun, J. H. Palmieri

comment:32 in reply to: ↑ 29 Changed 6 years ago by rws

  • Status changed from needs_work to needs_info

Replying to jdemeyer:

Replying to charpent:

The current version of sage-fix-pkg-checksums incorrectly sets the package version to the literal "VERSION" in checksums.ini, which is confusing and not helpful.

It's a feature, not a bug. It ensures that version info is stored in exactly one place, namely package-version.txt.

So, you think it's wontfix?

comment:33 Changed 6 years ago by jdemeyer

  • Authors Emmanuel Charpentier, Volker Braun, J. H. Palmieri deleted
  • Milestone changed from sage-6.4 to sage-duplicate/invalid/wontfix
  • Reviewers changed from Jeroen Demeyer to Emmanuel Charpentier, Volker Braun, J. H. Palmieri, Jeroen Demeyer
  • Status changed from needs_info to positive_review

comment:34 Changed 6 years ago by vbraun

  • Resolution set to wontfix
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.