GNOME Bugzilla – Bug 337510
switch from popt to GOption command line parsing
Last modified: 2006-05-13 18:09:17 UTC
see http://live.gnome.org/GnomeGoals/PoptGOption
I will work out a patch.
Created attachment 63089 [details] [review] switch from popt to goption, *not tested* switch from popt to goption. also some minor fixes. I could not test my changes since the build of gdm2 cvs fails and I was not able to quickly fix it. comments and suggestions as well as testing is most welcome.
Is this patch against CVS head? It seems to fail when I try to apply it complaining about a malformed patch. If this patch isn't against CVS head could you attach an updated version of the patch?
From i18n point of view this patch is a bit suboptimal. Source code lines like this: g_option_context_add_main_entries (ctx, xnest_only_options, "xnest chooser options"); should be replaced with g_option... (..., _("xnest chooser options")); so that usage message can be properly localized. Maybe some more thoughts should be given also how to format those messages in English, at least once "GNOME" was written as "Gnome" which is against GNOME style guide: http://developer.gnome.org/documents/style-guide/gnome-glossary-desktop.html Also POPT_ARGFLAG_ONEDASH is totally ignored in porting. This means that e.g. "gdmchooser -xdmaddress xxx" stops working (but instead it must be called with two dashes). This needs clear documentation so that users notice difference.
Created attachment 63228 [details] [review] switch from popt to goption, revision, *not tested* something weird happened and the cvs update produces rubbish which lead to the previous patch... this is the best I can come up with. cvs diff against cvs HEAD. I still cannot compile tough. i18n has been taken care of this time. also I switched to consistently use "main options" for all application options. indeed the onedash stuff has been dropped, I forgot to mention it. As far as I can tell there is no such option for GOption available.
This change cannot go into 2.14 if it breaks CLI. If you fix the code to support the ONEDASH arguments, I will put this into 2.14. Otherwise, it will need to wait until 2.16. Also, I will not accept this patch without the change from onedash to twodash arguments fully documented in the docs/gdm/gdm.xml file. Note that the command line options are documented in this file and any changes need to be documented there, at least. I'm also confused why the g_option_context_add_main_entries lines are still in the patch after Tommi commented that they should be removed.
Also, I get these compile errors trying to compile the code... "gdmXnestchooser.c", line 170: undefined symbol: args_remaining "gdmXnestchooser.c", line 170: non-constant initializer: op "U&" "gdmXnestchooser.c", line 182: non-constant initializer: op "U&" "gdmXnestchooser.c", line 469: undefined symbol: GOptioinContext "gdmXnestchooser.c", line 469: undefined symbol: ctx "gdmXnestchooser.c", line 481: warning: improper pointer/integer combination: op "=" And these errors with gdmchooser.c: "gdmchooser.c", line 1911: syntax error before or at: , "gdmchooser.c", line 1911: warning: enum type mismatch: op "=" "gdmchooser.c", line 1912: syntax error before or at: { "gdmchooser.c", line 1913: warning: syntax error: empty declaration "gdmchooser.c", line 1940: cannot recover from previous errors Line 1911 corresponds to: [ G_OPTION_REMAINING, 0, 0, G_OPTION_ARG_STRING_ARRAY, &hosts, NULL, NUL L }, "gdmXnestchooser.c", line 482: warning: improper pointer/integer combination: arg #1 "gdmXnestchooser.c", line 483: warning: improper pointer/integer combination: arg #1 "gdmXnestchooser.c", line 484: warning: improper pointer/integer combination: arg #1 "gdmXnestchooser.c", line 489: warning: improper pointer/integer combination: op "=" "gdmXnestchooser.c", line 490: warning: improper pointer/integer combination: arg #1 "gdmXnestchooser.c", line 491: warning: improper pointer/integer combination: arg #1 "gdmXnestchooser.c", line 492: warning: improper pointer/integer combination: arg #1 cc: acomp failed for gdmXnestchooser.c make[3]:
1.) Tommi did not ask to remove the line, just to make the string translatable. I will look into the ONEDASH stuff. 2.) I did a fresh cvs MAIN checkout, applied my patch and got no errors. weird. gcc version 4.0.2 20050808 (prerelease) (Ubuntu 4.0.1-4ubuntu9)
there is no file docs/gdm/gdm.xml -- did you mean docs/C/gdm.xml ? I have decided to drop onedash arguments and target the patch for 2.16 -- #gnome-love irc suggests this because the point arose what if we have the -option argument and the single character -o -p -t -i ... -s switches.... (which we do not have, but well...)
Created attachment 63799 [details] [review] switch from popt to GOption changes the -nodaemon to --nodaemon command line option fixes some compiler warnings I could compile and test it this time.
Brian, have you looked at this patch?
Yes, I mean the docs in docs/C/gdm.xml. These need to be updated, especially in the "GDM User Commands" section where the single dash arguments are documented. Also should probably mention the interface change in the "Interface Stability" section of the Overview. I won't accept this patch until the documentation is updated to reflect the change. Refer to generated HTML here: http://www.gnome.org/projects/gdm/docs/2.15/overview.html http://www.gnome.org/projects/gdm/docs/2.15/binaries.html The single-dash version of the --nodaemon argument is supported for compatibility with XDM. I notice you removed that comment from the documentation, so you are aware of this. I assume the single-dash version of arguments for gdmXnest are there also so GDM feels more compatible with other display managers. I suspect there is no good reason for gdmchooser to have single-dash arguments. I think it would be good to float this issue on the gdm-list@gnome.org mail alias and verify that people agree this is a good change before we commit this. Sound reasonable?
I'll have a look at the sections in the docs and overhaul them according to the change in the 'nodaemon' option. Feel free to consult the list.
Okay, I pinged the list. If I don't hear any screams that this is a really bad idea, I will accept your patch after you update the docs to reflect the change.
Created attachment 65319 [details] [review] switch from popt to GOption now with revised "GDM User Commands" and "Interface Stability" sections in the documentation. Note that I am probably not good at writing documentation, nor am I a native speaker.
Thanks for the patch, now committed to CVS head. Your patch didn't apply against CVS head - I had to fix it for gdmchooser.c. Also the code didn't compile on Solaris, I had to make a few minor changes, but it seems to work okay. Thanks again for working this through with me.