Issue 10920006: Expect MSI installers (Closed)
|
Created:
June 10, 2013, 3:27 p.m. by Wladimir Palant Modified:
June 19, 2013, 5:39 p.m. Visibility:
Public. |
DescriptionExpect MSI installers
Patch Set 1 #
Total comments: 16
Patch Set 2 : Changed platform name #Patch Set 3 : Separate function to run updater, using msiexec now #
MessagesTotal messages: 11
http://codereview.adblockplus.org/10920006/diff/1/AdblockPlusEngine.vcxproj File AdblockPlusEngine.vcxproj (right): http://codereview.adblockplus.org/10920006/diff/1/AdblockPlusEngine.vcxproj#n... AdblockPlusEngine.vcxproj:188: <AdditionalDependencies>libadblockplus.lib;shlwapi.lib;user32.lib;v8_base.lib;v8_snapshot.lib;winhttp.lib;winmm.lib;ws2_32.lib;msi.lib;%(AdditionalDependencies)</AdditionalDependencies> Is there any reason why msi is included both statically and dynamically? http://codereview.adblockplus.org/10920006/diff/1/src/engine/Updater.cpp File src/engine/Updater.cpp (right): http://codereview.adblockplus.org/10920006/diff/1/src/engine/Updater.cpp#newc... src/engine/Updater.cpp:123: UINT result = ::MsiInstallProductW(tempFile.c_str(), L"ACTION=INSTALL INSTALLUILEVEL=2"); Wouldn't it be better just to launch like "msiexec.exe /q /i " + tempfile.c_str()? We might still want to use an exe installer in future, and this way will be very similar to the exe installer launch. And that way we will leave it to Windows Installer to handle all the errors.
http://codereview.adblockplus.org/10920006/diff/1/AdblockPlusEngine.vcxproj File AdblockPlusEngine.vcxproj (right): http://codereview.adblockplus.org/10920006/diff/1/AdblockPlusEngine.vcxproj#n... AdblockPlusEngine.vcxproj:188: <AdditionalDependencies>libadblockplus.lib;shlwapi.lib;user32.lib;v8_base.lib;v8_snapshot.lib;winhttp.lib;winmm.lib;ws2_32.lib;msi.lib;%(AdditionalDependencies)</AdditionalDependencies> On 2013/06/10 16:47:56, Oleksandr wrote: > Is there any reason why msi is included both statically and dynamically? We need msi.lib to compile. We delay-load msi.dll because most of the time we don't need it. http://codereview.adblockplus.org/10920006/diff/1/src/engine/Updater.cpp File src/engine/Updater.cpp (right): http://codereview.adblockplus.org/10920006/diff/1/src/engine/Updater.cpp#newc... src/engine/Updater.cpp:123: UINT result = ::MsiInstallProductW(tempFile.c_str(), L"ACTION=INSTALL INSTALLUILEVEL=2"); MsiInstallProduct is IMHO a much cleaner solution - e.g. we don't rely on the PATH variable to be set up correctly, we don't need to do command line escaping etc. And we get a meaningful error code if something fails. If we ever need to switch back to executable installers - the code is still there in the repo history. However, I'm not in favor of doing something just because "we might need it sometime".
http://codereview.adblockplus.org/10920006/diff/1/AdblockPlusEngine.vcxproj File AdblockPlusEngine.vcxproj (right): http://codereview.adblockplus.org/10920006/diff/1/AdblockPlusEngine.vcxproj#n... AdblockPlusEngine.vcxproj:188: <AdditionalDependencies>libadblockplus.lib;shlwapi.lib;user32.lib;v8_base.lib;v8_snapshot.lib;winhttp.lib;winmm.lib;ws2_32.lib;msi.lib;%(AdditionalDependencies)</AdditionalDependencies> On 2013/06/11 04:29:56, Wladimir Palant wrote: > On 2013/06/10 16:47:56, Oleksandr wrote: > > Is there any reason why msi is included both statically and dynamically? > > We need msi.lib to compile. We delay-load msi.dll because most of the time we > don't need it. I don't get this. If it's statically linked, it's not being loaded, so loading cannot be delayed. If this is really necessary, I can't figure out why. http://codereview.adblockplus.org/10920006/diff/1/src/engine/Updater.cpp File src/engine/Updater.cpp (right): http://codereview.adblockplus.org/10920006/diff/1/src/engine/Updater.cpp#newc... src/engine/Updater.cpp:104: { I presume the two blocks are to avoid conflicts between the two variables called "result"? I think it'd be more clear if those were named distinctively then, e.g. dialogResult and installationResult. http://codereview.adblockplus.org/10920006/diff/1/src/engine/Updater.cpp#newc... src/engine/Updater.cpp:123: UINT result = ::MsiInstallProductW(tempFile.c_str(), L"ACTION=INSTALL INSTALLUILEVEL=2"); On 2013/06/11 04:29:56, Wladimir Palant wrote: > MsiInstallProduct is IMHO a much cleaner solution - e.g. we don't rely on the > PATH variable to be set up correctly, we don't need to do command line escaping > etc. And we get a meaningful error code if something fails. > > If we ever need to switch back to executable installers - the code is still > there in the repo history. However, I'm not in favor of doing something just > because "we might need it sometime". As long as "update.msi" is hardcoded above we'd need to push out an update to switch to exe anyway. http://codereview.adblockplus.org/10920006/diff/1/src/engine/main.cpp File src/engine/main.cpp (right): http://codereview.adblockplus.org/10920006/diff/1/src/engine/main.cpp#newcode174 src/engine/main.cpp:174: appInfo.platform = "win64"; Not sure what we'll want to use the platform for, but this is "firefox" for FF, right? Maybe better msie64 then.
http://codereview.adblockplus.org/10920006/diff/1/AdblockPlusEngine.vcxproj File AdblockPlusEngine.vcxproj (right): http://codereview.adblockplus.org/10920006/diff/1/AdblockPlusEngine.vcxproj#n... AdblockPlusEngine.vcxproj:188: <AdditionalDependencies>libadblockplus.lib;shlwapi.lib;user32.lib;v8_base.lib;v8_snapshot.lib;winhttp.lib;winmm.lib;ws2_32.lib;msi.lib;%(AdditionalDependencies)</AdditionalDependencies> Does it not compile if you remove msi.lib? On 2013/06/11 10:07:52, Felix H. Dahlke wrote: > On 2013/06/11 04:29:56, Wladimir Palant wrote: > > On 2013/06/10 16:47:56, Oleksandr wrote: > > > Is there any reason why msi is included both statically and dynamically? > > > > We need msi.lib to compile. We delay-load msi.dll because most of the time we > > don't need it. > > I don't get this. If it's statically linked, it's not being loaded, so loading > cannot be delayed. If this is really necessary, I can't figure out why. http://codereview.adblockplus.org/10920006/diff/1/src/engine/Updater.cpp File src/engine/Updater.cpp (right): http://codereview.adblockplus.org/10920006/diff/1/src/engine/Updater.cpp#newc... src/engine/Updater.cpp:123: UINT result = ::MsiInstallProductW(tempFile.c_str(), L"ACTION=INSTALL INSTALLUILEVEL=2"); That's my point. Stuff happens, and maybe for some reason we won't be able to do a proper update via an msi (for whatever reason). Exe is a plan b for that case, and it would be nice to support both. On 2013/06/11 10:07:52, Felix H. Dahlke wrote: > On 2013/06/11 04:29:56, Wladimir Palant wrote: > > MsiInstallProduct is IMHO a much cleaner solution - e.g. we don't rely on the > > PATH variable to be set up correctly, we don't need to do command line > escaping > > etc. And we get a meaningful error code if something fails. > > > > If we ever need to switch back to executable installers - the code is still > > there in the repo history. However, I'm not in favor of doing something just > > because "we might need it sometime". > > As long as "update.msi" is hardcoded above we'd need to push out an update to > switch to exe anyway.
http://codereview.adblockplus.org/10920006/diff/1/src/engine/Updater.cpp File src/engine/Updater.cpp (right): http://codereview.adblockplus.org/10920006/diff/1/src/engine/Updater.cpp#newc... src/engine/Updater.cpp:123: UINT result = ::MsiInstallProductW(tempFile.c_str(), L"ACTION=INSTALL INSTALLUILEVEL=2"); On 2013/06/11 10:24:49, Oleksandr wrote: > That's my point. Stuff happens, and maybe for some reason we won't be able to do > a proper update via an msi (for whatever reason). Exe is a plan b for that case, > and it would be nice to support both. > > On 2013/06/11 10:07:52, Felix H. Dahlke wrote: > > On 2013/06/11 04:29:56, Wladimir Palant wrote: > > > MsiInstallProduct is IMHO a much cleaner solution - e.g. we don't rely on > the > > > PATH variable to be set up correctly, we don't need to do command line > > escaping > > > etc. And we get a meaningful error code if something fails. > > > > > > If we ever need to switch back to executable installers - the code is still > > > there in the repo history. However, I'm not in favor of doing something just > > > because "we might need it sometime". > > > > As long as "update.msi" is hardcoded above we'd need to push out an update to > > switch to exe anyway. > Couldn't we just ship an exe with the msi and execute it? Guess I could live with that. If we really do need to move to an exe installer later, we could have a transitional msi wrapper for older versions.
http://codereview.adblockplus.org/10920006/diff/1/AdblockPlusEngine.vcxproj File AdblockPlusEngine.vcxproj (right): http://codereview.adblockplus.org/10920006/diff/1/AdblockPlusEngine.vcxproj#n... AdblockPlusEngine.vcxproj:188: <AdditionalDependencies>libadblockplus.lib;shlwapi.lib;user32.lib;v8_base.lib;v8_snapshot.lib;winhttp.lib;winmm.lib;ws2_32.lib;msi.lib;%(AdditionalDependencies)</AdditionalDependencies> On 2013/06/11 10:07:52, Felix H. Dahlke wrote: > I don't get this. If it's statically linked, it's not being loaded, so loading > cannot be delayed. It isn't statically link - you cannot link a system library statically. msi.lib merely contains the definitions of imports. And - no, it doesn't compile without. http://codereview.adblockplus.org/10920006/diff/1/src/engine/Updater.cpp File src/engine/Updater.cpp (right): http://codereview.adblockplus.org/10920006/diff/1/src/engine/Updater.cpp#newc... src/engine/Updater.cpp:104: { I would rather have distinct scopes for these two unrelated operations. http://codereview.adblockplus.org/10920006/diff/1/src/engine/Updater.cpp#newc... src/engine/Updater.cpp:123: UINT result = ::MsiInstallProductW(tempFile.c_str(), L"ACTION=INSTALL INSTALLUILEVEL=2"); I really don't see why we would need a plan b (one that we cannot test properly). And - yes, pushing out an update to switch to exe installers is exactly the way to go. http://codereview.adblockplus.org/10920006/diff/1/src/engine/main.cpp File src/engine/main.cpp (right): http://codereview.adblockplus.org/10920006/diff/1/src/engine/main.cpp#newcode174 src/engine/main.cpp:174: appInfo.platform = "win64"; On 2013/06/11 10:07:52, Felix H. Dahlke wrote: > Not sure what we'll want to use the platform for, but this is "firefox" for FF, > right? Maybe better msie64 then. It is used for some branching in the JS code. And - yes, I think that msie64 will be more consistent.
I adjusted the platform names as suggested by Felix.
LGTM http://codereview.adblockplus.org/10920006/diff/1/src/engine/Updater.cpp File src/engine/Updater.cpp (right): http://codereview.adblockplus.org/10920006/diff/1/src/engine/Updater.cpp#newc... src/engine/Updater.cpp:123: UINT result = ::MsiInstallProductW(tempFile.c_str(), L"ACTION=INSTALL INSTALLUILEVEL=2"); On 2013/06/11 14:42:10, Wladimir Palant wrote: > I really don't see why we would need a plan b (one that we cannot test > properly). And - yes, pushing out an update to switch to exe installers is > exactly the way to go. I'd vote for two functions then :)
Turned out that low integrity processes cannot access MSI service. Same goes for msiexec if we start it directly. So far the only option I found is running msiexec with elevated privileges.
LGTM 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
