-
Notifications
You must be signed in to change notification settings - Fork 3k
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
base: master
Are you sure you want to change the base?
Conversation
Open questions:
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
it is exactly what it does.
Anything else? EDIT: One more
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.
The same as mpv, you can do 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.
and if it's two files and you remove |
2ff04f0
to
a61b6a5
Compare
Download the artifacts for this pull request: |
303fff9
to
64f22b0
Compare
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. |
I vote for --register. Not a serious reason: Making it looks more different with mpv-install.bat. |
Ok, we can go with (software like madvr / lav filters, were for years using
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.
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. |
a180627
to
885fd00
Compare
Changed everything to
Added.
Added. Any more suggestions? I'm not sure what would be useful to add/change here. |
885fd00
to
b1cfb1b
Compare
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.
b1cfb1b
to
2d4d22f
Compare
I deleted the branch by accident and it nuked the PR, which cannot be reopened for recreated branches. Old PR: #15904