Issue 3967 - Properly handle non-existing file-mappings

packager.py

 # This Source Code Form is subject to the terms of the Mozilla Public
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 # Note: These are the base functions common to all packagers, the actual
 # packagers are implemented in packagerGecko and packagerChrome.
 
 import sys
 import os
 import re
@@ skipping 73 matching lines @@
         self.includedFiles = includedFiles
         self.ignoredFiles = ignoredFiles
         self.process = process
 
     def __setitem__(self, key, value):
         if self.process:
             value = self.process(key, value)
         dict.__setitem__(self, key, value)
 
     def isIncluded(self, relpath):
+        return relpath.split('/')[0] in self.includedFiles
+
+    def isignored(self, relpath):

[kzar, 2017/07/17 09:37:40]

Nit: Should it be is_ignored? (Kind of a question for Sebastian / Vasily)

[tlucas, 2017/07/17 12:29:48]

for the sake of readability i would agree. Acknowledged

         parts = relpath.split('/')
-        if not parts[0] in self.includedFiles:
-            return False
-        for part in parts:
-            if part in self.ignoredFiles:
-                return False
-        return True
+        return any([part in self.ignoredFiles for part in parts])
 
     def read(self, path, relpath='', skip=()):
         if os.path.isdir(path):
             for file in os.listdir(path):
                 name = relpath + ('/' if relpath != '' else '') + file
-                if name not in skip and self.isIncluded(name):
+                if (name not in skip
+                        and self.isIncluded(name) and not self.isignored(name)):

[kzar, 2017/07/17 09:37:40]

The indentation looks a bit funky here, but I'm rusty with Python. Did you check
that linting passes?

[tlucas, 2017/07/17 12:29:48]

flake8 didn't nag about this at all - and from what i understood, Sebastian
wanted exactly that (PEP-8 adherence)

[kzar, 2017/07/17 12:32:52]

Fair enough, let's see what he thinks.

[Sebastian Noack, 2017/07/17 14:02:31]

The following two styles are acceptable according to PEP-8
(https://www.python.org/dev/peps/pep-0008/#indentation):

  if (this_is_one_thing and
      that_is_another_thing):
      do_something()

  if (this_is_one_thing
          and that_is_another_thing):
      do_something()

I personally, prefer the former, which is also more consistent with what we have
in our existing code base.

Another option is to use some temporary variables in order to avoid wrapping in
the first place:

  included = self.isIncluded(name) and not self.isignored(name)
  if name not in skip and included:
      ...

[tlucas, 2017/07/17 16:34:12]

As discussed, outsourcing the logic to a variable imho seems to be the most
elegant solution for this, Done

                     self.read(os.path.join(path, file), name, skip)
         else:
             with open(path, 'rb') as file:
                 if relpath in self:
                     print >>sys.stderr, 'Warning: File %s defined multiple times' % relpath
                 self[relpath] = file.read()
 
     def readMappedFiles(self, mappings):
         for item in mappings:
             target, source = item
 
-            # Make sure the file is inside an included directory
-            if '/' in target and not self.isIncluded(target):
+            # Adhere to given ignorance

[kzar, 2017/07/17 09:37:40]

I'm not sure this comment adds much, maybe just remove it?

[tlucas, 2017/07/17 12:29:48]

Acknowledged.

+            if '/' in target and self.isignored(target):
                 continue
+
             parts = source.split('/')
             path = os.path.join(os.path.dirname(item.source), *parts)
             if os.path.exists(path):
                 self.read(path, target)
             else:
                 print >>sys.stderr, "Warning: Mapped file %s doesn't exist" % source
 
     def preprocess(self, filenames, params={}):
         import jinja2
         env = jinja2.Environment()
 
         for filename in filenames:
             env.autoescape = os.path.splitext(filename)[1].lower() in ('.html', '.xml')
             template = env.from_string(self[filename].decode('utf-8'))
             self[filename] = template.render(params).encode('utf-8')
 
     def zip(self, outFile, sortKey=None, compression=zipfile.ZIP_DEFLATED):
         with zipfile.ZipFile(outFile, 'w', compression) as zf:
             for name in sorted(self, key=sortKey):
                 zf.writestr(name, self[name])
 
     def zipToString(self, sortKey=None):
         buffer = StringIO()
         self.zip(buffer, sortKey=sortKey)
         return buffer.getvalue()