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

Adjust for restructured verified-mods.json #748

Merged

Conversation

Alystrasz
Copy link
Contributor

@Alystrasz Alystrasz commented Jul 8, 2024

Updates the launcher code to deal with adjusted verified mods JSON structure defined in R2Northstar/VerifiedMods#41.

The idea here is to allow installing mods from other sources than Thunderstore.

(Somehow) supercedes #642.

(should be merged with R2Northstar/VerifiedMods#41!)

TODOs

Testing

  1. To make your client use the verified mods list's new format, launch Northstar like this: .\NorthstarLauncher.exe -customverifiedurl=https://raw.githubusercontent.com/R2Northstar/VerifiedMods/refactor/include-mod-source-in-version-links/verified-mods.json
  2. Join mp lobby;
  3. Set allow_mod_auto_download to 1 through console (allow_mod_auto_download 1);
  4. sv_cheats 1;
  5. Fetch manifesto: script NSFetchVerifiedModsManifesto();
  6. Download mod: script NSDownloadMod("nyami.mp_brick", "1.0.2");

Looking at your logs, mod should be downloaded normally.

Each mod version now having its own download link, having a global
Thunderstore URL is therefore useless.
At the time, we only know how to install mods from Thunderstore,
so we throw an error when extracting mods from elsewhere.
@Alystrasz Alystrasz marked this pull request as draft July 9, 2024 23:25
@Alystrasz Alystrasz marked this pull request as ready for review July 30, 2024 17:45
@GeckoEidechse
Copy link
Member

bumping so it shows up in the webhook channel and hopefully someone tests it :>

@GeckoEidechse
Copy link
Member

Quick question until I get around testing this, what happens when we merge this PR but not R2Northstar/VerifiedMods#41 ?

Does client joining a server that requires verified mods crash or not?

Cause with #749 and #751 merged and released only recently, there's still clients that run older versions as the fetch happens before the version gate check, so even if a client would fail version gate, they'd crash before that already due to trying to fetch invalid manifest.
Hence I'm wondering if we should give R2Northstar/VerifiedMods#41 some more time but at the same time already prepare client for new structure by merging this PR as soon as it's tested/reviewed ^^

@Alystrasz
Copy link
Contributor Author

Quick question until I get around testing this, what happens when we merge this PR but not R2Northstar/VerifiedMods#41 ?

Does client joining a server that requires verified mods crash or not?

Cause with #749 and #751 merged and released only recently, there's still clients that run older versions as the fetch happens before the version gate check, so even if a client would fail version gate, they'd crash before that already due to trying to fetch invalid manifest. Hence I'm wondering if we should give R2Northstar/VerifiedMods#41 some more time but at the same time already prepare client for new structure by merging this PR as soon as it's tested/reviewed ^^

If we merge the launcher part but not the VerifiedMods counterpart, client won't crash but manifesto parsing will fail (without crashing with #749), thus preventing all mods from being downloaded.

@GeckoEidechse
Copy link
Member

If we merge the launcher part but not the VerifiedMods counterpart, client won't crash but manifesto parsing will fail (without crashing with #749), thus preventing all mods from being downloaded.

Ok that's what I was hoping for and would be temporarily be acceptable as MAD is behind a feature flag anyway. This way we can push out this PR even as part of a patch release as the effect is not noticeable by clients under normal circumstances and for dev stuff we can just pass the CLI flag to change the manifest location to that of the PR.

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.

Following the instructions, I get "Tried to download a mod that is not verified, aborting" in console.

Do note that this happens with every combination of using the old and new manifesto and using this PR and most recent release.
So most likely the issue is not in the PR itself but maybe the instructions or something else.

@GeckoEidechse
Copy link
Member

Setting this PR to draft for now until resolved so that I know which PRs are relevant to be looked at atm.

@GeckoEidechse GeckoEidechse marked this pull request as draft August 19, 2024 15:07
@Alystrasz Alystrasz marked this pull request as ready for review August 28, 2024 12:17
@Alystrasz
Copy link
Contributor Author

Following the instructions, I get "Tried to download a mod that is not verified, aborting" in console.

So this is due to the local verified mods manifest which is empty (due to NSDownloadMod not fetching manifest from GitHub, woopsies); NSFetchVerifiedModsManifesto must be invoked before installing mods.
Testing instructions were updated accordingly.

@GeckoEidechse GeckoEidechse self-requested a review August 29, 2024 12:30
@GeckoEidechse GeckoEidechse added needs code review Changes from PR still need to be reviewed in code almost ready to merge Apart from any small remaining other issues addressed by other labels, this would be ready to merge labels Aug 29, 2024
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.

Confirmed working in testing :D

image

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.

Code looks good :)

@ASpoonPlaysGames ASpoonPlaysGames added READY TO MERGE This mergeable right now and removed needs code review Changes from PR still need to be reviewed in code almost ready to merge Apart from any small remaining other issues addressed by other labels, this would be ready to merge labels Sep 7, 2024
@GeckoEidechse GeckoEidechse changed the title feat: Adjust for restructured verified-mods.json Adjust for restructured verified-mods.json Sep 7, 2024
@GeckoEidechse GeckoEidechse merged commit 8c546ed into R2Northstar:main Sep 7, 2024
4 checks passed
@Alystrasz Alystrasz deleted the feat/update-for-new-verified-json branch September 7, 2024 22:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
READY TO MERGE This mergeable right now
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants