Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
43273003:
Add script for generating a windows installer from an archived editor bundle. (Closed)
|
Created:
7 years, 1 month ago by ricow1 Modified:
7 years, 1 month ago Reviewers:
kustermann CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionAdd script for generating a windows installer from an archived editor bundle.
A few general notes:
We always generate an installer that will do a full upgrade, i.e., uninstall the old installer and reinstall the new one.
We need wix 3.6 or above, I will add a dependency for this to our bot that will run this. For local testing you can either put wix in your path or pass in the location
We use * for both product id and component component guids (i.e., wix will generate them on the fly. Since we don't track files across versions and we always do a full update this is fine. If we want to change to patch releases we need to change that functionality.
Committed: https://code.google.com/p/dart/source/detail?r=29348
Patch Set 1 #Patch Set 2 : #
Total comments: 56
Patch Set 3 : #Messages
Total messages: 3 (0 generated)
lgtm with (mostly style) comments I think you could reduce it to less than 300 lines :-) https://chromiumcodereview.appspot.com/43273003/diff/30001/tools/create_windo... File tools/create_windows_installer.py (right): https://chromiumcodereview.appspot.com/43273003/diff/30001/tools/create_windo... tools/create_windows_installer.py:13: # [--wix_bin <wix_bin_location>] Add "--print_wxs" as well here. https://chromiumcodereview.appspot.com/43273003/diff/30001/tools/create_windo... tools/create_windows_installer.py:15: # This script assumes that wix is either in path or passed in as --wis_bin. wis_bin -> wix_bin https://chromiumcodereview.appspot.com/43273003/diff/30001/tools/create_windo... tools/create_windows_installer.py:64: def GetVersion(version): GetVersion -> GetMicrosoftProductVersion https://chromiumcodereview.appspot.com/43273003/diff/30001/tools/create_windo... tools/create_windows_installer.py:65: split_string = version.split('.') split_string -> version_parts https://chromiumcodereview.appspot.com/43273003/diff/30001/tools/create_windo... tools/create_windows_installer.py:69: # Convert all to int to check that they are integers Remove comment, that's obvious. https://chromiumcodereview.appspot.com/43273003/diff/30001/tools/create_windo... tools/create_windows_installer.py:73: patch = int(split_string[3]) (major, minor, build, patch) = map(int, version_parts) https://chromiumcodereview.appspot.com/43273003/diff/30001/tools/create_windo... tools/create_windows_installer.py:90: xml_content.append(''.join(to_append)) Make it simpler, something like: def Append(data, new_line=True): xml_content.append((' ' * current_indentation) + data + ('\n' if new_line else '')) https://chromiumcodereview.appspot.com/43273003/diff/30001/tools/create_windo... tools/create_windows_installer.py:97: xml_content.append(data) Maybe: xml_content.append(data + ('\n' if new_line else '')) https://chromiumcodereview.appspot.com/43273003/diff/30001/tools/create_windo... tools/create_windows_installer.py:100: Append('<!--%s-->' % comment) Indentation https://chromiumcodereview.appspot.com/43273003/diff/30001/tools/create_windo... tools/create_windows_installer.py:116: return ''.join(to_append) return ' ' * len(indentation_string) + str https://chromiumcodereview.appspot.com/43273003/diff/30001/tools/create_windo... tools/create_windows_installer.py:127: GUID_PREFETCH_SIZE = 200 remove this GUID code, since you don't use it. https://chromiumcodereview.appspot.com/43273003/diff/30001/tools/create_windo... tools/create_windows_installer.py:163: return UPGRADE_CODE You could remove these two methods and use instance variables instead. def __init__(self): ... self.product_name = 'Dart Editor' self.manufacturer = 'Google Inc.' self.upgrade_code = UPGRADE_CODE and use these below. https://chromiumcodereview.appspot.com/43273003/diff/30001/tools/create_windo... tools/create_windows_installer.py:175: product), You always create these things manually, you could have a XmlTag (maybe a base class) class which you pass in a TagName and a map of properties, like this with XmlTag('Product', { 'Version' : ..., 'Name' : ..., }): # do som So you don't have these Append/TagIndent/AppendRaw calls all over. But you don't have to do it. https://chromiumcodereview.appspot.com/43273003/diff/30001/tools/create_windo... tools/create_windows_installer.py:186: IncreaseIndentation() You could extract the <Wix ...></Wix> to a different class -- no need for these methods here. https://chromiumcodereview.appspot.com/43273003/diff/30001/tools/create_windo... tools/create_windows_installer.py:201: self.close_wix() Move these three methods up (constructors should always be on the top of a class). https://chromiumcodereview.appspot.com/43273003/diff/30001/tools/create_windo... tools/create_windows_installer.py:238: def __exit__(self, *_): Sometimes you have "__enter__" before "__exit__" and sometimes after, you could be consistent. https://chromiumcodereview.appspot.com/43273003/diff/30001/tools/create_windo... tools/create_windows_installer.py:284: Append(TagIndent('Key="Software\Microsoft\%s"' % location, registry)) Make it a raw string or two \\. https://chromiumcodereview.appspot.com/43273003/diff/30001/tools/create_windo... tools/create_windows_installer.py:317: # We assume 1 file per component, a File is always a KeyPath Nobody knows what a 'KeyPath' is, you could add a comment about that somewhere. https://chromiumcodereview.appspot.com/43273003/diff/30001/tools/create_windo... tools/create_windows_installer.py:327: if utils.IsWindows(): That is strange, I'd remove this and put an if sys.platform != 'win32': raise Exception("This script can only be run on windows") at the beginning of 'main()' https://chromiumcodereview.appspot.com/43273003/diff/30001/tools/create_windo... tools/create_windows_installer.py:333: wxs_file = os.path.join(temp_dir, 'installer.wxs') Indentation https://chromiumcodereview.appspot.com/43273003/diff/30001/tools/create_windo... tools/create_windows_installer.py:336: print 'Saving wxs output to: %s' % wxs_file This comment is useless, since after running this script, the temporary directory will no longer exist and therefore the wxs_file doesn't exist either (so people can't actually look at that file) https://chromiumcodereview.appspot.com/43273003/diff/30001/tools/create_windo... tools/create_windows_installer.py:346: subprocess.call('%s %s -o %s' % (candle_bin, wxs_file, wixobj_file)) Please use 'subprocess.check_call' or veryfiy that exitcode is 0 https://chromiumcodereview.appspot.com/43273003/diff/30001/tools/create_windo... tools/create_windows_installer.py:348: subprocess.call('%s %s -o %s' % (light_bin, wixobj_file, msi_file)) msi_file -> options.msi_location + remove msi_file above https://chromiumcodereview.appspot.com/43273003/diff/30001/tools/create_windo... tools/create_windows_installer.py:356: version = GetVersion(options.version) You could validate here that the zip file exists. https://chromiumcodereview.appspot.com/43273003/diff/30001/tools/create_windo... tools/create_windows_installer.py:364: root_dir = 'RootInstallDir' You could actually remove this variable, and replace the two occurences by 'RootInstallDir' then it becomes clear that this is a special string. https://chromiumcodereview.appspot.com/43273003/diff/30001/tools/create_windo... tools/create_windows_installer.py:376: InstallFiles(os.path.join(temp_dir, 'dart')) Maybe rename to ListFiles? https://chromiumcodereview.appspot.com/43273003/diff/30001/tools/create_windo... tools/create_windows_installer.py:385: RegistryEntry('DartEditor') Add a comment explaining how these three calls fit together. https://chromiumcodereview.appspot.com/43273003/diff/30001/tools/create_windo... tools/create_windows_installer.py:387: with Feature(): Add a comment, like "We have only one feature and that consist of all the files=components we have listed above"
https://chromiumcodereview.appspot.com/43273003/diff/30001/tools/create_windo... File tools/create_windows_installer.py (right): https://chromiumcodereview.appspot.com/43273003/diff/30001/tools/create_windo... tools/create_windows_installer.py:13: # [--wix_bin <wix_bin_location>] On 2013/10/25 09:39:07, kustermann wrote: > Add "--print_wxs" as well here. Done. https://chromiumcodereview.appspot.com/43273003/diff/30001/tools/create_windo... tools/create_windows_installer.py:15: # This script assumes that wix is either in path or passed in as --wis_bin. On 2013/10/25 09:39:07, kustermann wrote: > wis_bin -> wix_bin Done. https://chromiumcodereview.appspot.com/43273003/diff/30001/tools/create_windo... tools/create_windows_installer.py:64: def GetVersion(version): On 2013/10/25 09:39:07, kustermann wrote: > GetVersion -> GetMicrosoftProductVersion Done. https://chromiumcodereview.appspot.com/43273003/diff/30001/tools/create_windo... tools/create_windows_installer.py:65: split_string = version.split('.') On 2013/10/25 09:39:07, kustermann wrote: > split_string -> version_parts Done. https://chromiumcodereview.appspot.com/43273003/diff/30001/tools/create_windo... tools/create_windows_installer.py:69: # Convert all to int to check that they are integers On 2013/10/25 09:39:07, kustermann wrote: > Remove comment, that's obvious. Done. https://chromiumcodereview.appspot.com/43273003/diff/30001/tools/create_windo... tools/create_windows_installer.py:73: patch = int(split_string[3]) On 2013/10/25 09:39:07, kustermann wrote: > (major, minor, build, patch) = map(int, version_parts) Done. https://chromiumcodereview.appspot.com/43273003/diff/30001/tools/create_windo... tools/create_windows_installer.py:90: xml_content.append(''.join(to_append)) On 2013/10/25 09:39:07, kustermann wrote: > Make it simpler, something like: > > def Append(data, new_line=True): > xml_content.append((' ' * current_indentation) + data + ('\n' if new_line > else '')) Done. https://chromiumcodereview.appspot.com/43273003/diff/30001/tools/create_windo... tools/create_windows_installer.py:97: xml_content.append(data) On 2013/10/25 09:39:07, kustermann wrote: > Maybe: > xml_content.append(data + ('\n' if new_line else '')) Done. https://chromiumcodereview.appspot.com/43273003/diff/30001/tools/create_windo... tools/create_windows_installer.py:100: Append('<!--%s-->' % comment) On 2013/10/25 09:39:07, kustermann wrote: > Indentation Done. https://chromiumcodereview.appspot.com/43273003/diff/30001/tools/create_windo... tools/create_windows_installer.py:116: return ''.join(to_append) On 2013/10/25 09:39:07, kustermann wrote: > return ' ' * len(indentation_string) + str Done. https://chromiumcodereview.appspot.com/43273003/diff/30001/tools/create_windo... tools/create_windows_installer.py:127: GUID_PREFETCH_SIZE = 200 On 2013/10/25 09:39:07, kustermann wrote: > remove this GUID code, since you don't use it. Done. https://chromiumcodereview.appspot.com/43273003/diff/30001/tools/create_windo... tools/create_windows_installer.py:163: return UPGRADE_CODE On 2013/10/25 09:39:07, kustermann wrote: > You could remove these two methods and use instance variables instead. > def __init__(self): > ... > self.product_name = 'Dart Editor' > self.manufacturer = 'Google Inc.' > self.upgrade_code = UPGRADE_CODE > > and use these below. Done. https://chromiumcodereview.appspot.com/43273003/diff/30001/tools/create_windo... tools/create_windows_installer.py:175: product), On 2013/10/25 09:39:07, kustermann wrote: > You always create these things manually, you could have a XmlTag (maybe a base > class) class which you pass in a TagName and a map of properties, like this > > with XmlTag('Product', { > 'Version' : ..., > 'Name' : ..., > }): > # do som > > So you don't have these Append/TagIndent/AppendRaw calls all over. > > > But you don't have to do it. Thought about it but decided against it to make this explicit https://chromiumcodereview.appspot.com/43273003/diff/30001/tools/create_windo... tools/create_windows_installer.py:186: IncreaseIndentation() On 2013/10/25 09:39:07, kustermann wrote: > You could extract the <Wix ...></Wix> to a different class -- no need for these > methods here. That would be 2 extra line, I really can't do that :-) https://chromiumcodereview.appspot.com/43273003/diff/30001/tools/create_windo... tools/create_windows_installer.py:201: self.close_wix() On 2013/10/25 09:39:07, kustermann wrote: > Move these three methods up (constructors should always be on the top of a > class). Done. https://chromiumcodereview.appspot.com/43273003/diff/30001/tools/create_windo... tools/create_windows_installer.py:238: def __exit__(self, *_): On 2013/10/25 09:39:07, kustermann wrote: > Sometimes you have "__enter__" before "__exit__" and sometimes after, you could > be consistent. Done. https://chromiumcodereview.appspot.com/43273003/diff/30001/tools/create_windo... tools/create_windows_installer.py:284: Append(TagIndent('Key="Software\Microsoft\%s"' % location, registry)) On 2013/10/25 09:39:07, kustermann wrote: > Make it a raw string or two \\. Done. https://chromiumcodereview.appspot.com/43273003/diff/30001/tools/create_windo... tools/create_windows_installer.py:317: # We assume 1 file per component, a File is always a KeyPath On 2013/10/25 09:39:07, kustermann wrote: > Nobody knows what a 'KeyPath' is, you could add a comment about that somewhere. Done. https://chromiumcodereview.appspot.com/43273003/diff/30001/tools/create_windo... tools/create_windows_installer.py:327: if utils.IsWindows(): On 2013/10/25 09:39:07, kustermann wrote: > That is strange, I'd remove this and put an > > if sys.platform != 'win32': > raise Exception("This script can only be run on windows") > > at the beginning of 'main()' Done. https://chromiumcodereview.appspot.com/43273003/diff/30001/tools/create_windo... tools/create_windows_installer.py:333: wxs_file = os.path.join(temp_dir, 'installer.wxs') On 2013/10/25 09:39:07, kustermann wrote: > Indentation Done. https://chromiumcodereview.appspot.com/43273003/diff/30001/tools/create_windo... tools/create_windows_installer.py:336: print 'Saving wxs output to: %s' % wxs_file On 2013/10/25 09:39:07, kustermann wrote: > This comment is useless, since after running this script, the temporary > directory will no longer exist and therefore the wxs_file doesn't exist either > (so people can't actually look at that file) I disagree, it make you be able to see the progress, the fact that I print the path seems sort of useless, but it does not hurt (I could for example see it that path was bogus) https://chromiumcodereview.appspot.com/43273003/diff/30001/tools/create_windo... tools/create_windows_installer.py:346: subprocess.call('%s %s -o %s' % (candle_bin, wxs_file, wixobj_file)) On 2013/10/25 09:39:07, kustermann wrote: > Please use 'subprocess.check_call' or veryfiy that exitcode is 0 Done. https://chromiumcodereview.appspot.com/43273003/diff/30001/tools/create_windo... tools/create_windows_installer.py:348: subprocess.call('%s %s -o %s' % (light_bin, wixobj_file, msi_file)) On 2013/10/25 09:39:07, kustermann wrote: > msi_file -> options.msi_location > + remove msi_file above Done. https://chromiumcodereview.appspot.com/43273003/diff/30001/tools/create_windo... tools/create_windows_installer.py:356: version = GetVersion(options.version) On 2013/10/25 09:39:07, kustermann wrote: > You could validate here that the zip file exists. Done. https://chromiumcodereview.appspot.com/43273003/diff/30001/tools/create_windo... tools/create_windows_installer.py:364: root_dir = 'RootInstallDir' On 2013/10/25 09:39:07, kustermann wrote: > You could actually remove this variable, and replace the two occurences by > 'RootInstallDir' then it becomes clear that this is a special string. ok https://chromiumcodereview.appspot.com/43273003/diff/30001/tools/create_windo... tools/create_windows_installer.py:376: InstallFiles(os.path.join(temp_dir, 'dart')) On 2013/10/25 09:39:07, kustermann wrote: > Maybe rename to ListFiles? Done. https://chromiumcodereview.appspot.com/43273003/diff/30001/tools/create_windo... tools/create_windows_installer.py:385: RegistryEntry('DartEditor') On 2013/10/25 09:39:07, kustermann wrote: > Add a comment explaining how these three calls fit together. Done. https://chromiumcodereview.appspot.com/43273003/diff/30001/tools/create_windo... tools/create_windows_installer.py:387: with Feature(): On 2013/10/25 09:39:07, kustermann wrote: > Add a comment, like "We have only one feature and that consist of all the > files=components we have listed above" Done. |
