GE-Proton: Refactor to use fetch_project_release_data
#524
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
This PR implements a long-standing TODO item of mine, which is to implement fetching checksum data on
fetch_project_release_data
. This allows ctmods that need can validate checksums (currently only GE-Proton and Proton-CachyOS) to migrate to usingfetch_project_release_data
without losing thechecksum
functionality.The main benefit here is that we can continue to bring, and to reduce duplication in child classes.
As an aside, I did test with Proton-CachyOS and it can be refactored to use this method as well (we can use
checksum_suffix=f'{arch}.sha512sum' so that it gets the checksum for the correct release asset even), and it is compatible with the changes in #523. So this additional functionality in
fetch_project_release_data` should be flexible enough for general use, I didn't want to implement something that just worked based on how GE-Proton might use it but how any ctmod might use it.Implementation
util.py
A few small changes were needed in
fetch_project_release_data
.We need to check all release assets now, so we can't
break
sadly, since we don't know if the asset we want will be before or after the main asset. Otherwise, the logic to fetch the asset we want is the same.To fetch the checksum, we take a new optional parameter to
fetch_project_release_data
calledchecksum_suffix
. Note thatchecksum_suffix
may not simply be a checksum type, but could include extra information to differentiate it from other checksum assets in the same release. This allows us to specify which checksum we want, as a release may have multiple checksums in its release assets. Proton-CachyOS is a good example, since it has multiple checksums in a release that are both.sha512sum
.There is an argument to be made that this may not be sufficient for more complex release asset names, and a regex may be more appropriate, but the same argument could be made for the
release_format
parameter where it may not be sufficient to differentiate releases. I think broadly this should be fine and generally, checksums by convention are named with the same name as the file they are hashed from but with a different suffix (i.e.archive.tar.gz
would have a checksum ofarchive.sha512sum
). So unless there comes a point whererelease_format
is not sufficient, then we should be fine withchecksum_suffix
as a generic string.If
checksum_suffix
is passed tofetch_project_release_data
, and if ourvalues
dictionary does not already have a checksum found inside of it (i.e. we have not found our desired checksum asset yet), then we want to find of the current asset has a checksum URL on it. To do this, we can re-useget_download_url_from_asset
with all of the same arguments that we give to it above to get the archive asset URL (i.e. the tarfile asset for GE-Proton) except that we want to find the download URL for the asset with therelease_format
ofchecksum_suffix
. If this gives us back a non-empty string we can assume that we found a release asset with thechecksum_suffix
that we expect, so we can assume that that is a checksum asset and we can insert its URL into ourvalues
dictionary undervalues['checksum']
.Note that if
checksum_suffix
is not generic enough, then the first found instance of an asset matching this string will be included. This is how it would work before though when used inside of__fetch_github_data
, so it's just a matter of usage. In later iterations of the loop, even if another asset matching our checksum asset is found, it will not be overwritten, since we don't go through the logic of callingget_download_url_from_asset
if ourvalues
dict already has achecksum
key (i.e. if we already found a checksum earlier, don't try to find one again).With all of that,
fetch_project_release_data
can return achecksum
in itsvalues
if we specify achecksum_suffix
to look for, which allows Ctmods like GE-Proton to use this method and still get back a checksum.GE-Proton Ctmod
The only change needed inside of GE-Proton is to replace all of our logic inside of
__fetch_github_data
with a call tofetch_project_release_data
. The only missing functionality in that util function before preventing it from being used in the GE-Proton ctmod was that it could not return a checksum.We pass
fetch_project_release_data
achecksum_suffix
of.sha512sum
. This is very broad, but this matches what we were passing to GE-Proton before, so I kept the same checksum name check here too.That is the only change needed to the GE-Proton ctmod to get it over to using the
fetch_project_release_data
util function, and now as a result, GE-Proton is fully refactored to using our util functions and doesn't have any network API calls to manage itself!Concerns
I am making an assumption here that checksum assets on GitLab will also use the
url
key the same way they do for all other release assets. We do not currently have any Ctmods using GitLab that verify a checksum, and I do not know any GitLab projects off-hand to check that have checksums in their assets to see what they return. This assumption may turn out to be wrong, and we may need to refactor in future if that is the case. But I think it is reasonably sane to assume that all release assets on GitLab will use the same key on the API, regardless of whether they're release tarballs or checksum files or something else :-)Also, this PR does modify
fetch_project_release_data
which used by other ctmods. More testing may be warranted to ensure nothing has broken :-) In my tests things seem fine, but it's late and I did not exhaustively test yet.Future Work
Since the logic in this PR to fetching checksums in this PR is not limited to sha512, in future we may want to make the way the GE-Proton ctmod verifies hashes a bit more generic. We may want to extract the logic that does the checks out of
get_tool
and into a single method that can be overridden by child classes, if ctmods that subclass GE-Proton were to use other checksums.If necessary, some of the functionality here could be broken out into a separate util file (something like
hashutils.py
), which might allow other ctmods with checksums to more easily integrate without having to copy around boilerplate code.Hope that explains the rationale for the changes made here well enough. More Ctmod refactoring! One day I'll realise my goal of a base Ctmod class 😉
Thanks!