From b5099530084deb61fcb9e60314579dd0daed79b5 Mon Sep 17 00:00:00 2001 From: David Ostrovsky Date: Sat, 6 May 2017 06:35:48 +0200 Subject: [PATCH 1/2] maven_jar compatibility to rules_closure#java_import_external This is a preparation change for bumping rules_closure to the HEAD, that we need to make transpilation from ES6 to ES5 actually work. Since this commit: [1] rules_closure moved from using maven_jar to home grown java_import_external rule. However, this introduced subtle incompatibility issue: [2], because java_import_external mounts external dependency under the root directory and not under the @foo/jar directory, like native and our own custom maven_jar rules are doing. This leads to the incompatibility, of how the external dependency are consumed, with the consequence, that artifacts are not found by the rule_closure rules, that were fetched with our own maven_jar rule. To rectify, extend our maven_jar rule and generate additional BUILD file in the root folder of the external dependency directory and alias it to the real one generated in the @foo/jar directory. For example, this BUILD file is now generated in root directory: alias( name = "javax_inject", actual = "@javax_inject//jar", ) This allows us to omit additional fetching of already fetched artifacts that are needed by rules_closures rules and let them reference to the downloaded artifacts in gerrit build. There is one complication though: java_import_external accepts deps attribute, in which case the artifacts with dependencies are not compatible with our own maven_jar custom rule, that doesn't support transitive dependencies. That means that we can only reuse such artifacts from the rules_closure build that don't have dependencies. Another problem, why we can't reuse even such artifacts that don't have dependencies specified, is different naming convention. gerrit uses simple names, like jsr305, but rules_closure is using canonical artifact names, e.g.: com_google_code_findbugs_jsr305, so that we get: * external/jsr305/jar/jsr305-3.0.1.jar # gerrit * external/com_google_code_findbugs_jsr305/jsr305-2.0.3.jar # closure Effectively, right now this change let us only reuse 3 common dependencies: * aopalliance * javax_inject * args4j Because the same name, we must solve the collision problem here. Another considered alternatives to this change would be: 1. Ask rules_closure project to migrate to consuming their dependencies from @foo/jar directory, so that it is compatible with native and custom maven_jar rules 2. Consume common dependencies in gerrit with rule_closure's own java_import_external rule [1] https://github.com/bazelbuild/rules_closure/commit/a47cc063d39b3958f31b4c5a992741020fd9ee3e [2] https://github.com/bazelbuild/rules_closure/issues/200 Change-Id: I31e6b863e43adaa1f983a8a9da1675f02cb803f8 --- tools/bzl/maven_jar.bzl | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/tools/bzl/maven_jar.bzl b/tools/bzl/maven_jar.bzl index 2dbeae7658..55bfae178f 100644 --- a/tools/bzl/maven_jar.bzl +++ b/tools/bzl/maven_jar.bzl @@ -64,12 +64,13 @@ def _format_deps(attr, deps): formatted_deps += " ]," return formatted_deps -def _generate_build_file(ctx, binjar, srcjar): +def _generate_build_files(ctx, binjar, srcjar): + header = "# DO NOT EDIT: automatically generated BUILD file for maven_jar rule %s" % ctx.name srcjar_attr = "" if srcjar: srcjar_attr = 'srcjar = "%s",' % srcjar contents = """ -# DO NOT EDIT: automatically generated BUILD file for maven_jar rule {rule_name} +{header} package(default_visibility = ['//visibility:public']) java_import( name = 'jar', @@ -86,10 +87,10 @@ java_import( {exports} ) \n""".format(srcjar_attr = srcjar_attr, - rule_name = ctx.name, - binjar = binjar, - deps = _format_deps("deps", ctx.attr.deps), - exports = _format_deps("exports", ctx.attr.exports)) + header = header, + binjar = binjar, + deps = _format_deps("deps", ctx.attr.deps), + exports = _format_deps("exports", ctx.attr.exports)) if srcjar: contents += """ java_import( @@ -99,6 +100,18 @@ java_import( """.format(srcjar = srcjar) ctx.file('%s/BUILD' % ctx.path("jar"), contents, False) + # Compatibility layer for java_import_external from rules_closure + contents = """ +{header} +package(default_visibility = ['//visibility:public']) + +alias( + name = "{rule_name}", + actual = "@{rule_name}//jar", +) +\n""".format(rule_name = ctx.name, header = header) + ctx.file("BUILD", contents, False) + def _maven_jar_impl(ctx): """rule to download a Maven archive.""" coordinates = _create_coordinates(ctx.attr.artifact) @@ -142,7 +155,7 @@ def _maven_jar_impl(ctx): if out.return_code: fail("failed %s: %s" % (args, out.stderr)) - _generate_build_file(ctx, binjar, srcjar) + _generate_build_files(ctx, binjar, srcjar) maven_jar = repository_rule( attrs = { From 0a6b9eab13dcc0544d5e610fece016bef5056d65 Mon Sep 17 00:00:00 2001 From: David Ostrovsky Date: Mon, 1 May 2017 23:42:15 +0200 Subject: [PATCH 2/2] PolyGerrit: Make ES6 to ES5 transpilation actually work Bump rules_closure version to this commit[1], that allows us to pass --force_inject_library=es6_runtime to closure compiler, that fixes missing injection of ES6 dependency with optimization level whitespace. One side effect of this change: because of the recently made change in rules_closure rules of how the external dependencies are consumed, we cannot reuse some common dependencies that were already fetched during gerrit build and must re-fetch them again, most notably: * asm * gson * guava * guice * soy The bad news here is, that re-fetching takes place with rules_closure's java_import_external rule, that is not using our own download_file.py utility and thus the artifacts are not cached in ~/.gerritcodereview directory, so that when the build is repeated on the same machine but on different clone of gerrit repository all rules_closure dependencies are going to be re-fetched again. Another complication of re-fetching is that the different versions of the artifacts are now fetched: e.g. Gerrit is using guava 21, and closure rule is using guava 20. The reason why we don't have the collision here is because gerrit mounts this dependency under @guava directory, whereas rules_closure is using canonical artifact name, so that we get: * external/com_google_guava/guava-20.0.jar # fetched by rules_closure * external/guava/jar/guava-21.0.jar # fetched by gerrit Test Plan: 1. conduct ES6 modification, e.g. apply this CL: [2] 2. run bazel build gerrit 3. verify that transpiled code actually work [1] https://github.com/bazelbuild/rules_closure/commit/f68d4b5a55c04ee50a3196590dce1ca8e7dbf438 [2] https://gerrit-review.googlesource.com/105104 Bug: Issue 6110 Change-Id: I3f3adf8ce5e613d45d1d0684b823e48e68a14080 --- WORKSPACE | 21 ++++++--------------- polygerrit-ui/app/BUILD | 8 ++++---- 2 files changed, 10 insertions(+), 19 deletions(-) diff --git a/WORKSPACE b/WORKSPACE index f136155638..40e400051b 100644 --- a/WORKSPACE +++ b/WORKSPACE @@ -6,9 +6,9 @@ load("//plugins:external_plugin_deps.bzl", "external_plugin_deps") http_archive( name = "io_bazel_rules_closure", - strip_prefix = "rules_closure-0.4.1", - sha256 = "ba5e2e10cdc4027702f96e9bdc536c6595decafa94847d08ae28c6cb48225124", - url = "http://bazel-mirror.storage.googleapis.com/github.com/bazelbuild/rules_closure/archive/0.4.1.tar.gz", + sha256 = "af1f5a31b8306faed9d09a38c8e2c1d6afc4c4a2dada3b5de11cceae8c7f4596", + strip_prefix = "rules_closure-f68d4b5a55c04ee50a3196590dce1ca8e7dbf438", + url = "https://bazel-mirror.storage.googleapis.com/github.com/bazelbuild/rules_closure/archive/f68d4b5a55c04ee50a3196590dce1ca8e7dbf438.tar.gz", # 2017-05-05 ) # File is specific to Polymer and copied from the Closure Github -- should be @@ -24,18 +24,9 @@ load("@io_bazel_rules_closure//closure:defs.bzl", "closure_repositories") # Prevent redundant loading of dependencies. closure_repositories( - omit_aopalliance=True, - omit_args4j=True, - omit_jsr305=True, - omit_gson=True, - omit_guava=True, - omit_guice=True, - omit_soy=True, - omit_icu4j=True, - omit_asm=True, - omit_asm_analysis=True, - omit_asm_commons=True, - omit_asm_util=True, + omit_aopalliance = True, + omit_args4j = True, + omit_javax_inject = True, ) ANTLR_VERS = "3.5.2" diff --git a/polygerrit-ui/app/BUILD b/polygerrit-ui/app/BUILD index 7c12fa27a2..abfb8f85e5 100644 --- a/polygerrit-ui/app/BUILD +++ b/polygerrit-ui/app/BUILD @@ -34,10 +34,9 @@ closure_js_library( name = "closure_lib", srcs = ["gr-app.js"], convention = "GOOGLE", - language = "ECMASCRIPT6", - suppress = [ - "JSC_BAD_JSDOC_ANNOTATION", - ], + # TODO(davido): Clean up these issues: http://paste.openstack.org/show/608548 + # and remove this supression + suppress = ["JSC_UNUSED_LOCAL_ASSIGNMENT"], deps = [ "//lib/polymer_externs:polymer_closure", "@io_bazel_rules_closure//closure/library", @@ -52,6 +51,7 @@ closure_js_binary( defs = [ "--polymer_pass", "--jscomp_off=duplicate", + "--force_inject_library=es6_runtime", ], language = "ECMASCRIPT5", deps = [":closure_lib"],