[Libreoffice] (un)localized MSI installer [REVIEW REQUEST]

[Tor Lillqvist]

>>> On 2011-01-14 at 15:41, <nthiebaud at gmail.com> wrote:
> On Thu, Jan 13, 2011 at 3:18 PM, Tor Lillqvist <tlillqvist at novell.com> wrote:
>>> So the problem must be in setup.exe, in some cases it fails to understand
>>> that it should use a transform. That is good, because it means we can debug 
> it.
>>
>> Which I did. Which was fun. Or at least entertaining. The code in 
> libs-core/desktop/win32/source/setup/setup.cpp is, what shall I  say, 
> amusing. It probably is based on code from Windows 3.1 era.
>>
>> The problem was in the SetupAppX::GetProfileSection() method. This method 
> loads one of the sections in the setup.ini file in the installer directory. 
> It does some "clever" buffer management, initially allocating a relatively 
> small buffer, and then growing it if the requested section doesn't fit. (With 
> our multi-language installer, the [languages] section is quite large.)
>>
>> To actually read the section, the Win32 function GetPrivateProfileSection() 
> is used. Unfortunately the author of the code didn't read the documentation 
> for that function's return value carefully enough. It says "If the buffer is 
> not large enough to contain all the key name and value pairs associated with 
> the named section, the return value is equal to nSize minus two". Alas, the 
> code was written as if the return value would be the required size, if the 
> passed buffer size was not big enough. (Which, sure, would be a saner API, 
> and match many other Win32 APIs.)
>>
>> The first bug was in the test whether the return value indicated a too small 
> buffer. The test never was true.
>>
>> But if that was fixed, it didn't help, because the code that attempted to 
> retry with a bigger buffer, instead of (for instance) multiplying the buffer 
> size by two, because of the same misunderstanding, didn't increase it at all. 
> Even if it would have increased it, with a fixed amount or multiplier, to be 
> really really correct, surely it should loop, not increase it just once and 
> retry.
>>
>> So, the attached patch seems to fix the problem. The initial buffer size is 
> now 10000 which de facto is big enough for us, but just in case, I kept the 
> resizing (now as a loop) and in that the size is limited for sanity to a 
> million characters or so. Should be enough. (And yes, I did verify that the 
> looping resizing code works by temporarily using a much smaller initial 
> buffer size.)
>>
>> The symptom of the bug was that only the first 20 or so languages in the 
> setup.ini file were recognized. German happens to be among those, which 
> explains why it worked for Thalion72. Hungarian, Swedish and Portuguese 
> (Brazil) are not, so they didn't work. In a test build with just a few 
> languages, they all worked...
>>
> why [...] instead of [...]

To minimize the change in the 3-3 and 3-3-0 branches; basically I changed 1) two '>' to '>=' (it should be enough to just test '==', but we wanted to be paranoid;), and 2) an 'if' to a 'while' with a sanity test to 'break' out of the loop. 

If you want to make the setup.exe source code more elegant, there is *lots* in there to improve... feel free to go wild, in master. But on the other hand, it works, and it's just a thin wrapper that launches the actual MSI installer, not really very high priority, is it?

And actually, I am not that sure setup.exe is really needed; the NSIS wrapper could start msiexec.exe directly instead of setup.exe, and the selection of what installer localisation to use (i.e. what TRANSFORMS=trans_foo.mst command line parameter to pass to msiexec) could be done in the NSIS code, too, one thinks. OAfter all, the NSIS wrapper already knows what localisation to use for itself.

--tml