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

[RFC] options: convert some guarded options to new warning mechanism #14624

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Dudemanguy
Copy link
Member

@Dudemanguy Dudemanguy commented Aug 1, 2024

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.

  1. I had to add a bunch of defines because of windows junk. It's not really a big deal but it's kind of ugly imo.
  2. Not really sure how we want to handle suboptions that are conditional and private somewhere. They could just be moved to options.h for example, but then that makes them not-private obviously. Not a blocker for the feature but something to think about.
Copy link

github-actions bot commented Aug 1, 2024

options/options.c Outdated Show resolved Hide resolved
options/options.c Outdated Show resolved Hide resolved
options/options.c Outdated Show resolved Hide resolved
{"android-surface-size", OPT_SIZE_BOX(android_surface_size)},
#endif
{"roundsmall", DWMWCP_ROUNDSMALL}),
.not_available = !HAVE_WIN32_DESKTOP},
Copy link
Contributor

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

Copy link
Member Author

@Dudemanguy Dudemanguy Sep 25, 2024

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.

Copy link
Contributor

@kasper93 kasper93 Sep 25, 2024

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.

Copy link
Member Author

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?

Copy link
Contributor

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?

Copy link
Member Author

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,
Copy link
Contributor

@kasper93 kasper93 Sep 24, 2024

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)
Copy link
Member Author

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.
@na-na-hi
Copy link
Contributor

na-na-hi commented Sep 25, 2024

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.

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.

@Dudemanguy
Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants