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

Figure out how the update checker changes with this installer #1229

Closed
leijurv opened this issue May 30, 2019 · 10 comments
Labels
Milestone

Comments

@leijurv
Copy link
Member

@leijurv leijurv commented May 30, 2019

First, there's no need to redownload the installer from the site, so the textual instructions for how to update need to be changed.

Second, the code that checks for an update should ideally follow the same logic as the installer (i.e. checking ImpactReleases and verifying gpg) instead of downloading a json from resources as it does now.

Also the impact update checker really needs to stop parsing its version to a double. It works through 4.6 but it broke for 4.6-beta and it wont work with 4.7.0. lol

And also, there's the question of if the installer can come with impact itself lol. Imagine if it were just an update button that Just Did It™️. This would be a little tricky. First, the installer would itself need to be added to the impact jar? Second, settings such as minecraft directory and optifine version would need to be extracted from the currently running json and replicated I guess? This could also be done for updates to dependencies like baritone. We made the explicit decision that a given impact version wouldn't specify any particular baritone version, so that baritone could be updated without having to release a whole new impact. So I suppose that either one having a new version should cause that to pop up. And obviously, to avoid spamming github, we would only download any release if it's newer than the one we have (if github has nothing newer than what's currently installed, theres no need to download any jsons or checksums or signatures). Furthermore, we should check if there is any newer impact version installed to versions. If there is, then they made an explicit decision to downgrade and we shouldnt bug them...... unless theres one newer than their newest installed version XD.

@leijurv leijurv added the Bug label May 30, 2019
@LeafHacker

This comment has been minimized.

Copy link
Contributor

@LeafHacker LeafHacker commented May 30, 2019

it broke for 4.6-beta

We parse it using regex so that isn't completely broken

the impact update checker really needs to stop parsing its version to a double

Yeah, String.compareTo should work

need to be extracted from the currently running json

This is harder than it sounds. From impact we have no way to know what launcher ran us, or what profile we need to update.

The version JSON doesn't know the profile either! We could hack this by passing the profile id and launcher type/location/etc as cli arguments from the profile, but users could edit/remove/break this of course.

the installer can come with impact itself

Yeah agreed, we should expose a public API from the installer and install it as a dependancy. Then we don't need to re-implement stuff like fetching impact versions.

The real question is do we launch the installer GUI or create a update GUI (which uses installer api lib)? If so, should it be a mode of the installer or a Minecraft GUI?

@leijurv

This comment has been minimized.

Copy link
Member Author

@leijurv leijurv commented May 30, 2019

We do know which launcher ran us since we have Mc.gameDir. We can also figure out the location on disk of our current jar using get Resource and parsing the resulting URI. And we can figure out which launcher profile was just launched by reading launcher_profiles.json and getting the most recently selected one (assuming vanilla). Forge is even easier.

I'd say that if we can get all the information we need then we can do the whole thing inline with no extra gui? Idk.

@leijurv

This comment has been minimized.

Copy link
Member Author

@leijurv leijurv commented May 30, 2019

Or maybe special behavior when it's just a hotfix where it's a really easy button.

@LeafHacker

This comment has been minimized.

Copy link
Contributor

@LeafHacker LeafHacker commented May 30, 2019

reading launcher_profiles.json and getting the most recently selected one

Forgot lastUsed was a thing, that is handy. Still can break if they launch multiple instances from different profiles. Is it worth still doing the pass in an arg meme and falling back to this? Or just do this and assume they're not doing anything fancy?

We do know which launcher ran us since we have Mc.gameDir

That only tells us where we are running. While it could be a good hint it's technically unrelated to the launcher dir. Its also user configurable. I'd rather not conflate run dir with launcher dir.

We can also figure out the location on disk of our current jar using get Resource and parsing the resulting URI.

That's an option. Although I wouldn't be surprised if some smart launchers are sharing the libraries directory with vanilla to avoid having two copies of all the libs needlessly.

do the whole thing inline with no extra gui?

Well we at least need a "do you want to update Profile X from launcher Y to Impact Z?" confirmation, and preferably a way to ignore particular updates (while on particular profiles/versions/rundirs?).

@LeafHacker LeafHacker added GUI and removed Bug labels May 30, 2019
@leijurv

This comment has been minimized.

Copy link
Member Author

@leijurv leijurv commented Jun 5, 2019

So I think I've gone 180 on how impact versions should interact with baritone versions in the future, and this issue is the reason for it.

For past versions (up to and including 4.6) that have no updater at all, it made sense to define in their json that any baritone version compatible with their minecraft version was acceptable. This matches the behavior of the maven hotfixing system (when you install you get the most recent baritone), except for that now in the installer, it's locked in place with a hash.

This is suboptimal, because now there is no mechanism to update baritone (and I just fixed a rare crash in baritone like 10 minutes ago, and realized that I have no way to get it out to people with 4.6).

In this instance it's not a big deal (it's extremely rare) but ideally in the future there would be a mechanism to update this.

The reason why this has changed my mind is that if an impact version defines that it works with one of many baritone versions, which you choose while installing Impact, then we have an issue where an update can go out, and only newer installs get it and older ones don't. This will make support almost impossible for one thing... Up until now, if you told me your version of impact, I could determine your version of baritone and it would always be the most recent.

Pains me to say this, but I've changed my mind and I think that the Impact version json should treat baritone just like any other dependency, from now forward. It should define a specific version with URL, hash, and size. This is really also just good practice and brings us to the Impact json fully defining the client itself, without having to separately fetch and trust dependencies (i know both are gpg but the point is the definition). Part of what's good about defining all the other dependencies in this way is that there is no question about what software is actually being run, and this should extend to baritone too. "4.7.0" should mean one specific piece of software including dependencies. Nothing can just change under the hood without knowing about it, which is the motivation to move from hotfixing mavens to cryptographically signed releases with hashes of every dependency (and now it will really be Every dependency).

This isn't a change we should make now to the most recent version json 4.6 or any previous because it has no mechanism to update to patches (4.6.1 etc) without a huge hassle. Concretely, 4.6 and previous should stay exactly as they are.

There are a couple reasons why I was previously against doing this. First, I liked the idea of being able to choose your baritone version separately from your Impact version (like in the installer). But really, this is just a gimmick and no one cares and no one does this. The most recent version has almost 100x more downloads than the second most recent (for a given version of minecraft). Second, I wanted to replicate the hotfixing in maven that previously existed, where I could update baritone and everyone would update automatically. This is the big one, but if this issue is implemented and updating becomes very easy (just the click of a button in-client), then it goes away. In that scenario, it would really be shoving updates down the throat of people who have explicitly desired to stay on their current version. And part of the goal of the installer system was to have no "silent" updates, where "4.6" could mean one of several different jars depending on at what time you downloaded it. Third, I wanted to be able to fix crashes in previous releases of Impact, by just updating baritone. This is just aint it chief, old versions are old versions, and patching anything but the most recent is kinda a yikes. There's nothing special about baritone that means that it should be able to silently auto update to fix crashes when impact doesn't do that. And again, it's updating people who explicitly denied an update. Fourth, I didn't like how any now release of baritone would also be a new patch to Impact. This is probably the largest remaining issue, and there are several ways to implement it. One way we discussed was to separate out the jar from the json, so Impact 4.7.2.json would point to 4.7.2.jar, but if a patch was added to a dependency, then 4.7.2-1.json would be released, still pointing to 4.7.2.jar. This is a massive yikes because it adds two different numbers as a patch, for not really any benefit. It's also bad because patch releases should happen to both the main jar and the json itself (i.e. one of its dependencies or something). i.e. if a new patch is released, the main jar will have to change at least the manifest within itself, so that it knows what version it itself is. (so its hash will change) Workarounds like having the installer edit the manifest are not going to be considered lol. Another approach that I think is fine is to introduce release branches. When 4.7.0 is released, a branch would be made off of 1.12.2 called 4.7-1.12.2. The first tag release would be 4.7.0-1.12.2, and this would also be the gradle version. Patches would be cherry picked onto it. An update to baritone would work the same way as a patch to the client itself. The baritone dependency would be updated in gradle, the patch of impact would be bumped, that would be tagged 4.7.1-1.12.2 and pushed. Circle would build the jar and json, verify that the baritone one is signed (might as well lol), and release 4.7.1-1.12.2. If no other code changes were made, Impact would be the same except its manifest version would be bumped, and the json would be updated too.

Then, the question is how the client should deal with these versions in the future. My view is that when a patch is released to your current version, the client should be almost kinda pushy about updating to it, since all it does is fix bugs. IMO the easier it is to ignore, the more detrimental it is to perception and experience. If everyone installs 4.7.0 and no one updates to 4.7.1 it will suck because there will probably be some crashes. So updating to a patch should be as easy as possible (ahem no adfly). Here's an example of how it could maybe work? When you launch 4.7.0, it should check the releases. There is a newer release 4.7.1 and it has a .asc. Then, it should check if 4.7.1 is installed. If it is, the user has seen 4.7.1, installed it, and decided to explicitly change the profile back to version 4.7.0. Their choice, ok, allow them to continue. If not, then it should pop up a message saying like "A patch (4.7.1) has been released to your current version of Impact (4.7.0). This is not a new version, so it can be added instantly, and will apply on restarting your client. Cancel OK". Cancel should close the game lol. OK should download 4.7.1 and put it in, then update the 4.7 launcher profile to point to 4.7.1 not 4.7.0. Then it should pop up a message saying "Success. Restart your game to enjoy the patches and fixes" or some shit, with only an OK button that closes it. So at this point, if they just hit play again, it'll be 4.7.1. If they ReAlLy have a reason to not update, they can change the version in the launcher for the 4.7 profile back to 4.7.0. Actually, we should remove the cancel button (its kinda hostile to close the game without explanation), and instead have some text explaining this (that they can remain on 4.7.0 by explicitly changing the version back in the launcher). I know it seems a little weird, but the intent is to make updating to patches the default, and staying on the current version the option if that makes sense. Like switching which is easier.

Thoughts?

@leijurv leijurv added the Internal label Jun 17, 2019
@leijurv leijurv added this to the 4.7 milestone Jul 5, 2019
@leijurv

This comment has been minimized.

Copy link
Member Author

@leijurv leijurv commented Jul 15, 2019

Done in 4.7

@leijurv leijurv closed this Jul 15, 2019
@Cerbiac

This comment has been minimized.

Copy link

@Cerbiac Cerbiac commented Jul 16, 2019

Vote up if you read Lejiurv's post start to finish. kek

@leijurv

This comment has been minimized.

Copy link
Member Author

@leijurv leijurv commented Jul 16, 2019

lol it was wild but this is implemented

if it can autopatch, it will give you that option (but it must be vanilla, unmodified profile, etc). if it's anything weird like multimc, forge, liteloader, or if it's a full new release not just a patch, it falls back to a button that just opens the bundled installer so its super ez

@Cerbiac

This comment has been minimized.

Copy link

@Cerbiac Cerbiac commented Jul 16, 2019

Yeah, can't wait to play with 4.7.

@leijurv

This comment has been minimized.

Copy link
Member Author

@leijurv leijurv commented Jul 16, 2019

@Cerbiac have at it lol

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.