Closed 6bdad4c1-1e26-4f2f-a442-a01a2292c181 closed 2 years ago
New commits:
ef4e8b0 | sage-sanity-check-package |
Branch: public/18826
Commit: ef4e8b0
sage-src scripts are just symptoms of failures of our package system. Instead of formalizing those hacks we should try to understand what is missing. IMHO:
And:
I agree that we are still a long way from having a clean build system, but what I want to do with this script is to help us clean the packages progressively.
If you want to implement the ideas you proposed, your only way to make them work will be to fix all packages at once. I don't think that it can be done, and I do not expect anybody to do it.
If we agree on some formalism for spkg-src
, we can start precisely this work: make sure that everything we do to a package is done there. This way if we ever want to move to something more constraining, we will know at least what we have to deal with.
In the immediate future, however, that would help a reviewer check that the 'updated package' proposed for review is only what it claims to be. No code was injected (to be run on everybody's computer), and also check that everything that is done appears in spkg-src
.
Thus, I still think that such a script would be a good addition: along with a rule that we should only accept spkg-src scripts that fit the assumptions (those are open to discussion). And slowly, we will add new spkg-src files and stabilize the situation.
Nathann
You forgot another important change to packages: either add an autotools build system or re-autotoolize after making some changes.
You forgot another important change to packages: either add an autotools build system or re-autotoolize after making some changes.
In this case, the 'diff' comparison is indeed useless, for different versions of the autotool scripts produce different outputs. If the autotoolizing is done by upstream, however, that's no problem.
Let us talk about solutions instead of just raising the problems: how do we address that?
I did not plan to have this script be anything else than a helper tool to review packages. On the other hand, we must have a way to check the sanity of the packages.
Of the packages which are autotoolized by our 'spkg-src' scripts, I would not be at all surprised to find out that they are like this because... its authors are Sage developpers.
Nathann
I agree with Nathann that spkg-src
scripts are inconsistent and should be improved.
I'd like to modify the assumptions a bit though:
spkg-src
script should only be run within a Sage shell.spkg-src
script should be runnable from any directory (do not assume the current directory is build/pkgs/PKGNAME
)$SAGE_DISTFILES
directory.$SAGE_DISTFILES
.So, ./sage --sh build/pkgs/PKGNAME/spkg-src
should "just work".
Replying to @nathanncohen:
In this case, the 'diff' comparison is indeed useless, for different versions of the autotool scripts produce different outputs.
This is solved by the autotools
experimental package, which enforces a particular version of configure
and similar programs.
Replying to @nathanncohen:
I did not plan to have this script be anything else than a helper tool to review packages.
You want a club to force others to spend time beautifying sage-src scripts. Thats IMHO the wrong direction, we should get rid of the sage-src
scripts instead of cementing in the status quo.
Yooooooooooo !
I agree with Nathann that
spkg-src
scripts are inconsistent and should be improved.
Yep T_T
I'd like to modify the assumptions a bit though:
- The
spkg-src
script should only be run within a Sage shell.
How can you check that? Or do you mean that my script should only run them from the inside of a sage shell? If so, would replacing 'spkg-src' with 'sage -sh spkg-src' in the script be sufficient?
- The
spkg-src
script should be runnable from any directory (do not assume the current directory isbuild/pkgs/PKGNAME
)
I do not know how to check that either. In a preliminary version of my script I wanted to work in a different directory, but I figured out that most spkg-src files would break at this step.
- The tarball should be created in the
$SAGE_DISTFILES
directory.
That would break the way the script currently works. It compares the file created by spkg-src with the one in upstream/
. How could we deal with that?
- The script shall not change any file (not even temporary) outside
$SAGE_DISTFILES
.
Well, it will need some temporary directory anyway.
So,
./sage --sh build/pkgs/PKGNAME/spkg-src
should "just work".
You are making requests which would make this script much more restrictive. If you already know how to implement some of them, feel free to add a commit on this public branch (with explicit error messages).
When we will have settled on that, we will also have to explain all this (and go over all assumptions) in the developer manual.
Thanks,
Nathann
You want a club to force others to spend time beautifying sage-src scripts. Thats IMHO the wrong direction, we should get rid of the
sage-src
scripts instead of cementing in the status quo.
If you see a way to work toward the idea that you have, then count me in. If you have wishes but no specific plan in mind, then I think that this script would help a lot, in the meantime.
Nathann
Replying to @nathanncohen:
How can you check that?
I didn't say that you script should check all conditions. We need to define how the spkg-src
script should be run, so we need to state the conditions precisely.
Or do you mean that my script should only run them from the inside of a sage shell?
Yes.
- The
spkg-src
script should be runnable from any directory (do not assume the current directory isbuild/pkgs/PKGNAME
)I do not know how to check that either.
cd /
"$SAGE_ROOT/build/pkgs/$PKGNAME/spkg-src
- The tarball should be created in the
$SAGE_DISTFILES
directory.That would break the way the script currently works. It compares the file created by spkg-src with the one in
upstream/
. How could we deal with that?
SAGE_REAL_DISTFILES="$SAGE_DISTFILES"
export SAGE_DISTFILES=/some/temporary/directory
cd /
"$SAGE_ROOT/build/pkgs/$PKGNAME/spkg-src
compare "$SAGE_REAL_DISTFILES/$TARBALL" "$SAGE_DISTFILES/$TARBALL"
Well, it will need some temporary directory anyway.
I think it could use $SAGE_DISTFILES
as temporary directory.
Replying to @nathanncohen:
If you see a way to work toward the idea that you have, then count me in. If you have wishes but no specific plan in mind, then I think that this script would help a lot, in the meantime.
+1
Plan:
Replying to @vbraun:
Delete fluff, recompress, run autotools, anything else?
I would like to add a condition:
.tar
extension of bzipped with a .tar.bz2
extension.Reason: Sage includes bzip2
so we have a guaranteed version of bzip2
.
Replying to @jdemeyer:
I think it could use
$SAGE_DISTFILES
as temporary directory.
See autotools/spkg-src
for an implementation (except it uses upstream
instead of $SAGE_DISTFILES
)
spkg-src
script should use sage-download-file
to download files (not a hard-coded curl
or wget
, which are not installed on all systems).Replying to @jdemeyer:
- Merge more than one upstream tarball (e.g. ATLAS, Singular)
Really the sad fact that the packaging system can only handle a single tarball ought to be remedied. This removes the need to combine tarballs AND fixes the autotools problem (just ship the autotools output in a separate tarball)
needs_work because we're still very much discussing.
- we should store the upstream URL (with a VERSION placeholder) somewhere, maybe in checksums.ini
In case you plan to write something like that, don't lose your time with regexes, i have such feature almost ready (moreover the latest version is self-determinated). I wrote this in november so there might be some new packages missing, also there is still the problem for packages that are bundles (e.g. rubiks), and some packages whose download URL is not deterministic because of some kind of hash. I am curently facing some health issues, so i try to minimize my work by not opening tickets for extra features, i will open one for this soon.
needs_work because we're still very much discussing.
Part of the discussion is about other matters than what this branch contains: I wrote a script that I have often needed, in order to check that the packages that are proposed on trac ticket as an updated of upstream are indeed what they claim to be.
Right now these operations have to be done manually, and often. Besides, as I am not the only one who does that I would say that they are of 'public interest'.
I do not mind discussions on wider problems, but I still need a script for those daily/weekly operations that need to be done.
Nathann
You are welcome to use your script, but you don't need to enforce your own conventions on others if we are about to change them.
You are welcome to use your script, but you don't need to enforce your own conventions on others if we are about to change them.
How are we supposed to check that packages updates are consistent? By doing exactly what this script does, manually?
Replying to @nathanncohen:
Part of the discussion is about other matters than what this branch contains
The current branch contains a script to check X without properly defining X.
That makes no sense to me, so my proposal is to first clearly define X, possibly changing it slightly to X' (that's the "discussion" part).
The current branch contains a script to check X without properly defining X.
That makes no sense to me, so my proposal is to first clearly define X, possibly changing it slightly to X' (that's the "discussion" part).
I do not mind. I just want something to happen. Somehow with tickets like that, it's like everybody it fighting so that absolutely nothing happens, and burying tickets. This branch exists, while the others alternatives do not.
About properly defininf X: so far all the restrictions/assumptions you proposed, if I am not mistaken, are more constraining that those I made. Except two:
I will implement this now, so please do not make me regret this later by asking me to do something impossible that you know I will not do, thus killing this ticket.
Nathann
Replying to @nathanncohen:
This branch exists, while the others alternatives do not.
More than once you have been angry with me that I write code on "your" ticket. So obviously, I'll avoid writing code on this ticket.
Branch pushed to git repo; I updated commit sha1. New commits:
fa4edd7 | trac #18226: New assumptions |
Replying to @nathanncohen:
About properly defininf X: so far all the restrictions/assumptions you proposed, if I am not mistaken, are more constraining that those I made.
Some other things we could check:
#!
.If there are any conditions that I mentioned that you don't agree with (regardless of whether the script on this ticket should check them), I'd like to know.
I made the requested modifications.
More than once you have been angry with me that I write code on "your" ticket. So obviously, I'll avoid writing code on this ticket.
I would be glad if you could help me on this ticket. If I am alone against two release managers, I know that this is as good as dead.
About pushing to 'my' tickets: my only objection is that you should refrain from pushing changes if you know that there is an opposition to them. If we agree that something must be done, then there is no problem in saying "i'll do it" and pushing the changes (when the branch is public).
Nathann
Hello,
- All directories in the tarball should have 0755 permission. All ordinary files should have 0755 or 0644 permission. Possibly add: every file which has 0755 permission must start with
#!
.
Oh. In order to avoid the ugly 0777 files?
- All symlinks in the tarball must be relative and point to an existing file.
I don't see any reason to oppose that, and I will pray that the current packages already satisfy it.
Could you add a commit to check that, if you think them necessary? And if you do, could you update the 'list of assumptions' that I added to the head of the 'sanity-check' script? We will need that to write the doc later.
If there are any conditions that I mentioned that you don't agree with (regardless of whether the script on this ticket should check them), I'd like to know.
Well, I would not have thought of those two things you just mentionned, but I see no reason to oppose them (unless we find out that 99% of the tarballs don't satisfy it). And I am afraid that I cannot remember anything else you mentionned that is not yet in my script.
Nathann
Replying to @nathanncohen:
I don't see any reason to oppose that, and I will pray that the current packages already satisfy it.
At least lrcalc
does not:
jdemeyer@tamiyo:/usr/local/src/sage-config/upstream$ ls -l lrcalc-1.1.7/m4
total 0
lrwxrwxrwx 1 jdemeyer jdemeyer 29 Jul 25 2012 libtool.m4 -> /usr/share/aclocal/libtool.m4
lrwxrwxrwx 1 jdemeyer jdemeyer 33 Jul 25 2012 lt~obsolete.m4 -> /usr/share/aclocal/lt~obsolete.m4
lrwxrwxrwx 1 jdemeyer jdemeyer 31 Jul 25 2012 ltoptions.m4 -> /usr/share/aclocal/ltoptions.m4
lrwxrwxrwx 1 jdemeyer jdemeyer 29 Jul 25 2012 ltsugar.m4 -> /usr/share/aclocal/ltsugar.m4
lrwxrwxrwx 1 jdemeyer jdemeyer 31 Jul 25 2012 ltversion.m4 -> /usr/share/aclocal/ltversion.m4
To refresh your mind about the conditions:
spkg-src
script should only be run within a Sage shell.spkg-src
script should be runnable from any directory (do not assume the current directory is build/pkgs/PKGNAME
)$SAGE_DISTFILES
directory.$SAGE_DISTFILES
..tar
extension of bzipped with a .tar.bz2
extension.spkg-src
script should use sage-download-file
to download files (not curl
or wget
, which are not installed on all systems).#!
.You have an additional assumption
# - When uncompressed, the archive creates a "src/" or a
# "<package_name>-<version>/" directory
which I think is not needed.
Such an assumption was needed for old-style spkg files due to the way that sage-spkg
works, but for new-style packages this is really not needed.
- The
spkg-src
script should only be run within a Sage shell.
I updated the sanity-check script on this respect.
- The
spkg-src
script should be runnable from any directory (do not assume the current directory isbuild/pkgs/PKGNAME
)
I checked that too, using your test.
- The tarball should be created in the
$SAGE_DISTFILES
directory.
Also checked.
- The script should not change any file (not even temporary) outside
$SAGE_DISTFILES
.
I do not know how to check that. Is it easily done? It does not seem so important that we should go out of our way to check it.
- The resulting tarball should be either uncompressed with a
.tar
extension of bzipped with a.tar.bz2
extension.
We also have many .tar.gz in 'upstream/'. But I do not see a need to enforce something like that.
- The
spkg-src
script should usesage-download-file
to download files (notcurl
orwget
, which are not installed on all systems).
Why would you make such restrictions? Those spkg-src scripts are run manually and by developpers, at the moment. They know how to deal with a 'wget not found'.
- All directories in the tarball should have 0755 permission. All ordinary files should have 0755 or 0644 permission. Possibly add: every file which has 0755 permission must start with
#!
.- All symlinks in the tarball must be relative and point to an existing file.
I do not think that we need any of those two, but I don't see any reason to oppose you on that. The best for this is probably to try it on several packages, and figure out if this is a reasonable assumption to make. If packages don't already satisfy that, I don't see what we could do against it.
Such an assumption was needed for old-style spkg files due to the way that sage-spkg works, but for new-style packages this is really not needed.
I do not see how to avoid it: if you do, you can add a commit.
Nathann
Replying to @nathanncohen:
We also have many .tar.gz in 'upstream/'.
I have nothing against that.
I'm just saying: if the tarball was created using a spkg-src
script, then it should be either a .tar
or .tar.bz2
file (there is really no reason to create a .gz
file, since those are usually larger than the corresponding .bz2
file).
Replying to @nathanncohen:
Why would you make such restrictions? Those spkg-src scripts are run manually and by developpers, at the moment. They know how to deal with a 'wget not found'.
Well, some spkg-src
scripts use wget
, others use curl
...
For portability, we should use sage-download-file
which is guaranteed to work.
I have nothing against that.
I'm just saying: if the tarball was created using a
spkg-src
script, then it should be either a.tar
or.tar.bz2
That could force us to decompress+recompress some archives given by upstream, just because we think our compression is always better?
Well, some
spkg-src
scripts usewget
, others usecurl
...For portability, we should use
sage-download-file
which is guaranteed to work.
I think that you want to give us a protection that we do not need. Assuming that a developer has wget on his computer is pretty mild...
Branch pushed to git repo; I updated commit sha1. New commits:
ef3d7c4 | trac #18826: Cleaning a bit some spkg-src scripts |
Replying to @nathanncohen:
That could force us to decompress+recompress some archives given by upstream, just because we think our compression is always better?
No, I clearly said if the package uses a spkg-src file. If there is not a spkg-src
file, then no recompression is needed.
Replying to @nathanncohen:
Assuming that a developer has wget on his computer is pretty mild...
wget
is not installed by default on OS X.wget
Sage package.No, I clearly said if the package uses a spkg-src file. If there is not a
spkg-src
file, then no recompression is needed.
You seem to assume that there is a spkg-src file only if there is something to change in the package. I did not assume that. That's why we do not understand each other.
Nathann, more seriously: can we please not change so many spkg-src
scripts on this ticket? I would really prefer to change just one or two as an example and leave the rest for a follow-up ticket.
Replying to @nathanncohen:
You seem to assume that there is a spkg-src file only if there is something to change in the package. I did not assume that. That's why we do not understand each other.
OK, here is an adjusted assumption:
spkg-src
script does compress the tarball, it should use bzip2
for that.Nathann, more seriously: can we please not change so many
spkg-src
scripts on this ticket? I would really prefer to change just one or two as an example and leave the rest for a follow-up ticket.
I don't mind. It is just very very hot in england right now, and that's the only thing I am able to do. So I stop trying to fix giac. About 'wget' I think that it is really going too far, but as you never give way on any point for the sake of good mood, well, I am forced to do it.
No problem for this bz2 thing, we had the same thing in mind all along.
Hello everybody,
This branch adds a "sage-sanity-check-package" script that uses the 'spkg-src' scripts. The goal is to have an easy way to check, when a new-style package gets udpated, that the new package is precisely "spkg-src" builds.
Right now our 'spkg-spkg' scripts all do different things. In order to use 'spkg-src' scripts in an automatic way, I made the following assumption:
When `./sage-src' is run in a build/pkg// folder, it creates the
package archive whose name is described in checksums.ini
There is un upstream/ an archive with the same name
Their contents are compared with 'diff -r'
Currently, almost none of our packages satisfy those assumptions (*).
What would you think of having a script like that? That would 'normalize' the situations of our spkg-src scripts, and we will solve the many inconsistencies in our packaging. That will also make sure that everything we do on a package is listed by the spkg-src script.
Have fun,
Nathann
() Some seem to, but then they are named file.tar.gz while the file is not gzipped. Or else the file in upstream/ is not exactly what is in checksum.ini. Or spkg-src downloads the latest version* instead of the one whose version is hardcoded in checsksums.ini. You get the picture.
Component: build
Branch/Commit: public/18826 @
5372942
Reviewer: Kwankyu Lee
Issue created by migration from https://trac.sagemath.org/ticket/18826