~lattis/muon

This thread contains a patchset. You're looking at the original emails, but you may wish to use the patch review UI. Review patch
3 2

[PATCH] implement automatic array conversion of even more args

Details
Message ID
<20210906214658.1105817-1-eschwartz@archlinux.org>
DKIM signature
pass
Download raw message
Patch: +8 -8
---
 src/functions/default.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/src/functions/default.c b/src/functions/default.c
index 18219d5..26017fe 100644
--- a/src/functions/default.c
+++ b/src/functions/default.c
@@ -87,7 +87,7 @@ func_project(struct workspace *wk, uint32_t _, uint32_t args_node, uint32_t *obj
		kw_version
	};
	struct args_kw akw[] = {
		[kw_default_options] = { "default_options", obj_array },
		[kw_default_options] = { "default_options", ARG_TYPE_ARRAY_OF | obj_any },
		[kw_license] = { "license" },
		[kw_meson_version] = { "meson_version", obj_string },
		[kw_subproject_dir] = { "subproject_dir", obj_string },
@@ -324,8 +324,8 @@ func_declare_dependency(struct workspace *wk, uint32_t _, uint32_t args_node, ui
		kw_include_directories,
	};
	struct args_kw akw[] = {
		[kw_link_with] = { "link_with", obj_array },
		[kw_include_directories] = { "include_directories", obj_any },
		[kw_link_with] = { "link_with", ARG_TYPE_ARRAY_OF | obj_any },
		[kw_include_directories] = { "include_directories", ARG_TYPE_ARRAY_OF | obj_any },
		0
	};

@@ -382,7 +382,7 @@ tgt_common(struct workspace *wk, uint32_t args_node, uint32_t *obj, enum tgt_typ
	struct args_kw akw[] = {
		[kw_sources] = { "sources", ARG_TYPE_ARRAY_OF | obj_any },
		[kw_include_directories] = { "include_directories", ARG_TYPE_ARRAY_OF | obj_any },
		[kw_dependencies] = { "dependencies", obj_array },
		[kw_dependencies] = { "dependencies", ARG_TYPE_ARRAY_OF | obj_any },
		[kw_install] = { "install", obj_bool },
		[kw_install_dir] = { "install_dir", obj_string },
		[kw_link_with] = { "link_with", ARG_TYPE_ARRAY_OF | obj_any },
@@ -391,9 +391,9 @@ tgt_common(struct workspace *wk, uint32_t args_node, uint32_t *obj, enum tgt_typ
		[kw_extra_files] = { "extra_files", obj_any }, // ignored
		[kw_target_type] = { "target_type", obj_string },
		/* lang args */
		[kw_c_args] = { "c_args", obj_array },
		[kw_cpp_args] = { "cpp_args", obj_array },
		[kw_objc_args] = { "objc_args", obj_array },
		[kw_c_args] = { "c_args", ARG_TYPE_ARRAY_OF | obj_any },
		[kw_cpp_args] = { "cpp_args", ARG_TYPE_ARRAY_OF | obj_any },
		[kw_objc_args] = { "objc_args", ARG_TYPE_ARRAY_OF | obj_any },
		[kw_link_args] = { "link_args", ARG_TYPE_ARRAY_OF | obj_string },
		0
	};
@@ -621,7 +621,7 @@ func_subproject(struct workspace *wk, uint32_t rcvr, uint32_t args_node, uint32_
		kw_default_options,
	};
	struct args_kw akw[] = {
		[kw_default_options] = { "default_options", obj_array },
		[kw_default_options] = { "default_options", ARG_TYPE_ARRAY_OF | obj_any },
		0
	};

-- 
2.33.0
Details
Message ID
<20210907011948.uvu73ccnvx7xwexf@hostomei>
In-Reply-To
<20210906214658.1105817-1-eschwartz@archlinux.org> (view parent)
DKIM signature
pass
Download raw message
Thanks for this!  Overall, I think (hope) we can be a little more strict
with some of these.  Basically using obj_any means that we have to write
some sort of custom type checking logic, so to the extent possible we
should let interp_args handle it.  Perhaps the meaning of
ARG_TYPE_ARRAY_OF is a little unclear; `ARG_TYPE_ARRAY_OF | obj_string`
for instance means this:

'hello' # gets coerced to ['hello']

or

['hello', 'there']

but not

['hello', 'there', false]

So it can provide even more type checking than plain old obj_array did.
I have made a few suggestions, but maybe I am to optimistic.  My plan so
far has been to implement the strictest possible checking, and slowly
loosen it until an acceptable portion of meson projects can be built
with muon.


> @@ -87,7 +87,7 @@ func_project(struct workspace *wk, uint32_t _, uint32_t args_node, uint32_t *obj
>  		kw_version
>  	};
>  	struct args_kw akw[] = {
> -		[kw_default_options] = { "default_options", obj_array },
> +		[kw_default_options] = { "default_options", ARG_TYPE_ARRAY_OF | obj_any },
>  		[kw_license] = { "license" },

Can we use `ARG_TYPE_ARRAY_OF | obj_string` instead?  Or can
default_options really be of _any_ type?

> -		[kw_link_with] = { "link_with", obj_array },
> -		[kw_include_directories] = { "include_directories", obj_any },
> +		[kw_link_with] = { "link_with", ARG_TYPE_ARRAY_OF | obj_any },
> +		[kw_include_directories] = { "include_directories", ARG_TYPE_ARRAY_OF | obj_any },

include_directories already has custom type coercion (see ther
`coerce_dirs()` call), so it should probably be left as obj_any.
I suppose link with could be more strict (obj_build_target), but I'm
not sure.

> @@ -382,7 +382,7 @@ tgt_common(struct workspace *wk, uint32_t args_node, uint32_t *obj, enum tgt_typ
>  	struct args_kw akw[] = {
>  		[kw_sources] = { "sources", ARG_TYPE_ARRAY_OF | obj_any },
>  		[kw_include_directories] = { "include_directories", ARG_TYPE_ARRAY_OF | obj_any },
> -		[kw_dependencies] = { "dependencies", obj_array },
> +		[kw_dependencies] = { "dependencies", ARG_TYPE_ARRAY_OF | obj_any },
>  		[kw_install] = { "install", obj_bool },

Same here, could we check for obj_dependency?

> -		[kw_c_args] = { "c_args", obj_array },
> -		[kw_cpp_args] = { "cpp_args", obj_array },
> -		[kw_objc_args] = { "objc_args", obj_array },
> +		[kw_c_args] = { "c_args", ARG_TYPE_ARRAY_OF | obj_any },
> +		[kw_cpp_args] = { "cpp_args", ARG_TYPE_ARRAY_OF | obj_any },
> +		[kw_objc_args] = { "objc_args", ARG_TYPE_ARRAY_OF | obj_any },

I think all these should be obj_string.

> @@ -621,7 +621,7 @@ func_subproject(struct workspace *wk, uint32_t rcvr, uint32_t args_node, uint32_
>  		kw_default_options,
>  	};
>  	struct args_kw akw[] = {
> -		[kw_default_options] = { "default_options", obj_array },
> +		[kw_default_options] = { "default_options", ARG_TYPE_ARRAY_OF | obj_any },

obj_string?

- Stone
Details
Message ID
<a3598277-d213-9c23-206e-a5c09090dbd9@archlinux.org>
In-Reply-To
<20210907011948.uvu73ccnvx7xwexf@hostomei> (view parent)
DKIM signature
pass
Download raw message
On 9/6/21 9:19 PM, Stone Tickle wrote:
> Thanks for this!  Overall, I think (hope) we can be a little more strict
> with some of these.  Basically using obj_any means that we have to write
> some sort of custom type checking logic, so to the extent possible we
> should let interp_args handle it.  Perhaps the meaning of
> ARG_TYPE_ARRAY_OF is a little unclear; `ARG_TYPE_ARRAY_OF | obj_string`
> for instance means this:
> 
> 'hello' # gets coerced to ['hello']
> 
> or
> 
> ['hello', 'there']
> 
> but not
> 
> ['hello', 'there', false]
> 
> So it can provide even more type checking than plain old obj_array did.
> I have made a few suggestions, but maybe I am to optimistic.  My plan so
> far has been to implement the strictest possible checking, and slowly
> loosen it until an acceptable portion of meson projects can be built
> with muon.


That makes sense -- I originally wrote it that way because it was the
most obvious change from "accepts obj_array containing obj_any" to
"accepts obj_any or obj_array containing obj_any".

Also, I assumed that it was probably being checked somewhere else
anyway, but indeed custom type checking logic "somewhere else" feels
like it would be sub-optimal.


>> @@ -87,7 +87,7 @@ func_project(struct workspace *wk, uint32_t _, uint32_t args_node, uint32_t *obj
>>  		kw_version
>>  	};
>>  	struct args_kw akw[] = {
>> -		[kw_default_options] = { "default_options", obj_array },
>> +		[kw_default_options] = { "default_options", ARG_TYPE_ARRAY_OF | obj_any },
>>  		[kw_license] = { "license" },
> 
> Can we use `ARG_TYPE_ARRAY_OF | obj_string` instead?  Or can
> default_options really be of _any_ type?


Nah, we can harden this while we are at it, because it indeed can only
be a string or array of strings (see e.g.
mesonbuild/interpreter/{interpreter.py,dependencyfallbacks.py} where it
is always passed through mesonbuild.mesonlib.stringlistify())


>> -		[kw_link_with] = { "link_with", obj_array },
>> -		[kw_include_directories] = { "include_directories", obj_any },
>> +		[kw_link_with] = { "link_with", ARG_TYPE_ARRAY_OF | obj_any },
>> +		[kw_include_directories] = { "include_directories", ARG_TYPE_ARRAY_OF | obj_any },
> 
> include_directories already has custom type coercion (see ther
> `coerce_dirs()` call), so it should probably be left as obj_any.
> I suppose link with could be more strict (obj_build_target), but I'm
> not sure.


"""
since 0.51.0) The arguments can also be custom targets. In this case
Meson will assume that merely adding the output file in the linker
command line is sufficient to make linking work. If this is not
sufficient, then the build system writer must write all other steps
manually.
"""

Admittedly this may potentially require additional logic at time of use
if muon expects to use build_target details to get the link name. I
didn't check.

It's not the most common thing ever, because *usually* people just use
library() to produce output files! But one example use would be a
custom_target which runs cargo and builds a rust project with a C
library binding output.


>> @@ -382,7 +382,7 @@ tgt_common(struct workspace *wk, uint32_t args_node, uint32_t *obj, enum tgt_typ
>>  	struct args_kw akw[] = {
>>  		[kw_sources] = { "sources", ARG_TYPE_ARRAY_OF | obj_any },
>>  		[kw_include_directories] = { "include_directories", ARG_TYPE_ARRAY_OF | obj_any },
>> -		[kw_dependencies] = { "dependencies", obj_array },
>> +		[kw_dependencies] = { "dependencies", ARG_TYPE_ARRAY_OF | obj_any },
>>  		[kw_install] = { "install", obj_bool },
> 
> Same here, could we check for obj_dependency?


Yes. I assume this already covers declare_dependency and find_library.


>> -		[kw_c_args] = { "c_args", obj_array },
>> -		[kw_cpp_args] = { "cpp_args", obj_array },
>> -		[kw_objc_args] = { "objc_args", obj_array },
>> +		[kw_c_args] = { "c_args", ARG_TYPE_ARRAY_OF | obj_any },
>> +		[kw_cpp_args] = { "cpp_args", ARG_TYPE_ARRAY_OF | obj_any },
>> +		[kw_objc_args] = { "objc_args", ARG_TYPE_ARRAY_OF | obj_any },
> 
> I think all these should be obj_string.


Right, that makes sense...


>> @@ -621,7 +621,7 @@ func_subproject(struct workspace *wk, uint32_t rcvr, uint32_t args_node, uint32_
>>  		kw_default_options,
>>  	};
>>  	struct args_kw akw[] = {
>> -		[kw_default_options] = { "default_options", obj_array },
>> +		[kw_default_options] = { "default_options", ARG_TYPE_ARRAY_OF | obj_any },
> 
> obj_string?


Quite.


-- 
Eli Schwartz
Arch Linux Bug Wrangler and Trusted User
Details
Message ID
<770c2b8d-6e8f-b4df-c4f9-6b73d8735d1d@archlinux.org>
In-Reply-To
<a3598277-d213-9c23-206e-a5c09090dbd9@archlinux.org> (view parent)
DKIM signature
pass
Download raw message
On 9/8/21 10:38 PM, Eli Schwartz wrote:
> On 9/6/21 9:19 PM, Stone Tickle wrote:
>> Thanks for this!  Overall, I think (hope) we can be a little more strict
>> with some of these.  Basically using obj_any means that we have to write
>> some sort of custom type checking logic, so to the extent possible we
>> should let interp_args handle it.  Perhaps the meaning of
>> ARG_TYPE_ARRAY_OF is a little unclear; `ARG_TYPE_ARRAY_OF | obj_string`
>> for instance means this:
>>
>> 'hello' # gets coerced to ['hello']
>>
>> or
>>
>> ['hello', 'there']
>>
>> but not
>>
>> ['hello', 'there', false]
>>
>> So it can provide even more type checking than plain old obj_array did.
>> I have made a few suggestions, but maybe I am to optimistic.  My plan so
>> far has been to implement the strictest possible checking, and slowly
>> loosen it until an acceptable portion of meson projects can be built
>> with muon.
> 
> 
> That makes sense -- I originally wrote it that way because it was the
> most obvious change from "accepts obj_array containing obj_any" to
> "accepts obj_any or obj_array containing obj_any".
> 
> Also, I assumed that it was probably being checked somewhere else
> anyway, but indeed custom type checking logic "somewhere else" feels
> like it would be sub-optimal.


After testing... some projects end up doing things like:

foo_dep = []
if get_option('foo')
    foo_dep = dependency('foo')
endif
bar_dep = dependency('bar')

executable('prog', 'prog.c', dependencies: [foo_dep, bar_dep])

So you end up with a recursive list of lists-and-dependencies. And when
meson consults this to find out what dependencies to use, it flattens
the list and detects those dependencies anywhere.

Personally, I do think this is an anti-pattern, people should either use
new-fangled feature options with

foo_dep = dependency('foo', required: get_option('foo'))

or do it the old-fashioned way:

foo_dep = dependency('', required: false)
if get_option('foo')
    foo_dep = dependency('foo')
endif


Either one will result in foo_dep always being of type "dependency",
which may be foo_dep.found() == false

I feel like this is probably a reasonable situation to say "don't rely
on this implicit and slightly strange behavior". And of course this
isn't documented.

I also found this bizarre one in SDL2_mixer:

feature_args = ['-DMUSIC_WAV', '-DMUSIC_OGG']

sdl2_mixer_lib = library(
    ....,
    c_args: [feature_args],
    ....
)

Which was kinda... weird. Totally pointless.

This is honestly too much oddness, coercing single objects into arrays
is one thing but turning byzantine nested structures into flat arrays is
a bit different.

But if you'd like to make this work too, it would be best done globally
during array parsing rather than by marking everything as obj_any, I guess.


>>> @@ -87,7 +87,7 @@ func_project(struct workspace *wk, uint32_t _, uint32_t args_node, uint32_t *obj
>>>  		kw_version
>>>  	};
>>>  	struct args_kw akw[] = {
>>> -		[kw_default_options] = { "default_options", obj_array },
>>> +		[kw_default_options] = { "default_options", ARG_TYPE_ARRAY_OF | obj_any },
>>>  		[kw_license] = { "license" },
>>
>> Can we use `ARG_TYPE_ARRAY_OF | obj_string` instead?  Or can
>> default_options really be of _any_ type?
> 
> 
> Nah, we can harden this while we are at it, because it indeed can only
> be a string or array of strings (see e.g.
> mesonbuild/interpreter/{interpreter.py,dependencyfallbacks.py} where it
> is always passed through mesonbuild.mesonlib.stringlistify())
> 
> 
>>> -		[kw_link_with] = { "link_with", obj_array },
>>> -		[kw_include_directories] = { "include_directories", obj_any },
>>> +		[kw_link_with] = { "link_with", ARG_TYPE_ARRAY_OF | obj_any },
>>> +		[kw_include_directories] = { "include_directories", ARG_TYPE_ARRAY_OF | obj_any },
>>
>> include_directories already has custom type coercion (see ther
>> `coerce_dirs()` call), so it should probably be left as obj_any.
>> I suppose link with could be more strict (obj_build_target), but I'm
>> not sure.
> 
> 
> """
> since 0.51.0) The arguments can also be custom targets. In this case
> Meson will assume that merely adding the output file in the linker
> command line is sufficient to make linking work. If this is not
> sufficient, then the build system writer must write all other steps
> manually.
> """
> 
> Admittedly this may potentially require additional logic at time of use
> if muon expects to use build_target details to get the link name. I
> didn't check.
> 
> It's not the most common thing ever, because *usually* people just use
> library() to produce output files! But one example use would be a
> custom_target which runs cargo and builds a rust project with a C
> library binding output.
> 
> 
>>> @@ -382,7 +382,7 @@ tgt_common(struct workspace *wk, uint32_t args_node, uint32_t *obj, enum tgt_typ
>>>  	struct args_kw akw[] = {
>>>  		[kw_sources] = { "sources", ARG_TYPE_ARRAY_OF | obj_any },
>>>  		[kw_include_directories] = { "include_directories", ARG_TYPE_ARRAY_OF | obj_any },
>>> -		[kw_dependencies] = { "dependencies", obj_array },
>>> +		[kw_dependencies] = { "dependencies", ARG_TYPE_ARRAY_OF | obj_any },
>>>  		[kw_install] = { "install", obj_bool },
>>
>> Same here, could we check for obj_dependency?
> 
> 
> Yes. I assume this already covers declare_dependency and find_library.


Unfortunately it does not:

/home/eschwartz/git/mesonbuild/wrapdb/subprojects/Chipmunk2D-Chipmunk-6.2.2/meson.build:20:46:
error: expected type dependency, got external_library
 20 | ], include_directories : inc, dependencies : m_dep)

So I will note in the commit message that I am not changing this.


>>> -		[kw_c_args] = { "c_args", obj_array },
>>> -		[kw_cpp_args] = { "cpp_args", obj_array },
>>> -		[kw_objc_args] = { "objc_args", obj_array },
>>> +		[kw_c_args] = { "c_args", ARG_TYPE_ARRAY_OF | obj_any },
>>> +		[kw_cpp_args] = { "cpp_args", ARG_TYPE_ARRAY_OF | obj_any },
>>> +		[kw_objc_args] = { "objc_args", ARG_TYPE_ARRAY_OF | obj_any },
>>
>> I think all these should be obj_string.
> 
> 
> Right, that makes sense...
> 
> 
>>> @@ -621,7 +621,7 @@ func_subproject(struct workspace *wk, uint32_t rcvr, uint32_t args_node, uint32_
>>>  		kw_default_options,
>>>  	};
>>>  	struct args_kw akw[] = {
>>> -		[kw_default_options] = { "default_options", obj_array },
>>> +		[kw_default_options] = { "default_options", ARG_TYPE_ARRAY_OF | obj_any },
>>
>> obj_string?
> 
> 
> Quite.



-- 
Eli Schwartz
Arch Linux Bug Wrangler and Trusted User
Reply to thread Export thread (mbox)