New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
glib-mkenums and glib-genmarshal support #718
Conversation
OK, it looks like |
296b4cc
to
58607ba
Compare
Rebased to get CI refreshed. |
0830a38
to
964bd6b
Compare
Some quick drive-by bikeshedding: looks ok at first glance, seems to map the tool arguments pretty much 1:1 to the functions, so should work fine. It might be possible to do it nicer though .. somehow (waves hands). It might be possible to combine the header + body creation function calls into one single call for example? That might require returning an array with two targets, or perhaps we can still return a single one and it just depends on the two? That might be nicer usability-wise, but not sure if there are cases where it would not work. Alternatively, for |
Maybe something like the following:
And then you would have:
|
The other annoyance is that if you pass If generating both, it'd be nice if the |
Why would it require an extra wrapper? We just have to make sure the .h file is generated first and we can do that behind the scenes with |
How would you get the It's not strictly necessary though, since the |
I thought that include would be done automatically by the generator. Well never mind then. |
I'm not sure I understand why an |
I was talking about I didn't think you'd generate both header and source file for |
I've modified |
How are these for some defaults? if is_header:
defaults = {
'fhead': '''#ifndef __{output}__
#define __{output}__
#include <glib-object.h>
G_BEGIN_DECLS
'''.format(output=output_filename.to_upper().underscorify()),
'fprod': '''
/* enumerations from \"@filename@\" */
''',
'vhead': '''GType @enum_name@_get_type (void);
#define @ENUMPREFIX@_TYPE_@ENUMSHORT@ (@enum_name@_get_type())
''',
'ftail': '''G_END_DECLS
#endif /* __{output}__ */'''.format(output=output_filename.to_upper().underscorify())
}
else:
defaults = {
'fhead': '''{headers}
'''.format(headers),
'fprod': '''
/* enumerations from "@filename@" */''',
'vhead': '''GType
@enum_name@_get_type (void)
{
static gsize id = 0;
static const G@Type@Value values[] = {''',
'vprod': ''' { @VALUENAME@, "@VALUENAME@", "@valuenick@" },''',
'vtail': ''' { 0, NULL, NULL }
};
if (g_once_init_enter (&id)) {
GType tmp = g_@type@_register_static ("@EnumName@", values);
g_once_init_leave (&id, tmp);
}
return (GType) id;
}'''
} where I'm not sure whether that casting thing that GStreamer does is needed. |
Any one of them could be overridden and if |
I'm not very familiar with gir but these defaults start to look a bit too magical. Is it ok to leave them empty for now? We can add stuff later once people start using it and complaining. |
From IRC, it's @thiblahute's contention that everyone writes the "same" template for mkenums (this isn't quite gir, btw.) |
Sorry for the delay. It's looking good, but just to be sure: if this was merged as-is, would it be good enough to fulfill all requirements for GStreamer so they could drop their custom hack code? @tp-m? |
Yes, it should be good enough for us, could be enhanced but... :) |
Is there a reason we're not going for generating both header and body at the same time with mkenums as well? |
Mostly @jpakkane isn't convinced? |
I thought the contention was on 'everyone uses the same template'. I think we can and should generate both mkenum body and header at the same time. |
If we merge the two then it would look something like this:
Seems reasonable to me and consistent with genmarshal. How do others feel? Unrelated, Also, just to be sure, there is never need to install the source file? (and if there is one person in the world who needs to do that, they can use a custom install script) |
First two arguments should be the header and the source code. I think Also |
Or, alternatively the first argument should be just the basename |
I was thinking maybe someone wants to not have the same basename for the header and body, but I can't think of a good reason for that, so w/e. Let's have it be the basename. 👍 |
So we're going to require a template, then? Not using the argument form? Or add twice the number of arguments for header vs. source? |
Ah, right. That was the issue. Not sure, undecided :) On a side note, I found a glib-mkenums use case I haven't seen before. Bit exotic imho, still haven't seen that in the wild, but might be interesting to check if we can cover it or not: |
To make things faster I created #817 which is this branch but mkenum converted into a single invocation as discussed. Please add your comments there. Thanks. |
This follows the same style as gnome.compile_resources, which produces both files from one invocation.
Dashes aren't allowed in keyword argument names.
db4ab0d
to
2840539
Compare
I pulled in the changes from #817 and then made it possible to support both forms. Basically, just don't require both templates to be specified. That way, we don't need to duplicate all of the optional arguments for each type; just specify one template at a time (cf. the second executable in the test.) If you don't specify any templates, then that's the same as generating everything from the arguments (cf. the third executable in the test.) The only "bug" there is that |
I think it's ok to rename that to |
(for instance, people use it to generate gsettings enum xml schema files that are installed, and that's not a header) |
Sorry, what is the implication of the comment about the |
So I guess we generally should just allow |
Or we could keep |
Personally I think the install bits are not important, if you are installing headers you already have a list of headers.. just throw the output of these into it. |
Ignoring the install_header this looks ok to me. Could someone with Gnome experience comment on whether this does everything that is needed to support real world Gnome stuff such as GStreamer? |
You cannot just use I only think it should not be named |
That's a shame.
Should only be installing a header...
I've used both mkenums and genmarhsal in 3 projects (which all just used templates) I've ported so far with success. EDIT: Though I remember if you didn't specify the template it just threw an empty file out which was useless and should error or something? |
That's on purpose. |
Slightly off topic but what is the proper way to pass these around then? foo_enums = gnome.mkenums(...)
# This fails, unresolved types
gnome.generate_gir(sources: foo_enums, ...)
# This works but has a missing dependency?
gnome.generate_gir(sources: ['foo-enum.h', 'foo-enum.c'], ...) In general I think Meson needs to be stricter and more verbose about types. |
|
I'd say it is time to finally merge this. Whatever issues still remain can be fixed afterwards. Thank you to all involved, this has most likely been the longest reviewed commit in the entire project. |
Note that I never changed the name from |
We should definitely change that I think. Can we not do a release till that is fixed? |
Okay, crap, we already released... Can we do a brown-paper-bag release with that changed please. |
As I said earlier, nothing but a header should ever be installed, why is this a problem? |
This is a WIP; it's almost complete except for the problem noted below.
It adds
gnome.mkenums
andgnome.genmarshal
(this namespace is a bit overloaded) which can convert to the enumeration and marshaller code from various sources.As noted on IRC, when compiling with
build
within the source it compiles fine, but when compiling outside the source, it fails. It seems like the working directory of compilation for the generated file is different. Unfortunately, the latter is the form used by the tests so it's not quite there yet.This is based on #712.