GNOME Bugzilla – Bug 676834
A few express installation fixes/improvements
Last modified: 2016-03-31 13:59:55 UTC
Here are a few express installation fixes/improvements.
Created attachment 214971 [details] [review] express,winxp,win2k3: Set display resolution & depth Although according to documentation[1], this should simple work but it doesn't. :( [1] http://unattended.msfn.org/unattended.xp/view/web/19/#display
Created attachment 214972 [details] [review] express: Set hostname in guest based on its box's name
Created attachment 214973 [details] [review] express,winxp,win2k3: Admin password required We need to set admin password to "*" to unset it. The same is not true for 'net' commandline we use to setup user account so we need to do admin password substitution differently for Windows XP and 2003.
Created attachment 214974 [details] [review] express,winxp,fedora: Setup user's avatar We show the user's avatar in the express install setup as if it'll be used so its very disapointing for user when its not. This patch fixes that for Windows XP and Fedora.
Review of attachment 214971 [details] [review]: this is a bad approach imho, as it won't fit in maximized (default) or window-size. Also using primary monitor size is perhaps not what is wanted. Spice handles resolution switching depending on window size. If you take driver & agent from git for windows, it's already pixel precise, and there is no scaling needed.
Review of attachment 214972 [details] [review]: ::: src/unattended-installer.vala @@ +169,3 @@ } + public virtual string fill_unattended_data (string data, string hostname) throws RegexError { You could call an overridable get_hostname() method instead of passing an additional argument, /me think
Created attachment 214976 [details] [review] express,winxp,win2k3: Set display resolution & depth Rebased on current master.
Created attachment 214977 [details] [review] express: Set hostname in guest based on its box's name Rebased on current master.
Review of attachment 214973 [details] [review]: "We need to set admin password to "*" to unset it" Is that something documented? If it is, it would be great you give a reference to the doc. Or am I getting this wrong?
Created attachment 214978 [details] [review] express,winxp,win2k3: Admin password required Rebased on current master.
Created attachment 214979 [details] [review] express,winxp,fedora: Setup user's avatar Rebased on current master.
Review of attachment 214974 [details] [review]: I trust it works, ack
(In reply to comment #6) > Review of attachment 214972 [details] [review]: > > ::: src/unattended-installer.vala > @@ +169,3 @@ > } > > + public virtual string fill_unattended_data (string data, string hostname) > throws RegexError { > > You could call an overridable get_hostname() method instead of passing an > additional argument, /me think hostname doesn't come from sublcasses but from the caller (VMCreator) through the setup() call. I don't like having this passed around either but i didn't see any better alternative. (In reply to comment #9) > Review of attachment 214973 [details] [review]: > > "We need to set admin password to "*" to unset it" > > Is that something documented? If it is, it would be great you give a reference > to the doc. > Or am I getting this wrong? http://unattended.msfn.org/unattended.xp/view/web/19/#guiunattended and I found it out after getting an error about this from installer. I even tried having just "" as value but that trick didn't help either. Seems we must put a *. OTOH I tried if the same could work for user password but in the context of 'net' commandline '*' means that you want to enter password interactively.
(In reply to comment #12) > Review of attachment 214974 [details] [review]: > > I trust it works, ack Yeah, it took a while to get it working but it does work. I was hoping for getting it to work for win7 as well but thats not trivial either and I must look into other things (e.g crashes) before spending more time on this..
(In reply to comment #5) > Review of attachment 214971 [details] [review]: > > this is a bad approach imho, As I mentioned in the log, this doesn't work at all so it was more like to get others' feedback/suggestion. > as it won't fit in maximized (default) or When i set the resolution to the resolution of my display, it fits fine in maximized mode. > window-size. Why wouldn't it work? It would be a separate bug if we can't scale as per Boxes window size. > Also using primary monitor size is perhaps not what is wanted. > > Spice handles resolution switching depending on window size. If you take driver > & agent from git for windows, it's already pixel precise, and there is no > scaling needed. Does that take care of setting the default resolution appropriately? The problem I'm trying to fix here is to set some default resolution that doesn't look as ugly as 800x600 (the default).
Created attachment 215002 [details] [review] express: Set hostname in guest based on its box's name Rebased after removal of attachment#214976 [details].
Created attachment 215003 [details] [review] express,winxp,win2k3: Admin password required Link to docs added.
Created attachment 215005 [details] [review] express,winxp,fedora: Setup user's avatar Rebased after removal of attachment#214976 [details]
Review of attachment 215002 [details] [review]: ack
Review of attachment 215003 [details] [review]: ack
Review of attachment 215005 [details] [review]: ack
(In reply to comment #15) > As I mentioned in the log, this doesn't work at all so it was more like to get > others' feedback/suggestion. Ok > > as it won't fit in maximized (default) or > > When i set the resolution to the resolution of my display, it fits fine in > maximized mode. It will be scaled down. In window maximized, the gnome panel still occupy space, so the guest resolution is scaled to fit. What is perhaps needed is an equivalent of spicec/virt-viewer --full-screen=autoconf at the spice level (something like sending current display settings when connecting). Trying to set the default display during installation doesn't help much, since as soon as spice is resized and/or change resolution, it will be forgotten. > Does that take care of setting the default resolution appropriately? The > problem I'm trying to fix here is to set some default resolution that doesn't > look as ugly as 800x600 (the default). It will look "ugly" as long as it is scaled. But with a pixel-exact resolution, thanks to latest driver & agent, it doesn't. Setting a resolution differently won't last.
Attachment 214979 [details] pushed as 2c3ba93 - express,winxp,fedora: Setup user's avatar Attachment 215002 [details] pushed as b2575a8 - express: Set hostname in guest based on its box's name Attachment 215003 [details] pushed as c96bbe1 - express,winxp,win2k3: Admin password required Attachment 215005 [details] pushed as 2c3ba93 - express,winxp,fedora: Setup user's avatar
Created attachment 215056 [details] [review] Store box name as an UnattendedInstaller property This allows to make the code setting the host name in unattended files much less convoluted.
Comment on attachment 215005 [details] [review] express,winxp,fedora: Setup user's avatar >Subject: [PATCH] express,winxp,fedora: Setup user's avatar > protected override string fill_unattended_data (string data, string hostname) throws RegexError { >- var host = hostname.replace (" ", "-"); >- var str = base.fill_unattended_data (data, host); >+ var str = base.fill_unattended_data (data, hostname); This didn't belong there :-/ >- var avatar_file = "/var/lib/AccountsService/icons/" + username; >+ var avatar_file = new UnattendedAvatarFile (this, "/var/lib/AccountsService/icons/" + username, avatar_format); How much of an internal AccountsService detail is "/var/lib/AccountsService/icons/"? Are applications supposed to be poking there?
Created attachment 215065 [details] [review] Store box name as an UnattendedInstaller property This allows to make the code setting the host name in unattended files much less convoluted.
Review of attachment 215065 [details] [review]: I don't like this approach either as vm_name property doesn't exactly fit into UnattendedInstaller. Instead of getting hostname from outside (VMCreator), perhaps it'd be better if UnattendedInstaller.setup takes vm_name as argument instead and create hostname from it internally (i-e half of your approach)? If we'd need more of the VM properties later, we can always create domain configuration before the UnattendedInstaller.setup and pass that around instead.
(In reply to comment #27) > I don't like this approach either as vm_name property doesn't exactly fit into > UnattendedInstaller. Instead of getting hostname from outside (VMCreator), > perhaps it'd be better if UnattendedInstaller.setup takes vm_name as argument > instead and create hostname from it internally (i-e half of your approach)? Fine with me. I can also call the property "system_name" and derive hostname from there, this is slightly more future-proof if we ever need this system name in other places. But I'm fine with doing this.hostname = m_name.replace (" ", "-"); in UnattendedInstaller::setup > If > we'd need more of the VM properties later, we can always create domain > configuration before the UnattendedInstaller.setup and pass that around > instead. I don't know which functions you have in mind for this passing around, but I think the UnattendedFile classes should not have knowledge at all of the VM configuration. Ideally, UnattendedTextFile would not hardcode a call to UnattendedInstaller::fill_unattended_data but instead we'd have a UnattendedTextFile:set_line_filter method and UnattendedTextFile would call this callback for each line when set. Too clueless about vala to do that myself though ;)
Created attachment 215130 [details] [review] Store box name as an UnattendedInstaller property This allows to make the code setting the host name in unattended files much less convoluted.
(In reply to comment #28) > (In reply to comment #27) > > I don't like this approach either as vm_name property doesn't exactly fit into > > UnattendedInstaller. Instead of getting hostname from outside (VMCreator), > > perhaps it'd be better if UnattendedInstaller.setup takes vm_name as argument > > instead and create hostname from it internally (i-e half of your approach)? > > Fine with me. I can also call the property "system_name" and derive hostname > from there, this is slightly more future-proof if we ever need this system name > in other places. But I'm fine with doing > this.hostname = m_name.replace (" ", "-"); > in UnattendedInstaller::setup Its less about what you call it but more about where it belongs. This information just doesn't belong here. Having said that, we can think of it as a cache so not really a big deal I guess. > > If > > we'd need more of the VM properties later, we can always create domain > > configuration before the UnattendedInstaller.setup and pass that around > > instead. > > I don't know which functions you have in mind for this passing around, The same ones that pass it around right now. > but I > think the UnattendedFile classes should not have knowledge at all of the VM > configuration. I don't agree. These classes are very much related: While GVirDomain.Config is configuration of the VM, these classes are setting-up configuration of the guest for that VM. Its pretty logical IMHO for guest configuration to be based on VM configuration. OTOH, UnattendedInstaller classes shouldn't have any knowledge about VM configuration. So if you have any suggestion on how we can fix that, that would be awesome!
(In reply to comment #30) > > Its less about what you call it but more about where it belongs. This > information just doesn't belong here. Having said that, we can think of it as a > cache so not really a big deal I guess. There are already username, password, timezone, keyboard properties in UnattendedInstaller, I don't see hostname as being different there. Whether all these things belong in UnattendedInstaller or in a VMConfiguration object is a different question ;) > > > but I > > think the UnattendedFile classes should not have knowledge at all of the VM > > configuration. > > I don't agree. These classes are very much related: While GVirDomain.Config is > configuration of the VM, these classes are setting-up configuration of the > guest for that VM. Its pretty logical IMHO for guest configuration to be based > on VM configuration. "These classes" ? The various UnattendedFile classes? I see them as simple wrapper around a source file to be mcopied to a floppy image, with some basic transformation to be applied during the copy. One of these classes happen to have an hardcoded callback into UnattendedInstaller, callback which may need VM configuration. I don't think we should use this as a pretext to make all UnattendedFile classes require this vm configuration. On the contrary we should see how we can avoid having some hardcoded callback and use something more generic to make UnattendedFile as standalone as possible. Then classes can be built that use UnattendedFile as a tool, and these classes will know about VM configuration. But UnattendedFile is far too basic to be seen as something more than a helper class. > > OTOH, UnattendedInstaller classes shouldn't have any knowledge about VM > configuration. There are plenty of things UnattendedInstaller shouldn't do... But I'm not going into cleaning that up...