GNOME Bugzilla – Bug 694211
Automatically install QXL drivers for Windows 7
Last modified: 2016-03-31 13:22:07 UTC
Now that we automatically install QXL drivers for Windows XP (bug#691546), we'd want to do that same for Windows 7. This will probably only need changes in libosinfo but creating it here to track this feature.
Created attachment 237375 [details] [review] installer: More robust drivers selection This patch ensures that we: 1. Don't add/setup same driver more than once. 2. Prefer same arch over compatible ones.
(In reply to comment #1) > Created an attachment (id=237375) [details] [review] > installer: More robust drivers selection > > This patch ensures that we: > > 1. Don't add/setup same driver more than once. > 2. Prefer same arch over compatible ones. In my testing, the needed changes in libosinfo for this feature break Boxes without this fix.
Review of attachment 237375 [details] [review]: ::: src/unattended-installer.vala @@ +785,3 @@ + else if (compatibility == CPUArchCompatibity.COMPATIBLE && drivers.lookup (location) == null) + drivers.insert (location, driver); + // We don't entertain compatiblity when word-size is different because 32-bit drivers compatibility, Compatibity or compatiblity ? ;) (not new in this patch) @@ +792,3 @@ + var ret = new GLib.List<DeviceDriver> (); + foreach (var driver in drivers.get_values ()) + ret.append (driver); why not http://www.valadoc.org/#!api=glib-2.0/GLib.List.copy of drivers.get_values() ?
Review of attachment 237375 [details] [review]: ::: src/unattended-installer.vala @@ +785,3 @@ + else if (compatibility == CPUArchCompatibity.COMPATIBLE && drivers.lookup (location) == null) + drivers.insert (location, driver); + // We don't entertain compatiblity when word-size is different because 32-bit drivers Yeah, its one of those words that I just have to misspell. I'll correct. :) @@ +792,3 @@ + var ret = new GLib.List<DeviceDriver> (); + foreach (var driver in drivers.get_values ()) + ret.append (driver); Because it makes a shallow copy.
Review of attachment 237375 [details] [review]: ::: src/unattended-installer.vala @@ +785,3 @@ + else if (compatibility == CPUArchCompatibity.COMPATIBLE && drivers.lookup (location) == null) + drivers.insert (location, driver); + // We don't entertain compatiblity when word-size is different because 32-bit drivers Fixing the spelling in the comment is fine to do in this patch as you are moving it around @@ +792,3 @@ + var ret = new GLib.List<DeviceDriver> (); + foreach (var driver in drivers.get_values ()) + ret.append (driver); Can we have a g_list_deep_copy helper in one of our util classes to do that? And by any chance, can this be implemented through List.copy + List.foreach?
Review of attachment 237375 [details] [review]: ::: src/unattended-installer.vala @@ +785,3 @@ + else if (compatibility == CPUArchCompatibity.COMPATIBLE && drivers.lookup (location) == null) + drivers.insert (location, driver); + // We don't entertain compatiblity when word-size is different because 32-bit drivers Yeah, I got that hidden message from your previous comment. :) @@ +792,3 @@ + var ret = new GLib.List<DeviceDriver> (); + foreach (var driver in drivers.get_values ()) + ret.append (driver); Yes to first question. You mean we copy the list and then ref its each member? That would look awkward in vala, if possible. AFAIK you either create a List<week Type> or List<Type>. I generalized version of the above code would suffice I think.
Review of attachment 237375 [details] [review]: ::: src/unattended-installer.vala @@ +792,3 @@ + var ret = new GLib.List<DeviceDriver> (); + foreach (var driver in drivers.get_values ()) + ret.append (driver); Yikes, this opens a can of worms: https://bugzilla.gnome.org/show_bug.cgi?id=694765 . I think we really should be making use of libgee as thats always much better supported in Vala than the low-level glib collection types. Libgee has been a blessed dep of gnome for some years now, is used by many gnome and non-gnome modules. Here is what yum tries to remove on my system for deps when I try to remove libgee. bluez caribou caribou-antler caribou-gtk2-module caribou-gtk3-module cheese cheese-libs control-center empathy folks gnome-bluetooth gnome-contacts gnome-dvb-daemon gnome-shell gnome-shell-extension-common gnome-shell-extension-user-theme gnome-tweak-tool orca pino pulseaudio-module-bluetooth python-caribou rygel shotwell tracker-ui-tools
Created attachment 237473 [details] [review] installer: More robust drivers selection v2: Correct spelling mistake in comment.
Review of attachment 237375 [details] [review]: ::: src/unattended-installer.vala @@ +792,3 @@ + var ret = new GLib.List<DeviceDriver> (); + foreach (var driver in drivers.get_values ()) + ret.append (driver); Let's keep your initial patch then with a more explicit comment that we need a deep copy. Or would it be bad to have get_drivers return a weak list?
Review of attachment 237375 [details] [review]: ::: src/unattended-installer.vala @@ +792,3 @@ + var ret = new GLib.List<DeviceDriver> (); + foreach (var driver in drivers.get_values ()) + ret.append (driver); The hashtable is getting destroyed at the end of the function so, yes we need to copy.
Review of attachment 237375 [details] [review]: ::: src/unattended-installer.vala @@ +792,3 @@ + var ret = new GLib.List<DeviceDriver> (); + foreach (var driver in drivers.get_values ()) + ret.append (driver); Yes, we need to make at least a shallow copy, what I'm asking is if a shallow copy would be enough as the Drivers objects seem to be coming from the osinfo DB database. I'm fine either way fwiw, shallow or deep copy.
Review of attachment 237375 [details] [review]: ::: src/unattended-installer.vala @@ +792,3 @@ + var ret = new GLib.List<DeviceDriver> (); + foreach (var driver in drivers.get_values ()) + ret.append (driver); Actually it might be enough. I haven't tried that but I don't really see any real benefits from that. A 'weak' modifier (or any modifier) usually makes the readers wonder about reasons for use of it so it'll only be a distraction IMHO. So I guess we don't need to update this patch then?
Review of attachment 237375 [details] [review]: ::: src/unattended-installer.vala @@ +792,3 @@ + var ret = new GLib.List<DeviceDriver> (); + foreach (var driver in drivers.get_values ()) + ret.append (driver); Sounds good to me with an explicit comment that we want a deep copy, not just g_list_copy(). g_list_prepend + g_list_reverse would be a better way to do the copy as usual.
Comment on attachment 237473 [details] [review] installer: More robust drivers selection Attachment 237473 [details] pushed as 87b0d6b - installer: More robust drivers selection
*** Bug 692295 has been marked as a duplicate of this bug. ***
With relavent patches landing into libosinfo's git master combined with the fix in this bug, this is now fixed. I'll look into vista too and hopefully the work that went into enabling this for win7 can be re-used as is for vista too.