Fixing package-boundary violation with srjar_deps

webrtc/generate_srcjar.py

+#!/usr/bin/env python
+#
+# Copyright (c) 2016 The WebRTC project authors. All Rights Reserved.

[kjellander_webrtc, 2017/01/04 14:25:21]

New files should have the current year.

[mbonadei, 2017/01/04 15:19:06]

Good catch, I am still writing 2016 everywhere. :-)

+#
+# Use of this source code is governed by a BSD-style license
+# that can be found in the LICENSE file in the root of the source
+# tree. An additional intellectual property rights grant can be found
+# in the file PATENTS.  All contributing project authors may
+# be found in the AUTHORS file in the root of the source tree.
+
+#TODO(mbonadei): move this script into chromium (build/android/gyp)

[kjellander_webrtc, 2017/01/04 14:25:21]

Comments usually don't start right after the #. 
I'm not sure the style guide mandates it but I think it's more readable, so
please add a space.

[mbonadei, 2017/01/04 15:19:06]

Acknowledged.

+# when approved
+
+#TODO(mbonadei): write some documentation on how to use this script
+
+import re
+import optparse
+import os
+import sys
+import zipfile
+import ntpath

[kjellander_webrtc, 2017/01/04 14:25:21]

sort alphabetically

[mbonadei, 2017/01/04 15:19:06]

Acknowledged.

+
+#TODO(mbonadei): fix this import and meke it relative to the chromium

[kjellander_webrtc, 2017/01/04 14:25:21]

meke -> make

[mbonadei, 2017/01/04 15:19:06]

Acknowledged.

+# build/android/gyp folder.
+sys.path.insert(0, os.path.abspath('../../build/android/gyp/util'))

[kjellander_webrtc, 2017/01/04 14:25:21]

Please build the path using os.path.join and os.pardir, like this:
os.path.join(os.pardir, os.pardir, 'build', 'android', 'gyp', 'util)

otherwise this will fail on Windows.

[mbonadei, 2017/01/04 15:19:06]

Acknowledged.

+import build_utils
+
+
+def FindZipPathFromSource(src_file):

[kjellander_webrtc, 2017/01/04 14:25:21]

Can you add a docstring what this method does?

I assume it's looking up a directory path to be used for the file inside the zip
file, based on the package name of the .java file?
That kind of text could go into the docstring.

[mbonadei, 2017/01/04 15:19:06]

Acknowledged.

+  with open(src_file, "r") as f:

[kjellander_webrtc, 2017/01/04 14:25:21]

You can leave out 'r', as it's the default if mode is omitted.

[mbonadei, 2017/01/04 15:19:06]

Acknowledged.

+    file_src = f.read()
+    package = re.search("package (.*);", file_src).group(1)

[kjellander_webrtc, 2017/01/04 14:25:21]

Please use single-quotes for all strings. The style guide [1] says "Be
consistent with your choice of string quote character within a file. Pick ' or "
and stick with it. ".
We usually have single-quotes in the WebRTC codebase, and we align with
Chromium's style recommendations.

[1] http://dev.chromium.org/chromium-os/python-style-guidelines and more
specifically
http://google.github.io/styleguide/pyguide.html?showone=Strings#Strings

[mbonadei, 2017/01/04 15:19:06]

Acknowledged.

+    zip_folder = package.replace('.', os.path.sep)
+    file_name = ntpath.basename(src_file)

[kjellander_webrtc, 2017/01/04 14:25:21]

I don't ntpath is useful here. Just regular os.path.basename seems better and
more portable.

[mbonadei, 2017/01/04 15:19:06]

Acknowledged.

+    return os.path.join(zip_folder, file_name)
+
+
+def DoMain(argv):
+  usage = 'usage: %prog [options] [output_dir] input_file(s)...'
+  parser = optparse.OptionParser(usage=usage)
+  parser.add_option('--srcjar',

[kjellander_webrtc, 2017/01/04 14:25:21]

For our use case this is a required flag, right? Then the comment should be
updated and also throw a parser.error if it's omitted.

[mbonadei, 2017/01/04 15:19:06]

Acknowledged.

I am not throwing parser.error in the next patch set because it prints a useful
message if not present (because there is no default).

+                    help='When specified, a .srcjar at the given path is '
+                    'created instead of individual .java files.')
+
+  options, args = parser.parse_args(argv)
+
+  if not args:
+    parser.error('Need to specify at least one input file')
+  input_paths = args
+
+  with zipfile.ZipFile(options.srcjar, 'w', zipfile.ZIP_STORED) as srcjar:
+    for src_path in input_paths:
+      zip_path = FindZipPathFromSource(src_path)

[kjellander_webrtc, 2017/01/04 14:25:21]

This manipulation just to figure out the path inside the zip file is a bit
annoying, but I guess it's OK.

[mbonadei, 2017/01/04 15:19:06]

Yeah, I agree but I think that probably it is the most reliable source for that
kind of information.

+      build_utils.AddToZipHermetic(srcjar, zip_path, src_path)
+
+
+if __name__ == '__main__':
+  DoMain(sys.argv[1:])