Eliminate dependence on VERSION.txt within Sage

[embray]

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.

CC: @kiwifb @jdemeyer @vbraun

Component: misc

Author: Erik Bray

Branch/Commit: c12e442

Reviewer: François Bissey

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

[sagetrac-git]

Changed commit from ba0177a to 669b09b

[sagetrac-git]

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

[jdemeyer]

comment:4

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.

[embray]

comment:5

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?

[kiwifb]

comment:6

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.

[embray]

comment:7

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.

[embray]

comment:8

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.

[vbraun]

comment:9

Conflicts...

Also, we should probably fix this properly instead of a last-minute effort that may or may not improve things.

[embray]

comment:10

I would say this is "properly"...

[sagetrac-git]

Changed commit from 669b09b to dabcba2

[sagetrac-git]

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

0c13a95	Remove all runtime dependency on a VERSION.txt file
dabcba2	Improve functionality of sage --version and sage --dumpversion so that it can work without SAGE_LOCAL existing yet

[embray]

comment:12

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

[jdemeyer]

comment:13

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.

[kiwifb]

comment:14

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.

[vbraun]

comment:15

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.

[kwankyu]

comment:16

VERSION_FULL is actually a banner. So how about this change?

VERSION_FULL --> VERSION_BANNER

[sagetrac-git]

Changed commit from dabcba2 to d87fd38

[sagetrac-git]

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

664af6b	fix these tests
d87fd38	Enforce the default value of SAGE_BANNER before running this test, since otherwise it might now pass

[embray]

comment:18

Replying to @kiwifb:

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.

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.

[embray]

comment:19

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_ROOT is 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.

[sagetrac-git]

Changed commit from d87fd38 to 3055ab9

[sagetrac-git]

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.

[sagetrac-git]

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

[kiwifb]

comment:33

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.

[kiwifb]

Reviewer: François Bissey

[saraedum]

comment:34

there are merge conflicts.

[sagetrac-git]

Changed commit from 37f67a7 to 6d00087

[sagetrac-git]

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

[embray]

comment:36

Rebased. I still believe this to be better than the current situation, though I would welcome concrete feedback and/or criticism.

[jdemeyer]

comment:38

Why did you move sourcing of sage-env and handling of options like --root inside src/bin/sage?

[embray]

comment:39

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.

[sagetrac-git]

Changed commit from 6d00087 to c12e442

[sagetrac-git]

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

[embray]

comment:41

Rebased again.

[kiwifb]

comment:42

Let's try to get it in again.

[vbraun]

Changed branch from u/embray/build/remove-version.txt to c12e442