| Message ID | 20241215192354.824108-1-pobrn@protonmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Barnabás, Thank you for the patch. On Sun, Dec 15, 2024 at 07:23:57PM +0000, Barnabás Pőcze wrote: > The v4l2 compatibility layer does not have external dependencies, > the usual benefits of a feature option do not apply. The main > motivation behind this change is making use of the `auto_features` > meson option that can change all feature options set to "auto" > by default to the desired value. > > This can be useful for two reasons: (1) using auto_features=disabled > and then building up the list of features that are desired in the > particular build; (2) using auto_features=enabled to achieve > (usually) more testing and compilation coverage. The first use case doesn't seem very useful to me, as the v4l2 option is set to false by default, so we can already start with the option disabled and enable it only when desired. The second use case is a bit more interesting. You will however need to update CI, and it will break all packaging scripts that set the option explicitly to false or true, so I wonder if it's really worth it. If there's a general consensus that we should change the option type, I'm OK with this patch. > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> > --- > meson_options.txt | 4 ++-- > src/v4l2/meson.build | 2 +- > 2 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/meson_options.txt b/meson_options.txt > index c91cd241a..08e224561 100644 > --- a/meson_options.txt > +++ b/meson_options.txt > @@ -84,6 +84,6 @@ option('udev', > description : 'Enable udev support for hotplug') > > option('v4l2', > - type : 'boolean', > - value : false, > + type : 'feature', > + value : 'auto', > description : 'Compile the V4L2 compatibility layer') > diff --git a/src/v4l2/meson.build b/src/v4l2/meson.build > index 58f53bf3d..e4daf8ae7 100644 > --- a/src/v4l2/meson.build > +++ b/src/v4l2/meson.build > @@ -1,6 +1,6 @@ > # SPDX-License-Identifier: CC0-1.0 > > -if not get_option('v4l2') > +if not get_option('v4l2').allowed() > v4l2_enabled = false > subdir_done() > endif
Hi 2024. december 15., vasárnap 20:52 keltezéssel, Laurent Pinchart <laurent.pinchart@ideasonboard.com> írta: > Hi Barnabás, > > Thank you for the patch. > > On Sun, Dec 15, 2024 at 07:23:57PM +0000, Barnabás Pőcze wrote: > > The v4l2 compatibility layer does not have external dependencies, > > the usual benefits of a feature option do not apply. The main > > motivation behind this change is making use of the `auto_features` > > meson option that can change all feature options set to "auto" > > by default to the desired value. > > > > This can be useful for two reasons: (1) using auto_features=disabled > > and then building up the list of features that are desired in the > > particular build; (2) using auto_features=enabled to achieve > > (usually) more testing and compilation coverage. > > The first use case doesn't seem very useful to me, as the v4l2 option is > set to false by default, so we can already start with the option > disabled and enable it only when desired. > > The second use case is a bit more interesting. > > You will however need to update CI, and it will break all packaging > scripts that set the option explicitly to false or true, so I wonder if > it's really worth it. If there's a general consensus that we should > change the option type, I'm OK with this patch. I think it makes sense conceptually to turn this into a feature option. The transition can be made relatively seamless if that is a concern: diff --git a/meson_options.txt b/meson_options.txt index c91cd241a..1f70da0f5 100644 --- a/meson_options.txt +++ b/meson_options.txt @@ -84,6 +84,7 @@ option('udev', description : 'Enable udev support for hotplug') option('v4l2', - type : 'boolean', - value : false, - description : 'Compile the V4L2 compatibility layer') + type : 'feature', + value : 'auto', + description : 'Compile the V4L2 compatibility layer', + deprecated : {'true': 'enabled', 'false': 'disabled'}) Regards, Barnabás Pőcze > > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> > > --- > > meson_options.txt | 4 ++-- > > src/v4l2/meson.build | 2 +- > > 2 files changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/meson_options.txt b/meson_options.txt > > index c91cd241a..08e224561 100644 > > --- a/meson_options.txt > > +++ b/meson_options.txt > > @@ -84,6 +84,6 @@ option('udev', > > description : 'Enable udev support for hotplug') > > > > option('v4l2', > > - type : 'boolean', > > - value : false, > > + type : 'feature', > > + value : 'auto', > > description : 'Compile the V4L2 compatibility layer') > > diff --git a/src/v4l2/meson.build b/src/v4l2/meson.build > > index 58f53bf3d..e4daf8ae7 100644 > > --- a/src/v4l2/meson.build > > +++ b/src/v4l2/meson.build > > @@ -1,6 +1,6 @@ > > # SPDX-License-Identifier: CC0-1.0 > > > > -if not get_option('v4l2') > > +if not get_option('v4l2').allowed() > > v4l2_enabled = false > > subdir_done() > > endif > > -- > Regards, > > Laurent Pinchart >
On Mon, Dec 16, 2024 at 10:59:34AM +0000, Barnabás Pőcze wrote: > 2024. december 15., vasárnap 20:52 keltezéssel, Laurent Pinchart írta: > > On Sun, Dec 15, 2024 at 07:23:57PM +0000, Barnabás Pőcze wrote: > > > The v4l2 compatibility layer does not have external dependencies, > > > the usual benefits of a feature option do not apply. The main > > > motivation behind this change is making use of the `auto_features` > > > meson option that can change all feature options set to "auto" > > > by default to the desired value. > > > > > > This can be useful for two reasons: (1) using auto_features=disabled > > > and then building up the list of features that are desired in the > > > particular build; (2) using auto_features=enabled to achieve > > > (usually) more testing and compilation coverage. > > > > The first use case doesn't seem very useful to me, as the v4l2 option is > > set to false by default, so we can already start with the option > > disabled and enable it only when desired. > > > > The second use case is a bit more interesting. > > > > You will however need to update CI, and it will break all packaging > > scripts that set the option explicitly to false or true, so I wonder if > > it's really worth it. If there's a general consensus that we should > > change the option type, I'm OK with this patch. > > I think it makes sense conceptually to turn this into a feature option. > The transition can be made relatively seamless if that is a concern: > > diff --git a/meson_options.txt b/meson_options.txt > index c91cd241a..1f70da0f5 100644 > --- a/meson_options.txt > +++ b/meson_options.txt > @@ -84,6 +84,7 @@ option('udev', > description : 'Enable udev support for hotplug') > > option('v4l2', > - type : 'boolean', > - value : false, > - description : 'Compile the V4L2 compatibility layer') > + type : 'feature', > + value : 'auto', > + description : 'Compile the V4L2 compatibility layer', > + deprecated : {'true': 'enabled', 'false': 'disabled'}) I didn't know about "deprecated", that's interesting. With that, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> and we can drop it after a few releases. > > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> > > > --- > > > meson_options.txt | 4 ++-- > > > src/v4l2/meson.build | 2 +- > > > 2 files changed, 3 insertions(+), 3 deletions(-) > > > > > > diff --git a/meson_options.txt b/meson_options.txt > > > index c91cd241a..08e224561 100644 > > > --- a/meson_options.txt > > > +++ b/meson_options.txt > > > @@ -84,6 +84,6 @@ option('udev', > > > description : 'Enable udev support for hotplug') > > > > > > option('v4l2', > > > - type : 'boolean', > > > - value : false, > > > + type : 'feature', > > > + value : 'auto', > > > description : 'Compile the V4L2 compatibility layer') > > > diff --git a/src/v4l2/meson.build b/src/v4l2/meson.build > > > index 58f53bf3d..e4daf8ae7 100644 > > > --- a/src/v4l2/meson.build > > > +++ b/src/v4l2/meson.build > > > @@ -1,6 +1,6 @@ > > > # SPDX-License-Identifier: CC0-1.0 > > > > > > -if not get_option('v4l2') > > > +if not get_option('v4l2').allowed() > > > v4l2_enabled = false > > > subdir_done() > > > endif
Quoting Laurent Pinchart (2024-12-16 11:04:04) > On Mon, Dec 16, 2024 at 10:59:34AM +0000, Barnabás Pőcze wrote: > > 2024. december 15., vasárnap 20:52 keltezéssel, Laurent Pinchart írta: > > > On Sun, Dec 15, 2024 at 07:23:57PM +0000, Barnabás Pőcze wrote: > > > > The v4l2 compatibility layer does not have external dependencies, > > > > the usual benefits of a feature option do not apply. The main > > > > motivation behind this change is making use of the `auto_features` > > > > meson option that can change all feature options set to "auto" > > > > by default to the desired value. > > > > > > > > This can be useful for two reasons: (1) using auto_features=disabled > > > > and then building up the list of features that are desired in the > > > > particular build; (2) using auto_features=enabled to achieve > > > > (usually) more testing and compilation coverage. > > > > > > The first use case doesn't seem very useful to me, as the v4l2 option is > > > set to false by default, so we can already start with the option > > > disabled and enable it only when desired. > > > > > > The second use case is a bit more interesting. > > > > > > You will however need to update CI, and it will break all packaging > > > scripts that set the option explicitly to false or true, so I wonder if > > > it's really worth it. If there's a general consensus that we should > > > change the option type, I'm OK with this patch. > > > > I think it makes sense conceptually to turn this into a feature option. > > The transition can be made relatively seamless if that is a concern: > > > > diff --git a/meson_options.txt b/meson_options.txt > > index c91cd241a..1f70da0f5 100644 > > --- a/meson_options.txt > > +++ b/meson_options.txt > > @@ -84,6 +84,7 @@ option('udev', > > description : 'Enable udev support for hotplug') > > > > option('v4l2', > > - type : 'boolean', > > - value : false, > > - description : 'Compile the V4L2 compatibility layer') > > + type : 'feature', > > + value : 'auto', > > + description : 'Compile the V4L2 compatibility layer', > > + deprecated : {'true': 'enabled', 'false': 'disabled'}) > > I didn't know about "deprecated", that's interesting. With that, > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > and we can drop it after a few releases. Indeed, I'm fine with that, Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> > > > > --- > > > > meson_options.txt | 4 ++-- > > > > src/v4l2/meson.build | 2 +- > > > > 2 files changed, 3 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/meson_options.txt b/meson_options.txt > > > > index c91cd241a..08e224561 100644 > > > > --- a/meson_options.txt > > > > +++ b/meson_options.txt > > > > @@ -84,6 +84,6 @@ option('udev', > > > > description : 'Enable udev support for hotplug') > > > > > > > > option('v4l2', > > > > - type : 'boolean', > > > > - value : false, > > > > + type : 'feature', > > > > + value : 'auto', > > > > description : 'Compile the V4L2 compatibility layer') > > > > diff --git a/src/v4l2/meson.build b/src/v4l2/meson.build > > > > index 58f53bf3d..e4daf8ae7 100644 > > > > --- a/src/v4l2/meson.build > > > > +++ b/src/v4l2/meson.build > > > > @@ -1,6 +1,6 @@ > > > > # SPDX-License-Identifier: CC0-1.0 > > > > > > > > -if not get_option('v4l2') > > > > +if not get_option('v4l2').allowed() > > > > v4l2_enabled = false > > > > subdir_done() > > > > endif > > -- > Regards, > > Laurent Pinchart
diff --git a/meson_options.txt b/meson_options.txt index c91cd241a..08e224561 100644 --- a/meson_options.txt +++ b/meson_options.txt @@ -84,6 +84,6 @@ option('udev', description : 'Enable udev support for hotplug') option('v4l2', - type : 'boolean', - value : false, + type : 'feature', + value : 'auto', description : 'Compile the V4L2 compatibility layer') diff --git a/src/v4l2/meson.build b/src/v4l2/meson.build index 58f53bf3d..e4daf8ae7 100644 --- a/src/v4l2/meson.build +++ b/src/v4l2/meson.build @@ -1,6 +1,6 @@ # SPDX-License-Identifier: CC0-1.0 -if not get_option('v4l2') +if not get_option('v4l2').allowed() v4l2_enabled = false subdir_done() endif
The v4l2 compatibility layer does not have external dependencies, the usual benefits of a feature option do not apply. The main motivation behind this change is making use of the `auto_features` meson option that can change all feature options set to "auto" by default to the desired value. This can be useful for two reasons: (1) using auto_features=disabled and then building up the list of features that are desired in the particular build; (2) using auto_features=enabled to achieve (usually) more testing and compilation coverage. Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> --- meson_options.txt | 4 ++-- src/v4l2/meson.build | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)