-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
[RFC] options: convert some guarded options to new warning mechanism #14624
base: master
Are you sure you want to change the base?
Conversation
c48c610
to
0559d7b
Compare
Download the artifacts for this pull request: |
0559d7b
to
29d5ad8
Compare
29d5ad8
to
31812a9
Compare
31812a9
to
7e56823
Compare
options/options.c
Outdated
{"android-surface-size", OPT_SIZE_BOX(android_surface_size)}, | ||
#endif | ||
{"roundsmall", DWMWCP_ROUNDSMALL}), | ||
.not_available = !HAVE_WIN32_DESKTOP}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe instead of defining all win32 dummy values in meson, we could try to not use them?
{"window-corners",
#if HAVE_WIN32_DESKTOP
OPT_CHOICE(window_corners,
{"default", DWMWCP_DEFAULT},
{"donotround", DWMWCP_DONOTROUND},
{"round", DWMWCP_ROUND},
{"roundsmall", DWMWCP_ROUNDSMALL})}
#else
OPT_NOTAVAILABLE}
#endif
formatting can be fixed to look better, but point is to avoid using those values. I feel like those dummy definitions are polluting too much
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do need to be sure to keep the same valid choices to avoid it erroring out. I could do something like this.
#if HAVE_WIN32_DESKTOP
OPT_CHOICE(window_corners,
{"default", DWMWCP_DEFAULT},
{"donotround", DWMWCP_DONOTROUND},
{"round", DWMWCP_ROUND},
{"roundsmall", DWMWCP_ROUNDSMALL})}
#else
OPT_CHOICE_ZERO(window_corners, "default", "donotround", "round", "roundsmall")
#endif
Only bummer with this approach is that the option names have to be typed twice but maybe not such a big deal.
Edit: Wasn't smart enough to think of a clever macro to shorthand it oh well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do need to be sure to keep the same valid choices to avoid it erroring out.
Can't we just skip checking choices for unavailable options?
EDIT: Also this would actually be required, because we don't want to run string validators on them, those can do platform specific checks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see how we skip setting choices short of skipping the option altogether?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I though the point was to show warning when using not compiled in option. So why it maters what args are used, if it is not available anyway?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well the choices could be set to nothing but then that makes them hard errors on the cli again which is inconsistent with the other ones.
{"js-memory-report", OPT_BOOL(js_memory_report), .not_available = !HAVE_JAVASCRIPT}, | ||
|
||
// Lua | ||
{"osc", OPT_BOOL(lua_load_osc), .flags = UPDATE_BUILTIN_SCRIPTS, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that it will not be hard error we can remove this from test
diff --git a/test/meson.build b/test/meson.build
index 44983f18d0..1b7bf11ff6 100644
--- a/test/meson.build
+++ b/test/meson.build
@@ -119,7 +119,7 @@ if get_option('libmpv')
exe = executable('libmpv-test', 'libmpv_test.c',
include_directories: incdir, link_with: libmpv)
file = join_paths(source_root, 'etc', 'mpv-icon-8bit-16x16.png')
- test('libmpv', exe, args: file, timeout: 60, should_fail: not features['lua'])
+ test('libmpv', exe, args: file, timeout: 60)
exe = executable('libmpv-encode', 'libmpv_encode.c',
include_directories: incdir, link_with: libmpv)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No that test still fails which I'd expect because loading the osc won't work.
mpv has an internal inconsistency where some options are guarded at compile time and others are not. One advantage of guarding certain options is that it prevents users from using something that can't possibly work (e.g. windows-specific options on a linux machine). However, this hurts the portability of mpv config files and means it's possible for configs to break depending on a machine. This is not so nice. Attempt to have our cake and eat it too by introducing a new unavailable boolean which can optionally be set when defining options. Naturally, you just set this to the negation of the preprocessor that applies. Instead of doing a hard error, mpv will print out a generic warning message that you are trying to use an option that cannot ever work with the binary. The downside here is that some windows-specific options have special defines so we have to deal with some if/else junk which makes it a bit uglier.
7e56823
to
c359833
Compare
Is it? The affected options won't work anyway if they are unavailable, and using them only hard quits if they are from the commandline, but doesn't quit if it's in a config file. This PR only changes the logs from errors to warnings and makes no meaningful difference other than allowing commandline usage. |
I feel like I've encountered something that errored out because of this in the past, but I guess I can't think of any examples. |
Here's my attempt at implementing what I suggested in #14618 (comment). The mechanism works fine. I implemented both per option and for sub options.