After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 725614 - Handling commandline options
Handling commandline options
Status: RESOLVED OBSOLETE
Product: gjs
Classification: Bindings
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gjs-maint
gjs-maint
Depends on: 727455
Blocks:
 
 
Reported: 2014-03-03 20:21 UTC by Jonas Danielsson
Modified: 2018-01-27 11:55 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gi: arg: add support for flat array arguments (8.44 KB, patch)
2014-05-24 23:34 UTC, Jonas Danielsson
rejected Details | Review

Description Jonas Danielsson 2014-03-03 20:21:03 UTC
Hi,

I was wondering if there is a preferred way to handle commandline options in gjs applications.

I noticed that it was not possible to use the GOptionEntry/GOptionContext things because arg_data required a void* pointer. Then I noticed the new stuff in GLib 2.40 that had the GApplication method add_main_option_entries that worked around tha with a VariantDict. The doc page I stumpled upon (https://people.gnome.org/~gcampagna/docs/Gio-2.0/Gio.Application.add_main_option_entries.html) went on and linked to GEntryOption that showed how to create a GOptionEntry as: 

let optionEntry = new GLib.OptionEntry({
    short_name: value
    flags: value
    arg: value
});

When I tried something like this in Maps, I get:
Gjs-WARNING **: JS ERROR: Error: Unable to construct struct type OptionEntry since it has no default constructor and cannot be allocated directly

Is there something one could do to be able to construct the OptionEntry using gjs.
Or is there some other way one should handle commandline options?
Comment 1 Giovanni Campagna 2014-03-04 13:32:09 UTC
Yes, unfortunately one cannot create GLib.OptionEntrys because they don't have a constructor. It's a documentation bug that they appear to have one.

What is more problematic, GOptionEntries are not even boxed types (they are not memory managed by glib, there is no destructor) so they can't really be used, even with a manual binding.
This should be fixed in glib first.
Comment 2 Jonas Danielsson 2014-03-05 13:27:53 UTC
Thanks!

I did GOptionEntry up as a boxed type in my local glib repository and a check with gobject-introspection tool tells me:

gobject-introspection$ jhbuild shell
gobject-introspection$ ./gi-dump-types g_option_entry_get_type
  <boxed name="GOptionEntry" get-type="g_option_entry_get_type"/>

But i still get the same error in Maps when I try to instanciate an GOptionentry.
Sorry if these are dumb questions, but: what's the next step if I want to keep at this?
Comment 3 Jasper St. Pierre (not reading bugmail) 2014-03-05 13:55:12 UTC
The biggest issue is that you can't use GOptionEntries directly, because they look like this:

  { "foo", 'f', 0, G_OPTION_ARG_NONE, &opt_foo, NULL },

What do you pass for the opt_foo arg in a bound environment?
Comment 4 Jonas Danielsson 2014-03-05 14:02:39 UTC
(In reply to comment #3)
> The biggest issue is that you can't use GOptionEntries directly, because they
> look like this:
> 
>   { "foo", 'f', 0, G_OPTION_ARG_NONE, &opt_foo, NULL },
> 
> What do you pass for the opt_foo arg in a bound environment?

I pass null, the new gapplication method add_main_option_entries accept that and puts the results in a VariantDict: https://people.gnome.org/~gcampagna/docs/Gio-2.0/Gio.Application.add_main_option_entries.html
Comment 5 Giovanni Campagna 2014-03-05 14:26:06 UTC
(In reply to comment #2)
> Thanks!
> 
> I did GOptionEntry up as a boxed type in my local glib repository and a check
> with gobject-introspection tool tells me:
> 
> gobject-introspection$ jhbuild shell
> gobject-introspection$ ./gi-dump-types g_option_entry_get_type
>   <boxed name="GOptionEntry" get-type="g_option_entry_get_type"/>
> 
> But i still get the same error in Maps when I try to instanciate an
> GOptionentry.
> Sorry if these are dumb questions, but: what's the next step if I want to keep
> at this?

You need to add a public constructor that accepts at least the non writable string fields.
During review at glib they will probably ask you to have a public copy method and a destructor, to make it a proper boxed type.
Comment 6 Jonas Danielsson 2014-03-07 12:52:58 UTC
Hi, I am not sure this approach will work.

The method on GApplication, add_main_option_entries takes a const GOptionEntry *.
And that should be a NULL terminated array of GOptionEntry structs.

But what I would pass will be a javascript array of GLib.GOptionEntry.new() instances. It seem that is not handled very good.

Is this approach doomed?

Is there some other way gjs apps could do commandline option handling. Maybe through libgjs?

Thanks.
Comment 7 Colin Walters 2014-05-21 13:14:37 UTC
I have a parser here: https://git.gnome.org/browse/gnome-continuous/tree/src/js/argparse.js?id=b4872501a8b29a116503c1c3e4f2a6cc6d7a8573

Options:

1) Upstream that, document, test in gjs
2) Create set of overrides for GLib.OptionContext in gjs?
Comment 8 Colin Walters 2014-05-21 13:29:55 UTC
The previous discussion looks like:

3) Box GOptionContext in GLib
   Which needs more investigation too
Comment 9 Jonas Danielsson 2014-05-22 11:07:21 UTC
(In reply to comment #8)
> The previous discussion looks like:
> 
> 3) Box GOptionContext in GLib
>    Which needs more investigation too

If we box GOptionEntry and/or GOptionContext can we get around the fact that g_option_context_add_main_entries (and g_application_add_main_option_entries) take const GOptionEntry *entries as argument which is a null-terminated array of GOptionEntry?

The python binding seem to create its own pygoptiongroup.c that is a wrapper for GOptionGroup. So that could be one way also if that is not to hacky.

(In reply to comment #7)
> I have a parser here:
> https://git.gnome.org/browse/gnome-continuous/tree/src/js/argparse.js?id=b4872501a8b29a116503c1c3e4f2a6cc6d7a8573
> 
> Options:
> 
> 1) Upstream that, document, test in gjs

That would be fine for me. We miss out on the fancy option parsing in GApplication then tho.

> 2) Create set of overrides for GLib.OptionContext in gjs?

I don't see what overrides would help. Any ideas?
Comment 10 Jasper St. Pierre (not reading bugmail) 2014-05-22 11:15:57 UTC
(In reply to comment #9)
> If we box GOptionEntry and/or GOptionContext can we get around the fact that
> g_option_context_add_main_entries (and g_application_add_main_option_entries)
> take const GOptionEntry *entries as argument which is a null-terminated array
> of GOptionEntry?

I think we can support flat valued arrays, but maybe the code I added in gjs was just for GValue.
Comment 11 Jonas Danielsson 2014-05-22 16:22:58 UTC
(In reply to comment #10)
> (In reply to comment #9)
> > If we box GOptionEntry and/or GOptionContext can we get around the fact that
> > g_option_context_add_main_entries (and g_application_add_main_option_entries)
> > take const GOptionEntry *entries as argument which is a null-terminated array
> > of GOptionEntry?
> 
> I think we can support flat valued arrays, but maybe the code I added in gjs
> was just for GValue.

Yeah it seems to be just for flat GValue arrays. Do you have any hint on making it general for all flat arrays? I'm at the airport on my way to a hackfest in London. I could hack a bit on it there if you think it's worthwhile.
Comment 12 Jasper St. Pierre (not reading bugmail) 2014-05-22 16:59:45 UTC
As long as you don't need the *exact* same struct pointer, it should be fine.

Check for !g_type_info_is_pointer(param_info) when getting an array. Then create the array the length you need (the struct should have the size of it), and copy the structs into the new array, and then pass that in.

It won't work with arbitrary boxed types, because boxed types *can* be private.
Comment 13 Simon Feltman 2014-05-23 05:34:30 UTC
(In reply to comment #9)
> The python binding seem to create its own pygoptiongroup.c that is a wrapper
> for GOptionGroup. So that could be one way also if that is not to hacky.

I would love to get rid of this code in the Python bindings and replace it with introspection if possible but I am not sure it will be very easy...

In regards to "const GOptionEntry *entries" being a "a NULL-terminated array of GOptionEntrys", I take it this means a flat array with a zeroed out last item as a sentinel. So we would need to look into GI and bindings and make sure they know deal with a NULL terminated flat array.

As Giovanni mentioned, beyond boxing, GOptionEntry would need a constructor which takes arguments for the string fields (transfer none) and copies these arguments into the fields, then the boxed de-allocator would additionally need to free these fields so they don't leak.

A problem with all this is g_option_group_add_entries [1] does not copy the string fields into the GOptionGroups internal storage of GOptionEntrys. That means the GOptionGroups internal storage could hold invalid pointers to the string fields in cases where the GOptionEntry is temporary in a GI based language:

   option_group.add_entries([GLib.OptionEntry.new_full(long_name="foo", ...)])

This would probably crash at option parsing time if the GOptionEntry boxed de-allocator is managing the memory of the string fields. Basically these APIs are for statically allocated arrays of GOptionEntrys in C AFAICT:

   static GOptionEntry options[] = {
       {"foo", ...},
       {NULL}
   }

So it seems like g_option_group_add_entries would also need to be changed to make its own copies of the string fields?

[1] https://git.gnome.org/browse/glib/tree/glib/goption.c?id=2.40.0#n2232
Comment 14 Jonas Danielsson 2014-05-24 23:34:56 UTC
Created attachment 277130 [details] [review]
gi: arg: add support for flat array arguments
Comment 15 Jonas Danielsson 2014-05-24 23:38:00 UTC
With the patch above, and the two patches against glib, I can do command line stuff from Maps, for example:

        let entries = [
            GLib.OptionEntry.new('version',
                                 'v'.charCodeAt(0), 0,
                                 GLib.OptionArg.NONE,
                                 null,
                                 _("Show the application's version"),
                                 null),
            GLib.OptionEntry.new('uri',
                                 'u'.charCodeAt(0), 0,
                                 GLib.OptionArg.STRING,
                                 null,
                                 _("Goto supplied geo URI"),
                                 null)
        ];
        this.add_main_option_entries(entries);
Comment 16 Matthias Clasen 2014-06-03 09:48:07 UTC
Since I've commented on irc, I'll repeat my opinion here: The entire GOptionEntry API smells of C convenience API to me. I'm not convinced that 1-1 translating it to bindings is a great idea.
Comment 17 Philip Chimento 2017-04-03 00:01:27 UTC
I agree with Matthias here, we shouldn't do this. There are a few candidates for including a more idiomatic argument parsing module in GJS:

- Colin's: https://git.gnome.org/browse/gnome-continuous/tree/src/js/argparse.js?id=b4872501a8b29a116503c1c3e4f2a6cc6d7a8573
- Mine: https://github.com/ptomato/jasmine-gjs/blob/master/src/options.js
- Something adapted from yargs which is the current gold standard in the Node world: https://www.npmjs.com/package/yargs
Comment 18 Philip Chimento 2017-04-03 00:01:59 UTC
Review of attachment 277130 [details] [review]:

Rejecting as per comment.
Comment 19 GNOME Infrastructure Team 2018-01-27 11:55:51 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gjs/issues/81.