Issue 4340 - Drop dependency on external xar tool

Issue 4340 - Drop dependency on external xar tool

Repository: hg.adblockplus.org/buildtools

[Wladimir Palant, 2016-08-17 10:07:21]

As discussed, I switched to using templates for XML generation here - this made
the code much easier to follow.

[Sebastian Noack, 2016-08-17 12:53:45]

https://codereview.adblockplus.org/29349885/diff/29349899/xarfile.py
File xarfile.py (right):

https://codereview.adblockplus.org/29349885/diff/29349899/xarfile.py#newcode28
xarfile.py:28: match = re.search(PRIVATE_KEY_REGEXP, data, re.S)
You can leave the with block after the data has been read.

However, as mentioned on the other review, it seems simpler (less duplication),
if you just combine those two function:

  def read_bio(keyfile):
      with open(keyfile, 'r') as file:
          data = file.read()

      certs = []
      key = None
      for match in re.finditer(r'-+BEGIN (.*?)-+(.*?)-+END \1-+', data, re.S):
          what, content = match.groups()
          if what == 'CERTIFICATE':
              certs.append(content)
          if what == 'PRIVATE KEY':
              key = RSA.importKey(content)

      if not key:
          raise Exception('Cound not find private key in file')

      return certs, key

https://codereview.adblockplus.org/29349885/diff/29349899/xarfile.py#newcode104
xarfile.py:104: checksum_length = len(get_checksum(''))
No need to hash any (empty) data to get the digest size:

  SHA.SHA1Hash.digest_size

https://codereview.adblockplus.org/29349885/diff/29349899/xarfile.py#newcode132
xarfile.py:132: header = struct.pack('>IHHQQI', XAR_HEADER_MAGIC,
XAR_HEADER_SIZE,
Note that you could avoid hard-coding the header size:

  HEADER_STRUCT = struct.Struct('>IHHQQI')
  ...
  header = HEADER_STRUCT.pack(XAR_HEADER_MAGIC, HEADER_STRUCT.size, ...)

https://codereview.adblockplus.org/29349885/diff/29349899/xartoc.xml.tmpl
File xartoc.xml.tmpl (right):

https://codereview.adblockplus.org/29349885/diff/29349899/xartoc.xml.tmpl#new...
xartoc.xml.tmpl:9: {% macro file(data) %}
Why a macro? Couldn't you simply nest the code below?

[Wladimir Palant, 2016-08-17 14:11:41]

https://codereview.adblockplus.org/29349885/diff/29349899/xarfile.py
File xarfile.py (right):

https://codereview.adblockplus.org/29349885/diff/29349899/xarfile.py#newcode28
xarfile.py:28: match = re.search(PRIVATE_KEY_REGEXP, data, re.S)
On 2016/08/17 12:53:45, Sebastian Noack wrote:
> You can leave the with block after the data has been read.
> 
> However, as mentioned on the other review, it seems simpler (less
duplication),
> if you just combine those two function:
> 
>   def read_bio(keyfile):
>       with open(keyfile, 'r') as file:
>           data = file.read()
> 
>       certs = []
>       key = None
>       for match in re.finditer(r'-+BEGIN (.*?)-+(.*?)-+END \1-+', data, re.S):
>           what, content = match.groups()
>           if what == 'CERTIFICATE':
>               certs.append(content)
>           if what == 'PRIVATE KEY':
>               key = RSA.importKey(content)
> 
>       if not key:
>           raise Exception('Cound not find private key in file')
> 
>       return certs, key

Done.

https://codereview.adblockplus.org/29349885/diff/29349899/xarfile.py#newcode104
xarfile.py:104: checksum_length = len(get_checksum(''))
On 2016/08/17 12:53:45, Sebastian Noack wrote:
> No need to hash any (empty) data to get the digest size:
> 
>   SHA.SHA1Hash.digest_size

Strictly speaking - no, it's not necessary. However, I'd rather not spread
implementation details throughout the code. It doesn't have to be SHA1 and I
strongly suspect that at some point in the future it longer will be.

https://codereview.adblockplus.org/29349885/diff/29349899/xarfile.py#newcode132
xarfile.py:132: header = struct.pack('>IHHQQI', XAR_HEADER_MAGIC,
XAR_HEADER_SIZE,
On 2016/08/17 12:53:45, Sebastian Noack wrote:
> Note that you could avoid hard-coding the header size:
> 
>   HEADER_STRUCT = struct.Struct('>IHHQQI')
>   ...
>   header = HEADER_STRUCT.pack(XAR_HEADER_MAGIC, HEADER_STRUCT.size, ...)

Done.

[Sebastian Noack, 2016-08-17 18:41:34]

https://codereview.adblockplus.org/29349885/diff/29349899/xartoc.xml.tmpl
File xartoc.xml.tmpl (right):

https://codereview.adblockplus.org/29349885/diff/29349899/xartoc.xml.tmpl#new...
xartoc.xml.tmpl:9: {% macro file(data) %}
On 2016/08/17 12:53:45, Sebastian Noack wrote:
> Why a macro? Couldn't you simply nest the code below? 

What is about this comment?

https://codereview.adblockplus.org/29349885/diff/29349945/xarfile.py
File xarfile.py (right):

https://codereview.adblockplus.org/29349885/diff/29349945/xarfile.py#newcode34
xarfile.py:34: if key is None:
See
https://codereview.adblockplus.org/29349869/diff/29349942/packagerSafari.py#n...

https://codereview.adblockplus.org/29349885/diff/29349945/xarfile.py#newcode129
xarfile.py:129: header = XAR_HEADER.pack(XAR_HEADER_MAGIC, XAR_HEADER.size,
Nit: I'd personally skip the variable:

  file.write(XAR_HEADER.pack(...))

[Wladimir Palant, 2016-08-17 18:52:18]

https://codereview.adblockplus.org/29349885/diff/29349899/xartoc.xml.tmpl
File xartoc.xml.tmpl (right):

https://codereview.adblockplus.org/29349885/diff/29349899/xartoc.xml.tmpl#new...
xartoc.xml.tmpl:9: {% macro file(data) %}
On 2016/08/17 12:53:45, Sebastian Noack wrote:
> Why a macro? Couldn't you simply nest the code below? 

How so? It's recursive.

[Sebastian Noack, 2016-08-17 18:58:46]

https://codereview.adblockplus.org/29349885/diff/29349899/xartoc.xml.tmpl
File xartoc.xml.tmpl (right):

https://codereview.adblockplus.org/29349885/diff/29349899/xartoc.xml.tmpl#new...
xartoc.xml.tmpl:9: {% macro file(data) %}
On 2016/08/17 18:52:18, Wladimir Palant wrote:
> On 2016/08/17 12:53:45, Sebastian Noack wrote:
> > Why a macro? Couldn't you simply nest the code below? 
> 
> How so? It's recursive.

Sorry, you are right. (But mind the other two comments?)

[Wladimir Palant, 2016-08-17 19:22:29]

https://codereview.adblockplus.org/29349885/diff/29349945/xarfile.py
File xarfile.py (right):

https://codereview.adblockplus.org/29349885/diff/29349945/xarfile.py#newcode34
xarfile.py:34: if key is None:
On 2016/08/17 18:41:33, Sebastian Noack wrote:
> See
>
https://codereview.adblockplus.org/29349869/diff/29349942/packagerSafari.py#n...

Done.

https://codereview.adblockplus.org/29349885/diff/29349945/xarfile.py#newcode129
xarfile.py:129: header = XAR_HEADER.pack(XAR_HEADER_MAGIC, XAR_HEADER.size,
On 2016/08/17 18:41:34, Sebastian Noack wrote:
> Nit: I'd personally skip the variable:
> 
>   file.write(XAR_HEADER.pack(...))

Done.

[Sebastian Noack, 2016-08-17 19:26:27]

LGTM