Move the Java tools deploy jars refrences to checked-in deploy jars #7114

iirina · 2019-01-14T14:40:29Z

instead of embedding the deploy jars built at the same time with the bazel binary.

This comes in preparation for moving the Java tools to remote repositories (see #6316).

Moving the tools happens in two phases:

Using checked-in deploy jars/binaries instead of those built when bazel was built
Moving the checked-in jars/binaries to a remote repository and make bazel reference them from there. This step can only happen under an incompatible flag.

This PR addresses the first point. Doing the move incrementally this way is safer and allows finding bugs earlier. It also allows moving the Java tools code base out of the bazel repository.

We are making this change gradually for each java tool defined in JAVA_TOOLS (found in src/BUILD).

iirina · 2019-01-14T15:12:27Z

@lberki PTAL. If/when this looks OK to you I'll do the same for the other deploy jars in JAVA_TOOLS.

src/BUILD

third_party/java/java_tools/README.md

iirina · 2019-01-15T11:56:40Z

@lberki I updated all the deploy jars defined in src/BUILD in the embedded_tools filegroup (defined by JAVA_TOOLS). The remaining targets in JAVA_TOOLS require a bit more work and I'll do take care of them separately in new PRs.

iirina · 2019-01-15T12:32:20Z

@lberki This PR is ready for review now, PTAL! Thanks!

third_party/BUILD

lberki · 2019-01-15T12:41:32Z

third_party/BUILD

+    name = "java_tools",
+    srcs = glob(
+        ["java/java_tools/*.jar"],
+        exclude = ["java/java_tools/SingleJar_deploy.jar"]


This change will need to be split (to a Git-on-Borg and a Google internal part). (But it's easier to review in one piece!)

Why does it need to be split?

third_party/java/java_tools/update_java_tools.sh

lberki · 2019-01-15T12:42:54Z

third_party/java/java_tools/update_java_tools.sh

@@ -0,0 +1,83 @@
+#!/bin/bash


nit: how about adding -eu: -e makes the shell exit early if a command returns a non-zero exit code and -u makes using undeclared variables an error (one less opportunity for typos!)

If you want to be fancy, you can also add -x that makes bash print every command it executes.

I didn't add any of those because:

-e because I still want to update some of the tools even if the others didn't build. I find it easier to debug if we keep the errors and print them at the end

-u because I get an error of $1 not being declared when I check if it equals "help", even though I do this check only after checking that $1 is not empty

x because bazel already pollutes the output a lot, by looking at it is more than enough

Let me know if you think otherwise.

On a second thought, I added -e. It can be more helpful to exit at the first error and not scroll through the whole log. 😆

third_party/java/java_tools/update_java_tools.sh

lberki · 2019-01-15T12:49:02Z

third_party/java/java_tools/update_java_tools.sh

+  local bazel_target="${1}"; shift
+  local binary=$(echo "bazel-bin/$bazel_target" | sed 's@:@/@')
+
+  bazel build "$bazel_target"


nit: how about issuing a single Bazel invocation that builds every tool? It's presumably faster.

Each invocation is ~0.1s, do we need to speed it up? This script is meant to be run manually and anything under a few seconds is the same IMO. Having them separately allows printing more useful error messages and following all the logic for one build in the same place.

third_party/java/java_tools/update_java_tools.sh

lberki · 2019-01-15T12:56:54Z

third_party/java/java_tools/update_java_tools.sh

+
+}
+
+for tool in "${tools_to_update[@]}"


Opt: bash associative arrays are scary. How about simply

function process() { bazel build $2 cp ... ... } process SingleJar src/java_tools/singlejar/java/com/google/devtools/build/singlejar:bazel-singlejar_deploy.jar process GenClass rc/java_tools/buildjar/java/com/google/devtools/build/buildjar/genclass:GenClass_deploy.jar

More repetitive, but also more readable for mere mortals.

Ah, this is exactly the way I started the script! However, it ended up having too much repetition especially because I wanted to be able to print the usage. Doing it this way requires repetition 3 times, and every time one of the targets or tool name changes we have to update the other 2 as well. I updated the documentation, does it make it more readable? I really want to avoid copying all these strings everywhere.

I'd be OK with slightly worse usage text if it makes the script simpler.

cushon · 2019-01-15T17:55:18Z

src/BUILD

@@ -115,43 +115,47 @@ py_binary(
    deps = [":create_embedded_tools_lib"],
 )

+# The tools Bazel uses to compile Java.
+# TODO(iirina): Gradually remove the targets here.


prefer TODO(<bug>)

third_party/java/java_tools/update_java_tools.sh

cushon · 2019-01-15T17:58:12Z

third_party/java/java_tools/update_java_tools.sh

+
+}
+
+for tool in "${tools_to_update[@]}"


I'd be OK with slightly worse usage text if it makes the script simpler.

cushon · 2019-01-15T17:59:11Z

third_party/java/java_tools/README.md

+To see how to update the tools run:
+
+```
+./third_party/java/java_tools/update_java_tools.sh help


When do we expect this script to be run? i.e. as part of the release process, or on-demand, or something else?

instead of embedding the deploy jars built at the same time with the bazel binary. This comes in preparation for moving the Java tools to remote repositories (see bazelbuild#6316). Moving the tools happens in two phases: 1. Using checked-in deploy jars/binaries instead of those built when bazel was built 2. Moving the checked-in jars/binaries to a remote repository and make bazel reference them from there. This step can only happen under an incompatible flag. This PR addresses the first point. Doing the move incrementally this way is safer and allows finding bugs earlier. It also allows moving the Java tools code base out of the bazel repository. We are making this change gradually for each java tool defined in JAVA_TOOLS (found in src/BUILD). Closes bazelbuild#7114. PiperOrigin-RevId: 229524951

Move JavaBuilder refrence to a checked-in deploy jar

4ef3aad

googlebot added the cla: yes label Jan 14, 2019

iirina added team-Rules-Java Issues for Java rules WIP labels Jan 14, 2019

iirina requested a review from lberki January 14, 2019 15:12

lberki approved these changes Jan 15, 2019

View reviewed changes

src/BUILD Outdated Show resolved Hide resolved

third_party/java/java_tools/README.md Outdated Show resolved Hide resolved

iirina added 3 commits January 15, 2019 09:42

Add third_party/java/java_tools/VanillaJavaBuilder_deploy.jar

63f7066

Add ExperimentalRunner, GenClass, Runner

252c82c

Add Turbine

e1d0d3d

iirina requested a review from a team as a code owner January 15, 2019 11:39

iirina removed the WIP label Jan 15, 2019

Update update_java_tools.sh

f568165

iirina changed the title ~~Move JavaBuilder refrence to a checked-in deploy jar~~ Move the Java tools deploy jars refrences to checked-in deploy jars Jan 15, 2019

Add JacocoCoverage and SingleJar

359bcbd

lberki reviewed Jan 15, 2019

View reviewed changes

iirina added 2 commits January 15, 2019 14:43

Addressed code review comments.

679c867

Improved documentation in update_java_tools.sh

07a033b

iirina mentioned this pull request Jan 15, 2019

Add Java tools deploy jars under third_party/java/java_tools #7127

Closed

cushon reviewed Jan 15, 2019

View reviewed changes

bazel-io closed this in f4604ba Jan 16, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move the Java tools deploy jars refrences to checked-in deploy jars #7114

Move the Java tools deploy jars refrences to checked-in deploy jars #7114

iirina commented Jan 14, 2019 •

edited

Loading

iirina commented Jan 14, 2019

iirina commented Jan 15, 2019 •

edited

Loading

iirina commented Jan 15, 2019

lberki Jan 15, 2019

iirina Jan 15, 2019

lberki Jan 15, 2019

iirina Jan 15, 2019

iirina Jan 15, 2019

lberki Jan 15, 2019

iirina Jan 15, 2019

lberki Jan 15, 2019

iirina Jan 15, 2019

cushon Jan 15, 2019

cushon Jan 15, 2019

cushon Jan 15, 2019

cushon Jan 15, 2019

Move the Java tools deploy jars refrences to checked-in deploy jars #7114

Move the Java tools deploy jars refrences to checked-in deploy jars #7114

Conversation

iirina commented Jan 14, 2019 • edited Loading

iirina commented Jan 14, 2019

iirina commented Jan 15, 2019 • edited Loading

iirina commented Jan 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iirina commented Jan 14, 2019 •

edited

Loading

iirina commented Jan 15, 2019 •

edited

Loading