CommitteeOfZero / polyversal-coz-linux-patcher

whose OS runs whose games
MIT License
33 stars 5 forks source link

Modified script to not need deck specification in commands #13

Closed Zi-SH closed 1 year ago

Zi-SH commented 1 year ago

There is no need to query the user as to whether the script is on a deck.

halcyonhippo commented 1 year ago

lsb_release is not available by default on all distros. I.E. I'm on RHEL 8 and when I run the command you've added it stops to ask me if I want to install the LSB core from my repo.

I feel this might cause additional complexity for some users, although this is Linux we're talking about so perhaps I'm being too conservative. However, if the main target for this script is SteamDeck users, this is probably a good change.

Macitron commented 1 year ago

lsb_release is not available by default on all distros. I.E. I'm on RHEL 8 and when I run the command you've added it stops to ask me if I want to install the LSB core from my repo.

Can be fixed with a check for if lsb_release exists, see https://stackoverflow.com/questions/592620/how-can-i-check-if-a-program-exists-from-a-bash-script

The new line with this change could be

if command -v lsb_release > /dev/null && [[ $(lsb_release -d) =~ "SteamOS Holo" ]]; then deck=1; fi
Zi-SH commented 1 year ago

However, if the main target for this script is SteamDeck users, this is probably a good change.

This is only for Steam Deck users. SteamOS 3 and greater are Arch based, and it has LSB by default.

Can be fixed with a check for if lsb_release exists

This is cleaner and could be added, though in the end it doesn't really matter so long as non-Decks don't report "SteamOS Holo".

halcyonhippo commented 1 year ago

This is only for Steam Deck users. SteamOS 3 and greater are Arch based, and it has LSB by default.

Ok, I was under the wrong impression for the target of this repo. I thought this patcher was intended for any Linux user, not just SteamDecks. Carry on.

ghost commented 1 year ago

This is only for Steam Deck users. SteamOS 3 and greater are Arch based, and it has LSB by default.

Ok, I was under the wrong impression for the target of this repo. I thought this patcher was intended for any Linux user, not just SteamDecks. Carry on.

It is for users of any Linux distro supporting Steam Play, as indicated here: https://github.com/CommitteeOfZero/multiversal-coz-linux-patcher#multiversal-coz-linux-patcher

These instructions and the included Bash script are intended to streamline installation of CoZ patches for Steam Play, including on Steam Deck.

It is not merely for Steam Deck or Arch Linux users. Steam OS 3.x and Arch Linux were merely the two distros tested against at launch, which is mentioned in the Troubleshooting section of the README.md. The intent is to support as many as possible, in fact; and the team welcomes PRs to fix issues that are distro-specific.

@Zi-SH LSB is deprecated on Debian since 2015 and has since then been deprecated on Debian derivatives, so I will not merge this unless /etc/os-release is read from instead. If you make that change, I can take a look and merge. It might be useful for @halcyonhippo to test at that point as well, since their distro does not include LSB by default. @Macitron 's suggestion to check for the existence of lsb_release is viable at present, but it is better to future-proof by reading from the now-standard /etc/os-release file.

Zi-SH commented 1 year ago

It is for users of any Linux distro supporting Steam Play, as indicated here: https://github.com/CommitteeOfZero/multiversal-coz-linux-patcher#multiversal-coz-linux-patcher

There seems to be a misunderstanding. I'm saying the pull request is very specifically directed at Deck users (it eliminates the 'deck' flag).

I will modify the pull to use os-release instead of lsb_release and validate it on the Steam Deck and a few other distros I have handy.

Zi-SH commented 1 year ago

Validated on every OS mentioned as a concern, and a few more.

Debian 10:

NAME="Debian GNU/Linux"
zish@deb:~$ if [[ $(cat /etc/os-release | grep "VERSION_CODENAME=holo") ]]; then echo Deck; fi
zish@deb:~$

RHEL 8:

[zish@rhel ~]$ cat /etc/os-release | sed -n '7 p'
PRETTY_NAME="Red Hat Enterprise Linux 8.7 (Ootpa)"
[zish@rhel ~]$ if [[ $(cat /etc/os-release | grep "VERSION_CODENAME=holo") ]]; then echo Deck; fi
[zish@rhel ~]$ 

Ubuntu 22.04:

zish@Jammy:~$ cat /etc/os-release | sed -n '1 p'
PRETTY_NAME="Ubuntu 22.04.1 LTS"
zish@Jammy:~$ if [[ $(cat /etc/os-release | grep "VERSION_CODENAME=holo") ]]; then echo Deck; fi
zish@Jammy:~$ 

Arch Linux 6.19:

COFFEEBOX :: ~ » cat /etc/os-release | sed -n '1 p'
NAME="Arch Linux"
COFFEEBOX :: ~ » if [[ $(cat /etc/os-release | grep "VERSION_CODENAME=holo") ]]; then echo Deck; fi
COFFEEBOX :: ~ »

Steam Deck (Holo):

(deck@CoffeeBar ~)$ cat /etc/os-release | sed -n '1 p'
NAME="SteamOS"
(deck@CoffeeBar ~)$ if [[ $(cat /etc/os-release | grep "VERSION_CODENAME=holo") ]]; then echo Deck; fi
Deck
(deck@CoffeeBar ~)$
ghost commented 1 year ago

It is for users of any Linux distro supporting Steam Play, as indicated here: https://github.com/CommitteeOfZero/multiversal-coz-linux-patcher#multiversal-coz-linux-patcher

There seems to be a misunderstanding. I'm saying the pull request is very specifically directed at Deck users (it eliminates the 'deck' flag).

I will modify the pull to use os-release instead of lsb_release and validate it on the Steam Deck and a few other distros I have handy.

And ah! I see. I did indeed misunderstand. Thanks for clarifying!