Create js_library and js_binary templates for closure compiling.

third_party/closure_compiler/js_binary.py

+# Copyright 2017 The Chromium Authors.  All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+"""Used by a js_binary action to compile javascript files.
+
+This script takes in a list of sources and dependencies and compiles them all
+together into a single compiled .js file.  The dependencies are ordered in a
+post-order, left-to-right traversal order.  If multiple instances of the same
+source file are read, only the first is kept. The script can also take in
+optional --defs argument which will add custom flags to the compiler.  Any
+extern files can also be passed in using the --extern flag.
+"""
+
+from argparse import ArgumentParser
+import os
+import subprocess

[Dan Beam, 2017/04/18 02:05:50]

it seems more common in chrome to use subprocess2 and Popen() and communicate().

is there a reason why you're not doing that?

[damargulis, 2017/04/19 00:42:12]

I based most of the code to run the jar from java_action.py
(https://cs.chromium.org/chromium/src/build/util/java_action.py?q=java_action....)

Is there any advantage of switching from check_call() to Popen()?

[Dan Beam, 2017/04/19 17:59:45]

why are you duplicating the code rather than just importing it and using it,
then?

+import sys
+
+EXIT_SUCCESS = 0

[Dan Beam, 2017/04/18 02:05:50]

unused

[damargulis, 2017/04/19 00:42:12]

Done.

+EXIT_FAILURE = 1
+
+def IsExecutable(path):
+  return os.path.isfile(path) and os.access(path, os.X_OK)
+

[Dan Beam, 2017/04/18 02:05:49]

file-level globals should have 2 \n between them

[damargulis, 2017/04/19 00:42:12]

Done.

+def FindCommand(command):
+  filepath, _ = os.path.split(command)
+  if filepath and IsExecutable(command):
+    return command
+
+  if sys.platform == 'win32':
+    # On Windows, if the command does not have an extension, cmd.exe will
+    # try all extensions from PATHEXT when resolving the full path.
+    command, ext = os.path.splitext(command)
+    exts = [ext] if ext else os.environ['PATHEXT'].split(os.path.pathsep)
+  else:
+    exts = ['']
+
+  for path in os.environ['PATH'].split(os.path.pathsep):
+    for ext in exts:
+      path = os.path.join(path, command) + ext
+      if IsExecutable(path):
+        return path
+
+  return None
+
+
+def RunCompiler(args):
+  java_path = FindCommand('java')
+  if not java_path:
+    sys.stderr.write('java: command not found\n')
+    sys.exit(EXIT_FAILURE)
+  return subprocess.check_call([java_path, '-jar'] + args)

[Dan Beam, 2017/04/18 02:05:49]

can you share this code across compile.py and compile2.py as well?

[damargulis, 2017/04/19 00:42:12]

compile.py and compile2.py use the Checker class, which runs the compiler
through the _run_jar method.  Since this is a private method, I don't want to
call that directly, and I don't want to use the exposed check method, as this
does a lot of additional work and modifications that I don't want done.  

I could change compile.py and compile2.py to use the RunCompiler method here,
but I'm not sure how useful that would be.  They are both run through .gyp
commands, and I thought that .gyp was being deprecated, so I assumed that these
files were being deprecated as well.  

[Dan Beam, 2017/04/19 17:59:45]

that's obviously easy to change
why not just make it public?
what do you think we're going to replace them with, if not the code you're
writing right now? :)

+
+def ParseDepList(dep):
+  """Parses a depenency list, returns |sources, deps|."""
+  assert os.path.isfile(dep), dep[:-11] + ' is not a js_library target'

[Dan Beam, 2017/04/18 02:05:50]

what does -11 mean here?

[damargulis, 2017/04/19 00:42:12]

This removes ".js_library" from the name of the file before printing it to the
console.  The name of the dep is created in the js_library template, and
".js_library" is automatically appended.  If the file doesn't exist however, it
means that it was not actually a js_library target.  I thought it would be
confusing to get an error message saying "target_name.js_library is not a
js_library target", since "target_name.js_library" doesn't exist.  This will
instead just print "target_name is not a js_library target", which I think will
be a more helpful error message.

+  with open(dep, 'r') as dep_list:
+    lines = [line.strip() for line in dep_list.readlines()]

[Dan Beam, 2017/04/18 02:05:49]

use dep_list.splitlines() instead of readlines() + strip()

[damargulis, 2017/04/19 00:42:12]

splitlines() is only available on strings, not files, but I was able to use
dep_list.read().splitlines()

+  split = lines.index('deps:')

[Dan Beam, 2017/04/18 02:05:49]

what if this fails?

[damargulis, 2017/04/19 00:42:12]

If the file exists, it should never fail, because the js_library rule that
creates it should always write the file in the form 
sources:
<list of sources>
deps:
<list of deps>

But I can put a check in to ensure this, and have it error out otherwise.

+  return lines[1:split], lines[split+1:]
+
+def PostOrder(deps, sources):

[Dan Beam, 2017/04/18 02:05:50]

can we name this something else?  this sounds like you're buying a sandwich
online

[damargulis, 2017/04/19 00:42:12]

Changed to CrawlDepsTree

+  """Parses the dependency tree creating a post-order listing of sources."""
+  for dep in deps:
+    new_sources, new_deps = ParseDepList(dep)
+
+    sources = PostOrder(new_deps, sources)
+    sources = sources + [source for source in new_sources
+                         if source not in sources]
+  return sources
+
+def main():
+  parser = ArgumentParser()
+  parser.add_argument('-c', '--compiler', required=True,
+                      help='Path to compiler')
+  parser.add_argument('-s', '--sources', nargs='*', default=[],
+                      help='List of js source files')
+  parser.add_argument('-o', '--output', required=True,
+                      help='Compile to output')
+  parser.add_argument('-d', '--deps', nargs='*', default = [],
+                      help='List of js_libarary dependencies')
+  parser.add_argument('-b', '--bootstrap',
+                      help='A file to include before all others')
+  parser.add_argument('-cf', '--config', nargs='*', default = [],
+                      help='A list of files to include after bootstrap and' \
+                      'before all others')
+  parser.add_argument('-df', '--defs', nargs='*', default = [],
+                      help='A list of custom flags to pass to the compiler. ' \
+                      'Do not include leading dashes')
+  parser.add_argument('-e', '--externs', nargs='*', default = [],
+                      help='A list of extern files to pass to the compiler')
+
+  args = parser.parse_args()
+  sources = PostOrder(args.deps, []) + args.sources
+
+  defs = ['--' + flag for flag in args.defs]
+
+  compiler_args = [
+      args.compiler,
+  ] + defs

[Dan Beam, 2017/04/18 02:05:49]

what is doing this formatting?  this would fit on one line...

[damargulis, 2017/04/19 00:42:12]

Done.

+
+  for extern in args.externs:
+    compiler_args += ['--externs=' + extern]

[Dan Beam, 2017/04/18 02:05:49]

compiler_args += ['--externs=%s' % e for e in args.externs]

[Dan Beam, 2017/04/18 02:05:50]

the python style guide discourages use of + for string concatenation in favor of
'%s' % arg

[Dan Beam, 2017/04/18 02:07:42]

oh, i guess it's a little more nuanced than that:
https://google.github.io/styleguide/pyguide.html#Strings

[damargulis, 2017/04/19 00:42:13]

I switched it to use substitution, as it turns it into a one-liner and I think
looks a little cleaner.

+
+  compiler_args += [
+      '--js_output_file',
+      args.output,
+      '--js',
+  ]
+  if(args.bootstrap):

[Dan Beam, 2017/04/18 02:05:49]

space after if

[Dan Beam, 2017/04/18 02:05:49]

no () (this ain't C)

[damargulis, 2017/04/19 00:42:12]

Done.

[damargulis, 2017/04/19 00:42:12]

Done.

+    compiler_args += [args.bootstrap]
+  compiler_args += args.config
+  compiler_args += sources
+  RunCompiler(compiler_args)
+
+if __name__ == '__main__':
+  sys.exit(main())

[Dan Beam, 2017/04/18 02:05:49]

it doesn't look like main() actually returns anything on success?

sys.exit() ends the program early, so no real reason to pass the return value,
IMO, just crash the program in the middle (just make sure to clean up first)

[damargulis, 2017/04/19 00:42:12]

Done.