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

Added: Support for running tools via Proton(tricks) and REDmod on Linux #1989

Merged
merged 19 commits into from
Sep 11, 2024

Conversation

Sewer56
Copy link
Member

@Sewer56 Sewer56 commented Sep 5, 2024

This PR adds support for:

  • Detection of protontricks
  • Running tools via Proton(tricks)
  • Running REDmod via Protontricks
    • Handles versions 1.8.0 upwards. Including breaking working dir change in upcoming version.
  • Diagnostic for missing protontricks
  • Diagnostic for missing REDmod
    • Clicking the link takes you directly to relevant store page.
  • Fixes incorrect casing of redMod.exe (previously redmod.exe)
  • Overlay messagebox with only 'OK' button
    • This is no longer used, I've kept this in case anyone thinks it will be useful.
    • Don't request changes if you don't want this, just remove it and push.
RedModDeploy.mp4

image

image

image

Design

The screenshots above were already shown to design; with no further feedback requested for the time being.

It may be useful to print out the list of REDmod(s) that caused for a certain diagnostic to be emitted, however we've decided to wait for now.

QA Matrix

Check the following:

  • Hyperlinks for REDmod DLC work on all stores, Windows and Linux
  • Nexus App via AppImage (you may get 'missing protontricks' error)
  • Cyberpunk 2077 without REDmod installed
  • Anything else you can think of.

Issues I'm aware of:

Tags

fixes #1982
fixes #1774
fixes #1915

@Sewer56 Sewer56 requested a review from erri120 September 5, 2024 17:46
@Sewer56 Sewer56 self-assigned this Sep 5, 2024
}
else
{
var batchPath = fs.GetKnownPath(KnownPath.EntryDirectory).Combine("Resources/Cyberpunk2077/deploy_redmod.bat");
Copy link
Member

@erri120 erri120 Sep 9, 2024

Choose a reason for hiding this comment

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

This won't work. I'd suggest making the bat file an embedded resource and creating the file at runtime in the temp directory or similar.

To prevent antivirus software from going crazy on Windows, we'd only want to embed on Linux.

Copy link
Member Author

Choose a reason for hiding this comment

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

If I recall correctly, I brought this up at standup.

Generating a .bat and extracting to disk was the thing I originally wanted to do.

However the people there requested I don't do that and make it a static file that ships with the App pre-extracted instead.

So I'm rather confused, why am I being asked here to do what I originally intended to do? This shouldn't be a case of something like Working Directory vs $OWD for AppImage.

There is the fact I didn't make the .bat file get copied on Linux only, but that's a separate fixup.

Copy link
Member

Choose a reason for hiding this comment

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

Did you test this with an AppImage? I want to avoid using CopyToOutputDirectory if possible, as it can make packaging difficult.

Copy link
Member Author

@Sewer56 Sewer56 Sep 9, 2024

Choose a reason for hiding this comment

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

Good point. Let me do that after I fixup CI.
Edit: And redownload game.

Copy link
Member Author

@Sewer56 Sewer56 Sep 9, 2024

Choose a reason for hiding this comment

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

Took me a bit to respond. I just downloaded Cyberpunk 3 times, because Steam would give me errors after downloading the entire 80GB.

Works fine on AppImage, just need it to not copy to output on Windows.
The batch script is not executed directly, so executable permission (or lack of) doesn't apply to it.

Copy link
Member Author

@Sewer56 Sewer56 Sep 10, 2024

Choose a reason for hiding this comment

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

I made the file not copy on Windows, however it depends on the Host OS the App was built in; due to MSBuild limitations.
Details in the commit contents.

Copy link
Member

Choose a reason for hiding this comment

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

I still think we should go with the embedded route. CopyToOutputDirectory is just a hassle.

Copy link
Member Author

@Sewer56 Sewer56 Sep 11, 2024

Choose a reason for hiding this comment

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

I wanted it too, if you remember however; standup requested I didn't. I followed their request.

Putting it as an embedded resource will make it embed in Windows too, due to same limitations of MSBuild that I'm suffering from here that require this terrible hack.

I don't want some silly AV software to have the wrong impressions because it sees a valid looking batch script starting with @echo off embedded raw within the binary. Unless you'd want me to compress it or something to hide it.

At the end of the day, security on Windows is all terrorism really. Be it the protection racket called code signing or otherwise.

Copy link
Member

@erri120 erri120 Sep 11, 2024

Choose a reason for hiding this comment

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

Putting it as an embedded resource will make it embed in Windows too

How? What you have currently only copies for Linux. EmbeddedResource can have a condition as well, the same condition should work here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I thought you wanted me to use the standard machinery here because the .targets file was unsightly to see.
If it's embedding via the targets file, then that would work.

@LukeNexusMods
Copy link
Collaborator

@Sewer56 Whats the best way to test this?

@Sewer56
Copy link
Member Author

Sewer56 commented Sep 9, 2024

@LukeNexusMods Try to install a REDmod like
https://www.nexusmods.com/cyberpunk2077/mods/9450/ to your game. You'll know it succeeded if files are generated in the r6/cache/modded folder on deploy (as seen in video clip)

I listed the things to watch out for QA in the opening post.

Today I became a PS2 developer. I acquired an EmotionEngine.

Unfortunately, the only emotion I experienced, was a lot of frustration as a developer.
2 hours wasted, and no decent solution that works within the confines of MSBuild.
@github-actions github-actions bot added the status-needs-rebase Set by CI do not remove label Sep 10, 2024
Copy link
Contributor

This PR conflicts with main. You need to rebase the PR before it can be merged.

Copy link
Contributor

This PR doesn't conflict with main anymore. It can be merged after all status checks have passed and it has been reviewed.

@github-actions github-actions bot removed the status-needs-rebase Set by CI do not remove label Sep 11, 2024
@Sewer56
Copy link
Member Author

Sewer56 commented Sep 11, 2024

Re-tested with main merged, including AppImage. We're all good.

@Sewer56 Sewer56 merged commit 0630524 into main Sep 11, 2024
12 checks passed
@Al12rs Al12rs deleted the run-protontricks branch September 18, 2024 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
3 participants