DavidoTek / ProtonUp-Qt

Install and manage GE-Proton, Luxtorpeda & more for Steam and Wine-GE & more for Lutris with this graphical user interface.
https://davidotek.github.io/protonup-qt
GNU General Public License v3.0
1.16k stars 39 forks source link

vkd3d: Use fetch_project_releases and fetch_project_release_data #323

Closed sonic2kk closed 6 months ago

sonic2kk commented 7 months ago

This PR applies the same changes in #318, #320, #321 to Valve's vkd3d-proton and the Lutris mirror vkd3d. The vkd3d-proton ctmod is the parent one and Lutris subclasses it, so we set the archive type for each of these.

There are also a couple of other changes made to remove the now-unused ghapi_rlcheck import from a couple of other ctmods (this is used internally in these functions instead).

image

image

image

Once again, there should be no user-facing impact here. The vkd3d-lutris version list may appear small but that's because it is small, it only goes back to v2.4, and it doesn't package all of the same versions as vkd3d-proton (v2.7, for example, is not on the list because there is no vkd3d-lutris v2.7, releases page link). There should be no change in behaviour compared to the main branch.


I wanted to note something here: vkd3d-proton packages its releases as .tar.zst while Lutris uses .tar.xz. In the vkd3d-proton ctmod. So when fetching releases we checked for both archive types in the one ctmod. One side effect of this was that if the format of the release changed, we could simply add on another filetype check. For example if Lutris switched to using .tar.zst as well.

With this change, however, we are locked into assuming one file type for each release. vkd3d and vkd3d-proton have used the same release format for all of their existence as far as I can see so I don't think this is likely to change, but we can consider refactoring fetch_project_release_data to (optionally?) take a list of archive types to check for. That way if vkd3d-proton or vkd3d-lutris changed their release formats with the next version for some reason, it would be straightforward to check.

Although I should note that if any ctmod made a change to their packaging like this we would need to do a refactor (not just by adding another check when fetching assets but also for extraction), so maybe it's a non-issue to assume each project will use only one archive type :smile:


The number of total open PRs was just too low, so I had no choice but to raise it :wink: Thanks!

DavidoTek commented 7 months ago

The number of total open PRs was just too low, so I had no choice but to raise it 😉 Thanks!

:laughing:

Thanks!

There are also a couple of other changes made to remove the now-unused ghapi_rlcheck import from a couple of other ctmods (this is used internally in these functions instead).

Great. Seems like we're increasing the cohesion of the individual functions. I think we can soon look into separating util into util.py and ctutil.py. We're alredy way above 800 lines.

The vkd3d-lutris version list may appear small but that's because it is small [...] There should be no change in behaviour compared to the main branch.

Okay. Yeah I noticed that most of the non-mainstream compatibility tools do not have that many releases. That's to be expected I guess.

vkd3d-proton packages its releases as .tar.zst while Lutris uses .tar.xz. In the vkd3d-proton ctmod [...] if the format of the release changed, we could simply add on another filetype check

That's good. I think now we have many of the "properties" as variables of the CtInstaller (CT_URL, CT_INFO_URL, and now also release_format). I just noticed that it might make sense to put that in a static variable, just like CT_URL etc. But let's not worry about that too much. I will consider that in case I do the "class attribute" refractor as mentioned in https://github.com/DavidoTek/ProtonUp-Qt/pull/314#issuecomment-1836556401

we can consider refactoring fetch_project_release_data to (optionally?) take a list of archive types to check for. That way if vkd3d-proton or vkd3d-lutris changed their release formats with the next version for some reason, it would be straightforward to check. Although I should note that if any ctmod made a change to their packaging like this we would need to do a refactor (not just by adding another check when fetching assets but also for extraction), so maybe it's a non-issue to assume each project will use only one archive type 😄

Hmm, it feels like that it requires quite a lot of effort for such a niece thing. Changing the parameter type from str to List[str] would break the API and require changing it everywhere. Adding a second optional parameter feels a bit unneccesary repeted to me. x: Union[str, List[str]] and type(x) == str: please don't do that :smile:

What we could do is following in case such a thing happens. But I don't want to commit to that, maybe one of the above options is better.

x = fetch_project_release_data(self.CT_URL, 'tar.xz', self.rs, tag=tag)
if not x:  # x == {}
    x = fetch_project_release_data(self.CT_URL, 'tar.zst', self.rs, tag=tag)
return x
sonic2kk commented 7 months ago

I think we can soon look into separating util into util.py and ctutil.py. We're alredy way above 800 lines.

I think this is a good idea, we could even try breaking out the common download logic into a function for this file too. It would be good to clean this up and keep util for anything that can be broadly applicable in the codebase :smile:

I will consider that in case I do the "class attribute" refractor

This crossed my mind too, but for consistency I kept with how I did it in the other PRs :-)

Hmm, it feels like that it requires quite a lot of effort for such a niece thing.

Yeah, I didn't go with it in this PR because it would end up requiring a refactor of the other usages as you pointed out, and that would add too much to re-test from my side and for your side as a reviewer, so I figured keeping the changes isolated was probably the way to go.

As well as this, how to approach such as a change would be a challenge. What you suggested, of simply calling with a different release format argument, is probably feasible. If we need to do this kind of change in future we could do it per-ctmod as it required, could be something like this:

# Maybe this is slightly more readable
release_data = fetch_project_release_data(self.CT_URL, 'tar.xz', self.rs, tag=tag) or fetch_project_release_data(self.CT_URL, 'tar.zst', self.rs, tag=tag)
return release_data

# But this should work just as well
return fetch_project_release_data(self.CT_URL, 'tar.xz', self.rs, tag=tag) or fetch_project_release_data(self.CT_URL, 'tar.zst', self.rs, tag=tag)

# If we ever have to check a significant amount of formats,
# like if some crazy ctmod had tons of versions, we could use a loop
# I don't see this ever being necessary but just documenting in case
for release_format in release_formats:
    if release_data := fetch_project_release_data(self.CT_URL, 'tar.xz', self.rs, tag=tag)
        return release_data

return {}  # Default return