Add an array type to user options #2390

dcbaker · 2017-09-29T17:23:45Z

In mesa (using autotools and scons) we take options such as --enable-dri-drivers=i965,i915,swrast,.... These options have a lot of possible values, so it makes sense to me to carry this pattern over to meson instead of having 30+ options to toggle each driver individually. I'm emulating it with a string type, and splitting, but it's impossible to validate that way.

This option looks just like a combo in meson_options.txt, except that value can take a list of options.

On the command line this is passed as a comma separated list of values, with white-space stripped.

dcbaker · 2017-10-10T22:55:46Z

ping

1ace · 2017-10-25T13:30:40Z

docs/markdown/Build-options.md

@@ -29,6 +30,8 @@ name.

 These options are accessed in Meson code with the `get_option` function.

+The array type is new in 0.43.0


I think this should be 0.44.0 now, right?

yes it should

dcbaker · 2017-10-26T00:07:55Z

the build failure is a traivs -> dockerhub issue.

liugang · 2017-10-26T11:34:47Z

This is very useful for some 3rd package, e.g ffmpeg. there have more than 100 possible values for decoders, filters

./configure  -h |grep list
  --list-decoders          show all available decoders
  --list-encoders          show all available encoders
  --list-hwaccels          show all available hardware accelerators
  --list-demuxers          show all available demuxers
  --list-muxers            show all available muxers
  --list-parsers           show all available parsers
  --list-protocols         show all available protocols
  --list-bsfs              show all available bitstream filters
  --list-indevs            show all available input devices
  --list-outdevs           show all available output devices
  --list-filters       show all available filters

./configure --list-decoders
aac anm dxa idcin mp3adu pcm_s16be sami vc1_mmal
aac_at ansi dxtory idf mp3adufloat pcm_s16be_planar sanm vc1_qsv aac_fixed ape
dxv iff_ilbm mp3float pcm_s16le screenpresso vc1_vdpau aac_latm apng eac3
ilbc_at mp3on4 pcm_s16le_planar sdx2_dpcm vc1image aasc ass eac3_at imc
mp3on4float pcm_s24be sgi vcr1 ac3 asv1 eacmv indeo2 mpc7 pcm_s24daud sgirle
vmdaudio ac3_at asv2 eamad indeo3 mpc8 pcm_s24le sheervideo vmdvideo ac3_fixed
atrac1 eatgq indeo4 mpeg1_cuvid pcm_s24le_planar shorten vmnc adpcm_4xm atrac3
eatgv indeo5 mpeg1_vdpau pcm_s32be sipr vorbis adpcm_adx atrac3p eatqi
interplay_acm mpeg1video pcm_s32le smackaud vp3 adpcm_afc aura eightbps
interplay_dpcm mpeg2_crystalhd pcm_s32le_planar smacker vp5 adpcm_aica aura2
eightsvx_exp interplay_video mpeg2_cuvid pcm_s64be smc vp6 ...

ffmpeg-3.2.4$ ./configure --list-decoders |wc
    156     468    4261
ffmpeg-3.2.4$ ./configure --list-encoders |wc
     64     192    1744
ffmpeg-3.2.4$ ./configure --list-hwaccels |wc
     20      59     747
ffmpeg-3.2.4$ ./configure --list-demuxers |wc
     90     270    2091
ffmpeg-3.2.4$ ./configure --list-muxers |wc
     51     151    1181
ffmpeg-3.2.4$ ./configure --list-parsers |wc
     13      38     262
ffmpeg-3.2.4$ ./configure --list-protocols |wc
     16      46     386
ffmpeg-3.2.4$ ./configure --list-bsfs |wc
      6      16     237
ffmpeg-3.2.4$ ./configure --list-indevs |wc
      8      22     182
ffmpeg-3.2.4$ ./configure --list-outdevs |wc
      4      11      74
ffmpeg-3.2.4$ ./configure --list-filters |wc
    103     309    3019

jpakkane · 2017-11-09T20:51:43Z

This is good stuff, just a few things.

What about the case when one of the values should contain a comma (for whatever reason)? Is it okay to say that in that case you should do "-Dkey=['foo', 'bar,baz']"? And further to not even implement that until someone actually needs it?

There should be unit tests for changing the value of one of these options. First to check that changing it is persistant and second that trying to set it to an incorrect value returns an error and that the old value remains.

dcbaker · 2017-11-10T00:36:01Z

How should I implement the "meson configure" portion of those tests? the standard framework doesn't have that kind of support, should I put them in manual tests or do you have something else in mind?

jpakkane · 2017-11-10T08:29:15Z

See run_unittests.py. All tests that are not the standard "build, test and install this project" go there.

dcbaker · 2017-11-16T21:48:07Z

Added test cases and rebased on current master.

nirbheek · 2017-11-27T02:28:27Z

docs/markdown/snippets/option-array-type.md

+values are passed as a comma separated list.
+
+    ```meson
+    option('array_opt', type : 'array', choices : ['one', 'two', 'three'], value : 'one')


Maybe we should require value: to always be an array type? In this case that would then be value : ['one']. Otherwise people will constantly think that value: can be a comma-separated list when they see a single-value value kwarg.

nirbheek · 2017-11-27T02:38:37Z

I quite like this option, and I think it will also be useful in gstreamer for our GL elements.

1ace · 2017-11-27T15:35:54Z

mesonbuild/optinterpreter.py

+    value = kwargs.get('value', choices[0])
+    if not isinstance(value, list):
+        if ',' in value:
+            raise OptionException('Array choices must be passed as an array, not a comma delemited string.')


"delimited", but I agree with @nirbheek, it's probably best to be consistent and always raise this exception (which means the bit after , goes away anyway)

jibsen · 2017-11-27T15:49:41Z

Would it make more sense calling it list instead of array?

It seems whenever you describe what it does, you say it makes it possible to pass a list of options. Also combo boxes are often used to select one item from a dropdown, and list boxes show a list of items you can select one or more from.

Just a random thought.

1ace · 2017-11-27T15:59:12Z

I also just encountered another place that could use this option type:

$ meson configure -D b_sanitize=undefined,address
mesonbuild.mesonlib.MesonException: Value "undefined,address" for combo option "b_sanitize" is not one of the choices. Possible choices are: "none", "address", "thread", "undefined", "memory", "address,undefined".

It supports address,undefined but not undefined,address...

I guess meson configure and others should also be updated once this lands to show whether an option is "one of" or "multiple of" when showing a list of possible values.

jpakkane · 2017-11-27T20:22:05Z

I thought that we had properly exposed the "no limits" array but apparently not. This is now done in #2696. That was also the reason for the string splitting mentioned above. Since we have not exposed it properly we can change the semantics to be the same as here: strings separated by commas and a properly quoted array if that is not sufficient.

Does that seem like a suitable base for this? Having an option for an array both with freeform and with enforced contents seems useful.

jpakkane · 2017-11-27T22:15:03Z

Or maybe we should bring these two together? Have an array type that may or may not have a limitation on the elements that are in it?

dcbaker · 2017-11-28T01:52:05Z

I guess it depends on how we want to expose the difference. If we want to expose it via whether choices is set then lets combine them. If we want to have two separate types then I think it's fine to have two types. The implementation can always be shared underneath if we want even if the meson level isn't.

dcbaker · 2017-11-28T18:54:10Z

Okay, I think I addressed all of the review comment so far with this latest push.

jpakkane · 2017-11-29T21:21:09Z

What should the default value be for this? Only the first element of the list (what this seems to have) is a bit off. It should probably be all enabled or all disabled. The former seems simpler because then you can do this to enable all:

option(..., choices : ['one', 'two', 'three'])

as opposed to having to duplicate them:

option(..., choices : ['one', 'two', 'three'], value : ['one', 'two', 'three'])

But setting the value to empty is still brief:

option(..., choices : ['one', 'two', 'three'], value : [])

dcbaker · 2017-11-29T21:44:35Z

That makes sense to me.

So that the shbang is ther rist line and ./run_unittests.py works.

This exposes the already existing UserStringArrayOption class through the meson_options.txt. The intention is to provide a way for projects to take list/array type arguments and validate that all of the elements in that array are valid without using complex looping constructrs.

dcbaker · 2017-11-29T22:25:32Z

I've changed the default value to be the same as the choices, I've also updated the documentation for the option types to be (hopefully) clearer.

jpakkane · 2017-11-30T21:19:53Z

Ci is fully booked for this night but I'll see how to make this work together with #2696, merge this and then do final fixups afterwards. At least winclibs and wincpplibs will be converted to this form.

If people have opinions on if there should be only one array type or whether freeform arrays should be a different type from an array with permitted values, do speak up.

jpakkane · 2017-12-02T18:48:04Z

Updated #2696. Please review.

jon-turney · 2017-12-02T21:05:43Z

docs/markdown/Build-options.md

+
+### Booleans
+
+Booleans may have values of either `true` or `false`. If not default value is


'If no default value'

jon-turney · 2017-12-02T21:06:15Z

docs/markdown/Build-options.md

+
+### Arrays
+
+Arrays allow one or more of the values in the `choices` parameter to be selected.


Should this say 'zero or more'?

1ace · 2017-12-04T10:25:24Z

(forgot hit send to post this on friday... 🤦‍♂️)

If people have opinions on if there should be only one array type or whether freeform arrays should be a different type from an array with permitted values, do speak up.

I misunderstood #2696 at first, so I gave an incorrect comment which you can ignore.

Looking at it all now, I think just having choices present or not to define whether choices are restricted sounds like a clear enough way of differentiating between the two.

I would suggest renaming this type to plain array though, as the fact those are strings in the freeform one is mostly irrelevant. Combo are not called "stringcombo" either, and arrays are more like "combo+" (in the regex sense of "one or more combo values"). There was a suggestion to rename array to list as well, both names are ok imo.

One other question is: should duplicates be allowed? I would lean towards "no", so that meson can catch human errors like optionfoo=item1,item2,item3,item3,item5.

jpakkane · 2017-12-04T22:14:52Z

The user facing name was already array, #2732 changes the internal name.

dcbaker force-pushed the submit/options-list branch 2 times, most recently from 8bf4c91 to b474b6a Compare October 1, 2017 03:19

1ace reviewed Oct 25, 2017

View reviewed changes

dcbaker force-pushed the submit/options-list branch from b474b6a to 9866346 Compare October 25, 2017 17:02

dcbaker force-pushed the submit/options-list branch from 9866346 to dc061c2 Compare November 16, 2017 19:47

nirbheek reviewed Nov 27, 2017

View reviewed changes

1ace reviewed Nov 27, 2017

View reviewed changes

1ace mentioned this pull request Nov 27, 2017

mconf: print choices of base options #2695

Merged

dcbaker force-pushed the submit/options-list branch from dc061c2 to e9aa293 Compare November 28, 2017 18:53

dcbaker force-pushed the submit/options-list branch from e9aa293 to 7d0b500 Compare November 28, 2017 21:39

dcbaker added 2 commits November 29, 2017 14:13

run_unittests.py: remove newline at top of file

7c779a7

So that the shbang is ther rist line and ./run_unittests.py works.

dcbaker force-pushed the submit/options-list branch from 7d0b500 to c9351ce Compare November 29, 2017 22:22

1ace mentioned this pull request Dec 1, 2017

Users can have array options #2696

Closed

jpakkane merged commit 793fc00 into mesonbuild:master Dec 2, 2017

jon-turney reviewed Dec 2, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an array type to user options #2390

Add an array type to user options #2390

dcbaker commented Sep 29, 2017

dcbaker commented Oct 10, 2017

1ace Oct 25, 2017

dcbaker Oct 25, 2017

dcbaker commented Oct 26, 2017

liugang commented Oct 26, 2017 •

edited

jpakkane commented Nov 9, 2017

dcbaker commented Nov 10, 2017

jpakkane commented Nov 10, 2017

dcbaker commented Nov 16, 2017

nirbheek Nov 27, 2017

nirbheek commented Nov 27, 2017

1ace Nov 27, 2017

jibsen commented Nov 27, 2017

1ace commented Nov 27, 2017 •

edited

jpakkane commented Nov 27, 2017

jpakkane commented Nov 27, 2017

dcbaker commented Nov 28, 2017

dcbaker commented Nov 28, 2017

jpakkane commented Nov 29, 2017

dcbaker commented Nov 29, 2017

dcbaker commented Nov 29, 2017 •

edited

jpakkane commented Nov 30, 2017

jpakkane commented Dec 2, 2017

jon-turney Dec 2, 2017

jon-turney Dec 2, 2017

1ace commented Dec 4, 2017

jpakkane commented Dec 4, 2017

		@@ -29,6 +30,8 @@ name.

		These options are accessed in Meson code with the `get_option` function.

		The array type is new in 0.43.0


		### Booleans

		Booleans may have values of either `true` or `false`. If not default value is


		### Arrays

		Arrays allow one or more of the values in the `choices` parameter to be selected.

Add an array type to user options #2390

Add an array type to user options #2390

Conversation

dcbaker commented Sep 29, 2017

dcbaker commented Oct 10, 2017

1ace Oct 25, 2017

Choose a reason for hiding this comment

dcbaker Oct 25, 2017

Choose a reason for hiding this comment

dcbaker commented Oct 26, 2017

liugang commented Oct 26, 2017 • edited

jpakkane commented Nov 9, 2017

dcbaker commented Nov 10, 2017

jpakkane commented Nov 10, 2017

dcbaker commented Nov 16, 2017

nirbheek Nov 27, 2017

Choose a reason for hiding this comment

nirbheek commented Nov 27, 2017

1ace Nov 27, 2017

Choose a reason for hiding this comment

jibsen commented Nov 27, 2017

1ace commented Nov 27, 2017 • edited

jpakkane commented Nov 27, 2017

jpakkane commented Nov 27, 2017

dcbaker commented Nov 28, 2017

dcbaker commented Nov 28, 2017

jpakkane commented Nov 29, 2017

dcbaker commented Nov 29, 2017

dcbaker commented Nov 29, 2017 • edited

jpakkane commented Nov 30, 2017

jpakkane commented Dec 2, 2017

jon-turney Dec 2, 2017

Choose a reason for hiding this comment

jon-turney Dec 2, 2017

Choose a reason for hiding this comment

1ace commented Dec 4, 2017

jpakkane commented Dec 4, 2017

liugang commented Oct 26, 2017 •

edited

1ace commented Nov 27, 2017 •

edited

dcbaker commented Nov 29, 2017 •

edited