Eliminate dependence on VERSION.txt within Sage
embray opened this issue · 47 comments
As discussed on #25056, that ticket introduced a small, but annoying runtime dependency on a file that lives in $SAGE_ROOT, VERSION.txt, which creates problems for downstream distributions.
This proposes to eliminate practically all dependence on VERSION.txt, by instead writing the same string to a new variable SAGE_VERSION_BANNER which is stored in both src/bin/sage-version.sh and src/sage/version.py. Although mildly redundant, this is at least consistent.
Originally I thought to just outright remove VERSION.txt since at this point it is not used directly anywhere in sagelib or sage-the-distribution. However, there are probably various external utilities that look for that file--I can confirm that the patchbot is one such utility, if nothing else. But there are likely others. So it's harmless to keep it.
Component: misc
Author: Erik Bray
Branch/Commit: c12e442
Reviewer: François Bissey
Issue created by migration from https://trac.sagemath.org/ticket/25150
Branch pushed to git repo; I updated commit sha1. New commits:
669b09b | Improve functionality of sage --version and sage --dumpversion so that it can work without SAGE_LOCAL existing yet |
Why "blocker"? My gut feeling says that this ticket has a non-trivial chance of breaking stuff, so it should be treated as a normal ticket for Sage 8.3.
Because we don't want to add an additional annoyance to Sage 8.2 for downstream packagers that they'll have to work around, only to remove that workaround for the next version. It's a simple change we can make now so why not?
For the record I have already worked around. I am not exactly convinced by the content of this ticket either. Admittedly some things have to be considered carefully, and I can see due diligence in action.
It doesn't break anything. VERSION.txt is a completely static file shipped in the Sage sources, generated by sage-update-version--nothing is changed about that. Within Sage itself it's used in just three minor places to get...a string: build/make/install (badly--it should use sage-version.sh instead which it now does). src/bin/sage (as added in #25056 in a less than ideal manner), and in a trivial way in src/bin/sage-start.
Replying to @kiwifb:
For the record I have already worked around. I am not exactly convinced by the content of this ticket either. Admittedly some things have to be considered carefully, and I can see due diligence in action.
You may have, but not Debian (which, granted, is in a state of limbo right now, but something we are hoping to fix), and conda at a minimum.
Conflicts...
Also, we should probably fix this properly instead of a last-minute effort that may or may not improve things.
I would say this is "properly"...
Please properly review this ticket. It fixes an issue that was introduced in development by #25056 and is problematic for non-source-based Sage installs (including on Windows it's not actually a problem here after all since I do install VERSION.txt; however, we should not be introducing known regressions at the last minute if we can easily fix them). The fix was carefully considered and tested, but all you're telling me is that's "not proper" based on a "gut feeling".
I'll leave it to Volker to decide on the blocker status.
The reason for my "git feeling" is that tickets touching the build system have a tendency to introduce subtle breakage. And this isn't a simple bug-fix either, it's really adding new functionality.
I don't think $SAGE_LOCAL/bin is a good place for sage-version.sh but this is a matter in some other tickets. I was mainly surprised by the extents of the changes wrought by this ticket and the fact there is still a mention of SAGE_ROOT - but I should never take that branch so it's probably OK. There is stuff all over the place where I did not expect it.
As Jeroen says it touches bits of the build system, while I don't expect any problem (and sage-on-gentoo doesn't really care) I understand him being worried about it.
At the end of the day, I'll probably patch less than before with this ticket.
Our existing implementation is far from ideal, but I also don't see a real improvement in this patch.
The way it should be is: there is a version py and sh file, for import/source in python and shell scripts. There shouldn't be python scripts looking at environment variables that where set elsewhere.
There should be some mechanism to check that SAGE_ROOT is not being used outside of the build system so this doesn't happen again, and that is not in the attached branch.
VERSION_FULL is actually a banner. So how about this change?
VERSION_FULL --> VERSION_BANNER
Replying to @kiwifb:
I don't think
$SAGE_LOCAL/binis a good place forsage-version.shbut this is a matter in some other tickets. I was mainly surprised by the extents of the changes wrought by this ticket and the fact there is still a mention ofSAGE_ROOT- but I should never take that branch so it's probably OK. There is stuff all over the place where I did not expect it.
The only "mention of SAGE_ROOT" is as an alternative "$SAGE_LOCAL/bin/sage-version.sh" if it does not exist.
I agree that's not a great place for that file, but it's where it already existed. I don't know what you do with it on gentoo (though having it there isn't the worst place for it). I think I would rather do away with sage-version.sh and instead put that info in sage-env-config, but I was trying to keep this change as undisruptive as possible.
Which is why, while I'm sorry to make a fight over this, I just feel like it's a little unfair to unilaterally brush it off without due consideration, especially since it was only to address a legitimate complaint of a regression for downstream packagers. I considered this change and its impact carefully. I don't necessarily expect anyone to take my word for that, but at least do me the same consideration.
I'll also note that at the very least d87fd38c is now necessary for the tests to pass on Docker. We force SAGE_BANNER=bare on Docker due to a bug in Docker (I think now fixed, but still in older versions), and this test as written was assuming the default value of SAGE_BANNER.
Replying to @vbraun:
Our existing implementation is far from ideal, but I also don't see a real improvement in this patch.
The way it should be is: there is a version py and sh file, for import/source in python and shell scripts. There shouldn't be python scripts looking at environment variables that where set elsewhere.
There should be some mechanism to check that
SAGE_ROOTis not being used outside of the build system so this doesn't happen again, and that is not in the attached branch.
+1 to all of the above in principle. However, you'll see in this branch that it actually eliminates several uses of $SAGE_ROOT, and leaves only one in the sage script, and only as a fallback if $SAGE_LOCAL/bin/sage-version.sh does not exist yet. I agree that's still not ideal, but also somewhat necessary since currently the sage script serves as an entrypoint to runtime capabilities of Sage, and to the "build system" (a fact I've complained about in #25130).
The goal there was that at the very least, running ./sage --version should work the same whether in a fresh clone of the repository (though that's actually already broken at the moment for unrelated reasons), or running some binary release with no $SAGE_ROOT.
Branch pushed to git repo; I updated commit sha1. New commits:
3055ab9 | Process reporting arguements to the sage command before attempting to source sage-env; this way at least those commands work out-of-the-box. |
Replying to @kwankyu:
VERSION_FULL is actually a banner. So how about this change?
VERSION_FULL --> VERSION_BANNER
Yeah, I think I agree with that.
Branch pushed to git repo; I updated commit sha1. New commits:
e6b76d0 | Rename SAGE_VERSION_FULL => SAGE_VERSION_BANNER |
The only problem with SAGE_VERSION_BANNER is it's potentially confusable with SAGE_BANNER. But I think that's not a problem so long as it's kept undocumented, for internal use.
If this ticket is too much trouble, as a compromise we can at least merge d87fd38c which is actually a (possible) test regression introduced by #25056, and actually caught by the patchbot but we ignored it.
I'd also appreciate an alternate fix the issue raised in this ticket that would be more acceptable, but if I'm the only one who cares then don't worry about it I guess.
I'm fine with just d87fd38c; I'd prefer not to merge the entire ticket for 8.2; distro packagers either have the issue already patched, can do it easily enough, or can wait until 8.3
Description changed:
---
+++
@@ -1,5 +1,5 @@
As discussed on #25056, that ticket introduced a small, but annoying runtime dependency on a file that lives in `$SAGE_ROOT`, `VERSION.txt`, which creates problems for downstream distributions.
-This proposes to eliminate practically all dependence on `VERSION.txt`, by instead writing the same string to a new variable `SAGE_VERSION_FULL` which is stored in both `src/bin/sage-version.sh` and `src/sage/version.py`. Although mildly redundant, this is at least consistent.
+This proposes to eliminate practically all dependence on `VERSION.txt`, by instead writing the same string to a new variable `SAGE_VERSION_BANNER` which is stored in both `src/bin/sage-version.sh` and `src/sage/version.py`. Although mildly redundant, this is at least consistent.
Originally I thought to just outright remove `VERSION.txt` since at this point it is not used directly anywhere in sagelib or sage-the-distribution. However, there are probably various external utilities that look for that file--I can confirm that the patchbot is one such utility, if nothing else. But there are likely others. So it's harmless to keep it.I still don't see what the big deal is, but okay. I will downgrade this ticket it and make a separate one for the test fix.
Does this need review at the moment? I did not read through the entire discussion but it appears that this needs work first? Also, there are merge conflicts.
There is a merge conflict, since I guess Volker made another release (this ticket edits some files that are normally otherwise only changed during releases). Otherwise, nobody has been able to provide particularly useful commentary on this ticket.
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
47068ae | Remove all runtime dependency on a VERSION.txt file |
96f4d3f | Improve functionality of sage --version and sage --dumpversion so that it can work without SAGE_LOCAL existing yet |
1d1fd34 | fix these tests |
4c2a937 | Process reporting arguements to the sage command before attempting to source sage-env; this way at least those commands work out-of-the-box. |
37f67a7 | Rename SAGE_VERSION_FULL => SAGE_VERSION_BANNER |
I am actually OK with the ticket in this form. My last comment may not have been clear enough considering the answer I got from it. I am happy for it to move to the next stage.
However it is marked to merge in 8.3 and some bits will need rebasing as soon as 8.2 is released (version.py and version.sh) so I am bit concerned about changing to positive right now. But I approve of it.
Reviewer: François Bissey
there are merge conflicts.
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
4c7c4c0 | Remove all runtime dependency on a VERSION.txt file |
1b85e02 | Improve functionality of sage --version and sage --dumpversion so that it can work without SAGE_LOCAL existing yet |
5d4e2bf | fix these tests |
671933d | Process reporting arguements to the sage command before attempting to source sage-env; this way at least those commands work out-of-the-box. |
6d00087 | Rename SAGE_VERSION_FULL => SAGE_VERSION_BANNER |
Rebased. I still believe this to be better than the current situation, though I would welcome concrete feedback and/or criticism.
Why did you move sourcing of sage-env and handling of options like --root inside src/bin/sage?
Depending on how you read the diff, I didn't move it at all :) At least, in terms of intent, what I was moving was:
if [ "$1" = '-dumpversion' -o "$1" = '--dumpversion' ]; then
sage_version
exit 0
fi
if [ "$1" = '-v' -o "$1" = '-version' -o "$1" = '--version' ]; then
sage_version -v
exit 0
fi
if [ "$1" = '-root' -o "$1" = '--root' ]; then
echo "$SAGE_ROOT"
exit 0
fi
if [ $# -gt 0 ]; then
if [ "$1" = '-h' -o "$1" = '-?' -o "$1" = '-help' -o "$1" = '--help' ]; then
usage
fi
if [ "$1" = "-advanced" -o "$1" = "--advanced" ]; then
usage_advanced
fi
fi
which I felt should work even from a clean source checkout. Previously these flags did not work in that case because sourcing sage-env would not work correctly. In other words, from a clean source checkout you can now do:
./sage --version
./sage --root
and so on.
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
b6364cc | Remove all runtime dependency on a VERSION.txt file |
49d1b3d | Improve functionality of sage --version and sage --dumpversion so that it can work without SAGE_LOCAL existing yet |
dae4836 | fix these tests |
0a19135 | Process reporting arguements to the sage command before attempting to source sage-env; this way at least those commands work out-of-the-box. |
c12e442 | Rename SAGE_VERSION_FULL => SAGE_VERSION_BANNER |
Rebased again.
Let's try to get it in again.
Changed branch from u/embray/build/remove-version.txt to c12e442