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

Initial native code for verified mod auto-downloading #545

Merged
merged 84 commits into from
Nov 3, 2023

Conversation

Alystrasz
Copy link
Contributor

@Alystrasz Alystrasz commented Sep 10, 2023

Description

This branch allows client to download a mod archive from the Thunderstore API, and extract included mods in the remote mods folder of the current game profile.

Not all mods can be automatically downloaded, as it would cause some security issues, and Thunderstore mod name cannot be deduced from actual mod name: to be eligible to auto-downloading, a mod must appear in the list of verified mods.

Said list and complete mod verification procedure are described here: https://github.com/R2Northstar/VerifiedMods

This branch exposes two commands to test the feature:

  • fetch_verified_mods retrieves verified mods list from the GitHub organization, and stores it locally;
  • download_mod does the actual mod downloading/extraction job.

What this PR is about

  • Downloading a mod archive
  • Extracting said archive to mods folder

What this PR is NOT about

  • Download progress indicator/messages or UI of any kind
  • Mod availability to the game (no reload_mods invoked)

Resources

How to test

  • Download and install DLL file from latest CI build;
  • Run the fetch_verified_mods command;
  • Download a mod with download_mod [mod_name] [mod_version] (only Fifty.mp_frostbite 0.0.1 is listed at the time of writing);
  • Check your remote mods directory, the downloaded mod should appear there.

Media

autodl-pr-opening.webm

Alystrasz and others added 30 commits August 21, 2023 02:55
Copy link
Contributor

@ASpoonPlaysGames ASpoonPlaysGames left a comment

Choose a reason for hiding this comment

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

Logic looks good to me at this point, my previous review comment about maybe making the mods list a convar was more a suggestion than a requirement, so don't consider that blocking

I don't rly know about the hash stuff so dont take my word as gospel about that being good

@Alystrasz
Copy link
Contributor Author

Getting HTTP errors when trying to download the mod.

Command used: download_mod Fifty.mp_frostbite 0.0.1

image

So this bug happens when using the -disablemodverification flag, which I initially used during development to bypass mod hash verification.

Now that most of the development of this feature is done, I feel like it might not be required: even if as a developer you wanna bypass checksum comparison, you still have to create an entry for the to-be-downloaded mod in the local verified mods list (to link a mod name to a Thunderstore repository).

Thoughts?

@EladNLG
Copy link
Member

EladNLG commented Oct 15, 2023

IMO, should fall back to interpreting the mod name as is.

@Alystrasz
Copy link
Contributor Author

IMO, should fall back to interpreting the mod name as is.

I feel like this point goes beyond the flag removal discussion, so I fixed in anyway in 7548c47.
When reproducing your use-case, you will still have an HTTP error, but because mod name is used as dependency string, and thus built URL doesn't exist and cannot be reached.

@Alystrasz Alystrasz requested review from catornot and EladNLG October 16, 2023 22:27
@Alystrasz Alystrasz mentioned this pull request Oct 29, 2023
12 tasks
Copy link
Member

@GeckoEidechse GeckoEidechse left a comment

Choose a reason for hiding this comment

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

Another case of confirmed working in testing.

image

(Loading the map itself after game restart caused a crash but that's unrelated to this PR I think ^^)

@GeckoEidechse
Copy link
Member

I'm gonna give the code one final read again tomorrow but with how many code reviews we have on it already I doubt I'm gonna find anything any more :P

So fingers crossed for merge tomorrow 👀

@Alystrasz
Copy link
Contributor Author

(Loading the map itself after game restart caused a crash but that's unrelated to this PR I think ^^)

image

I confirm this is unrelated to this PR; this will be implemented in a future PR (to be opened once this one is merged) :)

Copy link
Member

@GeckoEidechse GeckoEidechse left a comment

Choose a reason for hiding this comment

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

Aight, so in my own review I only found issues that could be considered to be bike-shedding and so I will move them to separate GitHub issues post merge.

Ofc my C++ knowledge is not that great so I could've missed something so I'm mostly relying on previous reviews made by others as well.

@GeckoEidechse GeckoEidechse changed the title feat: Mod auto-downloading Inital native code for verified mod auto-downloading Nov 3, 2023
@GeckoEidechse GeckoEidechse merged commit 13f2fac into R2Northstar:main Nov 3, 2023
2 checks passed
@GeckoEidechse GeckoEidechse removed the needs testing Changes from the PR still need to be tested label Dec 13, 2023
@Alystrasz Alystrasz deleted the feat/auto-dl branch December 27, 2023 00:10
@Alystrasz Alystrasz changed the title Inital native code for verified mod auto-downloading Initial native code for verified mod auto-downloading May 7, 2024
@Alystrasz Alystrasz mentioned this pull request May 16, 2024
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs code review Changes from PR still need to be reviewed in code
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants