Windows python ver check and improved update.bat

[O-J1]

Change Summary

• Implemented a version check to ensure that users have Python 3.10, 3.11, or 3.12 installed (including any maintenance or bugfix releases). If the installed Python version is outside this range, the installation process will halt, display a warning, and provide a direct link to the appropriate download page.

• Enhanced the update.bat script to be more robust. It now attempts a git pull on the currently checked-out branch. If that fails for any reason, the script automatically switches back to the master branch and performs a git pull there, reducing the likelihood of update failures where end users have made a branch. Please let me know if this is not desirable and I will adjust it.

Motivation

These changes address recurring instances in which users inadvertently installed or attempted to run OT on an incompatible Python version (<3.10 or ≥3.13). The revised checks help prevent such issues by clearly guiding users toward the correct Python installation. This is in a similar spirit to what Johnny did with the linux and mac scripts.

Tested and worked for me on Windows 11.

[O-J1]

This will resolve some of the issues mentioned in #670

[dxqbYD]

...and #562

[O-J1]

Tested on WSL2 (Ubuntu) and Windows 11. Ive tried Python 3.13.2 and 3.12.9 on both OS'es and it worked as expected. (prevented with 3.13).

• Automatic branch switching is removed
• Shifted to using version_check.py and modified it work transperantly with install.bat whilst still working with install.sh (tested and it does)
• Defined supported python versions in lib.include.sh

I would love for someone else to confirm before this is merged and review the code.

[Arcitec]

Thank you for taking on these improvements for Windows. It was sorely needed and will be a great improvement! ❤️

Sorry ahead of time for some possibly harsh-sounding comments in my review. I am a programmer, not a people-person, and I can be very direct in a "there's an issue here, here and here" way. It's my fault. 😄

[Arcitec]

The lib.include.sh file needs to be reverted.

The old error message already gives all information.

Here's what the current master code says without these changes:

$ ~/o/OneTrainer  master ± $ pyenv local 3.13  

$ ~/o/OneTrainer  master ± $ OT_PREFER_VENV=true ./start-ui.sh
[OneTrainer] Using Python Venv environment in "venv"...

[OneTrainer] Creating Python Venv environment in "venv"...

[OneTrainer] + python -m venv venv

[OneTrainer] + python scripts/util/version_check.py 3.10 3.13
Error: Your Python version is too high: 3.13.0 (main, Feb 22 2025, 23:07:22) [GCC
14.2.1 20250110 (Red Hat 14.2.1-7)]. Must be >= 3.10 and < 3.13.

[OneTrainer] Solution: Either install the required Python version via pyenv
(https://github.com/pyenv/pyenv) and set the project directory's Python version
with "pyenv install <version>" followed by "pyenv local <version>", or install
Miniconda if you prefer that we automatically manage everything for you
(https://docs.anaconda.com/miniconda/). Remember to manually delete any previous
Venv or Conda environment which was created with a different Python version.
Read "LAUNCH-SCRIPTS.md" for more detailed instructions.

As you can see, the user already sees what Python version range is supported:

• Error: Your Python version is too high: 3.13.0 (main, Feb 22 2025, 23:07:22) [GCC 14.2.1 20250110 (Red Hat 14.2.1-7)]. Must be >= 3.10 and < 3.13.

Introducing more variables that duplicate the exact same info is just additional maintenance burden. I appreciate what you were trying to do, but the necessary info is already there in master.

The revised "solution" error message is also not necessary since it's just redundant extra words mentioning "OneTrainer" (which is the app they are trying to run already - and it's also already in the message prefix of every line).

[Arcitec]

These changes are a mixed bag. I recommend reverting most of them.

• The change removes a reusable exit_wrong_version function and duplicates its error message and code twice instead. What the hell? Why?
• The old code intentionally outputs the entire value of sys.version so that we can see from user error logs what exact vendor version of Python they are using (system, native, Conda, RedHat, Windows, etc). You removed that and just output the major number, which provides no information about what Python vendor they are using (that's extra runtime info which is good to see when looking at newbie error logs, and can cut down on having to ask them for more details).
• Added a completely useless else: exit(0) which is already the default return value if nothing is done. If you don't exit with an error, you always exit with success automatically. That's basic. So there's no reason to duplicate default behavior. We should assume that the people reading/working on our code know basics and don't need a "exit 0" spelled out.
• Broke the usage help message by making zero arguments print a useless Python version number instead of saying Error: Version check requires 2 arguments: [min_ver] [too_high_ver]... If you want to know your own Python version, run python --version locally instead. ;)
• from __future__ import print_function is not necessary on Python 2.5.6 and maybe even earlier. 2.5.6 was the oldest version I checked (2.5 itself came out in 2005, but 2.5.6 came out in late 2006). I did some digging and see that technically, print("foo") on Python 2 is treated as print ("foo"), which is not a tuple, it's just a statement enclosed in parenthesis/grouping. The only time the __future__ comes into play is if we want to print a comma-separated list like in Python 3, such as print("Hello", "World"). So print() with a single argument (what we are doing in this code) has been supported for at least 20 years and most likely since the creation of Python. So we don't need this line, tbh. (Besides, good luck running OneTrainer on a machine that has an operating system which hasn't been updated for 20 years, haha.) The simplest question here is: Does that line improve the code by making it work differently? The answer is no. Using multiple comma-separated print-arguments in Python 3 is bad practice and we're not doing that here either. Edit: Actually, the proposed code removed all print() calls anyway so this line was extra pointless.
• The new str_to_tuple exception catching swallows the exception error and just outputs "Invalid version format" which is absolutely useless, since it actually provides less debugging information than the old code. Only OneTrainer developers are providing the arguments for the script anyway. And if a developer provides an incorrect argument, we already get a very clear error message. Here is an example of the latest master code if I provide invalid commas instead of periods:

$ python scripts/util/version_check.py 3,4 3,5
Traceback (most recent call last):
  File "/home/johnny/Code/OneTrainer/scripts/util/version_check.py", line 32, in <module>
    min_ver = str_to_tuple(sys.argv[1])
  File "/home/johnny/Code/OneTrainer/scripts/util/version_check.py", line 14, in str_to_tuple
    return tuple(map(lambda x: int(x, 10), data.split(".")))
  File "/home/johnny/Code/OneTrainer/scripts/util/version_check.py", line 14, in <lambda>
    return tuple(map(lambda x: int(x, 10), data.split(".")))
                               ~~~^^^^^^^
ValueError: invalid literal for int() with base 10: '3,4'

There's two good changes here: Exiting via sys.exit(). And outputting errors to stderr instead. But at the same time, using stderr is pointless and changes nothing, since we never run this script in a way that eats stdout anyway - and that change involves manually adding \n which is Unix-specific (does that always get translated to \r\n on Windows regardless of Python version?), and the sys.std* file handles are buffered and might need manual flushing to appear on screen, so I propose investigating more about sys.stderr.write's behavior before doing that change.

Edit: Yeah, the proposed stderr code seems to have introduced this buffering bug (the behavior depends on the OS and Python version): https://stackoverflow.com/questions/6298940/python-ctypes-how-do-i-flush-output-from-stderr

It's also still unclear whether sys.stderr.write() actually supports \n or if we should use os.linesep. If the handles are opened in text-mode, we should use \n, otherwise os.linesep. The old Python 2 docs are useless regarding details about those handles: https://docs.python.org/2/library/sys.html#sys.stderr

I can only test Python 2 on Linux (on Windows it's very hard to run such old software), so we'll never know if it's opened in text-mode and automatically outputs \n as \r\n on Windows correctly. :D I am just going to assume that the pipe handles are in text-mode though.

Edit: As part of this, I spotted a totally different thing in the existing code that doesn't work in Python 2.5. I'll whip up a new pull request for that and bring in all other changes here that made sense.

Update: Here's a pull request with the proposed stderr change, flushing implemented (just to make sure the stderr change always works), using sys.exit, and more backwards-compatible string formatting (that's the issue I just noticed on Python 2.5): #698. So the file should be reverted in this PR.

[Arcitec]

This exact error message is repeated twice. It seems like it could be moved to a goto section to reuse the same message a single time instead? Both instances end with the same goto end_error so I don't see any reason not to have the message itself in a goto wrong_python_version or something.

[Arcitec]

I don't write batch scripts, but this change stood out to me since it changes the meaning.

• Old: "VENV_DIR=%~dp0%venv"
• New: "VENV_DIR=%~dp0venv"

Both might work, but %var% with a terminating % is the way I see everyone using, and a quick web search seems to confirm that the terminating % is better if included (safer delimiter). I could be wrong here. Windows Batch files are the scripting language I hate more than anything. Even PowerShell is better. And nobody likes PowerShell... :)

Not seeing a variable name delimeter before the word venv appears really looks confusing either way (now it looks like a variable named ~dp0venv). So even if Batch scripting somehow figures out the variable without a % terminator, I think it looks way more sane to include it.