GNOME Bugzilla – Bug 676537
Move boxes to use new libosinfo API's for automated installations
Last modified: 2016-03-31 14:03:10 UTC
Was developed, as part of libosinfo, a new API for automated installations, introduced by: https://www.redhat.com/archives/virt-tools-list/2012-February/msg00236.html Some work is being done to fix/improve osinfo-install-scripts and can be checked here: https://www.redhat.com/archives/virt-tools-list/2012-May/msg00017.html and here: https://www.redhat.com/archives/virt-tools-list/2012-May/msg00080.html Considering that above patches will be applied (yes, it is a dependency), I'm glad to introduce a patch movin' boxes to use new libosinfo API's.
Created attachment 215040 [details] [review] The forgotten patch, porting boxes to use new libosinfo API Now, this patch is not applying on upstream code. I'm only waiting for some changes in unattended files to rebase and resend it. But, for now, a feedback if my approach is correct would be awesome.
Review of attachment 215040 [details] [review]: ::: src/fedora-installer.vala @@ -25,1 +23,1 @@ kbd_regex = new Regex ("BOXES_FEDORA_KBD"); what about the keyboard config? I recall Daniel had some API for that too, no? @@ -32,2 +30,2 @@ public FedoraInstaller.copy (InstallerMedia media) throws GLib.Error { - var source_path = get_unattended_dir ("fedora.ks"); + var config = new Osinfo.InstallConfig ("http://libosinfo.fedorahosted.org/config"); Never understood why libosinfo uses invalid HTTP URLs for IDs. Just a general comment, I know its not your fault. :) @@ -32,2 +30,3 @@ public FedoraInstaller.copy (InstallerMedia media) throws GLib.Error { - var source_path = get_unattended_dir ("fedora.ks"); + var config = new Osinfo.InstallConfig ("http://libosinfo.fedorahosted.org/config"); + config.set_user_login("BOXES_USERNAME"); coding-style nitpick: "(" -> " (". This needs fixing in many places.. @@ -99,3 @@ - str = kbd_regex.replace (str, str.length, 0, kbd); - - var repos = (use_remote_repos) ? "repo --name=fedora\nrepo --name=updates" : ""; Does libosinfo takes care of this? ::: src/unattended-installer.vala @@ -73,2 +73,2 @@ public UnattendedInstaller.copy (InstallerMedia media, - string unattended_src_path, + Osinfo.InstallConfig config, Coding style nitpick: arguments needs to be aligned. @@ -247,0 +248,5 @@ + protected virtual string install_script (Osinfo.InstallConfig config, + string profile, + string short_id) throws GLib.Error { ... 2 more ... I'm a bit confused on how things work here. My idea was that with port to libosinfo API we'll get rid of two things: 1. OS-specific subclasses of UnattendedInstaller. 2. All the code in UnattendedInstaller that does the replacing of variables in the script files. But if I understood your changes correctly, after your patches we first put variables into the script files ourselves and then do the replacement of those variables? If so, that kinda beats the point of using libosinfo for this. @@ -247,0 +248,7 @@ + protected virtual string install_script (Osinfo.InstallConfig config, + string profile, + string short_id) throws GLib.Error { ... 4 more ... coding-style nitpick: use of 'var': var cancellable = .. @@ -247,0 +248,8 @@ + protected virtual string install_script (Osinfo.InstallConfig config, + string profile, + string short_id) throws GLib.Error { ... 5 more ... Loading of Osinfo DB is not a very cheap operation and we already have it loaded in OSDatabase object so it'd be very much desirable to re-use that. @@ -247,0 +248,19 @@ + protected virtual string install_script (Osinfo.InstallConfig config, + string profile, + string short_id) throws GLib.Error { ... 16 more ... Even more coding-style nitpicks: os = filtered_list.get_nth (0) as Osinfo.Os; ::: src/win7-installer.vala @@ +38,3 @@ + config.set_user_login("BOXES_USERNAME") ; + + var profile = "jeos"; Don't think we want 'jeos'. We want a full-blown desktop for all OSs. Same for winxp-installer.vala.
(In reply to comment #2) > ::: src/win7-installer.vala > @@ +38,3 @@ > + config.set_user_login("BOXES_USERNAME") ; > + > + var profile = "jeos"; > > Don't think we want 'jeos'. We want a full-blown desktop for all OSs. Same for > winxp-installer.vala. What is jeos?
(In reply to comment #3) > (In reply to comment #2) > > ::: src/win7-installer.vala > > @@ +38,3 @@ > > + config.set_user_login("BOXES_USERNAME") ; > > + > > + var profile = "jeos"; > > > > Don't think we want 'jeos'. We want a full-blown desktop for all OSs. Same for > > winxp-installer.vala. > > > What is jeos? Just Enough Operating System: http://en.wikipedia.org/wiki/Just_enough_operating_system
(In reply to comment #4) > (In reply to comment #3) > > (In reply to comment #2) > > > ::: src/win7-installer.vala > > > @@ +38,3 @@ > > > + config.set_user_login("BOXES_USERNAME") ; > > > + > > > + var profile = "jeos"; > > > > > > Don't think we want 'jeos'. We want a full-blown desktop for all OSs. Same for > > > winxp-installer.vala. > > > > > > What is jeos? > > Just Enough Operating System: > http://en.wikipedia.org/wiki/Just_enough_operating_system What does it have to do with Windows Xp/7 installer?
(In reply to comment #2) > Review of attachment 215040 [details] [review]: > > ::: src/fedora-installer.vala > @@ -25,1 +23,1 @@ > kbd_regex = new Regex ("BOXES_FEDORA_KBD"); > > what about the keyboard config? I recall Daniel had some API for that too, no? Probably I missed something, so. > > @@ -32,2 +30,2 @@ > public FedoraInstaller.copy (InstallerMedia media) throws GLib.Error { > - var source_path = get_unattended_dir ("fedora.ks"); > + var config = new Osinfo.InstallConfig > ("http://libosinfo.fedorahosted.org/config"); > > Never understood why libosinfo uses invalid HTTP URLs for IDs. Just a general > comment, I know its not your fault. :) > > @@ -32,2 +30,3 @@ > public FedoraInstaller.copy (InstallerMedia media) throws GLib.Error { > - var source_path = get_unattended_dir ("fedora.ks"); > + var config = new Osinfo.InstallConfig > ("http://libosinfo.fedorahosted.org/config"); > + config.set_user_login("BOXES_USERNAME"); > > coding-style nitpick: "(" -> " (". This needs fixing in many places.. > > @@ -99,3 @@ > - str = kbd_regex.replace (str, str.length, 0, kbd); > - > - var repos = (use_remote_repos) ? "repo --name=fedora\nrepo > --name=updates" : ""; > > Does libosinfo takes care of this? Yes, at least I sent a patch taking care of this. > > ::: src/unattended-installer.vala > @@ -73,2 +73,2 @@ > public UnattendedInstaller.copy (InstallerMedia media, > - string unattended_src_path, > + Osinfo.InstallConfig config, > > Coding style nitpick: arguments needs to be aligned. > > @@ -247,0 +248,5 @@ > + protected virtual string install_script (Osinfo.InstallConfig config, > + string profile, > + string short_id) > throws GLib.Error { > ... 2 more ... > > I'm a bit confused on how things work here. My idea was that with port to > libosinfo API we'll get rid of two things: > > 1. OS-specific subclasses of UnattendedInstaller. > 2. All the code in UnattendedInstaller that does the replacing of variables in > the script files. > > But if I understood your changes correctly, after your patches we first put > variables into the script files ourselves and then do the replacement of those > variables? If so, that kinda beats the point of using libosinfo for this. I got it! :-) > > @@ -247,0 +248,7 @@ > + protected virtual string install_script (Osinfo.InstallConfig config, > + string profile, > + string short_id) > throws GLib.Error { > ... 4 more ... > > coding-style nitpick: use of 'var': var cancellable = .. > > @@ -247,0 +248,8 @@ > + protected virtual string install_script (Osinfo.InstallConfig config, > + string profile, > + string short_id) > throws GLib.Error { > ... 5 more ... > > Loading of Osinfo DB is not a very cheap operation and we already have it > loaded in OSDatabase object so it'd be very much desirable to re-use that. I didn't notice that we already have it loaded. My fault, sorry. > > @@ -247,0 +248,19 @@ > + protected virtual string install_script (Osinfo.InstallConfig config, > + string profile, > + string short_id) > throws GLib.Error { > ... 16 more ... > > Even more coding-style nitpicks: > > os = filtered_list.get_nth (0) as Osinfo.Os; > > ::: src/win7-installer.vala > @@ +38,3 @@ > + config.set_user_login("BOXES_USERNAME") ; > + > + var profile = "jeos"; > > Don't think we want 'jeos'. We want a full-blown desktop for all OSs. Same for > winxp-installer.vala. jeos, at least for Windows, has been generating a script *identical* than we have in gnome-boxes (but probably it will be changed with user avatar stuffs). For Fedora, I created a script using Desktop profile and I'm using it.
(In reply to comment #5) > (In reply to comment #4) > > (In reply to comment #3) > > > (In reply to comment #2) > > > > ::: src/win7-installer.vala > > > > @@ +38,3 @@ > > > > + config.set_user_login("BOXES_USERNAME") ; > > > > + > > > > + var profile = "jeos"; > > > > > > > > Don't think we want 'jeos'. We want a full-blown desktop for all OSs. Same for > > > > winxp-installer.vala. > > > > > > > > > What is jeos? > > > > Just Enough Operating System: > > http://en.wikipedia.org/wiki/Just_enough_operating_system > > What does it have to do with Windows Xp/7 installer? I had the same question, Daniel?
There are many different possible configurations for installing operating systems, and a kickstart file has to choose which to implement. For flexibility, libosinfo will allow multiple different kickstart files to be provided for each operating system, each tagged with a different "profile". To give a little consistency, the intent is to provide 2 standard profiles for each OS, a "jeos" profile which installs the bare minimum packet set, and a "desktop" profile which intells a typical graphical user desktop environment. For GNOME Boxes, you'd obviously want to use the kickstart files tagged with a "desktop" profile, rather than a "jeos" profile
(In reply to comment #8) > There are many different possible configurations for installing operating > systems, and a kickstart file has to choose which to implement. For > flexibility, libosinfo will allow multiple different kickstart files to be > provided for each operating system, each tagged with a different "profile". To > give a little consistency, the intent is to provide 2 standard profiles for > each OS, a "jeos" profile which installs the bare minimum packet set, and a > "desktop" profile which intells a typical graphical user desktop environment. > > For GNOME Boxes, you'd obviously want to use the kickstart files tagged with a > "desktop" profile, rather than a "jeos" profile Yeap. I considered it when I did the patch, but my point is: JEOS profile is generating, basically, the same script used by boxes *now*. With avatar stuffs, probably Desktop profile should be different than JEOS profile and, so, I'll add and use it.
(In reply to comment #9) > (In reply to comment #8) > > There are many different possible configurations for installing operating > > systems, and a kickstart file has to choose which to implement. For > > flexibility, libosinfo will allow multiple different kickstart files to be > > provided for each operating system, each tagged with a different "profile". To > > give a little consistency, the intent is to provide 2 standard profiles for > > each OS, a "jeos" profile which installs the bare minimum packet set, and a > > "desktop" profile which intells a typical graphical user desktop environment. > > > > For GNOME Boxes, you'd obviously want to use the kickstart files tagged with a > > "desktop" profile, rather than a "jeos" profile > > Yeap. I considered it when I did the patch, but my point is: JEOS profile is > generating, basically, the same script used by boxes *now*. Hence our question about what JEOS would mean for Windows? On Linux, you won't have X server and any DE mainly but in case of windows, you always have a DE. > With avatar stuffs, probably Desktop profile should be different than JEOS > profile and, so, I'll add and use it. If we are creating user account in JEOS case, I don't why we wouldn't set user avatar too?
(In reply to comment #10) > (In reply to comment #9) > > (In reply to comment #8) > > > There are many different possible configurations for installing operating > > > systems, and a kickstart file has to choose which to implement. For > > > flexibility, libosinfo will allow multiple different kickstart files to be > > > provided for each operating system, each tagged with a different "profile". To > > > give a little consistency, the intent is to provide 2 standard profiles for > > > each OS, a "jeos" profile which installs the bare minimum packet set, and a > > > "desktop" profile which intells a typical graphical user desktop environment. > > > > > > For GNOME Boxes, you'd obviously want to use the kickstart files tagged with a > > > "desktop" profile, rather than a "jeos" profile > > > > Yeap. I considered it when I did the patch, but my point is: JEOS profile is > > generating, basically, the same script used by boxes *now*. > > Hence our question about what JEOS would mean for Windows? On Linux, you won't > have X server and any DE mainly but in case of windows, you always have a DE. Maybe JEOS doesn't make sense for Windows and should rename it to "Desktop"? > > > With avatar stuffs, probably Desktop profile should be different than JEOS > > profile and, so, I'll add and use it. > > If we are creating user account in JEOS case, I don't why we wouldn't set user > avatar too? Agreed.
(In reply to comment #11) > (In reply to comment #10) > > (In reply to comment #9) > > > (In reply to comment #8) > > > > There are many different possible configurations for installing operating > > > > systems, and a kickstart file has to choose which to implement. For > > > > flexibility, libosinfo will allow multiple different kickstart files to be > > > > provided for each operating system, each tagged with a different "profile". To > > > > give a little consistency, the intent is to provide 2 standard profiles for > > > > each OS, a "jeos" profile which installs the bare minimum packet set, and a > > > > "desktop" profile which intells a typical graphical user desktop environment. > > > > > > > > For GNOME Boxes, you'd obviously want to use the kickstart files tagged with a > > > > "desktop" profile, rather than a "jeos" profile > > > > > > Yeap. I considered it when I did the patch, but my point is: JEOS profile is > > > generating, basically, the same script used by boxes *now*. > > > > Hence our question about what JEOS would mean for Windows? On Linux, you won't > > have X server and any DE mainly but in case of windows, you always have a DE. > > Maybe JEOS doesn't make sense for Windows and should rename it to "Desktop"? The JEOS install script files should identical to those currently provided by the Oz tool: https://github.com/clalancette/oz/tree/master/oz/auto While the Desktop install script files should be identical to those provided by Boxes
Created attachment 221989 [details] [review] A new version of the patch that do the boxes port to use new libosinfo API to install scripts This patch has as dependency the related patches in libosinfo side.
Created attachment 222000 [details] [review] And I forgot to amend a two small changes :(
Created attachment 222080 [details] [review] Rebased
Review of attachment 222080 [details] [review]: Looking very promising otherwise. ::: src/unattended-installer.vala @@ +185,3 @@ } + public virtual void set_config_properties (Osinfo.InstallScript script) { why virtual? @@ -265,2 +297,4 @@ if (child != express_toggle) express_toggle.bind_property ("active", child, "sensitive", BindingFlags.SYNC_CREATE); + + var script = scripts.get_nth (0) as Osinfo.InstallScript; why assume its the first script? @@ -267,0 +299,11 @@ + + var script = scripts.get_nth (0) as Osinfo.InstallScript; + if (script.has_config_param_name (Osinfo.INSTALL_CONFIG_PROP_REG_PRODUCTKEY) == false) ... 8 more ... We don't know if its Windows prodcut key after your patch @@ -275,3 +344,3 @@ } - protected virtual async void prepare_direct_boot (Cancellable? cancellable) throws GLib.Error {} + protected virtual async void prepare_direct_boot (Cancellable? cancellable) throws GLib.Error { this doesn't have to be pretected or virtual anymore @@ -279,1 +360,1 @@ protected virtual DomainDisk? get_unattended_disk_config () { same here: non-virtual private method it is now. @@ -291,2 +372,4 @@ disk.set_target_dev ("sdb"); + if (os_media.kernel_path == null || os_media.initrd_path == null) + { coding-style nitpick: '{' on the same line as 'if' @@ -312,2 +399,4 @@ } + // This is not so much generic, we need to find a better way to format language string for newer windows + private string get_language_names_by_os (Osinfo.Os os) { * the name suggests that method returns multiple names while it returns just one. * You can remove the '_by_os' prefix and the 'os' argument as that info is available in the instance field/property and its internal detail of this method. * IIRC Daniel objected on having language names mapped differently for different OS. it was the X keyboard mapping from console kbd mapping he didn't want in libosinfo. Let me know if i recall incorrectly, otherwise please move this to libosinfo. @@ +419,3 @@ + } + + // This code, relative to keyboard_layout, is put temporarily here, until we start to work on yet-not-created libvirt-installer * Exceeds 120 columns * Way too many commas. :) Better (IMHO) message: This X to console keyboard layout mapping related code will hopefully be soon moved to yet-to-be-created libvirt-installer @@ -314,0 +401,148 @@ + // This is not so much generic, we need to find a better way to format language string for newer windows + private string get_language_names_by_os (Osinfo.Os os) { + var languages = Intl.get_language_names (); ... 145 more ... No need to pass 'os' here either @@ +549,3 @@ + var keyboard = ""; + + switch (os.distro) { * since there is only 2 cases here, this will be much shorter but still clean equivalent: return (os.distro == "fedora") fetch_console_kbd_layout () : ""; * Also, do we really need to skip setting of kbd layout all together for other OSs? I would think we only need to do the mapping for fedora but we should still set the layout for other OSs. I think its currently unuset for others? @@ +743,1 @@ data_stream.newline_type = DataStreamNewlineType.ANY; We shouldn't need to do all this copying line-by-line anymore. This method should simply call script.generate_output() to create the file and copy method should then just copy it just like before.
Review of attachment 222080 [details] [review]: Correct status..
Review of attachment 222080 [details] [review]: Some more comments/questions. ::: src/unattended-installer.vala @@ +109,3 @@ + var script = list.data as Osinfo.InstallScript; + + newline_type = (strcmp (script.get_newline_type (os), Osinfo.INSTALL_SCRIPT_NEWLINE_TYPE_CR_LF) == 0) ? DataStreamNewlineType.CR_LF : DataStreamNewlineType.LF; Oh and when we remove the line-by-line copying, we wont need 'newline_type' either. @@ -179,4 +187,4 @@ - public virtual string fill_unattended_data (string data) throws RegexError { - var str = username_regex.replace (data, data.length, 0, username_entry.text); - str = password_regex.replace (str, str.length, 0, password); - str = timezone_regex.replace (str, str.length, 0, timezone); + public virtual void set_config_properties (Osinfo.InstallScript script) { + config = new Osinfo.InstallConfig ("http://libosinfo.fedorahosted.org/config"); + + if (script.has_config_param_name (Osinfo.INSTALL_CONFIG_PROP_USER_LOGIN)) Do we really need to check if each param exists in the script before setting it?
Created attachment 222235 [details] [review] work in progress this patch is not okay, yet. Need to fix regular expression to get language properly for newer windows and manage the installation progress.
(In reply to comment #19) > Created an attachment (id=222235) [details] [review] > work in progress > > this patch is not okay, yet. Need to fix regular expression to get language > properly for newer windows and manage the installation progress. No, we shouldn't need regular expression at all anymore. We need to move the mapping to XSL as discussed with danpb on IRC: <danpb> zeenix: i don't have time to get into details now, but wrt that keyboard/language thing - my thought was that we'd want to define some kind of data maps in the XML <danpb> zeenix: so from an API POV we can use a standard naming convention, and then use OS specific data maps to translate that in the install scripts <zeenix> makes sense <zeenix> danpb: maybe the standard naming convention could be the same as X/linux naming <danpb> zeenix: hehe, yeah, where in doubt use Linux as the standard :-) <zeenix> danpb: if we go for the X kbd naming conventions, we'll have to do the x to console mapping. You rejected that change, though that was doing it in the code
Review of attachment 222235 [details] [review]: Thats all i could fish this time around. :) ::: src/media-manager.vala @@ -109,1 +114,1 @@ switch (media.os.distro) { I hadn't seen this part of the patch. Why do we still need to have this OS-specific hardcoding? ::: src/unattended-installer.vala @@ -112,1 +105,5 @@ - add_unattended_file (new UnattendedTextFile (this, unattended_src_path, unattended_dest_name)); + + var list = scripts.get_elements (); + var script = list.data as Osinfo.InstallScript; ... 2 more ... why the need to deal with the first script file separately? If we need to do this, pretty sure there is something missing or wrong with libosinfo API. @@ -112,1 +105,10 @@ - add_unattended_file (new UnattendedTextFile (this, unattended_src_path, unattended_dest_name)); + + var list = scripts.get_elements (); + var script = list.data as Osinfo.InstallScript; ... 7 more ... I don't see the point of extracting a filename from the script here when you are passing the script itself. @@ +184,3 @@ + config = new Osinfo.InstallConfig ("http://libosinfo.fedorahosted.org/config"); + config.set_user_login (username); + config.set_user_password (password); You want to only set password if user provides one. @@ -265,2 +273,4 @@ if (child != express_toggle) express_toggle.bind_property ("active", child, "sensitive", BindingFlags.SYNC_CREATE); + + if (has_product_key () == false) * same here: no need to do equality check on booleans. Just 'if (!has_product_key ())' is good. * We also need to check if 'product key' is required or not (its not for win7 e.g) and return if its not. Yes, this might sound contrary to what i said about the params you set in set_config_properties() but the idea is not to ask user for things that are not required and most of those params in there are automatically subsituted. @@ -314,0 +374,5 @@ + private bool has_product_key () { + var list = scripts.get_elements (); + foreach (var s in list) { ... 2 more ... 1. you can remove the '== true' part. 2. coding-style nitpick: No need for '{}' on single statement blocks. @@ -314,0 +374,33 @@ + private bool has_product_key () { + var list = scripts.get_elements (); + foreach (var s in list) { ... 30 more ... As I said in the previous comment, we'll want to move all these mappings to XSL template in libosinfo.. @@ +699,3 @@ protected async File create (Cancellable? cancellable) throws GLib.Error { + installer.set_config_properties (script); + var output_dir = File.new_for_path ("."); * you don't want to create the file in current directory. Please put in the user cache dir (you get that using get_user_pkgcache utility function) * you also want to use a prefix too on the generated files to avoid possible conflicts (current code usesUnattendedInstaller.hostname for this purpose, i suggest you do the same).
(In reply to comment #21) > Review of attachment 222235 [details] [review]: > > Thats all i could fish this time around. :) > > ::: src/media-manager.vala > @@ -109,1 +114,1 @@ > switch (media.os.distro) { > > I hadn't seen this part of the patch. Why do we still need to have this > OS-specific hardcoding? It will be removed in the next patch. An additional support to remove this OS-specific hardcoding was added in libosinfo and is waiting for review. > > ::: src/unattended-installer.vala > @@ -112,1 +105,5 @@ > - add_unattended_file (new UnattendedTextFile (this, > unattended_src_path, unattended_dest_name)); > + > + var list = scripts.get_elements (); > + var script = list.data as Osinfo.InstallScript; > ... 2 more ... > > why the need to deal with the first script file separately? If we need to do > this, pretty sure there is something missing or wrong with libosinfo API. Yeah, we are missing something in libosinfo. It's waiting for review in virt-tools ML and will be fixed in the next patch. > > @@ -112,1 +105,10 @@ > - add_unattended_file (new UnattendedTextFile (this, > unattended_src_path, unattended_dest_name)); > + > + var list = scripts.get_elements (); > + var script = list.data as Osinfo.InstallScript; > ... 7 more ... > > I don't see the point of extracting a filename from the script here when you > are passing the script itself. Same than above. > > @@ +184,3 @@ > + config = new Osinfo.InstallConfig > ("http://libosinfo.fedorahosted.org/config"); > + config.set_user_login (username); > + config.set_user_password (password); > > You want to only set password if user provides one. I'm considering your comment for username and product-key as well. > > @@ -265,2 +273,4 @@ > if (child != express_toggle) > express_toggle.bind_property ("active", child, "sensitive", > BindingFlags.SYNC_CREATE); > + > + if (has_product_key () == false) > > * same here: no need to do equality check on booleans. Just 'if > (!has_product_key ())' is good. Fixed. > * We also need to check if 'product key' is required or not (its not for win7 > e.g) and return if its not. Yes, this might sound contrary to what i said about > the params you set in set_config_properties() but the idea is not to ask user > for things that are not required and most of those params in there are > automatically subsituted. I agree, but 'product key' is not a config param in win7 script. I don't will add because we are not using, but I'll add a check if the parameter is required or optional, as you suggested. > > @@ -314,0 +374,5 @@ > + private bool has_product_key () { > + var list = scripts.get_elements (); > + foreach (var s in list) { > ... 2 more ... > > 1. you can remove the '== true' part. > 2. coding-style nitpick: No need for '{}' on single statement blocks. Fixed. > > @@ -314,0 +374,33 @@ > + private bool has_product_key () { > + var list = scripts.get_elements (); > + foreach (var s in list) { > ... 30 more ... > > As I said in the previous comment, we'll want to move all these mappings to XSL > template in libosinfo.. I'll add in libosinfo and will send the patch to ML soon. > > @@ +699,3 @@ > protected async File create (Cancellable? cancellable) throws GLib.Error > { > + installer.set_config_properties (script); > + var output_dir = File.new_for_path ("."); > > * you don't want to create the file in current directory. Please put in the > user cache dir (you get that using get_user_pkgcache utility function) Fixed. > * you also want to use a prefix too on the generated files to avoid possible > conflicts (current code usesUnattendedInstaller.hostname for this purpose, i > suggest you do the same). Ahhh, right!
(In reply to comment #22) > (In reply to comment #21) > > Review of attachment 222235 [details] [review] [details]: > > ::: src/unattended-installer.vala > > @@ -112,1 +105,5 @@ > > - add_unattended_file (new UnattendedTextFile (this, > > unattended_src_path, unattended_dest_name)); > > + > > + var list = scripts.get_elements (); > > + var script = list.data as Osinfo.InstallScript; > > ... 2 more ... > > > > why the need to deal with the first script file separately? If we need to do > > this, pretty sure there is something missing or wrong with libosinfo API. > > Yeah, we are missing something in libosinfo. It's waiting for review in > virt-tools ML and will be fixed in the next patch. > > > > > @@ -112,1 +105,10 @@ > > - add_unattended_file (new UnattendedTextFile (this, > > unattended_src_path, unattended_dest_name)); > > + > > + var list = scripts.get_elements (); > > + var script = list.data as Osinfo.InstallScript; > > ... 7 more ... > > > > I don't see the point of extracting a filename from the script here when you > > are passing the script itself. > > Same than above. How is this related to missing API in libosinfo? > > > > @@ +184,3 @@ > > + config = new Osinfo.InstallConfig > > ("http://libosinfo.fedorahosted.org/config"); > > + config.set_user_login (username); > > + config.set_user_password (password); > > > > You want to only set password if user provides one. > > I'm considering your comment for username and product-key as well. They are different: * username is mandatory for all OSs so you can simply assume its there (i-e UI didn't let user continue w/o providing that). * product-key is either required or not presented at all. > > @@ -265,2 +273,4 @@ > > if (child != express_toggle) > > express_toggle.bind_property ("active", child, "sensitive", > > BindingFlags.SYNC_CREATE); > > + > > + if (has_product_key () == false) > > > > * same here: no need to do equality check on booleans. Just 'if > > (!has_product_key ())' is good. > > Fixed. > > > * We also need to check if 'product key' is required or not (its not for win7 > > e.g) and return if its not. Yes, this might sound contrary to what i said about > > the params you set in set_config_properties() but the idea is not to ask user > > for things that are not required and most of those params in there are > > automatically subsituted. > > I agree, but 'product key' is not a config param in win7 script. It should be and therefore will be, so please don't set it unless we have it. > I don't will add because we are not using, but I'll add a check if the > parameter is required or optional, as you suggested. The 'required' and 'optional' you'll need to check for telling the 'setup' page in UI that everything is ready. When setting things in the config, we need to check if values are available.
Fwiw, I see this series as 3.7 material, it's a pretty fundamental change, and it feels too risky to rush it less than one month before the 3.6 release, especially as the libosinfo bits are not all settled yet as I understand it.
(In reply to comment #24) > Fwiw, I see this series as 3.7 material, This was planned for 3.6 all along. Sorry, if I failed to communicate that enough. > it's a pretty fundamental change, and > it feels too risky to rush it less than one month before the 3.6 release, We still haven't made the 3.6 relase and currently we are in feature freeze and this is not a new feature but more of a refactoring/clean-up. I've been spending a lot of time in trying to make sure this doesn't break anything but if i fail, I'm pretty sure it'll be trivial stuff. > especially as the libosinfo bits are not all settled yet as I understand it. Its mostly settled in. We just need to get some details right. E.g We already have the keyboard/language related API/data and we just need to add mappings in XSL now.
(In reply to comment #25) > (In reply to comment #24) > > Fwiw, I see this series as 3.7 material, > > This was planned for 3.6 all along. Sorry, if I failed to communicate that > enough. We are doing time-based releases, not feature-based ones. > We still haven't made the 3.6 relase and currently we are in feature freeze and > this is not a new feature but more of a refactoring/clean-up. This is a rewrite in another language(s), not a refactoring/clean-up. > I've been > spending a lot of time in trying to make sure this doesn't break anything but > if i fail, I'm pretty sure it'll be trivial stuff. > Yep, and the burden to fix it will fall on other people shoulders as you'll be on holidays. > > especially as the libosinfo bits are not all settled yet as I understand it. > > Its mostly settled in. We just need to get some details right. E.g We already > have the keyboard/language related API/data and we just need to add mappings in > XSL now. At API/ABI freeze time (even if it's an external dep), I'd expect the code to be in git already.
This seems very likely to be yet another completely fruiteless discussion so I won't entertain it anymore. I told you exactly what you need to do to get these patches in. I suggest you try to make that happen rather than arguing about what a freeze means and what it doesn't mean and who would be fixing bugs when I'm on holidays..
(In reply to comment #27) > > I told you exactly what you need to do to get these patches in. these -> your
(In reply to comment #27) > I told you exactly what you need to do to get these patches in. I suggest you > try to make that happen rather than arguing about what a freeze means and what > it doesn't mean and who would be fixing bugs when I'm on holidays.. These are 2 totally distinct issues, getting the viostor patches in, and whether getting these libosinfo patches in is a good idea for getting these patches in. You cannot invalidate the arguments I'm making for the latter by attacking me about the former.
(In reply to comment #29) > (In reply to comment #27) > > I told you exactly what you need to do to get these patches in. I suggest you > > try to make that happen rather than arguing about what a freeze means and what > > it doesn't mean and who would be fixing bugs when I'm on holidays.. > > These are 2 totally distinct issues, getting the viostor patches in, and > whether getting these libosinfo patches in is a good idea for getting these > patches in. You cannot invalidate the arguments I'm making for the latter by > attacking me about the former. I have listend to your arguments and my decision remains the same so now its only and only about the conflict with your viostor patches. Good luck!
Created attachment 230113 [details] [review] updating the patch (now it is applicable on top of boxes git). still has some work to do, please take a look in "NOTES" part, of the commit message
Created attachment 230136 [details] [review] Use libosinfo APIs for the automated installations Let's drop all OSes-specific automation code in favor to use new automated installation API provided by libosinfo NOTES: It is not tested with the last changes. Why? Because libvirt is *really* annoying me with wrong linkages (and some confuse about jhbuild and real environment). I can not start gnome-boxes, crash. :( Still there are somethings to do and I really don't have time to do this, at least for now (with visa/move stuff). So, please, if someone (Zeeshan?) could fix this stuff I would be grateful. Feel free to change the code as you want. :) TODO: - use libosinfo to get product-key format - use libosinfo to set install-script/target disk - create a way to manage viostor stuff - remove extension from avatar image on windows scripts (libosinfo): - windows scripts expect something .bmp. and is a pita to get this info from mime_type (take a look in the commit). so, as the filename could be set for us, with the name we desire, let's remove the extension and remove the crappy code from get_source_file method - take a look in "FIXME:" and try to find a better way to manage them
(In reply to comment #32) > Created an attachment (id=230136) [details] [review] > > Still there are somethings to do and I really don't have time to do > this, at least for now (with visa/move stuff). > So, please, if someone (Zeeshan?) could fix this stuff I would be > grateful. Yeah, I'll take it from here. Good job on your project and hope you contribute in future as well. :)
(In reply to comment #32) > NOTES: > It is not tested with the last changes. > Why? Because libvirt is *really* annoying me with wrong linkages (and > some confuse about jhbuild and real environment). I can not start > gnome-boxes, crash. :( Installing libvirt a second time generally works around these issues. To make sure libvirt is properly installed, you can 'jhbuild run $prefix/sbin/libvirtd" and "jhbuild run virsh list --all"
(In reply to comment #32) > Created an attachment (id=230136) [details] [review] > TODO: > - use libosinfo to get product-key format > - use libosinfo to set install-script/target disk > - create a way to manage viostor stuff > - take a look in "FIXME:" and try to find a better way to manage them I'm on it. The first two are easy to solve but viostor will take some time. > - remove extension from avatar image on windows scripts (libosinfo): > - windows scripts expect something .bmp. and is a pita to get this > info from mime_type (take a look in the commit). so, as the filename > could be set for us, with the name we desire, let's remove the > extension and remove the crappy code from get_source_file method We dont really need a particular extension/filename here as scripts now give the file right name and extension when they copy/install it.
Created attachment 230374 [details] [review] installer: Use libosinfo APIs for the automated installations I addressed many of the issues in the previous version and now at least I'm able to successfully launch automated installations for fedora, win7 and winxp. There is still some issues to resolve before I submit a version thats mergable but I wanted to update the patch in case anyone wants to give it a try already. Oh and this assume my pending (and under discussion) keyboard layout patches for libosinfo on virt-tools list. TODO: - viostor driver installation - direct boot commandline should come from libosinfo - We still os-specific code for deciding what disk to use for unatteded files.
Created attachment 230770 [details] [review] configure: Require libosinfo >= 0.2.2 We'll need this version for latest installer and driver API.
Created attachment 230771 [details] [review] installer: Make two props of UnattendedFile, public Make dest_name & src_path properties of UnattendedFile interface, public.
Created attachment 230772 [details] [review] installer: Use libosinfo APIs for the automated installations Let's drop all OSes-specific automation code in favor of new automated installation API provided by libosinfo. Known issues: 1. Direct boot commandline should come from libosinfo. 2. We still have os-specific code for deciding what disk to use for unatteded files. 3. This breaks viostor drivers installation. I'll work on #1 next but hardcoded value for now isn't that bad as what we do currently isn't any different. Same for #2 and IMO the solution for that would go under (yet to be created) libvirt-builder. As for #3, a following patch in this series fixes that using new libosinfo API. co-author: Zeeshan Ali (Khattak) <zeeshanak@gnome.org>
Created attachment 230773 [details] [review] installer: Download & Install pre-installation drivers Use the new (yet to be reviewed) drivers API of libosinfo to automatically download and install all available pre-installable drivers. This also re-adds viostor for windows xp and 7 if libosinfo provides information about them.
Created attachment 230774 [details] [review] installer,data: Remove now redundant unattended data & sources
Created attachment 230775 [details] [review] iso-extractor: Remove now redundant enumerate_children()
Created attachment 230776 [details] [review] installer: Update visibility of various members We don't need virtual and protected members anymore as we don't have any subclasses of this class anymore.
Created attachment 230777 [details] [review] installer: Add try to delete inexistant file Don't try to delete the temporary file if it wasn't created.
Created attachment 230861 [details] [review] configure: Require valac >= 0.17.3 Dump valac requirement by 1 micro to be able to use asynchronous creation methods.
Created attachment 230862 [details] [review] media-manager: Async create_installer_media_from_media()
Created attachment 230863 [details] [review] installer: Async UnattendedInstaller.from_media()
Created attachment 230864 [details] [review] installer: Setup drivers in construction method This implies moving of drivers setup (which may involve downloading) from wizard's setup-to-review transition to preparing stage. For media automatically found by Boxes, drivers setup will happen even before user sees it listed in wizard's 'source' page. This should probably be squashed into patch: "installer: Download & Install pre-installation drivers" but i wanted to get feedback on this before I squash it.
Created attachment 230866 [details] [review] installer: Setup drivers in construction method v2: Remove now redundant FIXME comment.
Review of attachment 230772 [details] [review]: ::: src/unattended-installer.vala @@ +184,3 @@ + if (username != null) { + config.set_user_login (username); + config.set_user_realname (username); Don't we want to separate username and real name? That seems kind of important. I mean, its ok if you enter the real name and it auto-picks a username, but you should be able to pick a custom one. @@ +368,3 @@ + } + + private void on_key_text_inserted (string text, int text_length, ref int position) { I don't think this fully implements format checking. You need to also handle delete-text, or you can delete characters at a place other than the end and get it to go out of format. Also, it seems to only really handle inserts at the end. Inserts in the middle may shift existing text to the right, making it non-format conformant.
Review of attachment 230773 [details] [review]: ::: src/unattended-installer.vala @@ +613,3 @@ + var file_uri = location + "/" + filename; + var file = File.new_for_uri (file_uri); + var cached_path = get_drivers_cache (os.short_id + "-" + file.get_basename ()); Basing the cache on the basename only is risky, in case the filename is not versioned. Not sure what to do about this other than making sure all osinfo driver files are versioned though. @@ +617,3 @@ + // FIXME: Although this will only download a driver once (on first usage), this should be done at the + // 'preparation' phase of wizard and not when going from 'setup' to 'review'. + file = yield downloader.download (file, cached_path); Don't we want some kind of progress reporting on this?
Review of attachment 230866 [details] [review]: I don't think doing this earlier is any more right, it will just cause things to pause earlier, with no feedback. Rather, I think downloading should be initiated early, but not block anything until the user accepted the review and started the install. Then we should pause until all downloads are done with proper progress reporting.
Review of attachment 230772 [details] [review]: ::: src/unattended-installer.vala @@ +672,3 @@ + + var pixbuf_formats = Gdk.Pixbuf.get_formats (); + if (avatar_format != null) wouldn't hurt with some more brackets here @@ +681,3 @@ + } + else + pixbuf_format = pixbuf_formats.nth_data (0); This could be whatever, changing for each run. Seems better to fall back on a well defined format like png.
Review of attachment 230773 [details] [review]: ::: src/unattended-installer.vala @@ +587,3 @@ + foreach (var d in os.get_device_drivers ().get_elements ()) { + var driver = d as DeviceDriver; + if (driver.get_architecture () != os_media.architecture || !driver.get_pre_installable ()) Is this always correct? what about i386 vs i686 etc?
Review of attachment 230772 [details] [review]: Not replying to comments I agree with.. ::: src/unattended-installer.vala @@ +184,3 @@ + if (username != null) { + config.set_user_login (username); + config.set_user_realname (username); yikes, that must have been a c&p mistake. I'll fix... @@ +368,3 @@ + } + + private void on_key_text_inserted (string text, int text_length, ref int position) { it works pretty similarly to the existing code in winxp-installer.vala that was deleted after this patch. We can look a this later as long as this code doesn't introduce a regression, right?
Review of attachment 230773 [details] [review]: ::: src/unattended-installer.vala @@ +587,3 @@ + foreach (var d in os.get_device_drivers ().get_elements ()) { + var driver = d as DeviceDriver; + if (driver.get_architecture () != os_media.architecture || !driver.get_pre_installable ()) well that would require a bit more code to get right but for the moment we could live with exactly arch as the only drivers available through libosinfo are for both i386 and i686. I'll look into it and if it turns out to be too difficult, I'll add a FIXME here and add to top of my todo.. @@ +613,3 @@ + var file_uri = location + "/" + filename; + var file = File.new_for_uri (file_uri); + var cached_path = get_drivers_cache (os.short_id + "-" + file.get_basename ()); I have it on my todo to provide checksum for each file for validation/safety purposes but I think that will also solve this issue? @@ +617,3 @@ + // FIXME: Although this will only download a driver once (on first usage), this should be done at the + // 'preparation' phase of wizard and not when going from 'setup' to 'review'. + file = yield downloader.download (file, cached_path); Indeed we do and its on my todo list. I have a vague plan for some progress indicator API that we can pass around to such async calls. At the very least the progress bar (and label etc) should update after media detection succeeds in 'preparing' stage. About the FIXME, you might have noticed that I already proposed a solution for that in a separate patch.
Review of attachment 230866 [details] [review]: progress reporting as in "Downloading drivers.." under the VM before "Installing..."? IIRC, mccann said that once user reaches review, everything should be fully prepared (that was the reason to create VM & its storage before 'reivew') and I kinda agree with that. Preparing step seems like the right place to do all kinds of preparations. There is already a progress bar in there and a lable so there shouldn't be any issue showing progress to the user.
Review of attachment 230866 [details] [review]: I don't think that is what he meant. Every piece of information should be collected before the review so that we're pretty certain that an uninterrupted install can be done without user interaction. We don't expect the download to fail, so why should we force the user to busy wait while we download it rather than have that download as part of the unattended installation? But maybe I'm wrong.
Review of attachment 230772 [details] [review]: ::: src/unattended-installer.vala @@ +368,3 @@ + } + + private void on_key_text_inserted (string text, int text_length, ref int position) { yeah
Review of attachment 230773 [details] [review]: ::: src/unattended-installer.vala @@ +613,3 @@ + var file_uri = location + "/" + filename; + var file = File.new_for_uri (file_uri); + var cached_path = get_drivers_cache (os.short_id + "-" + file.get_basename ()); Does the osinfo info contain a checksum? Then we could use the checksum as the filename. Would solve this.
Review of attachment 230866 [details] [review]: Oh, currently we are OK with download failing. I actually want to also put checks for internet connectivity so we don't attempt to download if there is no connection.
Review of attachment 230773 [details] [review]: ::: src/unattended-installer.vala @@ +613,3 @@ + var file_uri = location + "/" + filename; + var file = File.new_for_uri (file_uri); + var cached_path = get_drivers_cache (os.short_id + "-" + file.get_basename ()); Not yet but should be trivial to add.
Review of attachment 230770 [details] [review]: ack
Review of attachment 230771 [details] [review]: ack
Review of attachment 230772 [details] [review]: The real name and the format checking issues can be fixed in a later version, but please fix the other things.
Review of attachment 230773 [details] [review]: ack, we can fix the remaining issues later.
Review of attachment 230774 [details] [review]: ack
Review of attachment 230775 [details] [review]: ack
Review of attachment 230776 [details] [review]: ack
Review of attachment 230777 [details] [review]: ack
Review of attachment 230861 [details] [review]: ack
Review of attachment 230862 [details] [review]: ack
Review of attachment 230863 [details] [review]: ack
Review of attachment 230866 [details] [review]: go with this for now.
Created attachment 230976 [details] [review] installer: Use libosinfo APIs for the automated installations * Add curly brackets * Default specifically to png (if supported).
Review of attachment 230976 [details] [review]: ::: src/unattended-installer.vala @@ +685,3 @@ + } + } else if (png_format != null) + pixbuf_format = png_format; // Fallback to PNG if supported But png_format is unset if the else is taken?
(In reply to comment #76) > Review of attachment 230976 [details] [review]: > > ::: src/unattended-installer.vala > @@ +685,3 @@ > + } > + } else if (png_format != null) > + pixbuf_format = png_format; // Fallback to PNG if supported > > But png_format is unset if the else is taken? Ouch, yes. this should be a separate 'if.
Created attachment 230979 [details] [review] installer: Use libosinfo APIs for the automated installations Corrected issue pointed out in previous review.
Review of attachment 230979 [details] [review]: ::: src/unattended-installer.vala @@ +687,3 @@ + + if (png_format != null) + pixbuf_format = png_format; // Fallback to PNG if supported if pixbuf_format == null && png_format != null?
Created attachment 230990 [details] [review] installer: Use libosinfo APIs for the automated installations * Hopefully I got the pixbuf format logic correct this time. :) * avatar location is expected to be a full path so fixed that.
Review of attachment 230990 [details] [review]: ack
Attachment 230770 [details] pushed as 1f1a189 - configure: Require libosinfo >= 0.2.2 Attachment 230771 [details] pushed as 121d790 - installer: Make two props of UnattendedFile, public Attachment 230773 [details] pushed as 563acf3 - installer: Download & Install pre-installation drivers Attachment 230774 [details] pushed as 52c5002 - installer,data: Remove now redundant unattended data & sources Attachment 230775 [details] pushed as 496d38c - iso-extractor: Remove now redundant enumerate_children() Attachment 230776 [details] pushed as ceed1d0 - installer: Update visibility of various members Attachment 230777 [details] pushed as 4bca019 - installer: Add try to delete inexistant file Attachment 230861 [details] pushed as be4843e - configure: Require valac >= 0.17.3 Attachment 230862 [details] pushed as 59e7e0a - media-manager: Async create_installer_media_from_media() Attachment 230863 [details] pushed as c72ab3b - installer: Async UnattendedInstaller.from_media() Attachment 230866 [details] pushed as abadcee - installer: Setup drivers in construction method Attachment 230990 [details] pushed as cd3a705 - installer: Use libosinfo APIs for the automated installations
Review of attachment 230773 [details] [review]: ::: src/unattended-installer.vala @@ +613,3 @@ + var file_uri = location + "/" + filename; + var file = File.new_for_uri (file_uri); + var cached_path = get_drivers_cache (os.short_id + "-" + file.get_basename ()); Oh checksum didn't turn out to be as trivial as i thought and both Daniel and Christophe share the opinion that we shouldn't add checksums as they don't solve the malicious tempering issue and when we solve that issue, checksums will probably become irrelevant: https://www.redhat.com/archives/virt-tools-list/2012-December/msg00242.html I'll now look into another way to tackle this as suggested by Daniel: <zeenix> [19:39:11] danpb: re: having version as part of URL: apps needing/wanting to cache files locally (like we are doing in Boxes) will than have to also save the URL/path they used for each file <danpb> [19:40:52] zeenix: that's not that hard to deal with <zeenix> [19:41:05] hmm.. not if version is part of the filename <danpb> [19:41:14] zeenix: just have a directory where you create symlinks to the cached file based on the md5sum of the URL <danpb> [19:41:30] cf, the desktop thumbnail cachiing spec