Pathoschild / SMAPI

The modding API for Stardew Valley.
https://smapi.io/
GNU Lesser General Public License v3.0
1.71k stars 258 forks source link

Linux Installation Script Logs Forever #929

Open tmressler opened 4 months ago

tmressler commented 4 months ago

Describe the bug Double clicking install on Linux.sh appears to do does nothing. However, unbeknownst to the user, it starts the installation script in the background and continuously writes outputs to /var/log/syslog until the device runs out of hard disk space (as keystrokes that aren't 1 or 2 followed by ENTER yield input error messages in an infinite loop).

To Reproduce

  1. Double click install on Linux.sh.
  2. Use computer as normal as installation script runs in the background, logging keystrokes.
  3. Eventually /var/log/syslog will grow to fill the remaining hard disk space.
  4. Computer freezes/crashes.

System Details Haven't tried reproducing this on other devices, but this was specifically with Pop-OS, streaming Stardew Valley to an NVidia Shield with an Xbox controller giving inputs to the Shield via Bluetooth. The computer froze while playing, seeming because controller inputs were also being interpreted as inputs to the installation script ran earlier (before realizing it had to be run via the terminal). The resulting syslog file was over 200GB.

HoodedDeath commented 3 months ago

Obviously caused by the install script simply executing ./internal/linux/SMAPI.Installer. Doing it this way just blindly hopes that there is a terminal currently open that the user is invoking the script through, but the Installing SMAPI on Linux instructions make no mention that the script needs to/should be run manually through a terminal.

A .desktop file could be used to ensure launching in a proper terminal, but either I can't figure it out, or the format doesn't support relative paths from the location of the .desktop file.

So I see only two options viable; either change the player instructions to tell users to invoke the script through a terminal, or change the script to check if it's already running in a terminal emulator. Given that SMAPI defaults to using xterm if it can't find another terminal (I've never seen SMAPI using anything but xterm, as command -v x-terminal-emulator never returns anything on any of my systems), it may be good enough to just use xterm when there's not already a terminal detected.

Probably the simpliest way to improve the install script would be to test for valid STDIN, run SMAPI.Installer directly if valid, else run SMAPI.Installer through xterm. The below code achieves that:

#!/usr/bin/env bash

cd "`dirname "$0"`"
if [ -t 0 ]; then
    ./internal/linux/SMAPI.Installer
else
    xterm ./internal/linux/SMAPI.Installer
fi

I'll probably work on a better install script soon, and try to get it merged. Until install on Linux.sh is improved, it will be problematic when you simply double click it, as the install guide's wording implies you can do.