From fe1c1e884cd8654aa90461cd941905116b301bef Mon Sep 17 00:00:00 2001 From: Googler Date: Wed, 7 Sep 2022 04:40:21 -0700 Subject: [PATCH] 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 b0688f99703fd47bf59157f847603d7c0313d8cf) --- aspect/tools/BUILD | 2 +- .../blaze/aspect/PackageParserIoProvider.java | 2 +- .../idea/blaze/aspect/PackageParserTest.java | 2 +- aswb/BUILD | 1 + base/BUILD | 1 + .../BuildEventProtocolOutputReader.java | 2 +- .../buildresult/OutputArtifactParser.java | 2 +- .../idea/blaze/base/ideinfo/ProtoWrapper.java | 2 +- .../buildfile/sync/BuildLangSyncPlugin.java | 2 +- .../idea/blaze/base/model/SyncData.java | 2 +- .../sync/aspects/strategy/AspectStrategy.java | 2 +- .../fastbuild/FastBuildAspectStrategy.java | 2 +- kotlin/BUILD | 1 + .../run/BlazeIntellijPluginDeployer.java | 2 +- proto/BUILD | 36 +++++-------------- proto/jarjar_protobuf_rule.txt | 1 - .../debugger/impl/SkylarkDebugProcess.java | 3 +- 17 files changed, 24 insertions(+), 41 deletions(-) delete mode 100644 proto/jarjar_protobuf_rule.txt diff --git a/aspect/tools/BUILD b/aspect/tools/BUILD index 31b569693eb..245c97a84c4 100644 --- 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 index 5106b28e700..26be71aaf31 100644 --- 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 index 59edb57db05..18a6b0ee88e 100644 --- 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 index 878926f25dd..ef84cca6662 100644 --- 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 index f089575da7e..f724495e336 100644 --- 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 index 70acbc897b3..64abc0aefd9 100644 --- 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 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 index 6b7e24199bc..0823b6e1cf8 100644 --- 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 index b1c5d1b6368..67d49c721e7 100644 --- 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 index 254e35f62b4..254af9feb79 100644 --- 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 index eb1294bc03f..7397756755a 100644 --- 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 index 8feddcbc421..4ed4feb1ae9 100644 --- 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 index 339920f311e..7c241b28875 100644 --- 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 index 8641ee1b162..40c25c82812 100644 --- 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 index 5e4363add02..e19678c3fac 100644 --- 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 index e853fd9597c..12fafc4f5a5 100644 --- 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 deleted file mode 100644 index d7fa74ae7db..00000000000 --- a/proto/jarjar_protobuf_rule.txt +++ /dev/null @@ -1 +0,0 @@ -rule com.google.protobuf.** com.google.repackaged.bazel.protobuf.@1 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 index 7d89013f510..86fe42eb81d 100644 --- 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 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(