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

Lutris-Wine: Simplify by inheriting GE-Proton #498

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

sonic2kk
Copy link
Contributor

@sonic2kk sonic2kk commented Mar 4, 2025

Overview

This PR simplifies the Lutris-Wine Ctmod by inheriting from GE-Proton. This is in a similar vein to what has been done recently with other Ctmods (#491, #494, #486).

In testing, this did not break any functionality when downloading GE-Proton for Steam or Lutris, and Lutris-Wine can still be downloaded as normal. I tested:

  • Lutris-Wine 7.2-2, which does not have any fshack asset attached.
  • Lutris-Wine 7.2, which has both an fshack and non-fshack asset attached to its release. Both downloaded successfully.

Implementation

GE-Proton Ctmod Changes

Some code in GE-Proton was rearranged to make this inheritance possible. Since Lutris-Wine requires a different extract directory name based on the GitHub data (we build the directory name partially from data['version'), we now fetch this data and return the extract directory name in a new method in the GE-Proton Ctmod called __get_data. This calls __fetch_github_data and returns to use the data and the protondir that we expect to extract.

Lutris-Wine Ctmod Changes

A lot of logic is repeated between the GE-Proton Ctmod and Lutris-Wine Ctmod; download and extract, as well as the boilerplate for download progress and such, is all the same. The notable methods we have to override are:

  • fetch_releases - Because we have some additional logic to include fshack as separate versions. The fshack builds are a release asset as part of a single release (release assets are files attached to a release, so we pick off the fshack archives on a release and list them separately).
    • This logic was simplified a bit in this PR from how it was originally implemented.
  • __fetch_github_data - Because we need special logic to say "if we selected an fshack version to download (is_fshack), skip any releases that don't have fshack- in their name"
    • We were even able to use our util#fetch_project_release_data here and give it an asset_condition that facilitates the fshack logic
  • __fetch_data - Because the benefit of implementing it in the GE-Proton Ctmod was to give us a "hook" for child classes to make modifications to the data and/or directory name, so overriding it allows us to specify our custom protondir name.
  • get_info_url - This fixes a bug I noticed while testing that also occurs on main when pressing the "Info" button on an fshack release. Because this works based on tag, it tries to look for lutris-7.2-fshack tag, which doesn't exist since fshack builds are release assets and not dedicated releases with dedicated tags. To fix this, we remove fshack- from our version string before passing it to the GE-Proton Ctmod's get_info_url.
    • Even without this PR, the approach to fixing the bug would've been the same, except without a super().get_info_url call, we would've just did something like return self.CT_URL + version.replace('fshack-', ''). But since I noticed it while working on this PR I thought I would fix it in the scope of this PR, and so the implementation was done this way by overriding the GE-Proton get_info_url :-)

Though GE-Proton doesn't use methods like util#fetch_project_release_data, even if it did we would still need to override here I think, because we would want to create a custom asset_condition. The only change would be that we could structure the code more like this:

    @override
    def __fetch_github_data(self, tag: str, is_fshack: bool):

        if is_fshack:
                    return fetch_project_release_data(self.CT_URL, self.release_format, self.rs, tag=tag, asset_condition=lambda asset: 'fshack' in asset['name'])
        # If not fshack, then we don't need an asset_condition,
        # so just call superclass method
        return super().__fetch_github_data(tag)

Since if GE-Proton didn't need an asset condition theoretically it would also look like return fetch_project_release_data(self.CT_URL, self.release_format, self.rs, tag=tag). It is a tiny bit cleaner, but not a whole lot :-)

However it is worth noting that GE-Proton could use that helper function, the main reason it can't from what I recalled from being in the code again is that GE-Proton needs to get a checksum file and we don't have that functionality right now. If the data returned from fetch_project_release_data could give us data['checksum'], then GE-Proton could move over to it ;-)

Future Work

I noticed while implementing this that Python now has an @override decorator, which I really like. It makes it clearer when reading which methods are overrides from their parent class, helps readibility a lot imo. I've included them in this PR, and will find time to go back and do this to other instances where we override methods if it is desired :-)


Once again, the main benefit here is to reduce duplicated code where we can help it, and it should help pave the way to moving towards a base Ctmod. If we can consolidate Ctmods to have as little repeated code as possible, and only have them implement the custom logic that they need, it should help us begin to see what needs to go into a base Ctmod.

As usual, all feedback is welcome. Thanks! :-)

@sonic2kk sonic2kk force-pushed the simplify-lutriswine branch from 87eef88 to 3be8e88 Compare March 4, 2025 23:18
@sonic2kk
Copy link
Contributor Author

sonic2kk commented Mar 4, 2025

Had to do a scary rebase because I forgot to sync my fork before working on this 😅 Shouldn't have broken anything.

@sonic2kk
Copy link
Contributor Author

sonic2kk commented Mar 6, 2025

Note that the __get_data approach could be a beneficial pattern for other Ctmods too, and not just a hack to implement the functionality for this PR. I have a proof-of-concept of how we could use this to allow the DXVK Nightly to inherit from the base DXVK Ctmod.

Open to discussing this approach though, maybe there's another path to achieve what we're looking for (even happy to discuss having them as two separate methods if we want).

@sonic2kk
Copy link
Contributor Author

sonic2kk commented Mar 6, 2025

Copying what I said in #499 (comment) for visibility:


Actually, this annotation may only be available in Python 3.10. Looking at the PEP (PEP 698), I believe it's only in 3.12, but we currently build against Python 3.10 for the AppImage. Python 3.10 is still getting security updates until October 31st 2026, so it is not really worth bumping just to get this extra feature.

I should really have my ProtonUp-Qt virtualenv based on at Python 3.10, not 3.12... Would need to test this against 3.12 to make sure this works, although looking at the docs, the Python 3.10 docs don't mention @override, but the Python 3.12 docs do.

Will need to confirm if this decorator is actually available to us.

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.

1 participant