-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Add tristate user option #3376
Add tristate user option #3376
Conversation
mesonbuild/coredata.py
Outdated
def validate_value(self, value): | ||
if value not in self.choices: | ||
raise MesonException('Value %s not one of accepted values.' % value) | ||
optionsstring = ', '.join(['"%s"' % (item,) for item in self.choices]) | ||
raise MesonException('Value "%s" for combo option "%s" is not one of the choices. Possible choices are: %s.' % (newvalue, self.name, optionsstring)) |
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.
[Flake8]
[F821] undefined name 'newvalue'
docs/markdown/Build-options.md
Outdated
@@ -60,6 +61,23 @@ default. | |||
|
|||
This type is available since version 0.44.0 | |||
|
|||
### Dependency | |||
|
|||
A `dependency` options has tree states: `required`, `not-required` or |
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.
options -> option, tree -> three (3)
docs/markdown/Build-options.md
Outdated
and instead of returning their string representation it returns `true`, `false` | ||
or [`Disabler`](Reference-manual.md#disabler) respectively. | ||
|
||
It is indented to be passed directly to the `required` keyword of |
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.
indented -> intended?
I'm also considering naming it: UserRequirementOption. Any preference? |
Hmm, it's not the same thing if dependency() returns not-found or a Disabler. I'm wondering if we should let user decide between both. Maybe the tristate we want is dependency('foo', required: get_option('my-tristate-option'), disabler_if_not_found: true) |
mesonbuild/coredata.py
Outdated
self.value = self.validate(newvalue, user_input) | ||
class UserRequirementOption(UserComboOption): | ||
static_choices = ['required', 'not-required', 'disabled'] | ||
def __init__(self, name, description, value, yielding=None): |
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.
[Flake8]
[E301] expected 1 blank line, found 0
mesonbuild/interpreter.py
Outdated
if isinstance(required, RequirementHolder): | ||
required = required.tobool() | ||
if not isinstance(required, bool): | ||
lib = dependencies.ExternalLibrary(name, None, |
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.
[Flake8]
[F821] undefined name 'name'
9e96f97
to
b7213d6
Compare
The first 2 commits are unrelated stuff I've seen while reading code, I could move to separate PR if needed. Here is what this PR does:
I'm still not really satisfied with names but it's not that bad IMHO. I think functionally it is what we need for GStreamer (described here). |
Sorry to dive straight into the bikeshedding but I really don't like the naming at all :) Why not just "feature" and enable/disable/auto? Much more intuitive IMHO. (Have to still look in more detail at the actual patchset.) |
That's what I suggested 5 months ago: #2411 (comment) But IIRC @jpakkan didn't like... Edit: actually you suggested it and I agreed. |
I don't remember him agreeing or disagreeing, and in any case I think we should just propose what makes most sense to us and then we can discuss it and take it from there. The reason 'feature' makes sense to me is because it's good UI and it's intuitive. Such optional feature/whatever options are not necessarily linked to dependencies, and the word 'requirement' is to me the exact opposite of what these are about, as I see it. |
Voilà, dropped unrelated patches (will open separate PR) and renamed to "feature" with "enabled", "disabled", "automatic" values. I also stopped making it a special kind of object in the interpreter, initially I did that because I wanted to add methods on it, but I think it's much simpler to use plain strings. So basically dependency() and friends accept |
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'll do another review of this once #3528 is merged, since I'm sure the two PRs conflict.
mesonbuild/coredata.py
Outdated
@@ -400,6 +408,7 @@ def get_builtin_option_default(optname, prefix='', noneIfSuppress=False): | |||
'backend': [UserComboOption, 'Backend to use.', backendlist, 'ninja'], | |||
'stdsplit': [UserBooleanOption, 'Split stdout and stderr in test logs.', True], | |||
'errorlogs': [UserBooleanOption, "Whether to print the logs from failing tests.", True], | |||
'automatic_features': [UserFeatureOption, "Override value of all 'automatic' features.", 'automatic'], |
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 call this "feature_options" because nothing else here refers to options.
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.
Good idea.
mesonbuild/interpreter.py
Outdated
if not isinstance(required, bool): | ||
raise InterpreterException('required must be boolean.') | ||
required = required_to_bool(required) | ||
kwargs['required'] = required |
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 really don't like this.
- The function doesn't actually convert the value of
required:
to abool
: it's a tristate. - Using strings means people will do string comparison in their build files, which sucks.
- People might pass strings to
required:
instead of the return value ofget_option()
, which should not be supported. - We lose context. With an object, we can print an error message that tells the user which option disabled or enabled a specific dependency.
The return value of get_option()
should be an opaque object with the following methods on it: enabled()
, disabled()
, auto()
.
mesonbuild/interpreter.py
Outdated
lib = dependencies.ExternalLibrary(libname, None, | ||
self.environment, | ||
self.compiler.language, | ||
silent=True) |
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 should print a message here saying that the dependency was disabled by xyz option.
docs/markdown/Reference-manual.md
Outdated
@@ -1505,6 +1509,8 @@ the following methods: | |||
library is searched for in the system library directory | |||
(e.g. /usr/lib). This can be overridden with the `dirs` keyword | |||
argument, which can be either a string or a list of strings. | |||
Since *0.46.0* the value of a [`feature`](Build-options.md#features) option | |||
can also be passed to the `required` keyword argument. |
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.
Please update all versions to 0.47.0
docs/markdown/Build-options.md
Outdated
|
||
- `enabled` is the same as passing `required: true`. | ||
- `automatic` is the same as passing `required: false`. | ||
- `disabled` cause it to always claim the dependency couldn't be found. |
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.
disabled
should not claim that the dependency couldn't be found. It should explicitly say that the dependency was not searched because it was disabled by xyz option.
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.
There are two different cases here: it can return either a not-found dependency (even if the dependency could be found) or a disabler. Should both of these be possible to do?
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.
That's PR #3386.
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.
Hmmmmm. I'm on the fence on whether they should be separate kwargs or should there be only one...
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.
Hmmmmm. I'm on the fence on whether they should be separate kwargs or should there be only one...
Are you speaking about the use_disabler
or the required
kwarg?
If what you mean is keeping required
kwarg as boolean, and add an extra kwarg for feature option (that's what you did in the dolphin PR) I think it's a bit weird too because those 2 kwarg would be mutually exclusive.
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.
Ah ok, that was my very first idea when I started working on this but I changed my mind on this. The reason is that it's not up to the user to decide the mechanism to use but up to the project developer.
For example if the developer wants to rely on the disabler mechanism he would code like this and expect to have either no app built or app with dep:
dep = dependency('foo', required: get_option('foo'))
app = executable('app', dependencies : dep)
But then if user does -Dfoo=return_not_found_without_looking
it will fail to build app because app cannot be built without dep.
So if we have 4 states it means the project must support both code paths, which completely defeat the point of having disabler to simplify the build system.
dep = dependency('foo', required: get_option('foo'))
if dep.found()
app = executable('app', dependencies : dep)
endif
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.
Note that one better possibility here is to have 3 states enabled/disabled/auto, but get_option('a-disabled-feature')
could actually return a disabler
object. That would mean no change is needed to dependency() because it would receive either true/false boolean or disabler
object.
I decided against that idea too for 3 reasons:
- Using disabler is nice but often require to refactor a lot your meson.build. Even if the result is probably often cleaner, that's still more work to get feature option adopted.
- the return value of get_option() becomes much harder to use if it's not passed directly to
required
kwarg. You cannot do stuff like this because you whole if block gets ignored:
if get_option('a-disabled-option').disabled()
....
endif
- You would still need to add
use_disabler
kwarg anyway for the case where you have not-required dependency but without a user option, and you could still want to use disabler mechanism in that case:
dep = dependency('foo', required: false, use_disabler : true)
app = executable('app', dependencies : dep)
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.
Converting a "not found" into a disabler (I'm assuming that you forgot to add that kwarg to the snippet above) does have use on its own. In fact, I'd wager that it would become fairly popular for some dependencies. Something like:
d = dependency('foobar', use_disabler : true)
If we wanted to replicate that with the option it would need to have five states:
auto
required
return_not_found_without_looking
return_disabler
return_disabler_if_not_found
Which gets overly complicated. The two kwarg approach seems like the correct thing to do here.
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.
d = dependency('foobar', use_disabler : true)
Hm, I'm not sure about returning disabler for a not-found required dep. It means you can do something if it's not-found... then you should have required:false
...
Or maybe I didn't understand your 5th state.
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.
Those two are unrelated. You are right that it should probably have required : false
.
The 5th state is what we would need to have if we don't have use_disabler
kwarg. But we should, since it simplifies things a lot.
mesonbuild/interpreter.py
Outdated
required = required_to_bool(required) | ||
kwargs['required'] = required | ||
if required is None: | ||
return ExternalProgramHolder(dependencies.NonExistingExternalProgram()) |
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.
Same here, print a message.
mesonbuild/interpreter.py
Outdated
required = required_to_bool(required) | ||
kwargs['required'] = required | ||
if required is None: | ||
return DependencyHolder(Dependency(name, {})) |
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.
Same here, print a message.
mesonbuild/coredata.py
Outdated
@@ -158,6 +158,12 @@ def validate_value(self, value, user_input=True): | |||
', '.join(bad), ', '.join(self.choices))) | |||
return newvalue | |||
|
|||
class UserFeatureOption(UserComboOption): | |||
static_choices = ['enabled', 'disabled', 'automatic'] |
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.
The option values should be short, so at least "automatic" should be "auto". And perhaps enabled/disabled should be true/false (which is already what people use).
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 like the idea of using true/false, that is what I ended up using in my combo options for the kind of thing this solves. To me, it's basically a boolean with an extra setting for when you do not want to force either.
If you wish to stick with enable/disable, I would use enable/disable instead of enabled/disabled.
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 also like true
/false
/auto
or yes
/no
/auto
. We will have to / should support true
/false
in any case, as people will turn boolean options into feature options with autodetect and that should be smooth. Also it's the most intuitive from a user point of view I think.
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.
Actually, thinking again about this, true/false is confusing because false
would have 2 different meanings when passed to `required' kwarg.
required: false
has the behaviour of 'auto'.required: get_option('an-option-set-to-false')
has the behaviour of a disabled feature.
I'm changing my mind in favour of enabled/disabled/auto
because that's the more explicit.
The return value of get_option() should be an opaque object with the following methods on it: enabled(), disabled(), auto().
See, why would you have a method enabled()
if the value is true
?
We say in English: "You should enable this feature" and not "You should true this feature" or "You should yes this feature".
Thanks for the comments, my first implementation was using a new object type but I decided to go with string because it simply the implementation. Before continuing working on this I would like @jpakkane to give his opinion to avoid going in one direction then change again. |
I also think passing an object around makes most sense on the meson build definition side, because it avoids the awkwardness of the type handling (is it a boolean? is it a string? is it something else? what is it?) An object makes it clear and unambigious and makes the build definitions much nicer to read. And it allows to pass around extra context, which will be useful in future. It also matches existing meson patterns like |
Note I also prefer true/false/auto actually, to mean it's a boolean with a 3rd state. And if projects convert a 'boolean' option to a 'feature' option it won't break existing scripts that have -Dfoo=true. |
I seem to remember booleans being case insensitive, do we want to handle |
It is case insensitive in command line, that's one of my first meson contribution a few months ago. |
AFAICR people worked out the requirements for this during the GStreamer hackfest a few weeks ago. It would be nice to see the result of that before committing. (I don't remember seeing that, but if it has been posted and I missed it, please post a link below.) The important things include:
|
@jpakkane I think the review @nirbheek did above are the only comments GStreamer had to make. As far as I know it's only implementation issues rather than functionality.
Not sure to understand why you mean. I guess you can always do: x264_dep = []
if get_option('video_codecs')
x264_dep = dependency('x264', required : get_option('x264_feature'))
endif
We could do both. I think it's useful to have get_option(), for example my demo patch for GStreamer use get_option() to avoid calling subdir() if the option is disabled. |
Our goal is to start with the minimal viable feature (tm) that will solve 99% of our use cases. We are confident we can then add bells and whistles on top of that. The immediate decision required now is whether it should be an object with methods on the |
Can you post an example of the kind of thing you have thought about using this in GStreamer and how it simplifies the build definitions? |
Before: option('foo', type: 'combo', choices ['yes', 'no', 'auto'], value: 'auto') foo_opt = get_option('foo')
foo_dep = []
if ['auto', 'yes'].contains(foo_opt)
foo_dep = dependency('foo', required : false)
if foo_opt == 'yes'
error('-Dfoo=yes passed, but foo not found')
else
message('-Dfoo=auto passed, and foo not found')
endif
endif After: option('foo', type: 'feature', value: 'auto') foo_dep = dependency('foo', required : get_option('foo')) The difference may seem small, but when multiplied with the tens of plugins in gstreamer, it's a lot. This same construct can also be used by all other projects, including GNOME (who will want Other advantages:
|
mesonbuild/coredata.py
Outdated
@@ -175,6 +175,11 @@ def validate_value(self, value, user_input=True): | |||
', '.join(bad), ', '.join(self.choices))) | |||
return newvalue | |||
|
|||
class UserFeatureOption(UserComboOption): | |||
static_choices = ['enabled', 'disabled', 'auto'] | |||
def __init__(self, name, description, value, yielding=None): |
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.
[Flake8]
[E301] expected 1 blank line, found 0
I updated the PR to address some of the review comments. What's left to discuss:
|
I don't mind if something is printed, as long as it doesn't talk about found/not-found if it didn't actually check because the feature has been disabled. So for example: if the Naming:
|
Yeah, this is a bit of a mess. I did start looking at consolidating that (see https://github.com/jon-turney/meson/tree/consolidate-dependency-check-report), but it's very hairy... I agree with tp-m that this does need good logging about what meson is doing, or confusion will ensue. |
18c6523
to
5884274
Compare
Rebased on master and added logging.
Currently it is enabled/disabled/auto everywhere: meson_options.txt, object.xyz() and -Dfoo=xyz. I think it's better to be consistent and use the same name everywhere, no? The reason I changed my mind against yes/no or true/false is (as I said in some comment above) it's confusing when passed to the |
I still think it should be Maybe someone else wants to opine. |
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.
Naming nitpicks/bikeshedding aside I really like this.
docs/markdown/Build-options.md
Outdated
@@ -62,6 +63,36 @@ default. | |||
|
|||
This type is available since version 0.44.0 | |||
|
|||
### Features | |||
|
|||
A `feature` option has three states: `true`, `false` or `auto`. It is intended |
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.
Does this need to be updated?
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.
yes, done.
mesonbuild/coredata.py
Outdated
@@ -549,6 +566,7 @@ def parse_cmd_line_options(args): | |||
'stdsplit': [UserBooleanOption, 'Split stdout and stderr in test logs.', True], | |||
'errorlogs': [UserBooleanOption, "Whether to print the logs from failing tests.", True], | |||
'install_umask': [UserUmaskOption, 'Default umask to apply on permissions of installed files.', '022'], | |||
'feature_options': [UserFeatureOption, "Override value of all 'auto' features.", 'auto'], |
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.
Wonder if we can come up with a better option name here that better captures what it actually does :)
auto_feature_default
, feature_auto_default
, auto_feature_override
, feature_auto_override
, feature_auto_default_override
, feature_auto_select
, dunno.
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 initially named it 'automatic_features', @nirbheek suggested 'feature_options', but I agree it would be nice to have 'auto' in the name somehow since it really only apply to auto features. I think auto_features_override
is what describe it the best but it's a bit long. Maybe just auto_features
.
e.g. meson -Dauto_features=disabled
that makes sense, it reads as "automatic features are disabled".
Naming things is really the hard part here... maybe we pay too much attention to small details we'll get used to 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.
I like -Dauto_features=disabled
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.
Ok, changed to that, also added a bit more doc and release snippet.
This is a special type of option to be passed to most 'required' keyword arguments. It adds a 3rd state to the traditional boolean value to cause those methods to always return not-found even if the dependency could be found. Since integrators doesn't want enabled features to be a surprise there is a global option "auto_features" to enable or disable all automatic features.
The most important question is whether this fullfills everything that GStreamer needs? (for extra bonus points, has it been actually tried) There is an unfortunate naming thing with the value |
@jpakkane: I think the naming is perfect as it is. We are talking about user interface here, it really is good as it is now. I don't see any confusion with the Disabler(), as that is not user-facing but something entirely different (and in any case it's the same thing conceptually just a different mechanism, they don't even do different things in the end). In addition there is another PR (#3386) that will allow developers to link the I think this PR provides what GStreamer needs, yes. And it would take care of one of our main blockers :) I think we should get this in and then build on that. (@nirbheek might have more implementation comments, I'm just talking about the interface and what it provides.) |
I updated the corresponding patch for gst-plugins-good here: https://bugzilla.gnome.org/show_bug.cgi?id=795107 |
The only thing that irks me is that having the
instead of
But this is hair splitting territory and we can add that later in a different PR if it is deemed to save enough keystrokes. Feature-wise this is ok. If no-one has implementation comments, then feel free to merge. |
Yes, there are other such improvements, but as you also said, we think those can be incrementally. |
I just wish we got syntax sugar to map |
Many projects need a tristate option, often named
enabled
,disabled
andauto
.enabled
means the feature must be built, and if dependencies are missing it should abort.disabled
means the feature must not be built even if dependencies are found.auto
means the feature should be built only if dependencies are found, skip it otherwise.This could already be achieved with a combo option:
But it is much nicer to do: