Remove repackaging of protobuf code.

I don't know why it's there, but I'm pretty sure it's no longer necessary. Removing it has several advantages: 1. code only depends on the protos they really need 2. we get the benefit of proto-specific lint checks which were lost in the build/import steps previously 3. Simpler build rules 4. Targets depending on the protos don't also pull in other things unknowningly, but instead have to declare all deps explicitly. Note, the code changes here are the result of 2 above: proto mesisages cannot be null. Some BUILD file changes to explicitly add deps (e.g. jsr305) were required as a result of 4. The IJ plugin classloader (com.intellij.ide.plugins.cl.PluginClassLoader) prefers classes from itself over the parent classloader. This differs from the standard Java classloader order. As a result, proto classes will always be loaded from the plugin instead of the underlying IDE (where they are also present). (cherry picked from commit b0688f9)
cpsauer · Sep 17, 2022 · fe1c1e8 · fe1c1e8
1 parent 2024a79
commit fe1c1e8
Show file tree

Hide file tree

Showing 17 changed files with 24 additions and 41 deletions.
diff --git a/aspect/tools/BUILD b/aspect/tools/BUILD
@@ -20,8 +20,8 @@ java_library(
     srcs = glob(["src/**/*.java"]),
     deps = [
         ":guava",
-        "//intellij_platform_sdk:jsr305",
         "//proto:proto_deps",
+        "@jsr305_annotations//jar",
     ],
 )
 

diff --git a/aspect/tools/src/com/google/idea/blaze/aspect/PackageParserIoProvider.java b/aspect/tools/src/com/google/idea/blaze/aspect/PackageParserIoProvider.java
@@ -16,7 +16,7 @@
 package com.google.idea.blaze.aspect;
 
 import com.google.common.annotations.VisibleForTesting;
-import com.google.repackaged.bazel.protobuf.MessageLite;
+import com.google.protobuf.MessageLite;
 import java.io.BufferedReader;
 import java.io.IOException;
 import java.io.OutputStream;

diff --git a/aspect/tools/tests/unittests/com/google/idea/blaze/aspect/PackageParserTest.java b/aspect/tools/tests/unittests/com/google/idea/blaze/aspect/PackageParserTest.java
@@ -24,7 +24,7 @@
 import com.google.common.collect.Maps;
 import com.google.devtools.intellij.aspect.Common.ArtifactLocation;
 import com.google.errorprone.annotations.CanIgnoreReturnValue;
-import com.google.repackaged.bazel.protobuf.MessageLite;
+import com.google.protobuf.MessageLite;
 import java.io.BufferedReader;
 import java.io.ByteArrayInputStream;
 import java.io.IOException;

diff --git a/aswb/BUILD b/aswb/BUILD
@@ -196,6 +196,7 @@ intellij_integration_test_suite(
         "//common/experiments",
         "//common/experiments:unit_test_utils",
         "//cpp",
+        "//intellij_platform_sdk:jsr305",
         "//intellij_platform_sdk:plugin_api_for_tests",
         "//intellij_platform_sdk:test_libs",
         "//java",

diff --git a/base/BUILD b/base/BUILD
@@ -446,6 +446,7 @@ intellij_unit_test_suite(
         ":unit_test_utils",
         "//common/experiments",
         "//common/experiments:unit_test_utils",
+        "//intellij_platform_sdk:jsr305",
         "//intellij_platform_sdk:plugin_api_for_tests",
         "//intellij_platform_sdk:test_libs",
         "//proto:proto_deps",

diff --git a/base/src/com/google/idea/blaze/base/command/buildresult/BuildEventProtocolOutputReader.java b/base/src/com/google/idea/blaze/base/command/buildresult/BuildEventProtocolOutputReader.java
@@ -146,7 +146,7 @@ private static BlazeArtifact parseTestFile(
   private static SourceArtifact parseLocalFile(
       BuildEventStreamProtos.File file, Predicate<String> fileFilter) {
     String uri = file.getUri();
-    if (uri == null || !uri.startsWith(URLUtil.FILE_PROTOCOL)) {
+    if (!uri.startsWith(URLUtil.FILE_PROTOCOL)) {
       return null;
     }
     try {

diff --git a/base/src/com/google/idea/blaze/base/command/buildresult/OutputArtifactParser.java b/base/src/com/google/idea/blaze/base/command/buildresult/OutputArtifactParser.java
@@ -60,7 +60,7 @@ public OutputArtifact parse(
         String configurationMnemonic,
         long syncStartTimeMillis) {
       String uri = file.getUri();
-      if (uri == null || !uri.startsWith(URLUtil.FILE_PROTOCOL)) {
+      if (!uri.startsWith(URLUtil.FILE_PROTOCOL)) {
         return null;
       }
       try {

diff --git a/base/src/com/google/idea/blaze/base/ideinfo/ProtoWrapper.java b/base/src/com/google/idea/blaze/base/ideinfo/ProtoWrapper.java
@@ -18,7 +18,7 @@
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Streams;
-import com.google.repackaged.bazel.protobuf.Message;
+import com.google.protobuf.Message;
 import java.util.Map;
 import java.util.function.Consumer;
 import java.util.function.Function;

diff --git a/base/src/com/google/idea/blaze/base/lang/buildfile/sync/BuildLangSyncPlugin.java b/base/src/com/google/idea/blaze/base/lang/buildfile/sync/BuildLangSyncPlugin.java
@@ -38,7 +38,7 @@
 import com.google.idea.blaze.base.sync.projectview.WorkspaceLanguageSettings;
 import com.google.idea.blaze.base.sync.workspace.ArtifactLocationDecoder;
 import com.google.idea.blaze.base.sync.workspace.WorkingSet;
-import com.google.repackaged.bazel.protobuf.InvalidProtocolBufferException;
+import com.google.protobuf.InvalidProtocolBufferException;
 import com.intellij.openapi.application.ApplicationManager;
 import com.intellij.openapi.diagnostic.Logger;
 import com.intellij.openapi.project.Project;

diff --git a/base/src/com/google/idea/blaze/base/model/SyncData.java b/base/src/com/google/idea/blaze/base/model/SyncData.java
@@ -19,7 +19,7 @@
 import com.google.common.collect.ImmutableMap;
 import com.google.devtools.intellij.model.ProjectData;
 import com.google.idea.blaze.base.ideinfo.ProtoWrapper;
-import com.google.repackaged.bazel.protobuf.Message;
+import com.google.protobuf.Message;
 import com.intellij.openapi.extensions.ExtensionPointName;
 import java.util.Arrays;
 import java.util.Objects;

diff --git a/base/src/com/google/idea/blaze/base/sync/aspects/strategy/AspectStrategy.java b/base/src/com/google/idea/blaze/base/sync/aspects/strategy/AspectStrategy.java
@@ -29,7 +29,7 @@
 import com.google.idea.blaze.base.model.BlazeVersionData;
 import com.google.idea.blaze.base.model.primitives.LanguageClass;
 import com.google.idea.common.experiments.BoolExperiment;
-import com.google.repackaged.bazel.protobuf.TextFormat;
+import com.google.protobuf.TextFormat;
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.InputStreamReader;

diff --git a/java/src/com/google/idea/blaze/java/fastbuild/FastBuildAspectStrategy.java b/java/src/com/google/idea/blaze/java/fastbuild/FastBuildAspectStrategy.java
@@ -22,7 +22,7 @@
 import com.google.idea.blaze.base.command.BlazeCommand;
 import com.google.idea.blaze.base.settings.BuildSystemName;
 import com.google.idea.blaze.base.util.BuildSystemExtensionPoint;
-import com.google.repackaged.bazel.protobuf.TextFormat;
+import com.google.protobuf.TextFormat;
 import com.intellij.openapi.extensions.ExtensionPointName;
 import java.io.BufferedInputStream;
 import java.io.File;

diff --git a/kotlin/BUILD b/kotlin/BUILD
@@ -102,6 +102,7 @@ intellij_integration_test_suite(
         "//base:unit_test_utils",
         "//common/experiments",
         "//common/experiments:unit_test_utils",
+        "//intellij_platform_sdk:jsr305",
         "//intellij_platform_sdk:kotlin_for_tests",
         "//intellij_platform_sdk:plugin_api_for_tests",
         "//intellij_platform_sdk:test_libs",

diff --git a/plugin_dev/src/com/google/idea/blaze/plugin/run/BlazeIntellijPluginDeployer.java b/plugin_dev/src/com/google/idea/blaze/plugin/run/BlazeIntellijPluginDeployer.java
@@ -32,7 +32,7 @@
 import com.google.idea.blaze.base.io.FileOperationProvider;
 import com.google.idea.blaze.base.model.primitives.Label;
 import com.google.idea.common.experiments.BoolExperiment;
-import com.google.repackaged.bazel.protobuf.TextFormat;
+import com.google.protobuf.TextFormat;
 import com.intellij.concurrency.AsyncUtil;
 import com.intellij.execution.ExecutionException;
 import com.intellij.openapi.util.Key;

diff --git a/proto/BUILD b/proto/BUILD
@@ -17,12 +17,6 @@ licenses(["notice"])
 
 create_proto_visibility_group()
 
-java_import(
-    name = "proto_deps",
-    jars = [":repackaged_proto_deps"],
-    visibility = PLUGIN_PACKAGES_VISIBILITY,
-)
-
 proto_library(
     name = "common_proto",
     srcs = ["common.proto"],
@@ -97,27 +91,13 @@ java_proto_library(
     deps = [":intellij_plugin_target_deploy_info_proto"],
 )
 
-genrule(
-    name = "repackaged_proto_deps",
-    srcs = [
-        ":proto_deps_binary_deploy.jar",
-        ":jarjar_protobuf_rule.txt",
-    ],
-    outs = [":repackaged_proto_deps.jar"],
-    cmd = """
-    $(location @jarjar//:jarjar_bin) \
-        process \
-        $(location :jarjar_protobuf_rule.txt) \
-        $(location :proto_deps_binary_deploy.jar) \
-        $@
-    """,
-    tools = ["@jarjar//:jarjar_bin"],
-)
-
-java_binary(
-    name = "proto_deps_binary",
-    main_class = "None",
-    runtime_deps = [
+# TODO delete this build rule. Dependants should use the proto targets they use
+# directly instead.
+java_library(
+    name = "proto_deps",
+    visibility = PLUGIN_PACKAGES_VISIBILITY,
+    exports = [
+        ":common_java_proto",
         ":fast_build_info_java_proto",
         ":intellij_ide_info_java_proto",
         ":intellij_plugin_target_deploy_info_java_proto",
@@ -126,6 +106,8 @@ java_binary(
         "//third_party/bazel/src/main/java/com/google/devtools/build/lib/starlarkdebug/proto:starlark_debugging_java_proto",
         "//third_party/bazel/src/main/protobuf:android_deploy_info_java_proto",
         "//third_party/bazel/src/main/protobuf:build_java_proto",
+        "//third_party/bazel/src/main/protobuf:command_line_java_proto",
         "//third_party/bazel/src/main/protobuf:deps_java_proto",
+        "@com_google_protobuf//:protobuf_java",
     ],
 )
diff --git a/proto/jarjar_protobuf_rule.txt b/proto/jarjar_protobuf_rule.txt
diff --git a/skylark/src/com/google/idea/blaze/skylark/debugger/impl/SkylarkDebugProcess.java b/skylark/src/com/google/idea/blaze/skylark/debugger/impl/SkylarkDebugProcess.java
@@ -17,7 +17,6 @@
 
 import static com.google.common.base.Preconditions.checkState;
 
-import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.starlarkdebugging.StarlarkDebuggingProtos;
@@ -576,7 +575,7 @@ private boolean individualThreadPausedByUser(StarlarkDebuggingProtos.PauseReason
   private void handleConditionalBreakpointError(
       XLineBreakpoint<XBreakpointProperties> breakpoint, PausedThread thread) {
     // TODO(brendandouglas): also navigate to the problematic breakpoint
-    String error = Preconditions.checkNotNull(thread.getConditionalBreakpointError().getMessage());
+    String error = thread.getConditionalBreakpointError().getMessage();
     String title = "Breakpoint Condition Error";
     String message =
         String.format(