Opened 2 years ago
Closed 2 years ago
#25150 closed defect (fixed)
Eliminate dependence on VERSION.txt within Sage
Reported by:  embray  Owned by:  

Priority:  critical  Milestone:  sage8.3 
Component:  misc  Keywords:  
Cc:  fbissey, jdemeyer, vbraun  Merged in:  
Authors:  Erik Bray  Reviewers:  François Bissey 
Report Upstream:  N/A  Work issues:  
Branch:  c12e442 (Commits)  Commit:  c12e4428445f9ae0ae905323fe31c022183cd0cc 
Dependencies:  Stopgaps: 
Description (last modified by )
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/sageversion.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 sagethedistribution. However, there are probably various external utilities that look for that fileI can confirm that the patchbot is one such utility, if nothing else. But there are likely others. So it's harmless to keep it.
Change History (43)
comment:1 Changed 2 years ago by
 Status changed from new to needs_review
comment:2 Changed 2 years ago by
 Cc jdemeyer vbraun added
comment:3 Changed 2 years ago by
 Commit changed from ba0177a2a016e83e0e35d9420f6beaca06fc6d50 to 669b09b47a74029a64298ff0d8d249e284a7b79e
comment:4 Changed 2 years ago by
Why "blocker"? My gut feeling says that this ticket has a nontrivial chance of breaking stuff, so it should be treated as a normal ticket for Sage 8.3.
comment:5 Changed 2 years ago by
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?
comment:6 followup: ↓ 8 Changed 2 years ago by
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.
comment:7 Changed 2 years ago by
It doesn't break anything. VERSION.txt is a completely static file shipped in the Sage sources, generated by sageupdateversion
nothing is changed about that. Within Sage itself it's used in just three minor places to get...a string: build/make/install
(badlyit should use sageversion.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/sagestart
.
comment:8 in reply to: ↑ 6 Changed 2 years ago by
Replying to fbissey:
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.
comment:9 Changed 2 years ago by
 Priority changed from blocker to major
Conflicts...
Also, we should probably fix this properly instead of a lastminute effort that may or may not improve things.
comment:10 Changed 2 years ago by
I would say this is "properly"...
comment:11 Changed 2 years ago by
 Commit changed from 669b09b47a74029a64298ff0d8d249e284a7b79e to dabcba29a87dbf7b427ceb0b2dada597ee458c6f
comment:12 Changed 2 years ago by
 Priority changed from major to blocker
Please properly review this ticket. It fixes an issue that was introduced in development by #25056 and is problematic for nonsourcebased 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".
comment:13 Changed 2 years ago by
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 bugfix either, it's really adding new functionality.
comment:14 followup: ↓ 18 Changed 2 years ago by
I don't think $SAGE_LOCAL/bin
is a good place for sageversion.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 sageongentoo 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.
comment:15 followup: ↓ 19 Changed 2 years ago by
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.
comment:16 followup: ↓ 21 Changed 2 years ago by
VERSION_FULL is actually a banner. So how about this change?
VERSION_FULL > VERSION_BANNER
comment:17 Changed 2 years ago by
 Commit changed from dabcba29a87dbf7b427ceb0b2dada597ee458c6f to d87fd38c91318ef231c9b0764d37fbac31389db3
comment:18 in reply to: ↑ 14 Changed 2 years ago by
Replying to fbissey:
I don't think
$SAGE_LOCAL/bin
is a good place forsageversion.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 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/sageversion.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 sageversion.sh
and instead put that info in sageenvconfig
, 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
.
comment:19 in reply to: ↑ 15 Changed 2 years ago by
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/sageversion.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
.
comment:20 Changed 2 years ago by
 Commit changed from d87fd38c91318ef231c9b0764d37fbac31389db3 to 3055ab9e6e49080c251ea0fffcd747b184a721b4
Branch pushed to git repo; I updated commit sha1. New commits:
3055ab9  Process reporting arguements to the sage command before attempting to source sageenv; this way at least those commands work outofthebox.

comment:21 in reply to: ↑ 16 Changed 2 years ago by
Replying to klee:
VERSION_FULL is actually a banner. So how about this change?
VERSION_FULL > VERSION_BANNER
Yeah, I think I agree with that.
comment:22 Changed 2 years ago by
 Commit changed from 3055ab9e6e49080c251ea0fffcd747b184a721b4 to e6b76d08acdb5ef109c37d256f15c8dfbce6c7db
Branch pushed to git repo; I updated commit sha1. New commits:
e6b76d0  Rename SAGE_VERSION_FULL => SAGE_VERSION_BANNER

comment:23 Changed 2 years ago by
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.
comment:24 Changed 2 years ago by
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.
comment:25 Changed 2 years ago by
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
comment:26 Changed 2 years ago by
 Description modified (diff)
comment:27 Changed 2 years ago by
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.
comment:28 Changed 2 years ago by
 Milestone changed from sage8.2 to sage8.3
 Priority changed from blocker to critical
comment:29 Changed 2 years ago by
 Status changed from needs_review to needs_work
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.
comment:30 Changed 2 years ago by
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.
comment:31 Changed 2 years ago by
 Commit changed from e6b76d08acdb5ef109c37d256f15c8dfbce6c7db to 37f67a79f5eea0b15917ebc7dd18633c4b6ad439
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 sageenv; this way at least those commands work outofthebox.

37f67a7  Rename SAGE_VERSION_FULL => SAGE_VERSION_BANNER

comment:32 Changed 2 years ago by
 Status changed from needs_work to needs_review
comment:33 Changed 2 years ago by
 Reviewers set to François Bissey
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.
comment:34 Changed 2 years ago by
 Status changed from needs_review to needs_work
there are merge conflicts.
comment:35 Changed 2 years ago by
 Commit changed from 37f67a79f5eea0b15917ebc7dd18633c4b6ad439 to 6d000872790c82d5531083b599a0f377672ddc31
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 sageenv; this way at least those commands work outofthebox.

6d00087  Rename SAGE_VERSION_FULL => SAGE_VERSION_BANNER

comment:36 Changed 2 years ago by
 Status changed from needs_work to needs_review
Rebased. I still believe this to be better than the current situation, though I would welcome concrete feedback and/or criticism.
comment:37 Changed 2 years ago by
 Status changed from needs_review to positive_review
comment:38 Changed 2 years ago by
 Status changed from positive_review to needs_info
Why did you move sourcing of sageenv
and handling of options like root
inside src/bin/sage
?
comment:39 Changed 2 years ago by
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 sageenv would not work correctly. In other words, from a clean source checkout you can now do:
./sage version
./sage root
and so on.
comment:40 Changed 2 years ago by
 Commit changed from 6d000872790c82d5531083b599a0f377672ddc31 to c12e4428445f9ae0ae905323fe31c022183cd0cc
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 sageenv; this way at least those commands work outofthebox.

c12e442  Rename SAGE_VERSION_FULL => SAGE_VERSION_BANNER

comment:42 Changed 2 years ago by
 Status changed from needs_review to positive_review
Let's try to get it in again.
comment:43 Changed 2 years ago by
 Branch changed from u/embray/build/removeversion.txt to c12e4428445f9ae0ae905323fe31c022183cd0cc
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits:
Improve functionality of sage version and sage dumpversion so that it can work without SAGE_LOCAL existing yet