From b2473b61cc2c61218f5be4057af0e42b63d00048 Mon Sep 17 00:00:00 2001 From: Dylan Baker Date: Wed, 21 Dec 2022 11:23:34 -0800 Subject: [PATCH] interpreter: add FeatureOption.enable_if and .disable_if MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This adds two new methods, that are conceptually related in the same way that `enable_auto_if` and `disable_auto_if` are. They are different however, in that they will always replace an `auto` value with an `enabled` or `disabled` value, or error if the feature is in the opposite state (calling `feature(disabled).enable_if(true)`, for example). This matters when the feature will be passed to dependency(required : …)`, which has different behavior when passed an enabled feature than an auto one. The `disable_if` method will be controversial, I'm sure, since it can be expressed via `feature.require()` (`feature.require(not condition) == feature.disable_if(condition)`). I have two defences of this: 1) `feature.require` is difficult to reason about, I would expect require to be equivalent to `feature.enable_if(condition)`, not to `feature.disable_if(not condition)`. 2) mixing `enable_if` and `disable_if` in the same call chain is much clearer than mixing `require` and `enable_if`: ```meson get_option('feat') \ .enable_if(foo) \ .disable_if(bar) \ .enable_if(opt) ``` vs ```meson get_option('feat') \ .enable_if(foo) \ .require(not bar) \ .enable_if(opt) ``` In the first chain it's immediately obvious what is happening, in the second, not so much, especially if you're not familiar with what `require` means. --- docs/markdown/snippets/feature_if_methods.md | 77 +++++++++++++++++++ docs/yaml/objects/feature.yaml | 60 +++++++++++++++ mesonbuild/interpreter/interpreterobjects.py | 44 +++++++++-- .../common/192 feature option/meson.build | 10 +++ 4 files changed, 186 insertions(+), 5 deletions(-) create mode 100644 docs/markdown/snippets/feature_if_methods.md diff --git a/docs/markdown/snippets/feature_if_methods.md b/docs/markdown/snippets/feature_if_methods.md new file mode 100644 index 000000000..9d7753446 --- /dev/null +++ b/docs/markdown/snippets/feature_if_methods.md @@ -0,0 +1,77 @@ +## Add a FeatureOption.enable_if and .disable_if + +These are useful when features need to be constrained to pass to [[dependency]], +as the behavior of an `auto` and `disabled` or `enabled` feature is markedly +different. consider the following case: + +```meson +opt = get_option('feature').disable_auto_if(not foo) +if opt.enabled() and not foo + error('Cannot enable feat when foo is not also enabled') +endif +dep = dependency('foo', required : opt) +``` + +This could be simplified to +```meson +opt = get_option('feature').disable_if(not foo, error_message : 'Cannot enable feature when foo is not also enabled') +dep = dependency('foo', required : opt) +``` + +For a real life example, here is some code in mesa: +```meson +_llvm = get_option('llvm') +dep_llvm = null_dep +with_llvm = false +if _llvm.allowed() + dep_llvm = dependency( + 'llvm', + version : _llvm_version, + modules : llvm_modules, + optional_modules : llvm_optional_modules, + required : ( + with_amd_vk or with_gallium_radeonsi or with_gallium_opencl or with_clc + or _llvm.enabled() + ), + static : not _shared_llvm, + fallback : ['llvm', 'dep_llvm'], + include_type : 'system', + ) + with_llvm = dep_llvm.found() +endif +if with_llvm + ... +elif with_amd_vk and with_aco_tests + error('ACO tests require LLVM, but LLVM is disabled.') +elif with_gallium_radeonsi or with_swrast_vk + error('The following drivers require LLVM: RadeonSI, SWR, Lavapipe. One of these is enabled, but LLVM is disabled.') +elif with_gallium_opencl + error('The OpenCL "Clover" state tracker requires LLVM, but LLVM is disabled.') +elif with_clc + error('The CLC compiler requires LLVM, but LLVM is disabled.') +else + draw_with_llvm = false +endif +``` + +simplified to: +```meson +_llvm = get_option('llvm') \ + .enable_if(with_amd_vk and with_aco_tests, error_message : 'ACO tests requires LLVM') \ + .enable_if(with_gallium_radeonsi, error_message : 'RadeonSI requires LLVM') \ + .enable_if(with_swrast_vk, error_message : 'Vulkan SWRAST requires LLVM') \ + .enable_if(with_gallium_opencl, error_message : 'The OpenCL Clover state trackers requires LLVM') \ + .enable_if(with_clc, error_message : 'CLC library requires LLVM') + +dep_llvm = dependency( + 'llvm', + version : _llvm_version, + modules : llvm_modules, + optional_modules : llvm_optional_modules, + required : _llvm, + static : not _shared_llvm, + fallback : ['llvm', 'dep_llvm'], + include_type : 'system', +) +with_llvm = dep_llvm.found() +``` diff --git a/docs/yaml/objects/feature.yaml b/docs/yaml/objects/feature.yaml index 01209eb53..9cb597be8 100644 --- a/docs/yaml/objects/feature.yaml +++ b/docs/yaml/objects/feature.yaml @@ -85,3 +85,63 @@ methods: type: str default: "''" description: The error Message to print if the check fails + +- name: enable_if + returns: feature + since: 1.1.0 + description: | + Returns the object itself if the value is false; an error if the object is + `'disabled'` and the value is true; an enabled feature if the object + is `'auto'` or `'enabled'` and the value is true. + + example: | + `enable_if` is useful to restrict the applicability of `'auto'` features, + particularly when passing them to [[dependency]]: + + ``` + use_llvm = get_option('llvm').enable_if(with_clang).enable_if(with_llvm_libs) + dep_llvm = dependency('llvm', require : use_llvm) + ``` + + posargs: + value: + type: bool + description: The value to check + + kwargs: + error_message: + type: str + default: "''" + description: The error Message to print if the check fails + +- name: disable_if + returns: feature + since: 1.1.0 + description: | + Returns the object itself if the value is false; an error if the object is + `'enabled'` and the value is true; a disabled feature if the object + is `'auto'` or `'disabled'` and the value is true. + + This is equivalent to `feature_opt.require(not condition)`, but may make + code easier to reason about, especially when mixed with `enable_if` + + example: | + `disable_if` is useful to restrict the applicability of `'auto'` features, + particularly when passing them to [[dependency]]: + + ``` + use_os_feature = get_option('foo') \ + .disable_if(host_machine.system() == 'darwin', error_message : 'os feature not supported on MacOS') + dep_os_feature = dependency('os_feature', require : use_os_feature) + ``` + + posargs: + value: + type: bool + description: The value to check + + kwargs: + error_message: + type: str + default: "''" + description: The error Message to print if the check fails diff --git a/mesonbuild/interpreter/interpreterobjects.py b/mesonbuild/interpreter/interpreterobjects.py index a750da95d..fa9171485 100644 --- a/mesonbuild/interpreter/interpreterobjects.py +++ b/mesonbuild/interpreter/interpreterobjects.py @@ -41,6 +41,9 @@ if T.TYPE_CHECKING: separator: str +_ERROR_MSG_KW: KwargInfo[T.Optional[str]] = KwargInfo('error_message', (str, NoneType)) + + def extract_required_kwarg(kwargs: 'kwargs.ExtractRequired', subproject: 'SubProject', feature_check: T.Optional[FeatureCheckBase] = None, @@ -97,6 +100,8 @@ class FeatureOptionHolder(ObjectHolder[coredata.UserFeatureOption]): 'require': self.require_method, 'disable_auto_if': self.disable_auto_if_method, 'enable_auto_if': self.enable_auto_if_method, + 'disable_if': self.disable_if_method, + 'enable_if': self.enable_if_method, }) @property @@ -134,22 +139,51 @@ class FeatureOptionHolder(ObjectHolder[coredata.UserFeatureOption]): def auto_method(self, args: T.List[TYPE_var], kwargs: TYPE_kwargs) -> bool: return self.value == 'auto' + def _disable_if(self, condition: bool, message: T.Optional[str]) -> coredata.UserFeatureOption: + if not condition: + return copy.deepcopy(self.held_object) + + if self.value == 'enabled': + err_msg = f'Feature {self.held_object.name} cannot be enabled' + if message: + err_msg += f': {message}' + raise InterpreterException(err_msg) + return self.as_disabled() + @FeatureNew('feature_option.require()', '0.59.0') @typed_pos_args('feature_option.require', bool) @typed_kwargs( 'feature_option.require', - KwargInfo('error_message', (str, NoneType)) + _ERROR_MSG_KW, ) def require_method(self, args: T.Tuple[bool], kwargs: 'kwargs.FeatureOptionRequire') -> coredata.UserFeatureOption: - if args[0]: + return self._disable_if(not args[0], kwargs['error_message']) + + @FeatureNew('feature_option.disable_if()', '1.1.0') + @typed_pos_args('feature_option.disable_if', bool) + @typed_kwargs( + 'feature_option.disable_if', + _ERROR_MSG_KW, + ) + def disable_if_method(self, args: T.Tuple[bool], kwargs: 'kwargs.FeatureOptionRequire') -> coredata.UserFeatureOption: + return self._disable_if(args[0], kwargs['error_message']) + + @FeatureNew('feature_option.enable_if()', '1.1.0') + @typed_pos_args('feature_option.enable_if', bool) + @typed_kwargs( + 'feature_option.enable_if', + _ERROR_MSG_KW, + ) + def enable_if_method(self, args: T.Tuple[bool], kwargs: 'kwargs.FeatureOptionRequire') -> coredata.UserFeatureOption: + if not args[0]: return copy.deepcopy(self.held_object) - if self.value == 'enabled': - err_msg = f'Feature {self.held_object.name} cannot be enabled' + if self.value == 'disabled': + err_msg = f'Feature {self.held_object.name} cannot be disabled' if kwargs['error_message']: err_msg += f': {kwargs["error_message"]}' raise InterpreterException(err_msg) - return self.as_disabled() + return self.as_enabled() @FeatureNew('feature_option.disable_auto_if()', '0.59.0') @noKwargs diff --git a/test cases/common/192 feature option/meson.build b/test cases/common/192 feature option/meson.build index d6de7449a..a2ebe32a0 100644 --- a/test cases/common/192 feature option/meson.build +++ b/test cases/common/192 feature option/meson.build @@ -15,6 +15,9 @@ assert(not required_opt.disabled(), 'Should be enabled option') assert(not required_opt.auto(), 'Should be enabled option') assert(required_opt.allowed(), 'Should be enabled option') assert(required_opt.require(true, error_message: 'xyz').enabled(), 'Should be enabled option') +assert(required_opt.enable_if(true, error_message: 'xyz').enabled(), 'Should be enabled option') +assert(required_opt.enable_if(false, error_message: 'xyz').enabled(), 'Should be enabled option') +assert(required_opt.disable_if(false, error_message: 'xyz').enabled(), 'Should be enabled option') assert(required_opt.disable_auto_if(true).enabled(), 'Should be enabled option') assert(required_opt.disable_auto_if(false).enabled(), 'Should be enabled option') assert(required_opt.enable_auto_if(true).enabled(), 'Should be enabled option') @@ -26,6 +29,10 @@ assert(optional_opt.auto(), 'Should be auto option') assert(optional_opt.allowed(), 'Should be auto option') assert(optional_opt.require(true).auto(), 'Should be auto option') assert(optional_opt.require(false, error_message: 'xyz').disabled(), 'Should be disabled auto option') +assert(optional_opt.enable_if(true).enabled(), 'Should be enabled option') +assert(optional_opt.enable_if(false).auto(), 'Should be auto option') +assert(optional_opt.disable_if(true).disabled(), 'Should be disabled auto option') +assert(optional_opt.disable_if(false).auto(), 'Should be auto option') assert(optional_opt.disable_auto_if(true).disabled(), 'Should be disabled auto option') assert(optional_opt.disable_auto_if(false).auto(), 'Should be auto option') assert(optional_opt.enable_auto_if(true).enabled(), 'Should be disabled auto option') @@ -37,6 +44,9 @@ assert(not disabled_opt.auto(), 'Should be disabled option') assert(not disabled_opt.allowed(), 'Should be disabled option') assert(disabled_opt.require(true).disabled(), 'Should be disabled option') assert(disabled_opt.require(false, error_message: 'xyz').disabled(), 'Should be disabled option') +assert(disabled_opt.enable_if(false).disabled(), 'Should be disabled option') +assert(disabled_opt.disable_if(true).disabled(), 'Should be disabled option') +assert(disabled_opt.disable_if(false).disabled(), 'Should be disabled option') assert(disabled_opt.disable_auto_if(true).disabled(), 'Should be disabled option') assert(disabled_opt.disable_auto_if(false).disabled(), 'Should be disabled option') assert(disabled_opt.enable_auto_if(true).disabled(), 'Should be disabled option')