GNOME Bugzilla – Bug 685826
win7 express installation broken for foreign ISOs (e.g US ISO on fi locale)
Last modified: 2016-03-31 13:54:42 UTC
Currently in the Windows 7 unattended XML file, we are setting these locale-related keys: <SetupUILanguage> <UILanguage>BOXES_LANG</UILanguage> </SetupUILanguage> <SystemLocale>BOXES_LANG</SystemLocale> <UILanguage>EN-US</UILanguage> <UserLocale>BOXES_LANG</UserLocale> From my testing, <SetupUILanguage> is not needed and can be removed. We may also want to add InputLocale entries to set the user keyboard (http://technet.microsoft.com/en-us/library/ff715773.aspx ) The various BOXES_LANG values are derived from the user locale (fr_FR gives fr-FR for example). For the record, the valid values for the UILanguage node are listed there http://go.microsoft.com/fwlink/?LinkID=200318 , so ideally we'd be limiting the locale values we accept. The problem with what we are doing is that Win7 ISOs generally only contain one language, so there's only 1 acceptable value for the UILanguage node. When it's wrong, a window will be displayed at the beginning at the unattended install asking which language to use. The way it's currently hardcoded will make things work with English win7 ISOs, but things break if you try other languages as en-US is no longer an acceptable value for the UILanguage node. Given that once we have a Windows ISO, we don't get to choose the language the UI will be displayed in, I'd tend to use the same value as UILanguage for the UserLocale and SystemLocale nodes (they set locale settings as date format, currency, ..., it's more consistent to use the default settings for the language being installed). I see 2 possible ways to solve this: 1) expand libosinfo to give us locale information about the ISOs it knows. This is probably the right way of fixing this, but filling the database with the needed info will be time consuming... 2) the win7 ISOs have a sources/lang.ini file which is very simple and gives us the values we can use for UILanguage: [Available UI Languages] fr-FR = 3 [Fallback Languages] fr-FR = en-us
This is somehow related to bug #666037. It has an interesting link to downloadable win2003 language packs http://www.microsoft.com/en-us/download/details.aspx?id=22681 but I couldn't find a similar page for win7.
I forgot to mention above that UILanguage must be present if we don't want to get the dialog asking for what language to use, and must have a valid value for the iso. I did not manage to find a generic value that would mean "use the default language for this ISO"
(In reply to comment #0) > Currently in the Windows 7 unattended XML file, we are setting these > locale-related keys: > > <SetupUILanguage> > <UILanguage>BOXES_LANG</UILanguage> > </SetupUILanguage> > <SystemLocale>BOXES_LANG</SystemLocale> > <UILanguage>EN-US</UILanguage> > <UserLocale>BOXES_LANG</UserLocale> > > From my testing, <SetupUILanguage> is not needed and can be removed. We may > also want to add InputLocale entries to set the user keyboard > (http://technet.microsoft.com/en-us/library/ff715773.aspx ) > > The various BOXES_LANG values are derived from the user locale (fr_FR gives > fr-FR for example). For the record, the valid values for the UILanguage node > are listed there http://go.microsoft.com/fwlink/?LinkID=200318 , so ideally > we'd be limiting the locale values we accept. > > The problem with what we are doing is that Win7 ISOs generally only contain one > language, so there's only 1 acceptable value for the UILanguage node. When it's > wrong, a window will be displayed at the beginning at the unattended install > asking which language to use. The way it's currently hardcoded will make things > work with English win7 ISOs, but things break if you try other languages as > en-US is no longer an acceptable value for the UILanguage node. I recall that hard coding of UILanguage was needed to get fi-FI locale working against an en-US ISO (I could be remembering it wrong though but it was definitely needed for something). > Given that once we have a Windows ISO, we don't get to choose the language the > UI will be displayed in, I'd tend to use the same value as UILanguage for the > UserLocale and SystemLocale nodes (they set locale settings as date format, > currency, ..., it's more consistent to use the default settings for the > language being installed). > > I see 2 possible ways to solve this: > > 1) expand libosinfo to give us locale information about the ISOs it knows. This > is probably the right way of fixing this, but filling the database with the > needed info will be time consuming... > > 2) the win7 ISOs have a sources/lang.ini file which is very simple and gives us > the values we can use for UILanguage: > > [Available UI Languages] > fr-FR = 3 > > [Fallback Languages] > fr-FR = en-us I wonder how much we should care about the case of users using ISO of a different locale than their own? Is it something important enough for 3.6? If so, getting 2nd solution above working before 3.6.1 would be awesome.
(In reply to comment #3) > I recall that hard coding of UILanguage was needed to get fi-FI locale working > against an en-US ISO (I could be remembering it wrong though but it was > definitely needed for something). Yes, that's what I'm saying with far too many words ;) To have unattended installs of en-US ISOs working on systems using a fi_FI locale (for example), UILanguage must be set to en-US, not to fi-FI or you'd get a prompt. But if you were to try a fi-FI ISO, the current en-US hardcoding would cause the ISO to prompt you for which language to use. > I wonder how much we should care about the case of users using ISO of a > different locale than their own? Didn't you run some tests with a en-US ISO and a fi_FI locale ? ;) If we want the user locale to work (and only it, not en-US), we need to replace en-US with BOXES_LANGUAGE in the unattended file. But I think supporting at least en-US and user locale ISOs is important. Supporting installation of French ISOs on your fi_FI system is indeed more of a corner case, but a clean fix for the installation of en-US and fi-FI on your system would fix these other cases as well.
Created attachment 226279 [details] [review] Make UnattendedInstaller::fill_unattended_data async This will be useful in the next patch in this series where we want to loopback mount a Win7 ISO to extract files from it.
Created attachment 226280 [details] [review] win7: get BOXES_LANG value from Windows ISO Windows 7 ISOs typically only support one language, and if the language we ask in the unattended file is not the one supported by the ISO, the unattended installation will be interrupted with a dialog window asking which language should be used. Windows 7 ISOs contain a sources/lang.ini file which indicates which language the ISO support. This commit loopback mounts the ISO to get the language to use from this file rather than trying to derive it from the user current locale. This fixes bgo#685826
Created attachment 226281 [details] [review] win7: Remove optional node from unattended xml file
The "win7: get BOXES_LANG value from Windows ISO" patch unfortunately does not work. fuseiso only handles ISOs with an actual ISO9660 filesystem while the win7 DVDs are UDF. I've checked that the rest of the patch does the right thing by stopping boxes in gdb right after the mount, and doing manually a loopback mount as root. So if this fuseiso limitation can be worked around, this patch should work. The bad news is that I probably won't have time to spend more time working on this before Tuesday, that's why I'm attaching this partial patch here.
Created attachment 226341 [details] [review] win7,express: Set UILanguage according to user's locale We have been hardcoding 'UILanguage' value to make express installation work for the case of user with a non-US locale using US install media. This however is a less likely scenerio (and therefore lower priority) than the case of user with non-US locale using install media of the same non-US locale. This patch fixes express installation for the latter case at the cost of breaking it for the former. Thanks to Christophe Fergeau for testing and finding this out.
Review of attachment 226341 [details] [review]: Handling the same locale seems to be the more common one, so ack. However its not uncommon to have non-US with US win7 guest, so we should try to fix the full bug.
Comment on attachment 226341 [details] [review] win7,express: Set UILanguage according to user's locale Attachment 226341 [details] pushed as 4b7ea4d - win7,express: Set UILanguage according to user's locale
Fwiw, iso-read/iso-info from git ( http://git.savannah.gnu.org/gitweb/?p=libcdio.git ) now can read files from an UDF image
Created attachment 231471 [details] [review] Use osinfo_db_identify_media osinfo_db_guess_os_from_media is deprecated in newer libosinfo and is replaced by osinfo_db_identify_media.
Created attachment 231472 [details] [review] Don't try to use unsupported languages in autoinstalls Now that libosinfo has an API to report which languages are supported by an ISO, we can fix a long-standing bug with Windows unattended installations: Windows ISOs only support one language most of the time, and we need to specify this language as the one to use for installation, otherwise Windows will ask a question. This commit will first check if the current installer supports one of the languages from the user locale and use that if found. Otherwise it will fallback to using one of the languages supported by the media. If the media does not have any associated language, it will use the user locale as the installation language.
Comment on attachment 226279 [details] [review] Make UnattendedInstaller::fill_unattended_data async There is no fill_unattended_data() method anymore to make async.
Review of attachment 231471 [details] [review]: You want to bump libosinfo requirement before this patch. Looks good otherwise.
Review of attachment 231472 [details] [review]: Looks good otherwise. ::: src/unattended-installer.vala @@ +106,3 @@ + var system_langs = Intl.get_language_names (); + var media_langs = new HashTable<string, unowned string> (str_hash, str_equal); + var media_langs_list = media.os_media.languages; since media_langs_list is only really used once, i think it only makes this code slightly confusing since we got media_langs variable as well. Just my feeling reading this code. @@ +111,3 @@ + foreach (var lang in media_langs_list) { + media_langs.add (lang); + if (default_lang == null) this will simply set default_lang to the first lang in the list so it doesn't have to be in the loop. You can just do this above: var default_lang =media_langs_list_nth(0);
Created attachment 231575 [details] [review] Use osinfo_db_identify_media osinfo_db_guess_os_from_media is deprecated in newer libosinfo and is replaced by osinfo_db_identify_media.
Created attachment 231576 [details] [review] Don't try to use unsupported languages in autoinstalls Now that libosinfo has an API to report which languages are supported by an ISO, we can fix a long-standing bug with Windows unattended installations: Windows ISOs only support one language most of the time, and we need to specify this language as the one to use for installation, otherwise Windows will ask a question. This commit will first check if the current installer supports one of the languages from the user locale and use that if found. Otherwise it will fallback to using one of the languages supported by the media. If the media does not have any associated language, it will use the user locale as the installation language.
Review of attachment 231575 [details] [review]: ACK ::: configure.ac @@ -53,3 @@ SPICE_GTK_MIN_VERSION=0.12.101 GUDEV_MIN_VERSION=165 -OSINFO_MIN_VERSION=0.2.2 should have mentioned before, I prefer to put the version bump into separate commits as means to ensure that I don't forget to mention it in release notes. No need to re-work the commit in bz for that though, just split it before pushing..
Review of attachment 231576 [details] [review]: ::: src/unattended-installer.vala @@ +106,3 @@ + var system_langs = Intl.get_language_names (); + var media_langs = new HashTable<string, unowned string> (str_hash, str_equal); + var media_langs_list = media.os_media.languages; So I take it that you don't see 'media_langs_list' as redundant? :) @@ +111,3 @@ + foreach (var lang in media_langs_list) { + media_langs.add (lang); + if (default_lang == null) whatever happend to my suggestion of having this outside the loop? @@ +117,3 @@ + foreach (var lang in system_langs) { + if (lang in media_langs) { + warning("matched %s", lang); warning? @@ +127,3 @@ + } + + warning("No media language, using %s locale", system_langs[0]); Not a big deal but its repeated thrice in this patch: space before '(' as per coding-style.
Review of attachment 231576 [details] [review]: ::: src/unattended-installer.vala @@ +106,3 @@ + var system_langs = Intl.get_language_names (); + var media_langs = new HashTable<string, unowned string> (str_hash, str_equal); + var media_langs_list = media.os_media.languages; After (partially) addressing your suggestions, it's used multiple times in this patch... @@ +111,3 @@ + foreach (var lang in media_langs_list) { + media_langs.add (lang); + if (default_lang == null) Dunno, I swear I had done this change. New patch will come @@ +117,3 @@ + foreach (var lang in system_langs) { + if (lang in media_langs) { + warning("matched %s", lang); Was meant to be debug()
Review of attachment 231575 [details] [review]: ::: configure.ac @@ -53,3 @@ SPICE_GTK_MIN_VERSION=0.12.101 GUDEV_MIN_VERSION=165 -OSINFO_MIN_VERSION=0.2.2 Hmm, a bit surprised by this request... Didn't you tell me several times that you were not interested in making other people's life easier if that meant more work for you?
Created attachment 231789 [details] [review] Don't try to use unsupported languages in autoinstalls Now that libosinfo has an API to report which languages are supported by an ISO, we can fix a long-standing bug with Windows unattended installations: Windows ISOs only support one language most of the time, and we need to specify this language as the one to use for installation, otherwise Windows will ask a question. This commit will first check if the current installer supports one of the languages from the user locale and use that if found. Otherwise it will fallback to using one of the languages supported by the media. If the media does not have any associated language, it will use the user locale as the installation language.
Review of attachment 231575 [details] [review]: ::: configure.ac @@ -53,3 @@ SPICE_GTK_MIN_VERSION=0.12.101 GUDEV_MIN_VERSION=165 -OSINFO_MIN_VERSION=0.2.2 Assuming that I did say that, I'm usually the one to write release notes so this will make my life harder, not others. And please stop it with grudges you hold against me..
Review of attachment 231575 [details] [review]: ::: configure.ac @@ -53,3 @@ SPICE_GTK_MIN_VERSION=0.12.101 GUDEV_MIN_VERSION=165 -OSINFO_MIN_VERSION=0.2.2 In this case, this makes my life harder in order to make yours easier. Don't use such excuses in the first place if you may need me to return the favour in the future as is the case here.
Review of attachment 231575 [details] [review]: ::: configure.ac @@ -53,3 @@ SPICE_GTK_MIN_VERSION=0.12.101 GUDEV_MIN_VERSION=165 -OSINFO_MIN_VERSION=0.2.2 I'm sorry to make your life so extremely difficult by asking you to put this 1 line change separately.
Review of attachment 231575 [details] [review]: ::: configure.ac @@ -53,3 @@ SPICE_GTK_MIN_VERSION=0.12.101 GUDEV_MIN_VERSION=165 -OSINFO_MIN_VERSION=0.2.2 I'm sorry that your past behaviour made it awkward for you to ask for such a change.
Review of attachment 231789 [details] [review]: Looks good
Attachment 231575 [details] pushed as 640a4f5 - Use osinfo_db_identify_media Attachment 231789 [details] pushed as 3dc5d0a - Don't try to use unsupported languages in autoinstalls
Review of attachment 231575 [details] [review]: ::: configure.ac @@ -56,2 @@ TRACKER_SPARQL=0.13.1 UUID_REQUIRED=1.41.3 I've now remembered that when asked to put a full link to the boxes tarball in the release notes to make packagers' life easier, you told me you wouldn't do that as this meant more work for you. And now you're asking for more work from us because it makes writing said release notes easier for you... Interesting transitivity
(In reply to comment #31) > Review of attachment 231575 [details] [review]: > > ::: configure.ac > @@ -56,2 @@ > TRACKER_SPARQL=0.13.1 > UUID_REQUIRED=1.41.3 > > I've now remembered that when asked to put a full link to the boxes tarball in > the release notes to make packagers' life easier, you told me you wouldn't do > that as this meant more work for you. > And now you're asking for more work from us because it makes writing said > release notes easier for you... > Interesting transitivity Can you please make-up your mind about whether or not you want to continue this fruitless discussion? Last night you yourself told me "<teuf> [20:09:10] let's not reiterate the conversation we had in this bug".
(In reply to comment #32) > > Can you please make-up your mind about whether or not you want to continue this > fruitless discussion? Hopefully the whole conversation wasn't fruitless... > Last night you yourself told me "<teuf> [20:09:10] let's > not reiterate the conversation we had in this bug". Yes, and the conversation stayed in this bug, it did not get rehashed on IRC, so things are consistent.
Instead of deciding where to not rehash this, could we perhaps decide to not write asshole comments like in this bug, and act like we're more than 5 years old?
Created attachment 232068 [details] [review] Fix restarting Boxes while an install is in progress The introduction of the use of osinfo_db_identify_media caused a regression when restarting Boxes while an installation is in progress: (gnome-boxes:3699): Boxes-WARNING **: media-manager.vala:50: Unknown media ID 'file:///home/foo/install-image.iso' The reason for that is that when using OsDatabase::guess_os_from_install_media() had a change of behaviour with respect to the media it returns. Before, the id of this media was corresponding to an entry in the OsinfoDB we are using. Now it's the id of a media created through osinfo_media_create_from_location, which is the media path. We store this id in our custom libvirt domain metadata, and MediaManager::create_installer_media_from_config expects the id to be found in the OsinfoDb database, and not to be a file path. This commit switches to using OsDatabase::guess_os_from_install_media() to determine os_media as this can handle a path.
Review of attachment 232068 [details] [review]: ::: src/media-manager.vala @@ +43,3 @@ var media_id = VMConfigurator.get_os_media_id (config); if (media_id != null) + os_media = yield os_db.guess_os_from_install_media(media_id, null); 1. this beats the point of keeping the media id in domain config. We really shouldn't be needing to guess media again. 2. Instead osinfo_db_identify_media() should be setting the ID (since its setting other props anyways).
Review of attachment 232068 [details] [review]: ::: src/media-manager.vala @@ +43,3 @@ var media_id = VMConfigurator.get_os_media_id (config); if (media_id != null) + os_media = yield os_db.guess_os_from_install_media(media_id, null); I wasn't sure changing the ID of an existing entity made a lot of sense, especially when it's not fully identical to the entity with the same ID (it has more data).
Review of attachment 232068 [details] [review]: ::: src/media-manager.vala @@ +43,3 @@ var media_id = VMConfigurator.get_os_media_id (config); if (media_id != null) + os_media = yield os_db.guess_os_from_install_media(media_id, null); Well, I guess now you understand my reservations against _identify_media() setting props on a passed media rather than returning a new (or rather copy of matched DB media). Now that I think of it, we'll now be re-identifying media even in case of tracker/gudev, where they tell us the ID of the media/OS. i-e On each run of Boxes, we'll now identify all known media even though it shouldn't be needed. Also, this code shouldn't be assuming that Boxes.OSDatabase is using path as ID.
Comment on attachment 232068 [details] [review] Fix restarting Boxes while an install is in progress This is obsolete now that osinfo_db_identify_media sets the ID of the media as well.