What's in the tar.gz:
* anaconda.process_and_launch_dogtail_script.diff The biggest patch of all. This adds a --dogtail option to the command line options for anaconda. Checks all possible conditions specifying a dogtail script: kickstart, boot cmd line, anaconda cmd line option. Downloads the script, fork and execute it before anaconda.intf.run().
* gui.py.always_load_a11y.diff Removed checking for dogtail. Always loads accessibility.
How to properly check if dogtail is required? import gui happens prior to parsing kickstart data. I would like to have os.environ["GTK_MODULES"] = "gail:atk-bridge" outside gui.py. Can anybody help me to clean this up?
* installdata.py.save_dogtail_kickstart.diff Added code that writes the dogtail option to the /root/anaconda-ks.cfg after install is complete.
* scripts.upd-instroot.dogtail_files.diff Added code to extract the missing files into stage2.img. Works fine, already sent to anaconda-devel-list.
These patches are against anaconda-11.1.2.36.
* pykickstart.data.add_dogtail.diff * pykickstart.parser.add_dogtail_command.diff Added dogtail command support for kickstart. You can now add dogtail <url> in the ks file. These 2 are posted to Red Hat Bugzilla. Until they make it to upstream they have to be added to the scripts/upd-instroot
pykickstart rpm version is 0.43-1.el5
Please review these patches.
Greeting, Alexander
On Fri, 2007-04-20 at 13:34 +0200, Alexander Todorov wrote:
What's in the tar.gz:
FYI -- in the future, it's a little easier to review if patches are attached directly rather than inside of archives where you have to open more apps to read through them. Comments are also a lot easier then.
- anaconda.process_and_launch_dogtail_script.diff
The biggest patch of all. This adds a --dogtail option to the command line options for anaconda. Checks all possible conditions specifying a dogtail script: kickstart, boot cmd line, anaconda cmd line option. Downloads the script, fork and execute it before anaconda.intf.run().
The boot command line should just lead to the option being passed on the anaconda command line from the loader. See loader2/loader.c's handling of the various args like resolution, xdriver, etc. It's also far more normal to set the value of anaconda.dogtail to None instead of "" for unset. We'll also eventually need to have the filename not be hard-coded as that opens things up to race conditions, especially as anaconda starts being run on "live" systems in cases like the live CD install.
- gui.py.always_load_a11y.diff
Removed checking for dogtail. Always loads accessibility.
What's the reasoning for always loading? Loading when needed instead is a lot nicer as it helps to reduce the memory footprint in cases where it's not needed.
- installdata.py.save_dogtail_kickstart.diff
Added code that writes the dogtail option to the /root/anaconda-ks.cfg after install is complete.
"" vs None again
- scripts.upd-instroot.dogtail_files.diff
Added code to extract the missing files into stage2.img. Works fine, already sent to anaconda-devel-list.
Why are the gconf bits needed here? At least in the past, loading the modules was good enough to get things initialized and that was all that the gconf key affected. ie, what's changed?
- pykickstart.data.add_dogtail.diff
- pykickstart.parser.add_dogtail_command.diff
Looks okay at a glance.
Please review these patches.
Overall, looking good... I do want to hold off on applying them until after we branch anaconda for F7 at this point just to avoid breaking things in the final freeze. But that shouldn't be a big deal.
Jeremy
Jeremy Katz wrote:
On Fri, 2007-04-20 at 13:34 +0200, Alexander Todorov wrote:
What's in the tar.gz:
FYI -- in the future, it's a little easier to review if patches are attached directly rather than inside of archives where you have to open more apps to read through them. Comments are also a lot easier then.
Thanks. Minor changes follow.
- anaconda.process_and_launch_dogtail_script.diff
The biggest patch of all. This adds a --dogtail option to the command line options for anaconda. Checks all possible conditions specifying a dogtail script: kickstart, boot cmd line, anaconda cmd line option. Downloads the script, fork and execute it before anaconda.intf.run().
The boot command line should just lead to the option being passed on the anaconda command line from the loader. See loader2/loader.c's handling of the various args like resolution, xdriver, etc.
So there is no need to check both? Only one of them is ok, right? Do I have to patch loader.c to parse dogtail=url and pass it to anaconda script? I suppose I will have to.
It's also far more normal to set the value of anaconda.dogtail to None instead of "" for unset.
Done.
We'll also eventually need to have the filename not be hard-coded as that opens things up to race conditions, especially as anaconda starts being run on "live" systems in cases like the live CD install.
Done, now uses tempfile.mkstemp to store the tescase in /tmp/ramfs/testcase.py.XXXXXX
- gui.py.always_load_a11y.diff
Removed checking for dogtail. Always loads accessibility.
What's the reasoning for always loading? Loading when needed instead is a lot nicer as it helps to reduce the memory footprint in cases where it's not needed.
in /usr/bin/anaconda line 863: anaconda.setInstallInterface(opts.display_mode) Imports gui.py and gtk if graphical install is selected.
line 926: instClass.setInstallData(anaconda) Parses kickstart data and sets instClass.ksdata.dogtail.
I was not sure how to modify anaconda to parse ks before loading gui without breaking something. That's why accessibility is loaded by default.
In gui.py only boot cmd line is checked. If dogtail command is added in kickstart it is not checked.
What is the prefered way to solve this issue? Should I parse kickstart file and check only for dogtail command just before `import gui' or leave it like that? Or should I make more modifications and have ks parsed only once before `import gui'?
- installdata.py.save_dogtail_kickstart.diff
Added code that writes the dogtail option to the /root/anaconda-ks.cfg after install is complete.
"" vs None again
Done
- scripts.upd-instroot.dogtail_files.diff
Added code to extract the missing files into stage2.img. Works fine, already sent to anaconda-devel-list.
Why are the gconf bits needed here? At least in the past, loading the modules was good enough to get things initialized and that was all that the gconf key affected. ie, what's changed?
In dogtail/utils.py --- import gconf gconfClient = gconf.client_get_default() a11yGConfKey = '/desktop/gnome/interface/accessibility'
def isA11yEnabled(): """ Checks if accessibility is enabled via gconf. """ return gconfClient.get_bool(a11yGConfKey) --- This requires gconf.so and gconfd-2 otherwise fails. libgconfbackend-xml.so is required to parse XML config file. .gconf directory and %gconf.xml are config files.
It may be easier to have a patch against dogtail that returns always true for isA11yEnabled() and does not use GConf
- pykickstart.data.add_dogtail.diff
- pykickstart.parser.add_dogtail_command.diff
Looks okay at a glance.
Now incorporated in scripts.upd-instroot.dogtail_files.diff. Is it better this way or leave pykickstart patches separated from anaconda patches?
Please review these patches.
Overall, looking good... I do want to hold off on applying them until after we branch anaconda for F7 at this point just to avoid breaking things in the final freeze. But that shouldn't be a big deal.
Jeremy
Thanks for review and comments.
Alexander.
On Wed, 2007-05-02 at 11:18 +0200, Alexander Todorov wrote:
Jeremy Katz wrote:
On Fri, 2007-04-20 at 13:34 +0200, Alexander Todorov wrote:
- anaconda.process_and_launch_dogtail_script.diff
The biggest patch of all. This adds a --dogtail option to the command line options for anaconda. Checks all possible conditions specifying a dogtail script: kickstart, boot cmd line, anaconda cmd line option. Downloads the script, fork and execute it before anaconda.intf.run().
The boot command line should just lead to the option being passed on the anaconda command line from the loader. See loader2/loader.c's handling of the various args like resolution, xdriver, etc.
So there is no need to check both? Only one of them is ok, right? Do I have to patch loader.c to parse dogtail=url and pass it to anaconda script? I suppose I will have to.
Correct to all of the above.
We'll also eventually need to have the filename not be hard-coded as that opens things up to race conditions, especially as anaconda starts being run on "live" systems in cases like the live CD install.
Done, now uses tempfile.mkstemp to store the tescase in /tmp/ramfs/testcase.py.XXXXXX
Sounds good. /me makes a mental note to blast /tmp/ramfs from orbit during F8 since everything in /tmp is really a ramfs now...
- gui.py.always_load_a11y.diff
Removed checking for dogtail. Always loads accessibility.
What's the reasoning for always loading? Loading when needed instead is a lot nicer as it helps to reduce the memory footprint in cases where it's not needed.
in /usr/bin/anaconda line 863: anaconda.setInstallInterface(opts.display_mode) Imports gui.py and gtk if graphical install is selected.
line 926: instClass.setInstallData(anaconda) Parses kickstart data and sets instClass.ksdata.dogtail.
I was not sure how to modify anaconda to parse ks before loading gui without breaking something. That's why accessibility is loaded by default.
In gui.py only boot cmd line is checked. If dogtail command is added in kickstart it is not checked.
What is the prefered way to solve this issue? Should I parse kickstart file and check only for dogtail command just before `import gui' or leave it like that? Or should I make more modifications and have ks parsed only once before `import gui'?
Hmmm, I guess I hadn't considered the case of just setting the dogtail info in the kickstart config. I'll need to think on if that makes sense. And also probably just see what the overhead is for loading the a11y modules. One thing that could work would be to notice the dogtail directive in the loader and then be able to pass it as an argument rather than parsing it in the second stage. That has other trade-offs, though. Chris -- any opinions?
- scripts.upd-instroot.dogtail_files.diff
Added code to extract the missing files into stage2.img. Works fine, already sent to anaconda-devel-list.
Why are the gconf bits needed here? At least in the past, loading the modules was good enough to get things initialized and that was all that the gconf key affected. ie, what's changed?
In dogtail/utils.py
[snip]
It may be easier to have a patch against dogtail that returns always true for isA11yEnabled() and does not use GConf
Ugh, yeah. dogtail really shouldn't be checking for a11y like this. It'll break under a number of circumstances.
Jeremy
anaconda-devel@lists.stg.fedoraproject.org