Utility methods for feature objects #8512

bonzini · 2021-03-10T13:29:37Z

This pull request adds utility methods to operate on feature objects. These methods should cover most needs for manual creation of feature objects (as in #5206). For example:

if opt_video.disabled()
    opt_video_x11 = opt_video
    opt_video_wayland = opt_video
    ...
endif

can be changed to this:

opt_video_x11 = opt_video_x11.require(opt_video)
opt_video_wayland = opt_video_wayland.require(opt_video)
...

The other method I am adding, may_disable_if, is useful to skip possibly expensive checks when it's known that a library will not be used.

xclaesse · 2021-03-10T15:57:50Z

mesonbuild/interpreter.py

+        return 'disabled' if not self.held_object else self.held_object.value
+
+    def as_disabled(self):
+        return FeatureOptionHolder(None, self.name, None)


Better pass a UserFeatureOption() instead of None, so you can remove the value property and all those changes.

Further, you're breaking the type annotations, they don't allow this to be None. So you either need to fix the type annotations, or use a UserFeatureOption(). I'd also prefer the latter.

docs/markdown/Reference-manual.md

xclaesse · 2021-03-10T16:16:47Z

docs/markdown/Reference-manual.md

+- `require(value, error_message: '')` *(since 0.58.0)*: returns
+  the object itself if the value is true; an error if the object is
+  `'enabled'` and the value is false; a disabled feature if the object
+  is `'auto'` or `'disabled'` and the value is false.


So this is the same as may_disable_if() except that it is an error if the user set that feature to enabled when it cannot be supported, right?

This makes me wonder if we even need may_disable_if(), the way I see feature options is that if user enable one it's should be a guarantee that if for any reason that cannot be satisfied they get an error and not a silently disabled feature.

may_disable_if() does not degrade enabled to disabled, only auto. This is why it has a slightly more complicated name than disabled_if().

The design should cover exactly what you mention here. The idea is that you use

libone = dependency('one', requires: get_option('one')) libtwo = dependency('two', requires: get_option('two').may_disable_if(not libone.found())

This way:

with -Dtwo=auto libtwo won't even be searched unless libone was found

with -Dtwo=enabled it will be an error if libtwo is absent, even if the build will end up not using it because libone is absent.

Hmm, I'm not really convinced by may_disable_if() tbh, do you have real use-case in qemu for that? It feels like .require() is easy and intuitive, but may_disable_if() is for subtle corner case that I'm not even sure to really grasp.

What about renaming value to condition here for clarity?

Could also change the error_message kwarg to be simply an optional 2nd positional argument, just like assert(condition, 'optional message')?

Maybe add working like "This describes a condition required to allow enabling this feature" ?

xclaesse · 2021-03-10T16:18:46Z

docs/markdown/Reference-manual.md

+       error_message: 'DirectX only available on Windows').allowed() then
+  src += ['directx.c']
+  config.set10('HAVE_DIRECTX', 1)
+endif


I find it more readable when not packing everything in the condition:

directx_opt = get_option('directx').require(host_machine.os == 'windows', error_message: 'DirectX only available on Windows' ) if directx_opt.allowed() ... endif

xclaesse · 2021-03-10T16:22:10Z

mesonbuild/interpreter.py

+        if not isinstance(args[0], bool):
+            raise InvalidArguments('boolean argument expected.')
+        error_message = kwargs.pop('error_message', '')
+        if error_message and not isinstance(error_message, str):


nitpicking

Suggested change

if error_message and not isinstance(error_message, str):

if not isinstance(error_message, str):

xclaesse · 2021-03-10T16:23:02Z

mesonbuild/interpreter.py

+
+    @noPosargs
+    @permittedKwargs({})
+    def allowed_method(self, args, kwargs):


@FeatureNew

xclaesse · 2021-03-10T16:23:14Z

mesonbuild/interpreter.py

+        return self.value == 'auto'
+
+    @permittedKwargs({'error_message'})
+    def require_method(self, args, kwargs):


@FeatureNew

xclaesse · 2021-03-10T16:23:52Z

mesonbuild/interpreter.py

+        return self.as_disabled()
+
+    @noKwargs
+    def may_disable_if_method(self, args, kwargs):


@FeatureNew and @typed_pos_args

xclaesse · 2021-03-10T16:29:02Z

mesonbuild/interpreter.py

+
+        if self.value == 'enabled':
+            prefix = 'Feature {} cannot be enabled'.format(self.name)
+            prefix = prefix + ': ' if error_message else ''


We could do a smart default error message, see how assert() uses AstPrinter to print the condition.

get_option('directx').require(host_machine.os == 'windows')
that would print Feature directx cannot be enabled: host_machine.os == 'windows'

I thought about it, but it's the opposite, in this case host_machine.os != 'windows'. It's also much more user-oriented than assert(), so an expression might not be very friendly.

dcbaker · 2021-03-10T18:00:35Z

mesonbuild/interpreter.py

+            raise InvalidArguments('Expected 1 argument, got %d.' % (len(args), ))
+        if not isinstance(args[0], bool):
+            raise InvalidArguments('boolean argument expected.')
+        error_message = kwargs.pop('error_message', '')


Please don't pop here, use get, Let's avoid mutation if possible.

jpakkane · 2021-03-12T16:58:57Z

opt_video_x11 = opt_video_x11.require(opt_video)

I don't really grasp why this is the way it is. You have to have had looked up opt_video_x11 already and overwriting a variable with itself is always a bit suspect. For example, if you have already used it by accident somewhere, then that old value persists. Could this be put in get_option somehow:

opt_video_x11 = get_option('video_x11', ..., if_enabled: opt_video)

bonzini · 2021-03-12T17:20:26Z

Could this be put in get_option somehow.

I like it, what about:

# if condition is false and no conflict_message: auto->disabled, enabled->enabled
# if condition is false and conflict_message is present: auto->disabled, enabled->error
get_option('video_x11',
      condition: **condition**,
      conflict_message: **string**)

xclaesse · 2021-03-12T18:53:51Z

The condition can be put on get_option() indeed, that would make sense.

I'm personally still not convinced by the enabled->enabled case, IMHO if a feature can only be used under certain condition and you set that feature enabled, you always should get an error if the condition is not met.

That way we could even support boolean options, if condition is false true->error?

dcbaker · 2021-03-12T23:26:39Z

Yeah, the semantics of enabled right now is "Use this or error", and disabled is "don't use this or error", so anytime enabled or disabled effectively degrades to auto we're breaking the semantics of the type.

bonzini · 2021-03-13T07:51:44Z

IMHO if a feature can only be used under certain condition and you set that feature enabled, you always should get an error if the condition is not met.

The enabled->enabled case is not for feature that can only be used under certain conditions; it's for dependencies that meson.build decides not to use for unrelated reasons that are essentially implementation details. For example, assume that dbus is for some internal reason used only on Linux but not on other POSIX systems—but it's implementation detail, therefore -Ddbus=enabled should not fail compilation on non-Linux systems.

But this is very different from degrading enabled to auto, to some extent it's the opposite. Having enabled->enabled will ensure that all the tests are performed even in cases where the dependencies is not used, but it will skip a pointless test and speed up meson if the feature is auto. In the example above, dbus will be tested for even on non-Linux systems.

xclaesse · 2021-03-15T17:38:56Z

@bonzini OOC do you have an actual use-case for may_disable_if() in qemu? I would prefer to leave that out of scope for now if you did that just because you think it could be useful.

Do I understand it correctly that it would be for example if an optional feature is not recommended on certain platforms. For example on Windows you could prefer directx over opengl, unless user explicitly enable opengl?

# On Windows disable opengl by default, unless user explicitly asked for it.
gl_opt = get_option('opengl').may_disable_if(platform == 'windows')
gl_dep = dependency('opengl', required: gl_opt)

xclaesse · 2021-03-15T17:41:57Z

Could be as well gl_opt = get_option('opengl').may_disable_if(directx_dep.found()) to mean that if we already build the directx backend better disable the opengl backend, unless user really wants both. Right?

bonzini · 2021-03-15T17:49:41Z

do you have an actual use-case for may_disable_if() in qemu? I would prefer to leave that out of scope for now if you did that just because you think it could be useful.

Yes, I have a bunch of

if get_option('xkbcommon').auto() and not have_system
  xkbcommon = not_found
else
  xkbcommon = dependency('xkbcommon', required: get_option('xkbcommon'),
                         method: 'pkg-config', kwargs: static_kwargs)
endif

which would become required: get_option('xkbcommon').may_disable_if(not have_system). The logic is: if system emulators are not enabled, xkbcommon is not used so degrade auto to disabled; but if -Dxkbcommon=enabled, check whether the dependency is present even if it will not be used.

Do I understand it correctly that it would be for example if an optional feature is not recommended on certain platforms.

That's another usecase that I hadn't thought of.

xclaesse · 2021-03-16T15:27:28Z

On 2nd thought, I think I prefer having those methods on FeatureOptionHolder rather than in get_option() because it is more flexible. Doing it in get_option() restricts us to kwargs that only apply to certain type of options, would make no sense on string options... Doing it on FeatureOptionHolder it's easier to add as many methods and arguments we want in the future.

Doing it in get_option() does not really fix @jpakkane concern anyway, because you could still do get_option('dirextx') in one place and get_option('directx', condition: platform == 'windows') somewhere else.

We have been discussing ways to combine feature options together since feature options have been introduced. This is the best proposal I've seen so far. The biggest difference with previous attemps is here we combine feature with boolean, instead of feature with feature, that makes a lot more sense IMHO.

dcbaker · 2021-03-16T16:39:20Z

I'm in agreement, especially because it could allow you to make complex conditional logic more readable, imagine (something pretty similar exists in mesa):

with_glx = get_option('glx', disable_if : host_machine.system() in ['windows', 'darwin'] or not (any_gl or any any_vk or any_video) and dep_x11.found() and dep_xlib.found() and dep_xcb.found() and dep_xcb_x11.found())

to

with_glx = get_option('glx').disable_if(host_machine.system() in ['windows', 'darwin'])'
with_glx = with_glx.disable_if(not (any_gl or any_vk or any_video))
with_glx = with_glx.disable_if(not (dep_x11.found() or dep_xcb.found() or dep_x11_xcb.found()))

or even:

with_glx = get_option('glx') \
    .disable_if(host_machine.system() in ['windows', 'darwin'])' \
    .disable_if(not (any_gl or any_vk or any_video)) \
    .disable_if(not (dep_x11.found() or dep_xcb.found() or dep_x11_xcb.found()))

I also have a real world use case today that I found that this would be awesome for. I have a project where all my tests use gtest, so I have this in my meson_options:

option('tests', type : 'feature')

and my meson.build

dep_gtest = dependency('gtest', disabler : true, required : get_option('tests'))

test_exe = executable(
   ...
   dependencies : dep_gtest
)

but this doesn't do exactly what I want because in a cross compile I only want to build tests by default if they'll actually run, so I could replace this with:

dep_gtest = dependency('gtest', disabler : true, required : get_option('tests').disable_if(not meson.can_run_host_binaries()))

So definately +1 from me for the feature.

Also, on the color of the bikeshed, I would disable_if or disable_auto_if to may_disable_if, as it's not really clear what the latter means (the first one is nice and terse, the second I think is more descriptive. We could even have a matching enable_auto_if to go along with it.

bonzini · 2021-03-16T18:24:44Z

I also have a real world use case today that I found that this would be awesome for

That one pretty much matches mine.

Also, on the color of the bikeshed, I would disable_if or disable_auto_if to may_disable_if

I added the "may" because it doesn't turn enabled into disabled, so disable_if is misleading. But disable_auto_if works for me too. I won't get back to this too soon, but it'll remain on my todo list.

xclaesse · 2021-03-16T18:31:36Z

I like disable_auto_if because it is the most descriptive and unambigous, even if it sounds a bit long. Maybe opt.auto_to_disabled(condition) ? I don't immediately see a use-case for auto_to_enabled() but it would be a nice parallel indeed.

dcbaker · 2021-03-16T20:10:27Z

opt_foo = get_option('foo')
opt_bar = get_option('bar').enable_auto_if(opt_foo.enabled())

maybe? Although that case having something like:

opt_foo = get_option('foo')
opt_bar = get_option('bar').auto_to(opt_foo)

might be more useful. Though that doesn't need to happen in this PR, just throwing ideas out.

tp-m · 2021-04-19T16:12:38Z

I'd love to make use of this for what it's worth.

Use case 1: https://gitlab.freedesktop.org/cairo/cairo/-/merge_requests/159/diffs

Use case 2: flipping auto defaults for GStreamer plugins based on some kind of "group" options (e.g. an option to enable/disable gpl plugins, or video plugins, or all external plugins).

This method simplifies the conversion of Feature objects to booleans. Often, one has to use the "not" operator in order to treat "auto" and "enabled" the same way. "allowed()" also works well in conjunction with the require method that is introduced in the next patch. For example, if get_option('foo').require(host_machine.system() == 'windows').allowed() then src += ['foo.c'] config.set10('HAVE_FOO', 1) endif can be used instead of if host_machine.system() != 'windows' if get_option('foo').enabled() error('...') endif endif if not get_option('foo').disabled() then src += ['foo.c'] config.set10('HAVE_FOO', 1) endif Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

This will allow adding "forced-off" Feature objects in the next patch. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

mensinda · 2021-06-01T14:51:34Z

OK, looks like vs2019x64vs is broken. The error is in AllPlatformTests.test_meson_compile, which seems to have stopped working randomly. It also fails for me in #8817.

I can also reproduce this error on master locally in my Windows 10 VM. Seems like a VS update broke something...

mensinda · 2021-06-01T15:23:05Z

Looks like this is definitely an MSBuild bug...

msbuild -target:mylib_sha '.\build\shared library linking test.sln'      # Works
msbuild -target:mycpplib_sha '.\build\shared library linking test.sln'   # Fails

Part of the .sln file:

Microsoft Visual Studio Solution File, Format Version 11.00
# Visual Studio 2019
Project("{8BC9CEB8-8B4A-11D0-8D11-00A0C91BC942}") = "mylib@sha", "mylib@sha.vcxproj", "{83B210AF-6664-4E8D-9B48-900E158BA88B}"
EndProject
Project("{8BC9CEB8-8B4A-11D0-8D11-00A0C91BC942}") = "prog@exe", "prog@exe.vcxproj", "{88597C1C-B700-472D-BD78-DA98CF318CBC}"
EndProject
Project("{8BC9CEB8-8B4A-11D0-8D11-00A0C91BC942}") = "mycpplib@sha", "mycpplib@sha.vcxproj", "{A6B275FC-5B0C-4B4E-BBA9-B1314796B91B}"
EndProject

Here, mylib@sha is defined first, followed by prog@exe and then mycpplib@sha. If I remove the first two Project() calls, the msbuild -target:mycpplib_sha command works. I guess that means that only the first target in the .sln file can be manually specified...

bonzini · 2021-06-04T11:45:25Z

Ping @jpakkane for review.

jpakkane · 2021-06-07T21:20:44Z

docs/markdown/Reference-manual.md

+for example based on other features or on properties of the host machine:
+
+```
+if get_option('directx').require(host_machine.os == 'windows',


That should probably be host_machine.system().

Add a method to perform a logical AND on a feature object. The method also takes care of raising an error if 'enabled' is ANDed with false. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Add a method to downgrade an option to disabled if it is not used. This is useful to avoid unnecessary search for dependencies; for example dep = dependency('dep', required: get_option('feature').disable_auto_if(not foo)) can be used instead of the more verbose and complex if get_option('feature').auto() and not foo then dep = dependency('', required: false) else dep = dependency('dep', required: get_option('feature')) endif or to avoid unnecessary dependency searches: dep1 = dependency('dep1', required: get_option('foo')) # dep2 is only used together with dep1 dep2 = dependency('dep2', required: get_option('foo').disable_auto_if(not dep1.found())) ``` Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Fix the following Python error: D:\a\1\s\run_unittests.py:6654: DeprecationWarning: invalid escape sequence \ self.assertEqual(libhello_nolib.get_pkgconfig_variable(escaped_var, {}), hello world) Use a raw string literal.

Forgotten in mesonbuild#8512.

Forgotten in #8512.

bonzini requested a review from jpakkane as a code owner March 10, 2021 13:29

bonzini force-pushed the feature-methods branch from 3be6f51 to ad0adc6 Compare March 10, 2021 14:43

xclaesse reviewed Mar 10, 2021

View reviewed changes

dcbaker reviewed Mar 10, 2021

View reviewed changes

bonzini marked this pull request as draft March 17, 2021 08:31

bonzini force-pushed the feature-methods branch 2 times, most recently from 5f0a818 to 60620f1 Compare May 16, 2021 16:01

bonzini marked this pull request as ready for review May 17, 2021 08:28

bonzini force-pushed the feature-methods branch 2 times, most recently from 7c382d8 to 8eaa3e0 Compare May 24, 2021 06:30

bonzini added 2 commits May 31, 2021 16:01

interpreter: wrap access to Feature value

5298d8e

This will allow adding "forced-off" Feature objects in the next patch. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

bonzini force-pushed the feature-methods branch from 8eaa3e0 to ede6692 Compare May 31, 2021 14:02

mensinda mentioned this pull request Jun 1, 2021

Split dependencies.base #8817

Merged

bonzini force-pushed the feature-methods branch from 08efbda to 62c09a8 Compare June 1, 2021 15:23

mensinda mentioned this pull request Jun 1, 2021

Can only specify the first target in .sln with -target: dotnet/msbuild#6503

Closed

jpakkane reviewed Jun 7, 2021

View reviewed changes

bonzini added 3 commits June 8, 2021 10:18

interpreter: add feature.require()

2f2d99e

Add a method to perform a logical AND on a feature object. The method also takes care of raising an error if 'enabled' is ANDed with false. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

run_unittests.sh: fix Python DeprecationWarning

6497e52

Fix the following Python error: D:\a\1\s\run_unittests.py:6654: DeprecationWarning: invalid escape sequence \ self.assertEqual(libhello_nolib.get_pkgconfig_variable(escaped_var, {}), hello world) Use a raw string literal.

bonzini force-pushed the feature-methods branch from 62c09a8 to 6497e52 Compare June 8, 2021 08:18

jpakkane merged commit a4a61b6 into mesonbuild:master Jun 8, 2021

bonzini deleted the feature-methods branch October 14, 2021 20:02

bonzini mentioned this pull request Dec 21, 2021

RFC: A way to instantiate custom "feature" objects #5206

Open

eli-schwartz added a commit to eli-schwartz/meson that referenced this pull request Feb 9, 2022

add some forgotten FeatureNew annotations

aaebf3d

Forgotten in mesonbuild#8512.

eli-schwartz mentioned this pull request Feb 9, 2022

add some forgotten FeatureNew annotations #9981

Merged

eli-schwartz added a commit to eli-schwartz/meson that referenced this pull request Feb 10, 2022

add some forgotten FeatureNew annotations

6c79e97

Forgotten in mesonbuild#8512.

nirbheek pushed a commit that referenced this pull request Feb 14, 2022

add some forgotten FeatureNew annotations

5e5099e

Forgotten in #8512.

Aetf mentioned this pull request Jun 6, 2022

Build system overhaul Aetf/kmscon#45

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Utility methods for feature objects #8512

Utility methods for feature objects #8512

bonzini commented Mar 10, 2021

xclaesse Mar 10, 2021

dcbaker Mar 10, 2021

xclaesse Mar 10, 2021

bonzini Mar 10, 2021

xclaesse Mar 12, 2021

xclaesse Mar 16, 2021

xclaesse Mar 10, 2021

xclaesse Mar 10, 2021

xclaesse Mar 10, 2021

xclaesse Mar 10, 2021

xclaesse Mar 10, 2021

xclaesse Mar 10, 2021

bonzini Mar 10, 2021

dcbaker Mar 10, 2021

jpakkane commented Mar 12, 2021

bonzini commented Mar 12, 2021

xclaesse commented Mar 12, 2021

dcbaker commented Mar 12, 2021

bonzini commented Mar 13, 2021

xclaesse commented Mar 15, 2021

xclaesse commented Mar 15, 2021

bonzini commented Mar 15, 2021

xclaesse commented Mar 16, 2021

dcbaker commented Mar 16, 2021 •

edited

Loading

bonzini commented Mar 16, 2021

xclaesse commented Mar 16, 2021

dcbaker commented Mar 16, 2021

tp-m commented Apr 19, 2021

mensinda commented Jun 1, 2021

mensinda commented Jun 1, 2021

bonzini commented Jun 4, 2021

jpakkane Jun 7, 2021

	if error_message and not isinstance(error_message, str):
	if not isinstance(error_message, str):

Utility methods for feature objects #8512

Utility methods for feature objects #8512

Conversation

bonzini commented Mar 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpakkane commented Mar 12, 2021

bonzini commented Mar 12, 2021

xclaesse commented Mar 12, 2021

dcbaker commented Mar 12, 2021

bonzini commented Mar 13, 2021

xclaesse commented Mar 15, 2021

xclaesse commented Mar 15, 2021

bonzini commented Mar 15, 2021

xclaesse commented Mar 16, 2021

dcbaker commented Mar 16, 2021 • edited Loading

bonzini commented Mar 16, 2021

xclaesse commented Mar 16, 2021

dcbaker commented Mar 16, 2021

tp-m commented Apr 19, 2021

mensinda commented Jun 1, 2021

mensinda commented Jun 1, 2021

bonzini commented Jun 4, 2021

Choose a reason for hiding this comment

dcbaker commented Mar 16, 2021 •

edited

Loading