Rework installation of windows games #624
Conversation
|
Thanks! I really appreciate this work. I'll review it this week, hopefully tonight. |
|
@sharkwouter You're welcome. Take all the time you need. There's more to come. |
|
Cool! It's best to keep PRs small, preferably a single feature. That makes it much easier to review. This one is fine, though, I can review this one. |
There was a problem hiding this comment.
Hey @GB609, thanks again for this PR. I found some small things, but this is looking pretty good. I added one little note for myself and some for you. This does look like a big improvement, though.
|
I think I found an issue with the shortcut fix in here: |
|
I would suggest leaving out the menu shortcuts fix for now. We can figure that out later. |
|
This change does actually fix 1nsane, which is really cool to see. |
|
I found this: https://documentation.help/Inno-Setup/topic_setupcmdline.htm We can actually use the |
|
You might want to prevent Innoextract from being used for extraction honestly. It looks like with this change innoextract is being executed to extract the game and then the extracted files are fully ignored. |
|
I found another bug. If the innoextract installation works, it is not possible to launch the game anymore with this change, since the game files are not inside the prefix and the new code does not account for that. |
Where is the issue with the shortcut here? This is the installer you are running. Shortcuts (=*.desktop files) are not generated for the install command, but for the game's launch command |
I saw this as well. But i wasn't sure how many of the installers actually support that switch (or most of the others). After i got some issues with the /SILENT switch for some games i tried to use only as few setup switches as possible. And the same effect of the 'exporting' the windows shortcuts can be achieved with winemenubuilder=d, so why bother with uncertain windows apis? |
It does. |
I've got another code addition that makes usage of inno configurable. Just didn't include it here to keep the changes as small as possible. But why do you think the extracted files are ignored? I haven't changed the entry point to windows installation in installer.py def extract_windows(game: Game, installer: str, temp_dir: str, language: str, use_innoextract: bool):
err_msg = extract_by_innoextract(installer, temp_dir, language, use_innoextract)
if err_msg:
err_msg = extract_by_wine(game, installer, temp_dir)
return err_msgAnd i didn't really touch the innoextract method. If innoextract install does not create an error, it will just skip over wine. Otherwise the wine method is used. Of course the wine installer itself can not use the temporary files extracted by innoextract because those are the actual content of the installer, not the installer itself. If i try to use the pre-extracted files directly, i might miss out on whatever the self-extracting installer does as preparation step. But i'll try to run a manual test for that to verify that innoextract still works and wine is used as fallback if not. There are 2 flows where i could see problems:
Second variant
|
I tried the changes to the installer with as many games as i could without going bankrupt for now. If you followed the game test table in #622 you could see what i've tested with. Even asked for help in terms of install test reports. |
If i leave it out, shortcuts created for wine games will break and not work anymore unless i let the code generate a wrapper start.sh script to use instead. But if you really don't want that fix and have doubts about it, i can remove it again. |
|
There was one game which I tested for which innoextract did not fail and then the installer was used afterwards to do the installation. The innoextract files were not deleted. I'm sorry about not remembering which game that was. |
No, it's fine. This part needs a rework or addition. I mostly focused on wine and don't even have inno installed. |
|
You should install innoextract. It is also used for determining what languages are supported by the installer. If you don't have it, you'll probably always get English, not 100% sure. It has been ages since that code was added. |
|
I agree that i should also test with inno as long as both shall be supported (which still makes sense to have another fallback). Maybe it's better to reverse the current logic then. First try wine and fall back to innoextract. From looking over the code i can't see how innoextract would not fail and the wine installer is started afterwards, unless some of the file move and remove operations in extract_by_innoextract just don't work as expected. def extract_windows(game: Game, installer: str, temp_dir: str, language: str, use_innoextract: bool):
err_msg = extract_by_innoextract(installer, temp_dir, language, use_innoextract)
if err_msg:
err_msg = extract_by_wine(game, installer, temp_dir)
return err_msgAnd thus wine installer should not run. There's just no path out of extract_by_innoextract with a none-empty return that would also leave files in the temporary directory. Honestly, if the change wasn't so large, i would rewrite the entire installer.py to throw Errors instead of handling everything with returning error_messages. That would largely unclutter the code and make error handling so much easier. But i would have to change a lot of tests for that. That's just too much to do at once. The entire language customization is done only in (and for) the innoextract installer currently. No value from extract_inno related to language is passed to the installation in wine. And the installation in wine never used the extraction results from inno. Even before my change the installer in wine would simply be pointed to the temporary directory that was also used by innoextract before to install/overwrite what's in there. |
|
I don't think we need to support both if using the installer works fine honestly, but we do use Innoextract for language stuff here: minigalaxy/minigalaxy/installer.py Lines 376 to 395 in a2c04d3 |
Exactly. But the method lang_install you point out is only called from extract_by_innoextract itself. There is no other occurrence yet. Maybe i can leverage that could to help search for the game language. I'm fine with dropping innoextract for installation otherwise. Windows games must run within wine anyway. If the install already fails, successfully running a manually extracted game will always a be a bit of a gamble. |
|
I just found that /NOICONS does not work unfortunately. We might have to remove the shortcut files manually. That is unrelated to this PR, though. |
That's why i set winemenubuilder=d for the wine command running the install. The windows shortcuts will be generated by the install and exist in the wineprefix directory, but they will NOT be exported to the linux application menu. |
In my tests this causes wine to crash when it tries to create shortcuts, though. Does wine have an option that prevents this? |
Thats strange. Ok, i abbreviated the command. It must be |
|
Okay, my bad, seems the crash was unrelated. I got confused by the error about creating shortcuts, but it does seem to work now I did another test. |
There was a problem hiding this comment.
Okay, I looked into it once more and did some more testing. What I found is that most of my comments were either unrelated or you clarified them.
I also created a merge request for your branch to resolve some issues with the exe_cmd function. I hope that is welcomed and helps you out.
If the left over comments are resolved, I will merge this. Then we can look into left over wine related issues. I don't think this PR is creating new ones at the moment apart from the ones by the _exe_cmd change.
|
I found that the reason for a successful innoextract being seen as a failure is that the check to see if it failed only looks at if the stderr contains something. If it does it fails. This is addressed in the change I made in the PR I linked. Otherwise this seems to be a big improvement. I just wanted to clarify that. I would also like to thank you for pushing me to take another look at this code. There are some issues in it which we are finding now because of that. |
I was under the impression that extract_by_inno did not check any string returns from the _exe_cmd, but rather checked that the return code is != 0. The only place where i have seen any evalution of stderr output is in extract_linux that does some special exception when return code ==1 and a certain string is found in the output. Anyway, thanks for the feedback. Glad it's useful. I think it's better to fix stuff upstream if possible. Wouldn't want to maintain my own fork in the long run. About the stderr separation. Can i suggest a simpler way? I'll try to finish up the open points tonight if possible. |
|
I was thinking about that, since stderr output will be smaller. We could just do stderr.read at the end. I'll update my PR to your branch. |
* All installations now try to use a fixed folder 'c:\game' within the wineprefix as install target. * Wine installer tries unattended mode first and falls back to wizard dialog on failure The fallback will break if user changes the target directory in the wizard, but it's better then not working at all and the directory requirement can be documented. * Adjust launcher.py to use wine-internal paths whereever possible
use env for wine when launching
* Fix _exe_cmd in installer.py * Only print if print_output is on * Set stdout and stderr to non-blocking in exe_cmd function
GB609
force-pushed
the
feature/rework_wine_installer
branch
from
772d6f4 to
22de635
Compare
|
I think i've got all for now. It's not as complete as i've wanted it to be, but we can do the rest in subsequent feature branches. |
|
Thanks! |
Description
This PR offers a complete rework of the installer.py:extract_windows installation method for windows games.
The changes to the installer and how wine environment variables are handled made it also necessary to touch the launcher to correctly configure wine prefixes on launch.
Wherever necessary and possible, wine commands now use 'env' as prefix for configuration, Minigalaxy will not modify its own environment any longer.
I have also used shlex for parsing and command generation to improve handling of quotes and spaces. This makes it possible to use env variables or arguments containing spaces in the game properties (if quoted correctly).
installer.py:_exe_cmd was also reworked to use poll() and readline instead of communicate(). The reason for that is that wine can be very verbose with its stdout and i'm trying to prevent process buffers from filling up and thus effectively halting an installation. I think i've had this with a game that also installed directx afterwards where using communicate would never return because the directx installation was apparently frozen.
The downside of these changes is that they are not entirely free of potentially breaking backwards compatibility. Games that save directory paths to registry and were installed through wine before this PR might break (although it is more likely they didn't work before).
I've tried to keep the effects as small as possible, by adding a few lines of code in launcher.py which try to restore the c:\game link on game launching. Desktop shortcuts are not regenerated and launching over shortcuts won't apply the fix.
But i've also added a note in the readme about this. If that is not the right place, just tell me where and what i shall add instead.
The initial change to the installation is also included in #622, but with some enhancements to also handle umu. The fallback solution is new to this branch.
Checklist
Edit: Adding TODOs from discussion here for easier tracking:
2516779478:
2516801895:
2520299969