Opened 3 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: sage-8.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 klee)

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.

Change History (43)

comment:1 Changed 3 years ago by embray

  • Status changed from new to needs_review

comment:2 Changed 3 years ago by embray

  • Cc jdemeyer vbraun added

comment:3 Changed 3 years ago by git

  • Commit changed from ba0177a2a016e83e0e35d9420f6beaca06fc6d50 to 669b09b47a74029a64298ff0d8d249e284a7b79e

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

669b09bImprove functionality of sage --version and sage --dumpversion so that it can work without SAGE_LOCAL existing yet

comment:4 Changed 3 years ago by jdemeyer

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.

comment:5 Changed 3 years ago by embray

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 follow-up: Changed 3 years ago by 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.

comment:7 Changed 3 years ago by embray

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.

comment:8 in reply to: ↑ 6 Changed 3 years ago by embray

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 3 years ago by vbraun

  • Priority changed from blocker to major

Conflicts...

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

comment:10 Changed 3 years ago by embray

I would say this is "properly"...

comment:11 Changed 3 years ago by git

  • Commit changed from 669b09b47a74029a64298ff0d8d249e284a7b79e to dabcba29a87dbf7b427ceb0b2dada597ee458c6f

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

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

comment:12 Changed 3 years ago by embray

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

Last edited 3 years ago by embray (previous) (diff)

comment:13 Changed 3 years ago by jdemeyer

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.

comment:14 follow-up: Changed 3 years ago by fbissey

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.

comment:15 follow-up: Changed 3 years ago by 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.

comment:16 follow-up: Changed 3 years ago by klee

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

VERSION_FULL --> VERSION_BANNER

comment:17 Changed 3 years ago by git

  • Commit changed from dabcba29a87dbf7b427ceb0b2dada597ee458c6f to d87fd38c91318ef231c9b0764d37fbac31389db3

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

664af6bfix these tests
d87fd38Enforce the default value of SAGE_BANNER before running this test, since otherwise it might now pass

comment:18 in reply to: ↑ 14 Changed 3 years ago by embray

Replying to fbissey:

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.

comment:19 in reply to: ↑ 15 Changed 3 years ago by embray

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.

comment:20 Changed 3 years ago by git

  • Commit changed from d87fd38c91318ef231c9b0764d37fbac31389db3 to 3055ab9e6e49080c251ea0fffcd747b184a721b4

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

3055ab9Process reporting arguements to the sage command before attempting to source sage-env; this way at least those commands work out-of-the-box.

comment:21 in reply to: ↑ 16 Changed 3 years ago by embray

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 3 years ago by git

  • Commit changed from 3055ab9e6e49080c251ea0fffcd747b184a721b4 to e6b76d08acdb5ef109c37d256f15c8dfbce6c7db

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

e6b76d0Rename SAGE_VERSION_FULL => SAGE_VERSION_BANNER

comment:23 Changed 3 years ago by embray

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 3 years ago by embray

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 3 years ago by vbraun

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 3 years ago by klee

  • Description modified (diff)

comment:27 Changed 3 years ago by embray

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 3 years ago by embray

  • Milestone changed from sage-8.2 to sage-8.3
  • Priority changed from blocker to critical

comment:29 Changed 3 years ago by saraedum

  • 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 3 years ago by embray

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.

Last edited 3 years ago by embray (previous) (diff)

comment:31 Changed 3 years ago by git

  • Commit changed from e6b76d08acdb5ef109c37d256f15c8dfbce6c7db to 37f67a79f5eea0b15917ebc7dd18633c4b6ad439

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

47068aeRemove all runtime dependency on a VERSION.txt file
96f4d3fImprove functionality of sage --version and sage --dumpversion so that it can work without SAGE_LOCAL existing yet
1d1fd34fix these tests
4c2a937Process reporting arguements to the sage command before attempting to source sage-env; this way at least those commands work out-of-the-box.
37f67a7Rename SAGE_VERSION_FULL => SAGE_VERSION_BANNER

comment:32 Changed 3 years ago by embray

  • Status changed from needs_work to needs_review

comment:33 Changed 3 years ago by fbissey

  • 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 saraedum

  • Status changed from needs_review to needs_work

there are merge conflicts.

comment:35 Changed 2 years ago by git

  • Commit changed from 37f67a79f5eea0b15917ebc7dd18633c4b6ad439 to 6d000872790c82d5531083b599a0f377672ddc31

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

4c7c4c0Remove all runtime dependency on a VERSION.txt file
1b85e02Improve functionality of sage --version and sage --dumpversion so that it can work without SAGE_LOCAL existing yet
5d4e2bffix these tests
671933dProcess reporting arguements to the sage command before attempting to source sage-env; this way at least those commands work out-of-the-box.
6d00087Rename SAGE_VERSION_FULL => SAGE_VERSION_BANNER

comment:36 Changed 2 years ago by embray

  • 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 fbissey

  • Status changed from needs_review to positive_review

comment:38 Changed 2 years ago by jdemeyer

  • Status changed from positive_review to needs_info

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

Last edited 2 years ago by jdemeyer (previous) (diff)

comment:39 Changed 2 years ago by embray

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.

comment:40 Changed 2 years ago by git

  • Commit changed from 6d000872790c82d5531083b599a0f377672ddc31 to c12e4428445f9ae0ae905323fe31c022183cd0cc

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

b6364ccRemove all runtime dependency on a VERSION.txt file
49d1b3dImprove functionality of sage --version and sage --dumpversion so that it can work without SAGE_LOCAL existing yet
dae4836fix these tests
0a19135Process reporting arguements to the sage command before attempting to source sage-env; this way at least those commands work out-of-the-box.
c12e442Rename SAGE_VERSION_FULL => SAGE_VERSION_BANNER

comment:41 Changed 2 years ago by embray

  • Status changed from needs_info to needs_review

Rebased again.

comment:42 Changed 2 years ago by fbissey

  • Status changed from needs_review to positive_review

Let's try to get it in again.

comment:43 Changed 2 years ago by vbraun

  • Branch changed from u/embray/build/remove-version.txt to c12e4428445f9ae0ae905323fe31c022183cd0cc
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.