Issue 10879048: Use minor upgrade mechanism for updates, set installer versionautomatically (Closed)
|
Created:
June 11, 2013, 1:21 p.m. by Wladimir Palant Modified:
June 19, 2013, 5:39 p.m. Visibility:
Public. |
DescriptionUse minor upgrade mechanism for updates, set installer version
automatically
Patch Set 1 #
Total comments: 14
Patch Set 2 : Added default version #Patch Set 3 : Looks like I misunderstood nmake documentation, previous change made settings VERSION parameter ext… #MessagesTotal messages: 9
LGTM, with a nit. http://codereview.adblockplus.org/10879048/diff/1/build_release.py File build_release.py (right): http://codereview.adblockplus.org/10879048/diff/1/build_release.py#newcode31 build_release.py:31: buildnum, dummy = subprocess.Popen(['hg', 'id', '-R', basedir, '-n'], stdout=subprocess.PIPE).communicate() I'd consider buildnum = subprocess.Popen(...).communicate()[0] cleaner.
http://codereview.adblockplus.org/10879048/diff/1/build_release.py File build_release.py (right): http://codereview.adblockplus.org/10879048/diff/1/build_release.py#newcode31 build_release.py:31: buildnum, dummy = subprocess.Popen(['hg', 'id', '-R', basedir, '-n'], stdout=subprocess.PIPE).communicate() On 2013/06/11 13:53:22, Felix H. Dahlke wrote: > I'd consider buildnum = subprocess.Popen(...).communicate()[0] cleaner. Seriously? [0] at the end of a long line is very non-obvious IMHO.
On 2013/06/11 14:43:18, Wladimir Palant wrote: > http://codereview.adblockplus.org/10879048/diff/1/build_release.py > File build_release.py (right): > > http://codereview.adblockplus.org/10879048/diff/1/build_release.py#newcode31 > build_release.py:31: buildnum, dummy = subprocess.Popen(['hg', 'id', '-R', > basedir, '-n'], stdout=subprocess.PIPE).communicate() > On 2013/06/11 13:53:22, Felix H. Dahlke wrote: > > I'd consider buildnum = subprocess.Popen(...).communicate()[0] cleaner. > > Seriously? [0] at the end of a long line is very non-obvious IMHO. Just my rule of thumb, no destructuring when I only want one value. But I'll leave this up to you :) I think _ is quite popular in Python for unused variables.
(1) We have two different kinds of developer use cases, the first is developing the installer itself, the second is preparing installers for release. Overall, these changes do not consider the first use case. Since both of these use cases need to coexist, it seems that we'll have conditional compilation (WiX and NMAKE preprocessor) as a permanent part of the build environment. For example, it's a simple matter to conditionally compile out the UI in the MSI files. The only overhead for this is having two artifact MSI's (one with UI, one without) in the build tree. The benefit is reduced delivery bandwidth. (2) We have a major upgrade required to move the 32-bit DLL. I haven't researched whether the present modifications will support that. See the detailed comments below on that particular issue. The bigger issue, though, is that the command line arguments for installing an MSI are properly associated with the MSI itself. They shouldn't be considered constant across all the possible and future MSI we'll deliver. The update mechanism should deliver not just a file payload, but also a string, which for Windows Installer can be used to hold the command line arguments. (We can ignore it on other platforms.) An additional benefit of such a change would be to allow delivery of patch files. http://codereview.adblockplus.org/10879048/diff/1/WixInstaller/Makefile File WixInstaller/Makefile (right): http://codereview.adblockplus.org/10879048/diff/1/WixInstaller/Makefile#newco... WixInstaller/Makefile:20: !error VERSION parameter is required Mandating a version number is a needless burden for the maintainer of the installer (me, currently). I don't build releases each time I build installers to test, which means I won't be running the build_release.py script each and every time. This modification more-or-less presumes that the Makefile primarily supports the release script. We need a default version number here. A default can use a very high build number, such as 0.8.63333, that won't ever interfere with a release. Alternately, we can cache the current build number in such a way that we don't need to be calling 'hg' each time we make an installer. http://codereview.adblockplus.org/10879048/diff/1/WixInstaller/Makefile#newco... WixInstaller/Makefile:55: Candle = candle -nologo -dNoDefault -dVersion=$(VERSION) "-dConfiguration=$(Configuration)" $(CANDLE_FLAGS) $(*F).wxs -out $@ No issue here with this modification, since we have a default configuration declared. http://codereview.adblockplus.org/10879048/diff/1/WixInstaller/adblockplusie.wxs File WixInstaller/adblockplusie.wxs (right): http://codereview.adblockplus.org/10879048/diff/1/WixInstaller/adblockplusie.... WixInstaller/adblockplusie.wxs:32: <?define Product_ID="3caab5a9-43a7-4a57-8373-b7d0952b709c" ?> We'll need that second Product_ID soon enough for an upcoming major upgrade required to move the 32-bit DLL to the correct location. http://codereview.adblockplus.org/10879048/diff/1/build_release.py File build_release.py (right): http://codereview.adblockplus.org/10879048/diff/1/build_release.py#newcode62 build_release.py:62: os.path.join(basedir, "build", "x64", "adblockplusie-%s-en-us-x64.msi" % version)) I would redo the batch file as a Makefile rather than as a python script. I've used the batch file regularly to do whole builds just for testing the installer, but (for example) the requirement for signing always generates errors that I have to manually ensure aren't fatal. In other words, there are two reasons to do a full build like this. One is to build for release, the other is to build all the assets for testing installers. This is best done with a Makefile so that we can specify multiple targets. I can write the Makefile, and we can go with this script for now. We'll likely need to keep a bit of the python script that invokes 'hg' to grab the current version number, but as I mentioned in another comment I don't need the release version number to test the installer. http://codereview.adblockplus.org/10879048/diff/1/src/engine/Updater.cpp File src/engine/Updater.cpp (right): http://codereview.adblockplus.org/10879048/diff/1/src/engine/Updater.cpp#newc... src/engine/Updater.cpp:123: UINT result = ::MsiInstallProductW(tempFile.c_str(), L"ACTION=INSTALL INSTALLUILEVEL=2 REINSTALL=ALL REINSTALLMODE=vomus MSIENFORCEUPGRADECOMPONENTRULES=1"); We can't always use minor upgrades for all upgrades, just some of them. I don't know at this point if this syntax will support major upgrades or not. See http://msdn.microsoft.com/en-us/library/aa370037%28v=vs.85%29.aspx . From that page "A minor upgrade can be used to add new features and components but cannot reorganize the feature-component tree." Moving the 32-bit DLL from the 64-bit Program Files to the 32-bit one is exactly such a modification. We'll need to use a major upgrade to make that change. It might be the case that this is the only one of those we'll ever need, but I doubt it. This code has already shipped out. If this doesn't work for major upgrade, it likely means that we'll need to make a (first) minor upgrade to change this so that we then make a (second) major upgrade.
I uploaded a change to the makefile. All other comments are hopefully addressed below. On 2013/06/11 14:45:18, Felix H. Dahlke wrote: > Just my rule of thumb, no destructuring when I only want one value. But I'll > leave this up to you :) I think _ is quite popular in Python for unused > variables. Then I'd rather leave it as is - that code was copied straight from buildtools and is the common approach there and in sitescripts. I won't mind changing it throughout our codebase eventually however. On 2013/06/11 15:41:46, Eric wrote: > For example, it's a simple matter to conditionally compile out the UI in the MSI > files. The only overhead for this is having two artifact MSI's (one with UI, one > without) in the build tree. The benefit is reduced delivery bandwidth. Please don't double the number of our installers for a minimal bandwidth improvement - keep it as simple as possible. What might be worth investigating is reducing the UI of the current installer if somehow possible, currently the update isn't completely headless. > The update > mechanism should deliver not just a file payload, but also a string, which for > Windows Installer can be used to hold the command line arguments. This will add lots of complexity to the server. Is 1.4.11 to 1.8.53 a major upgrade or a minor one? What about 1.6.44 to 1.8.53? Currently the server only needs to provide a static file listing the latest version per platform. You are proposing keeping track of all versions we release and the installer changes between them. IMHO that's going to be a lot more difficult than supporting a constant command line for all installers. > An additional benefit of such a change would be to allow > delivery of patch files. Frankly, I don't see us using patch files - that will also cause lots of management overhead if we are going to produce more than one release per year. http://codereview.adblockplus.org/10879048/diff/1/WixInstaller/Makefile File WixInstaller/Makefile (right): http://codereview.adblockplus.org/10879048/diff/1/WixInstaller/Makefile#newco... WixInstaller/Makefile:20: !error VERSION parameter is required nmake VERSION=0.8.63333 doesn't look too complicated to me. But sure, we can have VERSION default to 99.9 if you prefer. http://codereview.adblockplus.org/10879048/diff/1/WixInstaller/adblockplusie.wxs File WixInstaller/adblockplusie.wxs (right): http://codereview.adblockplus.org/10879048/diff/1/WixInstaller/adblockplusie.... WixInstaller/adblockplusie.wxs:32: <?define Product_ID="3caab5a9-43a7-4a57-8373-b7d0952b709c" ?> We can just change the product ID when we implement changes that require it. I don't really see a need to generated IDs in advance. http://codereview.adblockplus.org/10879048/diff/1/build_release.py File build_release.py (right): http://codereview.adblockplus.org/10879048/diff/1/build_release.py#newcode62 build_release.py:62: os.path.join(basedir, "build", "x64", "adblockplusie-%s-en-us-x64.msi" % version)) On 2013/06/11 15:41:46, Eric wrote: > I would redo the batch file as a Makefile rather than as a python script. I would definitely be opposed to that - a Makefile is very inflexible (in particular, getting the version number from Version.h should be difficult) and hard to maintain. Note in particular how nmake is dependent on the current work directory. We have a dependency on Python anyway (via libadblockplus and gyp), so we should use it. > but (for example) the requirement for signing always generates errors that I > have to manually ensure aren't fatal. Do you want to change that script then to make signing optional then? The script can show a warning if the signing file is missing, along the lines of: "Warning: Creating an unsigned build. Add the path to the signing file as a command line parameter to create a signed build." > This is best done with a Makefile so that > we can specify multiple targets. The handling of command line parameters for this script can easily be changed. http://codereview.adblockplus.org/10879048/diff/1/src/engine/Updater.cpp File src/engine/Updater.cpp (right): http://codereview.adblockplus.org/10879048/diff/1/src/engine/Updater.cpp#newc... src/engine/Updater.cpp:123: UINT result = ::MsiInstallProductW(tempFile.c_str(), L"ACTION=INSTALL INSTALLUILEVEL=2 REINSTALL=ALL REINSTALLMODE=vomus MSIENFORCEUPGRADECOMPONENTRULES=1"); On 2013/06/11 15:41:46, Eric wrote: > We can't always use minor upgrades for all upgrades, just some of them. Yes, most of them I hope. > I don't > know at this point if this syntax will support major upgrades or not. Not trivially. http://community.flexerasoftware.com/showthread.php?178239-msi-major-upgrade-... indicates however that REINSTALL and REINSTALLMODE parameters can be reset in the installer in case of a major upgrade. That's something to be tested before our first major upgrade. If it comes really bad then we will have to ship a minor update removing these parameters before we ship the major update. > See http://msdn.microsoft.com/en-us/library/aa370037%2528v=vs.85%2529.aspx . From > that page "A minor upgrade can be used to add new features and components but > cannot reorganize the feature-component tree." Moving the 32-bit DLL from the > 64-bit Program Files to the 32-bit one is exactly such a modification. There is a better explanation on http://msdn.microsoft.com/en-us/library/windows/desktop/aa370093%28v=vs.85%29.... My impression is that moving a component to a different location is fine, merely removing it is problematic. But that's something we will find out soon enough.
Looks like I misunderstood nmake documentation, previous change made settings VERSION parameter externally impossible.
LGTM All my concerns are longer-term and don't affect the immediate changes here. http://codereview.adblockplus.org/10879048/diff/1/WixInstaller/Makefile File WixInstaller/Makefile (right): http://codereview.adblockplus.org/10879048/diff/1/WixInstaller/Makefile#newco... WixInstaller/Makefile:20: !error VERSION parameter is required On 2013/06/12 09:46:34, Wladimir Palant wrote: > nmake VERSION=0.8.63333 doesn't look too complicated to me. But sure, we can > have VERSION default to 99.9 if you prefer. I've been bitten by typos on the command line already. As a result, I rather prefer things I can check out in the editor rather than hoping the command line history has what I need. http://codereview.adblockplus.org/10879048/diff/1/build_release.py File build_release.py (right): http://codereview.adblockplus.org/10879048/diff/1/build_release.py#newcode62 build_release.py:62: os.path.join(basedir, "build", "x64", "adblockplusie-%s-en-us-x64.msi" % version)) On 2013/06/12 09:46:34, Wladimir Palant wrote: > I would definitely be opposed to that - a Makefile is very inflexible (in > particular, getting the version number from Version.h should be difficult) and > hard to maintain. (as I mentioned in email) The incremental calculation part of 'make' is the thing I need the most. Not simple to do in a python script. http://codereview.adblockplus.org/10879048/diff/1/src/engine/Updater.cpp File src/engine/Updater.cpp (right): http://codereview.adblockplus.org/10879048/diff/1/src/engine/Updater.cpp#newc... src/engine/Updater.cpp:123: UINT result = ::MsiInstallProductW(tempFile.c_str(), L"ACTION=INSTALL INSTALLUILEVEL=2 REINSTALL=ALL REINSTALLMODE=vomus MSIENFORCEUPGRADECOMPONENTRULES=1"); I'm going to open a Discourse thread on this topic. I think we can handle this without the overhead you're worried about, but it relies on an msi.dll call I need to look up.
There is a default version number now and Felix changed the upgrade mechanism to major upgrades again so all concerns seem to be addressed - closing this. 
|
