GNOME Bugzilla – Bug 136953
Can we have support for popt?
Last modified: 2004-12-22 21:47:04 UTC
Hi guys, In order to be able to write applications that can be gnome-session compliant, it would be nice to have support for popt... Thanks!
I hear you! I've been wanting this for a long time too. I've been thinking, perhaps we can use/integrate with pypopt: http://www.agrajag.net/projects/pypopt/ Unfortunately, I don't have enough time at the moment to effectively tackle this problem.
Yes, understand - well, here is my proposal: I could have a look at pypopt and see if I can start the integration, and then send patches to the responsible pygtk person, how's that sound? But I do have to be honest as well: 1. I may end up not having enough time to do it (but I am willing to try it) 2. I am stuck with Redhat 8.0 and cannot upgrade, so my patches would be based on gnome-python2-1.99.11-8; So, if no one else has the bandwidth to do this, and if you think I could help, then I'll try. If this is ok, there is also the possibility of using python-popt (was active until redhat-7.3 I think). What do you think? Cheers Rubens
Well, if you proceed, just attach patches to this bug report, and we'll take a look. Patches against gnome-python2-1.99.11-8 should be ok. Regarding python-popt, I can't find it anywhere (homepage, source tarball) on google or freshmeat, only RPMs. Therefore, I'm a bit reluctant to use this code. Finally, I would acknowledge that we have two approaches for this: 1. Add a pypopt/python-popt dependency to gnome-python; 2. Copy and distribute the pypopt/python-popt code inside gnome-python. I'm still not sure which is the best solution. Can anyone else comment?
I know that pypopt is used heavily by Red Hat, so maybe you have to look in their packages, probably anaconda. And since it's not widely distributed yet we might just be able to included it in the gnome-python distribution since if I remember correctly it's not a huge module. It also makes sense if it's not maintained or easily accessible Anyway, I like the idea to have good popt integration in gnome-python.
Fair enough - I'll have a look at pypopt and anaconda, and will try something along the lines of Gustavo's option (2). I'll pop in #pygtk in case I need some directions :)
Heya Rubens, Just a remark on your reservation #2: you should always code patches against gnome-python CVS HEAD. You can install it into a local (~rubens/local) directory and test it using a specified PYTHONPATH without touching your local installation. Looking forward to working with you again!
Ok - so after getting lots of help from jdahlin and kiko (thanks guys :) I am posting an initial patch here, which adds support for popt (standalone), popt (with gnome) and (hopefully) GnomeModuleInfo. I added some notes to the patch, so these may clarify/explain the decisions I had to take. This is a patch against gnome-python-2.0.1-RC2.
Created attachment 26018 [details] [review] Patch to add popt support Now the patch :)
Created attachment 26019 [details] [review] Patch to add popt support (text only now)
Ok, I tested it and it seems to work. Some comments: 1. You ask this in the readme file; I really think gnome-python's popt should be placed in the gnome namespace, i.e. gnome.popt. 2. This doesn't feel very elegant to me: if (rc > 95) and (rc < 100): arg = ctx.getOptArg() print "rc: %s; valuetype: %s; %s" % (rc, type(arg), arg) It forces you to use a number range to identify the options. I wish you had chosen to integrate pypopt instead. With pyopt you can do this instead: (...) ctx = pypopt.getcontext("testpopt", sys.argv, options, 1, 0) options.no_error_detection = ctx.argdict['no-error-detection'] options.disable_delays = ctx.argdict['disable-delays'] options.interactive = ctx.argdict['interactive'] options.gui = ctx.argdict['enable-gui'] options.sloppy = ctx.argdict['sloppy'] options.max_rate_packets = ctx.argdict['max-rate-packets'] (...) 3. ctx = p._get_popt_context() Names starting with underscore are supposed to be private, but _get_popt_context is clearly public. We should remove the leading underscore, if you don't mind. 4. Another pyopt advantage is that it automatically parses the argument string values to int or float as appropriate, looking at the type indicated in the options table. This code always presents argument values as strings. That's all I can think of. Other than this, nice work! ;-)
> 3. ctx = p._get_popt_context() > Names starting with underscore are supposed to be private, but > _get_popt_context is clearly public. We should remove the leading underscore, > if you don't mind. Well, I suggested he used an underscore for a reason: avoiding potential clashes with future GnomeProgram API (given that it's an API contrived in gnome-python that doesn't match a GnomeProgram call). Do you have a suggested alternative? get_popt_context_() ? Should we investigate the likelihood of a crash? > I wish you had chosen to integrate pypopt instead. Hmmm -- is that not an option, Rubens, Gustavo? http://www.agrajag.net/projects/pypopt/
Well, just ask yourself this: How likely is it that a function called 'gnome_program_get_popt_context' would be added that _doesn't_ do exactly the same thing as our own '_get_popt_context'? In case the answer is 'not likely at all', then we have no problem, because if gnome_program_get_popt_context is added to the C library, it does exactly the same thing as our own, therefore shadowing the C library function doesn't matter.
Hmmm, I don't have such high expectations for sanity in the GnomeProgram API, but I suspect we should just trust you on this and get on with life.
About gjc comments: 1. Ok - not a problem, I will do this. 2. Agreed - shouldnt be hard to integrate pypopt - just that the code itself seemed to be more convoluted to me, but thats ok. I will do this. 3. This one is easy ;-) 4. There is a method in python-popt to do this as well, if using pypopt doesnt work, I will reuse that. Will post the patch when I am finished.
Ok - couldnt find you in #pygtk, so here are my new comments: 1. Did this, and just realised that doing "import gnome.popt" may be a bit weird if you just want to use popt without the gnome stuff (which I guess is unlikely, but anyway). 3. Did this. 2 and 4: Now I remember why I decided to not use the "getValue" method from python-popt, and why using the funky features of pypopt could cause problems: Both python-popt (I had to change this when integrating) and pypopt create a poptContext object which hold some additional data to just the context id in order to provide the features you like. The problem is, these niceties are initialised when you call popt_get_context(), which is NOT called by the application when you are using gnome! All you do is grab the poptContext id once gnome is finished with the parameters, and then reuse it. But then, all of the internal PypoptContext object members are not initialised. Soooooo - if you really want to have all of these nice features, all I can think of is that the user would not be able to call gnome.program.get_popt_context() (I would remove it), and he/she would have to do the (kind-of) nonsense call to popt.get_context() with command line parameters and the popt table all over again once gnome.init() is finished. If you are happy with this, then we can have all of the features from these libraries, no problemo - unless you can think of a different solution of course :) Cheers
Created attachment 26143 [details] [review] Patch now using pypopt. Here are the changes.
Usability? What does this have to do with usability? :-)
Just avoid conflicts/race conditions, I'll be reviewing/debugging this code this afternoon...
Instead of pypopt.error_noarg, pypopt.error_badopt, etc., we should have pypopt.NoArgError, pypopt.BadOptError, etc. In addition, module constants (popt.arg_int, etc.) should be in ALL_CAPS style. Does everyone agree?
Yes, and the constants should just point to the builtins str, int, double and None.
The problem is, I'm not sure how to handle POP_ARG_VAL in that case. From popt(3): "POPT_ARG_VAL causes arg to be set to the (integer) value of val when the argument is found. This is most often useful for mutually‐exclusive arguments in cases where it is not an error for multiple arguments to occur and where you want the last argument specified to win; for example, "rm ‐i ‐f". POPT_ARG_VAL causes the parsing function not to return a value, since the value of val has already been used."
Okay, let's do it the other way around then, so str, int, double and None maps to POPT_*
I'm worried about two issues: 1. the first string option bug already mentioned, but still can't find the bug; 2. passing standard gnome options makes parsing application options fail; example: python gnome_popt.py --enable-sound --aaa asd
Found this comment in libgnome: /* Make a copy of argv, the thing is that while we know the * original argv will live until the end of the program, * there are those evil people out there that modify it. * Also, this may be some other argv, think 'fake argv' here */ This means that libgnome doesn't modify argv to remove the arguments that it processes. Which probably means we have a dead end. :| I'm beginning to think this is an impossible task, or at least so "hackish" that is not worth the trouble...
What about trying to use the callback functionality from popt? We could have a dummy callback implemented in poptmodule.c, that would call a python callback in the python popt options table... In this way, the poptOptions table would only allow callbacks... Does that sound feasible (or too hackish)?
Hm.. I did not know of this callback functionality. It is probably a good idea!...
OK, looking at the man page, the callback functionality is exactly what we need! See this: "This is especially usefull when included option tables are being used, as the program which provides the top‐level option table doesn’t need to be aware of the other options which are provided by the included table." I'm also thinking we could use a different approach. The general idea is that we don't need a full-blown popt module, we just need "popt-light", we minimal functionality. Let me explain what I'd like to have with an example: ----BEGIN EXAMPLE---- # (longName, shortName, type , default, flags, descrip , argDescrip) table=[("foo" , 'f' , int , 1 , 0 , "Foobar", "Level"), ("aaa" , 'a' , str , 'a' , 0 , 'aaa' , "AAA"), ("bbb" , 'b' , None , None , 0 , 'bbb' , "BBB"), ("ccc" , 'c' , float, 2.5 , 0 , 'ccc' , "BBB")] prog = gnome.init ("myprog", "1.0", gnome.libgnome_module_info_get(), sys.argv, table) leftover_args, argdict = prog.get_popt_args() ----END EXAMPLE---- Now, this is what I'd call elegant. Moreover, it would be a lot more maintainable, because I'm thinking we don't need to integrate the pypopt/popt-python module at all. We only need to be able to create a popt option table to pass into gnome_program_init. The POPT_ARG_XXX values are not needed, we can just use python types (int, str, float, bool, etc.). I think we should sacrifice POP_ARG_VAL. We do need the POPT_ARGFLAG_XXX constants; these can be defined directly in the gnome module, preserving the POPT_ prefix. Some hints on how to perform all this: 1- The option dictionary (argdict) is created first; 2- The table is constructed, all elements are of type POPT_ARG_CALLBACK, callback is our C static func callback, descrip is the callback data, which points to a structure containting (optiontype, argdict); While the table is constructed, the argdict is initialized to the default values; 3- We call gnome_program_init with the table. Then callback is called for each option: a) Callback parses option value, and stores result in argdict; 4- Before returning from the gnome_progam_init wrapper, we store a reference to the argdict in the GnomeProgram object, with g_object_set_data; 5. New method get_popt_args() returns (argdic, leftoverargs) this way: a) argdic is obtained with g_object_get_data from the GnomeProgram; b) we can obtain the popt context from GnomeProgram, and from the poptContext we obtain the leftoverargs Rubens, are you still up for this? I could do it too, but it will happen much faster if you do it instead of me. :)
I like your example - let's try that, it is (theoretically) cleaner and simpler than having gnome.popt. (I was playing with popt in C - the callback has to be defined once in the table, everything after that row is handled via the callback) Just some clarifications: On point 1: so do you want to have keys in the dict for all options, even for the ones that are not passed in the command line and have no defaults? On point 2: already commented on this :) On point 3: excellent! On points 4 and 5: The argdict could still be stored in a "poptContext" python object, what do you think? This could remove the need for the g_object_get_data()? Because of 5.b we would still need a "99% fat free gnome.popt" any way, isnt it? (or shall we call it the mini-me.popt) Anyway, I dont mind doing this. Can take a few days, if you're ok with this.
Created attachment 26465 [details] [review] Yet another popt patch, for 2.0.2. So here is the new patch for popt - a couple of remarks: * please someone check that the Python reference decs/incs are correct, as I am not that experienced with that; * There is a known leak there - it is marked in the code, argv cannot be deleted after gnome_program_init() is called, as it is used by get_popt_args().
Rubens, indentation in that patch looks funky; are you mixing tabs and spaces? It appears the relevant file uses 4-space indents in the area you're editing (and yes, mixed tabs and spaces are horrific).
Created attachment 26492 [details] [review] And now - a bit tab/space problem cleaner and with sigaction One interesting note - I managed to get hold of a Fedora Core 1 distro to test this, and 2.0.2 did not build after './configure', 'make'. It stopped in the bonobo directory asking me to run 'aclocal; autoconf; automake'...
Created attachment 26535 [details] [review] Final patch that went into CVS Committed to CVS HEAD, with some code simplification, and memory leak fixes. I am a bit afraid to commit to the 2.0 branch, because that is a stable branch, and only bug fixes should be committed. Perhaps someday we'll backport, after extensive testing in HEAD.
Rubens, give it some testing and if you're happy, resolve as FIXED. What's your next bug? :-) Thanks to everybody that chipped in to make this happen.
I found a bug, most likely introduced by my code simplifications :-P It segfaults when no popt table is given. The following command shows the fix: cvs diff -r 1.9 -r 1.10 gnome/gnome.override
Looks fixed to me. File new bugs for regressions and other missing bits.
Actually, let me reopen with a reminder that I need to implement standalone popt parsing, not integrated with (independent of) gnome.init(). I have this strange use case involving the new bonobo.Application API, where sometimes a program is required to parse popt options multiple times, but you can only gnome.init() once. This should be fairly trivial to implement. All code is already there; I just need to reuse the existing functions.
How about opening a new bug for that? Better than leaving bugs with code checked in dangling..
Added to CVS such function: leftover_args, argdict = gnome.popt_parse(argv, popt_table, flags=0) Closing. I hope everyone agrees with the name, otherwise I can change it.
Unless there's risk that it collides with a "real" popt_parse method coming from C GNOME popt itself, I think that's fine.
The email in pygtk list about parsing repeated options makes me think we can easily support this. Also, looking at the code, I discovered a bug. switch (opt->argInfo & POPT_ARG_MASK) { case POPT_ARG_INT: item = PyInt_FromLong(*(int *)(opt->arg)); break; (...) If the same option is given more than once, we leak a python object. We could fix this bug and support multiple options at the same time if we automatically append multiple option values to a list, and return the list instead of a single value. We could invent a new flag that indicates whether we accept multiple values for an option, and if so we always return a list even when there is only one value. This simplifies the application programming downstream, I think..
Oops, sorry there is no leak after all :-P
Committed to CVS multiple value per option support. I ended up doing a bit differently. I couldn't find any "safe" flag value: there's always some risk that popt might decide to use such value in the future. For details on the code, see: http://cvs.gnome.org/viewcvs/gnome-python/gnome-python/gnome/gnome.override?r1=1.13&r2=1.14 http://cvs.gnome.org/viewcvs/gnome-python/gnome-python/examples/popt/popt.py?r1=1.2&r2=1.3