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

WinUpdater: Check OS and VC++ Redist versions #11051

Merged
merged 1 commit into from Oct 21, 2022

Conversation

shuffle2
Copy link
Contributor

@shuffle2 shuffle2 commented Sep 11, 2022

I've changed the PR so that the metadata is in a loose .txt file next to the Dolphin binary. I think that will make implementing it on MacOS simpler (which someone who knows what they're doing on macos can implement).

The PR results in updating safely failing (no files are changed) the update if any of these conditions occur (note this behavior describes actual PR state, the discussion following this post is outdated):

  1. OS version is lower than one specified by incoming build
  2. VC++ Redist version is lower than one specified by incoming build AND installing the Redist update fails.
    1. The Redist version takes into account the version indicated in the existing dolphin install as well as the one actually installed on the system. This means someone with loose redist DLLs can update Dolphin without triggering the redist update until it's actually required. At that point, they need to arrange the update themselves.

PR ready for review/merge.

The macos implementation should be simple and can be done in followup PR.

@shuffle2 shuffle2 force-pushed the update-vcredist branch 3 times, most recently from f982e63 to a191e88 Compare September 11, 2022 06:46
@Zopolis4
Copy link
Contributor

I am significantly against this in its current state. This will, from what I can tell, force a requirement of admin access which I feel should be made clear and optional.

@shuffle2
Copy link
Contributor Author

Currently if the redist update fails for any reason the failure is ignored.

I assume you’re complaining about this because you are using windows with a user account which does not have ability to install things. you need to understand that is the extreme minority.

@Zopolis4
Copy link
Contributor

I am aware that this is a minority group, but I am nevertheless reasonably against changes that would make me be unable to run dolphin (as this would have done if it required the update to go through. However, since it fails and does not break the rest of the updater, this is fine.

@BhaaLseN
Copy link
Member

We could probably prompt the user if they want to do this, slap the UAC shield on the "yes" button, and if they chose to not do it (or it fails) point them to the URL so they can do it themselves (or contact their administrator to do it for them)

@mbc07
Copy link
Contributor

mbc07 commented Sep 11, 2022

With this PR, does the UAC prompt always appear for the updater or only when it tries to install an updated VC++ Redist?

@shuffle2
Copy link
Contributor Author

shuffle2 commented Sep 11, 2022

We could probably prompt the user if they want to do this, slap the UAC shield on the "yes" button, and if they chose to not do it (or it fails) point them to the URL so they can do it themselves (or contact their administrator to do it for them)

The UAC shield is on the button in the redist's UI, but the /passive argument makes it run as if that "OK" was automatically pressed. So it will pop UAC then (if enabled), and then prompt for admin creds (if your account can't auto elevate). We can simply not pass /passive to require the user click the button. I'm not in favor of it but doesn't really matter, I guess.

With this PR, does the UAC prompt always appear for the updater or only when it tries to install an updated VC++ Redist?

Only if it tries to update the redist (and you have UAC enabled).

It should be noted that redist installation failure currently doesn't change the overall flow - Updater will still try to launch the new dolphin. This is currently fine (apart from obvious fact that user won't have latest redist, then :)). However, if redist install failed and there is a mismatch between the major versions of the redist we probably do want to show a warning and prevent the upgrade. To do that, it may make more sense to integrate into the manifest (or otherwise move the redist install to a point in Updater where preventing the update is still possible).

On a related note, I realized similar functionality can be used by Updater to warn the user / prevent update if they're updating to a build which is targeting a version of the OS higher than what they're currently running. I think that seems useful on all Updater platforms (macOS + windows), so it should be embedded into the manifest.

@shuffle2 shuffle2 force-pushed the update-vcredist branch 2 times, most recently from 004a578 to 1b27d13 Compare September 12, 2022 11:21
@shuffle2 shuffle2 changed the title Updater: Automatically update VC++ Redist WinUpdater: Check OS and VC++ Redist versions Sep 12, 2022
@shuffle2 shuffle2 force-pushed the update-vcredist branch 6 times, most recently from a2e7d34 to 91c4e18 Compare September 12, 2022 21:58
@shuffle2 shuffle2 closed this Sep 15, 2022
@JMC47
Copy link
Contributor

JMC47 commented Oct 21, 2022

This seems to be a more compliant way to handle the Windows Updater without breaking people's stuff. It shouldn't change anything for running Dolphin for most (all?) users, because if you do have a user account, you can manually update VC redists and it should work fine.

@JMC47 JMC47 merged commit 00e23da into dolphin-emu:master Oct 21, 2022
11 checks passed
@shuffle2 shuffle2 deleted the update-vcredist branch October 21, 2022 22:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants