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

Merge PrismLauncher:8.0 to unmojang #116

Merged
merged 397 commits into from
Nov 17, 2023
Merged

Conversation

evan-goode
Copy link
Contributor

Merge tag '8.0' into unmojang-8.0

Resolves: #115

Upstream changelog: https://github.com/PrismLauncher/PrismLauncher/releases/tag/8.0

(Imcomplete) list of changes in 8.0 relevant to PollyMC:

TheKodeToad and others added 30 commits August 11, 2023 00:39
Signed-off-by: TheKodeToad <[email protected]>
Signed-off-by: TheKodeToad <[email protected]>
Co-authored-by: TheKodeToad <[email protected]>
Signed-off-by: Alexandru Ionut Tripon <[email protected]>
Signed-off-by: Trial97 <[email protected]>
Signed-off-by: Trial97 <[email protected]>
Signed-off-by: Trial97 <[email protected]>
Signed-off-by: TheKodeToad <[email protected]>
@evan-goode
Copy link
Contributor Author

FYI, I didn't know this until I Googled "how to review a merge PR" and found https://haacked.com/archive/2014/02/21/reviewing-merge-commits/, but you can run git diff-tree --cc on this commit and it will show only hunks that changed as part of the conflict resolution, which results in only about an 800 line diff.

@lemonzest79
Copy link

Just tried the debug build of this from the github actions and things seem to be working ok after I found the extra info for the api url and stuff as it did not migrate my Ely.by accounts after doing so I could log into my 2fa enabled ely.by accounts (needed to do the usual password:token method as no 2fa input box anymore that was available with 7.2) afterwards I could log into my ely.by enabled servers ok and skins worked.

@evan-goode evan-goode marked this pull request as ready for review November 15, 2023 17:14
@evan-goode evan-goode marked this pull request as draft November 15, 2023 17:14
Upstream changelog: https://github.com/PrismLauncher/PrismLauncher/releases/tag/8.0

(Imcomplete) list of changes in 8.0 relevant to PollyMC:
- Prism has dropped support for Mojang accounts
  - PrismLauncher/PrismLauncher#1656
  - Added it back to PollyMC primarily since the Mojang authentication
    code is also used for authlib-injector accounts. Some users may also
    still want to be able to log in to Mojang accounts for some reason:
    fn2006#115 (comment),
    even if they do not provide authentication to multiplayer servers.
  - To my surprise, I could still log in to a Mojang account, get its
    skin, and launch the game. Authentication to servers, of course, did
    not work.
  - Tested: logging into authlib-injector, Mojang, and MSA accounts and
    launching the game.
- Prism has added a built-in updater for Windows and Linux that I assume
  provides similar functionality to Sparkle on macOS:
  PrismLauncher/PrismLauncher#1268. There is a
  Launcher_UPDATER_GITHUB_REPO variable in CMakeLists.txt that specifies
  the repo checked by the updater. I have changed this to the PollyMC
  repository, and since PollyMC follows almost the same CI/release
  patterns as Prism Launcher, it might work with PollyMC, too, but I
  have not tested it yet.
- Add macOS support to Nix
  - PrismLauncher/PrismLauncher#1485
  - Untested
- The "PollyMC-Linux-8.0.tar.gz" and similar build artifacts were
  renamed to "PollyMC-Linux-8.0-Qt5.tar.gz" to match their
  "PollyMC-Linux-8.0-Qt6.tar.gz" counterparts.
- Removed Windows MSVC Legacy builds
- Removed Quilt Beacon toggle, the beacon is now always disabled:
  PrismLauncher/PrismLauncher#1547,
  PrismLauncher/meta#23
- Prism removed FTB support in 7.0, but I tested creating an FTB
  instance to make sure it still works in 8.0

Signed-off-by: Evan Goode <[email protected]>
@evan-goode
Copy link
Contributor Author

Sorry for the force pushes, I screwed up by merging 8.0 on top of commits from another feature branch, and then re-writing history on the other branch.

Just tried the debug build of this from the github actions and things seem to be working ok after I found the extra info for the api url and stuff as it did not migrate my Ely.by accounts

Thanks for testing. I just filed #117 to auto-migrate Ely.by accounts to the new format, would you mind seeing whether that works for you (once the CI finishes the build you need)? That branch is based on 7.2 not 8.0 FYI.

Maybe we should add a message to that dialog box about 2FA on Ely.by. Adding the dedicated field back is also doable, but it doesn't make much sense since it's an Ely.by-specific feature. If there's some other auth server that expects 2FA formatted like hunter2-12356 instead of hunter2:123456 then it's not gonna work.

@@ -853,6 +894,107 @@ Application::Application(int& argc, char** argv) : QApplication(argc, argv)

detectLibraries();

// check update locks
{
auto update_log_path = FS::PathCombine(m_dataPath, "logs", "prism_launcher_update.log");
Copy link
Owner

Choose a reason for hiding this comment

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

Perhaps change the references to prism_launcher in this block of code to pollymc?
Though changing it would cause more hassle during future merges and I doubt most users care about the name of the update log file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, the updater code has a lot of references to the Prism Launcher name. There's also a prismlauncher_update.cfg file and several lock files/marker files like .prism_launcher_update.lock. I pushed a commit that changes the names of the cfg and log files but leaves most everything else alone.

Honestly, I would prefer not even building the updater (Launcher_BUILD_UPDATER NO) and instead properly package PollyMC for more systems and encourage users to use a package manager. But that's my personal preference, I'm a big time package manager fan. Some people will always still be using the AppImage I guess, and it's nice to have for them.

I don't think changing a few names in the updater will cause too many additional merge conflicts, but it is an awkward thing to test.

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah I agree on preferring proper packages to self updating appimages, the updater mainly benefits Windows users since package managers (WinGet, Choco, etc) are an obscure thing there.

@evan-goode
Copy link
Contributor Author

The updater seems to work on the AppImage, at least to report that there are no updates, but of course I couldn't test the actual updating.

Original authlib-injector work had a bug making offline accounts not
save correctly.
@evan-goode
Copy link
Contributor Author

Test Nix build on macOS, works great. Going to mark this no longer a draft since it's closer to mergeable.

@evan-goode evan-goode marked this pull request as ready for review November 16, 2023 06:17
<!--The ID below indicates app support for Windows 8 -->
<supportedOS Id="{4a2f28e3-53b9-4441-ba9c-d69d4a4a6e38}"/>
<!--The ID below indicates app support for Windows 8.1 -->
<supportedOS Id="{1f676c76-80e1-4239-95bb-83d0f6d0da78}"/>
Copy link
Owner

Choose a reason for hiding this comment

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

Did Prism drop support for Windows 8 and older?
If so I'd like to revert this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it seems Prism 8.0 dropped support for Windows < 10 altogether. I don't think there were ever MinGW Legacy builds (at least released by the CI), just MSVC.

Adding back support would be easier said than done. The motivation for dropping legacy Windows was the EoL of OpenSSL 1.1: https://discord.com/channels/1031648380885147709/1031731112352948245/1171545855526981763.

We could build OpenSSL 1.1 ourselves (or find some other build), but we'd be shipping an EoL crypto library to EoL operating systems...

Copy link
Owner

Choose a reason for hiding this comment

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

Adding back support would be easier said than done. The motivation for dropping legacy Windows was the EoL of OpenSSL 1.1

Ah okay, I assumed they'd just done it for the sake of it but in that case I'm fine with following them in dropping support.

Copy link
Owner

@fn2006 fn2006 left a comment

Choose a reason for hiding this comment

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

Alright finally finished sifting through all the diffs.

I'm assuming you forgot to readd output["type"] = "Mojang"; to else if (type == AccountType::Mojang) at AccountData::saveState() in launcher/minecraft/auth/AccountData.cpp, if you omit that intentionally and I'm wrong about it being needed feel free to correct me.

As for my second comment, does that affect both MSVC and MinGW builds or just MSVC?
If it's the latter I'm fine with just recommending MinGW builds for users of older Windows versions but if it affects both then I'd like it reverted.

Copy link
Owner

@fn2006 fn2006 left a comment

Choose a reason for hiding this comment

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

Alright I'm happy with the state this is in now, merging.

@fn2006 fn2006 merged commit 74e7850 into fn2006:unmojang Nov 17, 2023
10 checks passed
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.