official-stockfish / nnue-pytorch

Stockfish NNUE (Chess evaluation) trainer in Pytorch
GNU General Public License v3.0
332 stars 100 forks source link

[easy_train] Potential issue with msys2 and PATH #186

Open Sopel97 opened 2 years ago

Sopel97 commented 2 years ago

...msys2\mingw64\bin needs to be in PATH, but that might not be setup by default, in which case we need to modify the environment programatically

ppigazzini commented 2 years ago

I just installed msys2 in a sandbox with the latest official installer, the path is not updated automatically and there is not an option. The user can add "C:\msys64\usr\bin;C:\msys64\mingw64\bin;" in the path environment variable (first folder for msys2 applications like grep, second one for the mingw-w64 applications if any). Passing an env to subprocess is an easy option but the user must be aware to update the msys2 path with a not standard msys2 installation. Chocolatey at example installs msys2 in C:\tools\msys64\

Sopel97 commented 2 years ago

we can probably infer the path from where gcc, because we already require gcc in path, and the only way for newer gcc on windows appears to be msys2

ppigazzini commented 2 years ago

The standard module winreg is not an option because the msys2 installer (official and with choco) makes a nearly portable install and doesn't register the application under HKEY_LOCAL_MACHINE\Software. The cmd where or the powershell Get-ChildItem cmdlet seems to be the only options for an automatic discovery, but there is the risk to make a long search in a wrong disk. A safer alternative is to run gcc -v with subprocess adding to the env a couple of standard paths (eg C:\msys64\mingw64\bin and C:\tools\msys64\mingw64\bin) and on a failure ask the right path to the user.

There are other mingw-w64 recent distributions like https://jmeubank.github.io/tdm-gcc/articles/2021-05/10.3.0-release but IMO better to have msys2 as requirement.

Sopel97 commented 2 years ago

According to https://superuser.com/a/480115/388191 where only searches cwd and directories from PATH. So we should be fine.

ppigazzini commented 2 years ago

In this case where works only if we ask the user to update the PATH (a viable option of course).

Sopel97 commented 2 years ago

Does where gcc not work in the sandbox you set up? If gcc was not in path then the issue reported on discord would have failed during verification, much earlier.

ppigazzini commented 2 years ago

Check where /? to view all the options. It scanned all C: drive to find grep.exe

C:\Users\WDAGUtilityAccount>where /R C:\ grep.exe
C:\msys64\usr\bin\grep.exe

Another option: we use a worker.cmd launcher on windows, the user doesn't need to update the PATH

@echo off
set PATH=C:\tools\msys64\mingw64\bin;C:\tools\msys64\usr\bin;%PATH%
env\bin\python3.exe -i worker.py
Sopel97 commented 2 years ago

Alternatively we may have to fall back to have the user provide msys2 directory as an argument, with an attempt to infer it otherwise.

Sopel97 commented 2 years ago

Another option: we use a worker.cmd launcher on windows, the user doesn't need to update the PATH

What do we set the path to though

ppigazzini commented 2 years ago

Another option: we use a worker.cmd launcher on windows, the user doesn't need to update the PATH

What do we set the path to though

Standard installation paths used by chocolatey, see the scripts https://github.com/ppigazzini/worker-setup

Sopel97 commented 2 years ago

Ah, so you basically want the script to install msys? My msys2 directory is close to 4GB, I don't think it's wise to force a setup.

ppigazzini commented 2 years ago

Yes, the windows worker setup is meant for the CPU contributor not able to follow the installation tutorial on the wiki. Chocolatey has the plus to auto update msys2 after the installation and the minus to use a not standard installation path. The script installs only the required applications (and uses wget + unzip instead than the space hungrier git) , but you are right that msys2 takes some space. Fortunately users never protested about that.

# install packages if not already installed
pacman -S --noconfirm --needed unzip make mingw-w64-x86_64-gcc mingw-w64-x86_64-python3
Sopel97 commented 2 years ago

Okay. I think you have a point and it would be a good idea to fallback to automatic msys installation if not provided by the user. I'll try to prepare something around tomorrow.