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

Patch openssl, add AppImage interactive option, create mesa wrapper script #35

Merged

Conversation

peeley
Copy link
Contributor

@peeley peeley commented Aug 17, 2023

This PR does the following:

  1. Uncomments the portion of patcher.sh that patches the runner binary to point to libcurl instead of openssl 1.0.0
  2. Moves the wrapper script for Mesa support into its own file
  3. Adds an option in interactive mode to allow for producing the native binary instead of the AppImage

Regarding item 1, I've tested on NixOS following the installation docs in the README using patchelf 0.15.0 and experienced no issues. I believe that the bug mentioned in previous versions of patchelf is no longer an issue.

I'm attempting to get the AM2RLauncher package working on NixOS, and these changes have allowed me to do so when pointing the launcher at my fork of this repo with these changes. Any comments/feedback would be appreciated :)

peeley added 2 commits August 13, 2023 13:47
After testing manually on NixOS, it appears that `patchelf 0.15.0` is
able to properly patch out references to openssl libs in favor of
libcurl.

This change also adds a prompt for installing as an AppImage, allowing
the interactive mode to also install AM2R as a native binary.
This is necessary to fix a bug in AM2RLauncher, wherein the game crashes
on launch because the Mesa workaround is not ported/included by the launcher.
@Miepee
Copy link
Contributor

Miepee commented Aug 17, 2023

Thanks for the PR!
Unfortunately, I won't be able to have the time to review this for a bit, so please have some patience while I get the time. If I haven't given a review in a month, shoot me another ping.

Some other things which also may be important:

  • This was commented out back then, because it meant that this project suddenly has a very new requirement, which would make it unable to be run on slightly older distros, like Ubuntu 20.04. What is currently the status with patchelf 0.15.0? Are there any distros still using an older version, where the bug that doesn't patch this properly exists? Because while I think that patching security vulnerabilities out is very important, I'd also like this to not have such a hard cutoff, which may mean that this should be an option provided (with true as default, and probably no way to change it in interactive mode).

  • In which way does your WIP nixos am2rlauncher package require this?

  • Also requires some thinking on how the am2rlauncher should handle this. Should it patch only when a new enough version of patchelf is available? also expose a setting for it? only patch for the non-appimage ifdef? You probably already made some thoughts on that. Any changes that make the launcher use this also need an update on the flatpak build, but that's nothing too major.

  • Would be nice if there was a way where we can detect whether the user's installed patchelf version is high enough to not have the bug, and throw a user readable error if that's the case. As otherwise it'd mean that patching is done, script says everything was all right, but the runner was screwed up in a way where it doesn't run anymore properly, which would be bad UX.

@peeley
Copy link
Contributor Author

peeley commented Aug 29, 2023

No worries, I'm pretty busy too (apologies for such a late reply)! I've also pushed a new commit to my branch that uses a new --patchopenssl flag to do enable OpenSSL patching, given this is an alternative to AppImage and should only be done in limited scenarios. Regardless, to answer your concerns:

  • Ubuntu 20.04 uses patchelf 0.10, and that version appears to work. Ubuntu 23.04 uses 0.14.3, which also appears to work. The latest version of patchelf is 0.18.0, which also works after some local testing. To be totally honest, I've hard a hard time reproducing any kind of bug originating from patchelf during my testing.
  • You can check out my WIP fork of AM2RLauncher here (you can ignore flake.* files, those are package definitions for the NixOS repo). In short, AppImages are incompatible with NixOS due to assumptions AppImage makes about dynamic linking. As a result, AM2RLauncher has to build and launch the native binary. The current AM2RLauncher does not include the Mesa wrapper bugfix, which needs to be included in this repository for it to be copied into the PatchData directory that the launcher creates.
  • In my fork of the launcher, I've also added a PATCHOPENSSL constant that enables the launcher to run patchelf during the profile installation step. Without the presence of this constant, the launcher should skip over the OpenSSL patching. The Mesa wrapper is still copied over, however.

@Miepee
Copy link
Contributor

Miepee commented Aug 29, 2023

I've hard a hard time reproducing any kind of bug originating from patchelf during my testing.

Just to double check, you tested this by running the patcher and then try to run the game, right? I would need to dig out the conversations I had with IvarOnBones regarding this, but when I tried to use it (probably on Arch? I really can't remember, sorry), the binary just became corrupted where it wouldn't even run properly anymore. Worth putting on the list for me I suppose when I go properly review the PR.


Checked out the am2rlauncher fork. In general seems nice, but have a few gripes i'll get to when you'll actually PR the changes. (Alternatively, you can also hop into #am2r-development in Matrix/Discord, and we can have a chat there).

@peeley
Copy link
Contributor Author

peeley commented Aug 30, 2023

Just to double check, you tested this by running the patcher and then try to run the game, right?

Yep, the command I've been using to test so far is ./patcher.sh -m --patchopenssl && ./am2r_1.5.5/runner (from the repository root).

I'll probably hold off on upstreaming the AM2RLauncher fork until this is stabilised, it could also use some cleanup too.

Copy link
Contributor

@Miepee Miepee left a comment

Choose a reason for hiding this comment

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

apart from small typo, looks fine to me otherwise

@Miepee
Copy link
Contributor

Miepee commented Aug 31, 2023

FWIW, this was the original patchelf bug report
NixOS/patchelf#214

I'd have to try later if i can repro it on arch, but since it's now optional, it shouldn't be much of an issue.

Co-authored-by: Miepee <38186597+Miepee@users.noreply.github.com>
@peeley
Copy link
Contributor Author

peeley commented Sep 2, 2023

Awesome, thank you!

Regarding the bug report - the changes to patcher.sh use replace-needed rather than remove-needed (and it appears that replace-needed doesn't exhibit the binary corruption behavior), so hopefully we're good to go.

@Miepee Miepee merged commit 1ed9efc into AM2R-Community-Developers:master Sep 2, 2023
@Miepee
Copy link
Contributor

Miepee commented Sep 2, 2023

Thank you for the PR :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants