From 1cbeda49785c7b5171ecb6ad58eeeaa562223a91 Mon Sep 17 00:00:00 2001 From: David Ostrovsky Date: Sat, 6 May 2017 06:35:48 +0200 Subject: [PATCH 1/5] 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 (cherry picked from commit b5099530084deb61fcb9e60314579dd0daed79b5) --- 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 c78fb72fd92c5cd0e3554a90b407e81cf3a66151 Mon Sep 17 00:00:00 2001 From: Kasper Nilsson Date: Tue, 25 Apr 2017 13:55:01 +0200 Subject: [PATCH 2/5] Add transpilation to PolyGerrit Utilize the Closure compiler in Bazel to transpile. As part of this, a rather large file of 'externs' must be added in order to call external code. This file is specific to Polymer and copied from the Closure Github, and should be synced any time there are major changes to Polymer. Test Plan: - run `bazel build polygerrit` and verify that whitespaces are removed from resulting gr-app.js file - run `bazel build Documentation:licenses.txt` and verify that the new dependency is listed in resulting bazel-genfiles/Documentation/licenses.txt TODO in later changes: - Get closure optimizations working - Explore sourcemaps possibilities - Maybe use closure linting? Change-Id: Ic358743dda7286fea3ac1e95a7991a92c96d6341 (cherry picked from commit 1ea918bd367c091fb4128ab33d8ca7c61cfe770c) --- WORKSPACE | 34 ++++++++++++++++++++++++++++++++++ lib/polymer_externs/BUILD | 26 ++++++++++++++++++++++++++ polygerrit-ui/app/BUILD | 38 ++++++++++++++++++++++++++++++++++++-- tools/bazel.rc | 2 +- 4 files changed, 97 insertions(+), 3 deletions(-) create mode 100644 lib/polymer_externs/BUILD diff --git a/WORKSPACE b/WORKSPACE index b2caccd466..0787e1040c 100644 --- a/WORKSPACE +++ b/WORKSPACE @@ -4,6 +4,40 @@ load("//tools/bzl:maven_jar.bzl", "maven_jar", "GERRIT", "MAVEN_LOCAL") load("//lib/codemirror:cm.bzl", "CM_VERSION", "DIFF_MATCH_PATCH_VERSION") 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", +) + +# File is specific to Polymer and copied from the Closure Github -- should be +# synced any time there are major changes to Polymer. +# https://github.com/google/closure-compiler/blob/master/contrib/externs/polymer-1.0.js +http_file( + name = "polymer_closure", + sha256 = "5a589bdba674e1fec7188e9251c8624ebf2d4d969beb6635f9148f420d1e08b1", + url = "https://raw.githubusercontent.com/google/closure-compiler/775609aad61e14aef289ebec4bfc09ad88877f9e/contrib/externs/polymer-1.0.js", +) + +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, +) + ANTLR_VERS = "3.5.2" maven_jar( diff --git a/lib/polymer_externs/BUILD b/lib/polymer_externs/BUILD new file mode 100644 index 0000000000..2f1bdbded2 --- /dev/null +++ b/lib/polymer_externs/BUILD @@ -0,0 +1,26 @@ +# Copyright (C) 2017 The Android Open Source Project +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +package( + default_visibility = ["//visibility:public"], +) + +load("@io_bazel_rules_closure//closure:defs.bzl", "closure_js_library") + +closure_js_library( + name = "polymer_closure", + srcs = ["@polymer_closure//file"], + data = ["//lib:LICENSE-Apache2.0"], + no_closure_library = True, +) diff --git a/polygerrit-ui/app/BUILD b/polygerrit-ui/app/BUILD index 94f9bc80de..62a5c6376a 100644 --- a/polygerrit-ui/app/BUILD +++ b/polygerrit-ui/app/BUILD @@ -3,6 +3,7 @@ package( ) load("//tools/bzl:genrule2.bzl", "genrule2") +load("@io_bazel_rules_closure//closure:defs.bzl", "closure_js_library", "closure_js_binary") load( "//tools/bzl:js.bzl", "bower_component_bundle", @@ -29,6 +30,31 @@ vulcanize( deps = ["//polygerrit-ui:polygerrit_components.bower_components"], ) +closure_js_library( + name = "closure_lib", + srcs = ["gr-app.js"], + language = "ECMASCRIPT6", + suppress = [ + "JSC_BAD_JSDOC_ANNOTATION", + "JSC_TYPE_PARSE_ERROR", + "JSC_MISPLACED_MSG_ANNOTATION", + ], + deps = [ + "//lib/polymer_externs:polymer_closure", + "@io_bazel_rules_closure//closure/library", + ], +) + +closure_js_binary( + name = "closure_bin", + # Known issue: Closure compilation not compatible with Polymer behaviors. + # See: https://github.com/google/closure-compiler/issues/2042 + compilation_level = "WHITESPACE_ONLY", + defs = ["--polymer_pass"], + language = "ECMASCRIPT3", + deps = [":closure_lib"], +) + filegroup( name = "top_sources", srcs = [ @@ -42,6 +68,14 @@ filegroup( srcs = glob(["styles/**/*.css"]), ) +filegroup( + name = "app_sources", + srcs = [ + "closure_bin.js", + "gr-app.html", + ], +) + genrule2( name = "polygerrit_ui", srcs = [ @@ -49,7 +83,7 @@ genrule2( "//lib/js:highlightjs_files", ":top_sources", ":css_sources", - ":gr-app", + ":app_sources", # we extract from the zip, but depend on the component for license checking. "@webcomponentsjs//:zipfile", "//lib/js:webcomponentsjs", @@ -57,7 +91,7 @@ genrule2( outs = ["polygerrit_ui.zip"], cmd = " && ".join([ "mkdir -p $$TMP/polygerrit_ui/{styles,fonts,bower_components/{highlightjs,webcomponentsjs},elements}", - "cp $(locations :gr-app) $$TMP/polygerrit_ui/elements/", + "for f in $(locations :app_sources); do ext=$${f##*.}; cp -p $$f $$TMP/polygerrit_ui/elements/gr-app.$$ext; done", "cp $(locations //lib/fonts:sourcecodepro) $$TMP/polygerrit_ui/fonts/", "for f in $(locations :top_sources); do cp $$f $$TMP/polygerrit_ui/; done", "for f in $(locations :css_sources); do cp $$f $$TMP/polygerrit_ui/styles; done", diff --git a/tools/bazel.rc b/tools/bazel.rc index 4ed16cfd5a..ab974d955b 100644 --- a/tools/bazel.rc +++ b/tools/bazel.rc @@ -1,2 +1,2 @@ -build --workspace_status_command=./tools/workspace-status.sh +build --workspace_status_command=./tools/workspace-status.sh --strategy=Closure=worker test --build_tests_only From d1581ec71ce3b3a03f593a3a2cef4628e874eb99 Mon Sep 17 00:00:00 2001 From: Kasper Nilsson Date: Wed, 26 Apr 2017 16:17:47 +0200 Subject: [PATCH 3/5] Fix Closure errors in PolyGerrit The Closure Compiler is very picky with regard to JSDoc formatting. This change fixes a few invalid JSDoc issues, and removes the corresponding suppresses from the BUILD file. Additionally, modify the transpilation target language to ES5. After this change, there should no longer be warnings from the Closure Compiler during building of PolyGerrit. Change-Id: If7566e40c2c419c55f2a634c0f558c6cc263f61c --- polygerrit-ui/app/BUILD | 10 ++++++---- .../elements/change/gr-change-view/gr-change-view.js | 8 ++++---- .../diff/gr-diff-highlight/gr-range-normalizer.js | 4 ++-- .../diff/gr-diff-selection/gr-diff-selection.js | 2 +- .../app/elements/diff/gr-diff-view/gr-diff-view.js | 2 +- .../gr-ranged-comment-layer/gr-ranged-comment-layer.js | 2 +- 6 files changed, 15 insertions(+), 13 deletions(-) diff --git a/polygerrit-ui/app/BUILD b/polygerrit-ui/app/BUILD index 62a5c6376a..30d72cb6a7 100644 --- a/polygerrit-ui/app/BUILD +++ b/polygerrit-ui/app/BUILD @@ -33,11 +33,10 @@ vulcanize( closure_js_library( name = "closure_lib", srcs = ["gr-app.js"], + convention = "GOOGLE", language = "ECMASCRIPT6", suppress = [ "JSC_BAD_JSDOC_ANNOTATION", - "JSC_TYPE_PARSE_ERROR", - "JSC_MISPLACED_MSG_ANNOTATION", ], deps = [ "//lib/polymer_externs:polymer_closure", @@ -50,8 +49,11 @@ closure_js_binary( # Known issue: Closure compilation not compatible with Polymer behaviors. # See: https://github.com/google/closure-compiler/issues/2042 compilation_level = "WHITESPACE_ONLY", - defs = ["--polymer_pass"], - language = "ECMASCRIPT3", + defs = [ + "--polymer_pass", + "--jscomp_off=duplicate", + ], + language = "ECMASCRIPT5_STRICT", deps = [":closure_lib"], ) diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js index f4892de7bf..2347c0fa5e 100644 --- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js +++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js @@ -1095,16 +1095,16 @@ }, /** - * @desc get the line height of an element to the nearest integer. - * */ + * Get the line height of an element to the nearest integer. + */ _getLineHeight: function(element) { var lineHeightStr = getComputedStyle(element).lineHeight; return Math.round(lineHeightStr.slice(0, lineHeightStr.length - 2)); }, /** - * @desc new max height for the related changes section, shorter than - * the existing change info height. + * New max height for the related changes section, shorter than the existing + * change info height. */ _updateRelatedChangeMaxHeight: function() { // Takes into account approximate height for the expand button and diff --git a/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-range-normalizer.js b/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-range-normalizer.js index 8685d7d57e..e870169b3d 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-range-normalizer.js +++ b/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-range-normalizer.js @@ -65,8 +65,8 @@ * Performs a synchronous in-order traversal from top to bottom of the node * element, counting the length of the syntax until child is found. * - * @param {!Element} The root DOM element to be searched through. - * @param {!Element} The child element being searched for. + * @param {!Element} node The root DOM element to be searched through. + * @param {!Element} child The child element being searched for. * @return {number} */ _getTextOffset: function(node, child) { diff --git a/polygerrit-ui/app/elements/diff/gr-diff-selection/gr-diff-selection.js b/polygerrit-ui/app/elements/diff/gr-diff-selection/gr-diff-selection.js index c354882a65..fa1aeb2f4d 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-selection/gr-diff-selection.js +++ b/polygerrit-ui/app/elements/diff/gr-diff-selection/gr-diff-selection.js @@ -191,7 +191,7 @@ * Query the diff object for the lines from a particular side. * * @param {!string} side The side that is currently selected. - * @return {string[]} An array of strings indexed by line number. + * @return {Array.string} An array of strings indexed by line number. */ _getDiffLines: function(side) { if (this._linesCache[side]) { diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js index ee2febe45c..d1ac8426bd 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js +++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js @@ -481,7 +481,7 @@ * If the URL hash is a diff address then configure the diff cursor. */ _loadHash: function(hash) { - var hash = hash.replace(/^#/, ''); + hash = hash.replace(/^#/, ''); if (!HASH_PATTERN.test(hash)) { return; } if (hash[0] === 'a' || hash[0] === 'b') { this.$.cursor.side = DiffSides.LEFT; diff --git a/polygerrit-ui/app/elements/diff/gr-ranged-comment-layer/gr-ranged-comment-layer.js b/polygerrit-ui/app/elements/diff/gr-ranged-comment-layer/gr-ranged-comment-layer.js index 5300ef6893..1425a79520 100644 --- a/polygerrit-ui/app/elements/diff/gr-ranged-comment-layer/gr-ranged-comment-layer.js +++ b/polygerrit-ui/app/elements/diff/gr-ranged-comment-layer/gr-ranged-comment-layer.js @@ -172,7 +172,7 @@ var ranges = this.get(['_commentMap', side, lineNum]) || []; return ranges .map(function(range) { - var range = { + range = { start: range.start, end: range.end === -1 ? line.text.length : range.end, hovering: !!range.comment.__hovering, From ca826ec8de0bcae2fee35c3f4fc929f42b177761 Mon Sep 17 00:00:00 2001 From: Paladox none Date: Thu, 27 Apr 2017 18:16:39 +0000 Subject: [PATCH 4/5] PolyGerrit: Change ECMASCRIPT5_STRICT to ECMASCRIPT5 ECMASCRIPT5_STRICT broke polygerrit for me in chrome and safari. Changing it to ECMASCRIPT5 fixed it. Bug: Issue 6090 Change-Id: I9e8e04a581f8946f3b18069ff938463ae808ffce --- polygerrit-ui/app/BUILD | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/polygerrit-ui/app/BUILD b/polygerrit-ui/app/BUILD index 30d72cb6a7..7c12fa27a2 100644 --- a/polygerrit-ui/app/BUILD +++ b/polygerrit-ui/app/BUILD @@ -53,7 +53,7 @@ closure_js_binary( "--polymer_pass", "--jscomp_off=duplicate", ], - language = "ECMASCRIPT5_STRICT", + language = "ECMASCRIPT5", deps = [":closure_lib"], ) From ac88bd0fa8164d43be3e1d8319c1738219ece83a Mon Sep 17 00:00:00 2001 From: David Ostrovsky Date: Mon, 1 May 2017 23:42:15 +0200 Subject: [PATCH 5/5] 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 0787e1040c..f63b8b3f49 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"],