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

osdep/w32_install: add install/uninstall options for Windows #15912

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

kasper93
Copy link
Contributor

@kasper93 kasper93 commented Feb 19, 2025

I deleted the branch by accident and it nuked the PR, which cannot be reopened for recreated branches. Old PR: #15904

@kasper93
Copy link
Contributor Author

kasper93 commented Feb 19, 2025

Open questions:

  • Should --install be renamed to --register?

I think users are more familiar with install word. There were voices that install has to move binary somewhere else, but looking at definition https://www.merriam-webster.com/dictionary/install

to set up for use or service

it is exactly what it does.

  • Should --install in fact move mpv.exe into user programs?
    I prefer keeping it portable, the --install / --uninstall command are not tied to installation itself. For example you can move binary, install again and it will work. There is no need to uninstall anything, before moving.

  • Should archive-exts list be added to mpv's supported formats
    I don't see a reason why not. It is still user decision to select application to use. This is handled externally by Windows. Although I agree, mpv it likely not primary choice for archives, it's nice to have it suggested in "open with" drop-down.

Anything else?

EDIT:

One more

  • Do we want icon for this mpv-install, mpv-uninstall thing?
    And if, so contributions welcome. It would be awkward to use the same icon as for mpv.

Most softwares' installer, main exec and uninstaller are detached. And not all sw use the same dir for main exec and uninstaller.

Yes we don't have dedicated installer, which drops dozen of files everywhere. My objective is to make this as clean and light as possible, while still making things work in Windows.

Some installers could detect the installed one and could even directly uninstall it.

The same as mpv, you can do mpv --uninstall from any mpv binary and it will work correctly. In fact while --install is handy to be in mpv, because it can be parametrized easily for used extensions, without creating whole another application or using installer for that. The --uninstall command is mostly portable. Though I still don't like the idea of dropping files in random places. But this is one of the questions above. If people want this, we can copy the .exe to programs...

Though I see no real issue, if someone uses mpv.exe and removes it, nothing bad happens... Worse case thay have stale entry in control panel, which will be pruned by windows. We can also completely skip uninstall entry in control panel if this is the main concern. But I still don't see why if user breaks it, it's mpv's fault.

mpv is the only special case I've meet that it could do three jobs by the same single file.

and if it's two files and you remove uninstall.bat what happens? mpv.exe is single binary, we really don't need full installer to handle it. Also mpc-hc, foobar2000, lav filters, 7zip all have in place installation. foobar2000 even does it on every start of application. madvr/lav filters have bat files, that lives along .dll, but it's still calls in the .dll regsvr. So if you remove them, well you no longer can uninstall.

@kasper93 kasper93 force-pushed the mpv-reg-windows branch 3 times, most recently from 2ff04f0 to a61b6a5 Compare February 19, 2025 03:07
Copy link

github-actions bot commented Feb 19, 2025

Download the artifacts for this pull request:

Windows
macOS

@kasper93 kasper93 force-pushed the mpv-reg-windows branch 3 times, most recently from 303fff9 to 64f22b0 Compare February 19, 2025 06:22
@avih
Copy link
Member

avih commented Feb 19, 2025

Docs changes LGTM. Thanks.

I don't feel strongly about any of the open questions.

Moving the mpv binary would indeed make it a normal installer, but it's more work. I'd imagine there's a good reason that usually an installer application is used (inno setup etc), because it looks simple but I imagine there are many details to take care of - which personally I'm not familiar with.

I think registering mpv inplace is enough. Adding an actual installer would be fine too, but I don't think it's required.

Re the option name, I think the fact that it adds an uninstall entry at the control panel (which is a good thing) kind of makes us choose "install", for consistency. But "register" also a valid choice, because it is indeed a better description of the action.

Bottom line, either one would be OK IMO.

Re icon, not necessary IMO but it would be nicer to have one. I don't think mpv icon would be too bad, but also, there are enough free icons out there, and a generic cog icon or some such would do fine IMO. Or like the original PR did, pick an icon from an existing system file. Just ensure/test that it still works (does the registration/whatever) even if the file is missing.

@hooke007
Copy link
Contributor

I vote for --register. Not a serious reason: Making it looks more different with mpv-install.bat.

@kasper93
Copy link
Contributor Author

kasper93 commented Feb 19, 2025

I vote for --register.

Ok, we can go with --register and --unregister, while install/uninstall term might be more familiar to regular joe when manipulation software. Also note I won't use --deregister, contrary to what might seem more fighting according to https://english.stackexchange.com/questions/25931/unregister-vs-deregister, but the --unregister feels nicer and is what also Windows itself used for operations like that.

(software like madvr / lav filters, were for years using install, hance why this was my first choice, but no matter)

Moving the mpv binary would indeed make it a normal installer, but it's more work. I'd imagine there's a good reason that usually an installer application is used (inno setup etc), because it looks simple but I imagine there are many details to take care of - which personally I'm not familiar with.

After consideration this is not a job for this command. It looks simple if you forget that normally you have more than single executable to package. Creating installer is still possible and valid on top of this registration command. Note that it can be self-expanding archive, which would serve similar purpose. The point is if you have to handle more files, like additional .dlls, icons, whatever. It makes only sense to have separate packaging solution.

But we need to distinguish what this PR does, which is registration of media plater from packaging the mpv into self-extracting or self-installing single-click solution.

Also like I was hinting on IRC, once you start moving things, you end up looking like malware https://devblogs.microsoft.com/oldnewthing/20230911-00/?p=108749 :) That's of course a joke and not applicable to mpv, but if you want to uninstall the uninstall.exe itself, it's always funny exercise.

Bonus chatter: If we were using cmake, we would have all sort of packaging solutions for free with cpack that integrates with install command. Meson doesn't have this convenience.

Re icon, not necessary IMO but it would be nicer to have one. I don't think mpv icon would be too bad, but also, there are enough free icons out there, and a generic cog icon or some such would do fine IMO. Or like the original PR did, pick an icon from an existing system file. Just ensure/test that it still works (does the registration/whatever) even if the file is missing.

This is in fact detail. I went back and forth few times, changing this install/uninstall helpers from showing command line window and not. And I settled to show it, which means, it won't have manifest or icon. But that's fine... it shows little tiny command line icon.

@kasper93 kasper93 force-pushed the mpv-reg-windows branch 2 times, most recently from a180627 to 885fd00 Compare February 19, 2025 16:17
@kasper93
Copy link
Contributor Author

I vote for --register. Not a serious reason: Making it looks more different with mpv-install.bat.

Changed everything to register. If feels awkward at places, but I will get used to it.

Perhaps the documentation should be updated to indicate that installation is required to make STMC show the icon and program name.

Added.

Please consider changing the first sentence to something along those lines, if you too think it can make it clearer.

Added.


Any more suggestions? I'm not sure what would be useful to add/change here.

@avih
Copy link
Member

avih commented Feb 19, 2025

Please consider changing the first sentence to something along those lines, if you too think it can make it clearer.

Added.

Thanks.

Apologies for wasting both out time due to me not reading the whole section carefully before embarking on a discussion.

This wrapper is meant to run mpv.exe, not anything else.
On Linux, we have `mpv.desktop`, and on macOS, we have `osxbundle`, both
of which handle file associations and protocol registration in the
system environment.

On Windows, this information is stored in the registry, so this commit
adds support for it.

It registers the application, supported file types, and supported
protocols, and adds an uninstall entry so users can remove all
registrations via the control panel. Note that this does not remove the
binary itself.

The implementation is fully portable. There are no external installers,
as mpv handles everything automatically. This should improve usability
when moving binaries and so on.

- `mpv --register` registers mpv (see verbose output for a list of actions).
- `mpv --unregister` reverts all changes made during installation.
- `mpv --register-rpath <string>` allows specifying a string to be
  prepended to `PATH` before running mpv. This is useful when using
external dependencies that shouldn't be added globally to `PATH`.
They call `--register` / ``--unregister` for users who don't want to open
console and do it manually, this is one-click solution.
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.

3 participants