Issue 3498 - Improve file mapping / skipping logic

Issue 3498 - Improve file mapping / skipping logic

[kzar, 2016-01-12 09:57:55]

Patch Set 1

[Wladimir Palant, 2016-01-12 12:16:33]

https://codereview.adblockplus.org/29333368/diff/29333369/packager.py
File packager.py (right):

https://codereview.adblockplus.org/29333368/diff/29333369/packager.py#newcode96
packager.py:96: self.read(os.path.join(path, file), name, skip)
No, skip as implemented right now is only meant to apply on top level. With this
change we will skip the chrome directory in any subdirectory for Gecko builds.
Maybe it will work correctly if the check above is changed into |name not in
skip|.

https://codereview.adblockplus.org/29333368/diff/29333369/packagerChrome.py
File packagerChrome.py (right):

https://codereview.adblockplus.org/29333368/diff/29333369/packagerChrome.py#n...
packagerChrome.py:332: skip = []
Please don't introduce a new option for that.

  skip = metadata.options('mapping') if metadata.has_section('mapping') else []

Note that the same logic has to be added to the other packagers as well.

https://codereview.adblockplus.org/29333368/diff/29333369/packagerChrome.py#n...
packagerChrome.py:336: files.read(baseDir, skip=skip)
Things become inconsistent if you move this up, other packagers read mapped
files first. While I'm not sure why we did it like this, the order should be
changed everywhere or nowhere. I'm leaning towards nowhere, there doesn't seem
to be a good reason for it.

[kzar, 2016-01-12 13:09:48]

Patch Set 2 : Addressed Wladimir's feedback

(I've again tested that builds for AdBlock and Adblock Plus don't change with
these changes and that the warnings when building AdBlock are suppressed.)

https://codereview.adblockplus.org/29333368/diff/29333369/packager.py
File packager.py (right):

https://codereview.adblockplus.org/29333368/diff/29333369/packager.py#newcode96
packager.py:96: self.read(os.path.join(path, file), name, skip)
On 2016/01/12 12:16:33, Wladimir Palant wrote:
> No, skip as implemented right now is only meant to apply on top level. With
this
> change we will skip the chrome directory in any subdirectory for Gecko builds.
> Maybe it will work correctly if the check above is changed into |name not in
> skip|.

Done.

https://codereview.adblockplus.org/29333368/diff/29333369/packagerChrome.py
File packagerChrome.py (right):

https://codereview.adblockplus.org/29333368/diff/29333369/packagerChrome.py#n...
packagerChrome.py:332: skip = []
On 2016/01/12 12:16:33, Wladimir Palant wrote:
> Please don't introduce a new option for that.
> 
>   skip = metadata.options('mapping') if metadata.has_section('mapping') else
[]
> 
> Note that the same logic has to be added to the other packagers as well.

Ah, much better idea, Done.

https://codereview.adblockplus.org/29333368/diff/29333369/packagerChrome.py#n...
packagerChrome.py:336: files.read(baseDir, skip=skip)
On 2016/01/12 12:16:33, Wladimir Palant wrote:
> Things become inconsistent if you move this up, other packagers read mapped
> files first. While I'm not sure why we did it like this, the order should be
> changed everywhere or nowhere. I'm leaning towards nowhere, there doesn't seem
> to be a good reason for it.

Yes I agree and it no longer matters now that we automatically skip mapped files
anyway.

[Wladimir Palant, 2016-01-12 13:17:06]

https://codereview.adblockplus.org/29333368/diff/29333376/packagerGecko.py
File packagerGecko.py (right):

https://codereview.adblockplus.org/29333368/diff/29333376/packagerGecko.py#ne...
packagerGecko.py:350: files.read(path, 'chrome/%s' % name)
Same skip parameter has to be specified here as well. So I'd do the following
above:

  skip.append('chrome')

And then specify skip=skip in the files.read() calls here and above.

[kzar, 2016-01-12 13:26:03]

Patch Set 3 : Pass the skip parameter for both Gecko Files.read calls

https://codereview.adblockplus.org/29333368/diff/29333376/packagerGecko.py
File packagerGecko.py (right):

https://codereview.adblockplus.org/29333368/diff/29333376/packagerGecko.py#ne...
packagerGecko.py:350: files.read(path, 'chrome/%s' % name)
On 2016/01/12 13:17:06, Wladimir Palant wrote:
> Same skip parameter has to be specified here as well. So I'd do the following
> above:
> 
>   skip.append('chrome')
> 
> And then specify skip=skip in the files.read() calls here and above.

Done.

[Wladimir Palant, 2016-01-12 13:40:02]

LGTM but please have Sebastian look at this as well - I'm not sure he agrees
with the approach.

[Sebastian Noack, 2016-01-12 15:53:31]

I wonder, why we don't simply call readMappedFiles() after read() and remove the
code logging the warning from readMappedFiles(). Wouldn't that do essentially
the same, but by removing instead of adding complexity?

https://codereview.adblockplus.org/29333368/diff/29333381/packager.py
File packager.py (right):

https://codereview.adblockplus.org/29333368/diff/29333381/packager.py#newcode95
packager.py:95: if (skip == None or name not in skip) and self.isIncluded(name):
Nit: While changing this line anyway |is None|. Or perhaps make the argument
default to () eliminating this check.

https://codereview.adblockplus.org/29333368/diff/29333381/packagerChrome.py
File packagerChrome.py (left):

https://codereview.adblockplus.org/29333368/diff/29333381/packagerChrome.py#o...
packagerChrome.py:332: if metadata.has_section('mapping'):
How about unifying the logic here:

try:
  mappings = metadata.items('mapping')
except NoSectionError:
  mappings = []

files.readMappedFiles(mappings)
files.read(baseDir, skip=[opt for opt, _ in mappings])

[kzar, 2016-01-12 17:31:37]

Patch Set 3 : Pass the skip parameter for both Gecko Files.read calls

(Note I haven't tested everything again for this change, will do at the end as
it's time consuming.)

I think this way is preferable because we keep the warning. Sure I can't see how
the warning would be triggered now but it could very well help us avoid a
mistake in the future.

https://codereview.adblockplus.org/29333368/diff/29333381/packager.py
File packager.py (right):

https://codereview.adblockplus.org/29333368/diff/29333381/packager.py#newcode95
packager.py:95: if (skip == None or name not in skip) and self.isIncluded(name):
On 2016/01/12 15:53:31, Sebastian Noack wrote:
> Nit: While changing this line anyway |is None|. Or perhaps make the argument
> default to () eliminating this check.

Done.

https://codereview.adblockplus.org/29333368/diff/29333381/packagerChrome.py
File packagerChrome.py (left):

https://codereview.adblockplus.org/29333368/diff/29333381/packagerChrome.py#o...
packagerChrome.py:332: if metadata.has_section('mapping'):
On 2016/01/12 15:53:31, Sebastian Noack wrote:
> How about unifying the logic here:
> 
> try:
>   mappings = metadata.items('mapping')
> except NoSectionError:
>   mappings = []
> 
> files.readMappedFiles(mappings)
> files.read(baseDir, skip=[opt for opt, _ in mappings])

I've done it slightly differently but good point, Done.

[kzar, 2016-01-12 17:33:56]

Patch Set 5 : Remove redundant parenthesis

[Sebastian Noack, 2016-01-12 17:57:44]

On 2016/01/12 17:31:37, kzar wrote:
> Patch Set 3 : Pass the skip parameter for both Gecko Files.read calls
> 
> (Note I haven't tested everything again for this change, will do at the end as
> it's time consuming.)
> 
> I think this way is preferable because we keep the warning. Sure I can't see
how
> the warning would be triggered now but it could very well help us avoid a
> mistake in the future.

How is dead code a preferable thing? ;) But it's probably not too important. So
LGTN.

[Sebastian Noack, 2016-01-13 10:39:35]

LGTN = LGTM

[kzar, 2016-01-13 11:11:56]

Patch Set 6 : Fixed typo in Gecko packager

Sorry just noticed a typo during final testing. Does this Look Good To Noack?

[Sebastian Noack, 2016-01-13 11:17:51]

LGTM