Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Linux] Added option to prefer system libs #1564

Merged
merged 12 commits into from
Jul 13, 2022
Merged

Conversation

Nocccer
Copy link
Collaborator

@Nocccer Nocccer commented Jul 11, 2022


Use the following Checklist if you have changed something on the Backend or Frontend:

  • Tested the feature and it's working on a current and clean install.
  • Tested the main App features and they are still working on a current and clean install. (Login, Install, Play, Uninstall, Move games, etc.)
  • Created / Updated Tests (If necessary)
  • Created / Updated documentation (If necessary)

@Nocccer Nocccer added pr:ready-to-merge This PR is fully ready for merge. pr:ready-for-review Feature-complete, ready for the grind! :P labels Jul 11, 2022
@Nocccer Nocccer linked an issue Jul 11, 2022 that may be closed by this pull request
Copy link
Collaborator

@CommandMC CommandMC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In terms of code, looks good to me. Will give this a test on a fresh VM without dependencies tomorrow

src/screens/Settings/components/WineSettings/index.tsx Outdated Show resolved Hide resolved
src/screens/Settings/components/WineSettings/index.tsx Outdated Show resolved Hide resolved
electron/launcher.ts Outdated Show resolved Hide resolved
electron/launcher.ts Outdated Show resolved Hide resolved
Nocccer and others added 4 commits July 12, 2022 01:14
Co-authored-by: Mathis Dröge <34034631+CommandMC@users.noreply.github.com>
Co-authored-by: Mathis Dröge <34034631+CommandMC@users.noreply.github.com>
Co-authored-by: Mathis Dröge <34034631+CommandMC@users.noreply.github.com>
Co-authored-by: Mathis Dröge <34034631+CommandMC@users.noreply.github.com>
@Nocccer
Copy link
Collaborator Author

Nocccer commented Jul 11, 2022

In terms of code, looks good to me. Will give this a test on a fresh VM without dependencies tomorrow

Nice thank you. What do you think about moving LD_PRELOAD to setupENVVars ?

@CommandMC
Copy link
Collaborator

Note that since you ran yarn i18next, you might have to update the translation files manually as well
I'd recommend only running the command once you're absolutely sure no translations will change

@CommandMC
Copy link
Collaborator

What do you think about moving LD_PRELOAD to setupEnvVars ?

Yeah I was wondering why you did it in callRunner itself, it would definitely fit into setupEnvVars better

@Nocccer
Copy link
Collaborator Author

Nocccer commented Jul 11, 2022

What do you think about moving LD_PRELOAD to setupEnvVars ?

Yeah I was wondering why you did it in callRunner itself, it would definitely fit into setupEnvVars better

I wanted to place it as near as possible to spawn for testing.

@Nocccer Nocccer requested a review from CommandMC July 11, 2022 23:24
electron/launcher.ts Outdated Show resolved Hide resolved
@Nocccer
Copy link
Collaborator Author

Nocccer commented Jul 12, 2022

You need to switch wine version once, else the lib and lib32 are not stored correctly.

@CommandMC
Copy link
Collaborator

Yup, looks even better now.
Setting up a testing VM for this right now, gimme a bit

@CommandMC
Copy link
Collaborator

Hm, the "common" No such file or directory error seems to still happen. Log from testing VM:
Quail-lastPlay.log

@Nocccer
Copy link
Collaborator Author

Nocccer commented Jul 12, 2022

@CommandMC maybe we need to add wine bin folder aswell?

@CommandMC
Copy link
Collaborator

Hm, I don't think that would help anything

Controversial take here:
In my opinion, we shouldn't support this dependency-less usage of Heroic on anything other than the Flatpak. And luckily, this issue does not happen there.
We should instead add Wine's dependencies to our own (in the repos we're shipping to). Lutris does it like that, Bottles does it like that, Steam does it like that (in fact both Lutris and Bottles just depend on wine itself)

Now this doesn't mean this PR is irrelevant. I'd say it doesn't hurt to prefer these shipped libraries, and maybe we can one day figure out why this error happens and add something that works around it. For now though, doing this shot-in-the-dark fixing when there's a clear solution just doesn't make sense IMO

@Nocccer
Copy link
Collaborator Author

Nocccer commented Jul 12, 2022

Hm, I don't think that would help anything

Controversial take here: In my opinion, we shouldn't support this dependency-less usage of Heroic on anything other than the Flatpak. And luckily, this issue does not happen there. We should instead add Wine's dependencies to our own (in the repos we're shipping to). Lutris does it like that, Bottles does it like that, Steam does it like that (in fact both Lutris and Bottles just depend on wine itself)

Now this doesn't mean this PR is irrelevant. I'd say it doesn't hurt to prefer these shipped libraries, and maybe we can one day figure out why this error happens and add something that works around it. For now though, doing this shot-in-the-dark fixing when there's a clear solution just doesn't make sense IMO

I thought wine-ge ships all necessary libs to run it. I don't understand why we need runtimes if wine-ge can include all of them.
So we need a runtime like lutris. Would be intresting if wine-ge still works without lutris runtime enabled, maybe i miss a path ?

Is it useful to add wine as a dependency for none flatpak (deb, rmp, etc.) ?

@CommandMC
Copy link
Collaborator

I thought wine-ge ships all necessary libs to run it. I don't understand why we need runtimes if wine-ge can include all of them.

This is a completely uneducated guess here, but it might be the case that just having the files isn't enough.

I don't really know how all of the Wine dependencies interact with the host system, but there's a similar thing where I do know how it works: GameMode.
It's, according to the official repo, a daemon/lib combo. So there's a library (libgamemodeauto.so.0) that then communicates with a daemon running in the background. If this daemon isn't running, even if you have the library, it won't work. Something similar might be going on here

@Nocccer
Copy link
Collaborator Author

Nocccer commented Jul 12, 2022

I thought wine-ge ships all necessary libs to run it. I don't understand why we need runtimes if wine-ge can include all of them.

This is a completely uneducated guess here, but it might be the case that just having the files isn't enough.

I don't really know how all of the Wine dependencies interact with the host system, but there's a similar thing where I do know how it works: GameMode. It's, according to the official repo, a daemon/lib combo. So there's a library (libgamemodeauto.so.0) that then communicates with a daemon running in the background. If this daemon isn't running, even if you have the library, it won't work. Something similar might be going on here

GE suggest to use the system libs. Strange. But we could add wine as a dependecy for deb, pacman and rpm in the package.json. This will make sure people install wine, if they wanna install heroic via this three options.

@CommandMC
Copy link
Collaborator

GE suggest to use the system libs

He does? Where? As far as I know, Wine-GE was built to run inside the Lutris Runtime:
image

But we could add wine as a dependecy for deb, pacman and rpm in the package.json. This will make sure people install wine, if they wanna install heroic via this three options.

I'd say that would be a sensible thing to do, yes

@Nocccer
Copy link
Collaborator Author

Nocccer commented Jul 12, 2022

GE suggest to use the system libs

He does? Where? As far as I know, Wine-GE was built to run inside the Lutris Runtime:
image

But we could add wine as a dependecy for deb, pacman and rpm in the package.json. This will make sure people install wine, if they wanna install heroic via this three options.

I'd say that would be a sensible thing to do, yes

In the summary section he suggest system libs over runtime libs:
https://www.gloriouseggroll.tv/how-to-get-out-of-wine-dependency-hell/

That also explains, why sometimes the steam runtime break things in heroic.

@CommandMC
Copy link
Collaborator

I believe that guide is mostly unmaintained by now. Judging by the discord messages, his stance on the matter has changed.
Well, we're not here to gossip about other people.

The Steam Runtime not working correctly is most likely an issue in our implementation, maybe we're not setting a variable or not considering an edge case. I don't really bother with it anymore since Wine-GE is just the better option

@Nocccer
Copy link
Collaborator Author

Nocccer commented Jul 13, 2022

@CommandMC i tested the app image of this PR on my steam deck in desktop mode. I could run WheelsOfAurelia with wine-ge.
Just wonder what lib is missing on your end? Can you try WheelsOfAurelia aswell? I think the error is game depended, because some games need optional libs, which are not shipped by wine-ge

@Nocccer
Copy link
Collaborator Author

Nocccer commented Jul 13, 2022

Before we merge this with wine as dependency for deb, pacman and rpm, i wanna know if @flavioislima has a argument against it?

@CommandMC
Copy link
Collaborator

It's definitely not game-specific, the command that's failing is the wineboot one (that's ran for every game to create the prefix). I also cannot even run the wine executable on the Terminal (getting the same error there). This is on a fresh EndeavourOS install (which is pretty minimal in terms of pre-installed stuff), if you have the time, feel free to test yourself.

Although it is good to know that it isn't failing on the Deck. Maybe I'll try to find out which exact dependency it is that makes it work later

@Nocccer
Copy link
Collaborator Author

Nocccer commented Jul 13, 2022

It's definitely not game-specific, the command that's failing is the wineboot one (that's ran for every game to create the prefix). I also cannot even run the wine executable on the Terminal (getting the same error there). This is on a fresh EndeavourOS install (which is pretty minimal in terms of pre-installed stuff), if you have the time, feel free to test yourself.

Although it is good to know that it isn't failing on the Deck. Maybe I'll try to find out which exact dependency it is that makes it work later

I tested on my PC with just setting LD_LIBRARY_PATH to only the wine lib folders and remove all system folders.
Game still starts. Maybe it is some utils that are missing on your VM?

niklas@pop-os:~/.config/heroic/tools/wine/Wine-GE-Proton7-20/bin$ ldd ./wine
	libpthread.so.0 => /lib/i386-linux-gnu/libpthread.so.0 (0xf7f09000)
	libdl.so.2 => /lib/i386-linux-gnu/libdl.so.2 (0xf7f04000)
	libc.so.6 => /lib/i386-linux-gnu/libc.so.6 (0xf7ccf000)
	/lib/ld-linux.so.2 (0xf7f2a000)

These libs are not in the wine lib folders.

@CommandMC
Copy link
Collaborator

There's the next thing I'm wondering about: On the VM, it just says this:

$ ldd ./wine
        not a dynamic executable

Everything behaves like the file doesn't even exist, but it's there:

$ file ./wine
./wine: ELF 32-bit LSB executable, Intel 80386, version 1 (SYSV), dynamically linked, interpreter /lib/ld-linux.so.2, for GNU/Linux 3.2.0, BuildID[sha1]=3f2fbf89cf13086a94e8053b26ddd80c2e003039, stripped

@Nocccer
Copy link
Collaborator Author

Nocccer commented Jul 13, 2022

not a dynamic executable

Do you have a 64 bit VM? I don't know if 32 and 64Bit makes a difference here.
Trying to setup a 64bit VM atm, but VirtualBox does not show me 64bit option. I enabled the AMD option in the bios already ...

@CommandMC
Copy link
Collaborator

CommandMC commented Jul 13, 2022

I don't think you can just make a 32-bit VM, it's up to your host CPU which architecture your VM is

lib32-libelf is the culprit, installing that makes ldd work fine & the executable launches

@Nocccer
Copy link
Collaborator Author

Nocccer commented Jul 13, 2022

I don't think you can just make a 32-bit VM, it's up to your host CPU to decide the architecture

lib32-libelf is the culprit, installing that makes ldd work fine & the executable launches

Modern CPU's like Ryzen's support 64 Bit and 32Bit VM's. I think 64bit support exist for a long time now.
I enabled the wrong Bios option. Now it works. Will test with Ubunutu 22.04 64Bit

@Nocccer
Copy link
Collaborator Author

Nocccer commented Jul 13, 2022

I don't think you can just make a 32-bit VM, it's up to your host CPU which architecture your VM is

lib32-libelf is the culprit, installing that makes ldd work fine & the executable launches

@CommandMC have the same problem like you. Question is what we need to add as dependecy for the user?
https://askubuntu.com/questions/806710/cant-run-elf-32-bit-on-x86-x64-server-even-after-adding-i386

For Ubuntu i can't just install libelf. Needed to install: libc6-i386 and enable i386 before.

@Nocccer
Copy link
Collaborator Author

Nocccer commented Jul 13, 2022

Tried this out on a fresh Ubuntu VM. Like CommandMC i had problems with the wine executable.
This is a problem of missing library:
debian: libc6-i386
arch: lib32-glibc

If this lib is installed, everything works as expected and no system wine is needed. I don't know if it is possible to add this libs as dependencies for deb, pacman and rpm, because they are 32bit libs?

Note:

@CommandMC
Copy link
Collaborator

I don't know if it is possible to add this libs as dependencies for deb, pacman and rpm, because they are 32bit libs?

As far as I know, you can depend on 32-bit packages just fine in the AUR (and in official Arch repos).

A more hacky solution would be to invoke the package manager and see if the package is installed:

$ pacman -Q lib32-libelf
lib32-libelf 0.187-1
$ echo $?
0
$ pacman -Q somepackagethatisntinstalled
error: package 'somepackagethatisntinstalled' was not found
$ echo $?
1

@Nocccer
Copy link
Collaborator Author

Nocccer commented Jul 13, 2022

I don't know if it is possible to add this libs as dependencies for deb, pacman and rpm, because they are 32bit libs?

As far as I know, you can depend on 32-bit packages just fine in the AUR (and in official Arch repos).

A more hacky solution would be to invoke the package manager and see if the package is installed:

$ pacman -Q lib32-libelf
lib32-libelf 0.187-1
$ echo $?
0
$ pacman -Q somepackagethatisntinstalled
error: package 'somepackagethatisntinstalled' was not found
$ echo $?
1

You can add every dependency to electron-builder in the package.json. This will install the package alongside heroic. But i wonder what happens if i386 is not enabled.

@Nocccer Nocccer merged commit 969b8dc into beta Jul 13, 2022
@Nocccer Nocccer deleted the feat/option-toggle-system-libs branch July 13, 2022 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:ready-for-review Feature-complete, ready for the grind! :P pr:ready-to-merge This PR is fully ready for merge.
Projects
None yet
3 participants