Add support to auto-index Bazel builds with an aspect #610

olafurpg · 2023-07-07T13:52:28Z

Previously, the Bazel example required monkeypatching the java_library rule in order to inject the SemanticDB compiler plugin. This PR is an exploration to replace monkeypatching with aspects, which is a Bazel feature that seems made for our exact use-case. The goal of this PR is to make it possible to auto-index Bazel codebases with as little changes as possible to the existing build configuration in WORKSPACE and BUILD files.

With this change, it should be possible to index most Java/Bazel codebases with the following command

scip-java index --bazel-scip-java-binary=$(which scip-java)

Test plan

See new bazel_aspect CI job that indexes scip-java itself and the example workspace
Green CI.

olafurpg · 2023-07-07T13:53:45Z

When I currently run the aspect I'm getting this error here

❯ bazel build //src/main/java/example  --aspects aspects/semanticdb.bzl%semanticdb_aspect --output_groups=semdb --define=execroot=$(realpath bazel-$(basename $(pwd))) --define=semdb_path=$(cs fetch com.sourcegraph:semanticdb-javac:0.8.22 --classpath) --nojava_header_compilation

INFO: Analyzed target //src/main/java/example:example (0 packages loaded, 0 targets configured).
INFO: Found 1 target...
ERROR: /Users/olafurpg/dev/sourcegraph/scip-java/examples/bazel-example/src/main/java/example/BUILD:13:13: Building src/main/java/example/example-with-semdb.jar (1 source file) failed: (Exit 1): java failed: error executing command external/remotejdk11_macos_aarch64/bin/java -XX:-CompactStrings '--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED' '--add-exports=jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED' ... (remaining 17 arguments skipped)
src/main/java/example/Example.java:2: error: package com.google.common.util.concurrent does not exist
import com.google.common.util.concurrent.Futures;
                                        ^
src/main/java/example/Example.java:6: error: cannot find symbol
		System.out.println(Futures.immediateCancelledFuture());
		                   ^
  symbol:   variable Futures
  location: class Example
Aspect //aspects:semanticdb.bzl%semanticdb_aspect of //src/main/java/example:example failed to build
Use --verbose_failures to see the command lines of failed build steps.
INFO: Elapsed time: 0.130s, Critical Path: 0.06s
INFO: 2 processes: 2 internal.

My guess is that we're not correctly propagating the jvm_import dependencies resulting in the Guava dependency to be missing from the classpath. I will continue investigating next week.

olafurpg · 2023-07-12T15:10:45Z

I tried to reproduce the bazel_aspect issue inside a Linux container but I got the indexing working. Need more investigation why the CI is failing here 🤔 I'm wondering if it's related to bazelbuild/bazel#12485

I briefly investigated the Maven test failures and confirmed it's a regression from the main branch, but I don't know what's causing the regression. It's weird that only Maven is affected, the Gradle tests are passing. I turned on verbose logs with -X but it didn't reveal anything obvious causing the failure

Previously, the Bazel example required monkeypatching the `java_library` rule in order to inject the SemanticDB compiler plugin. This PR fixes this problem by adding support to automatically index Bazel codebases without modifying the build configuration (WORKSPACE or BUILD files). See scip_java.bzl in the diff for more details about how this works. To make this functionality work, this commit extends the functionality of the SCIP build tool to optionally allow users to specify a custom classpath, processorpath, and javac options.

.github/workflows/ci.yml

keynmol · 2023-07-13T09:52:25Z

scip-java/src/main/scala/com/sourcegraph/scip_java/buildtools/ScipBuildTool.scala

-  val ConfigFileName = "lsif-java.json"
+  val ConfigFileNames = List("scip-java.json", "lsif-java.json")
+  val isIgnoredAnnotationProcessor = Set(
+    "",


IMO this is confusing - let's replace it with isEmpty check at use site instead.

I removed "", it wasn't needed.

keynmol · 2023-07-13T09:56:39Z

scip-java/src/main/scala/com/sourcegraph/scip_java/buildtools/BazelBuildTool.scala

+      case None =>
+        1
+      case Some(aspectLabel) =>
+        val buildCommand = ListBuffer.empty[String]


nit: I think it's more idiomatic to use List.newBuilder[String] if you don't access the full accumulated result before you finish appending to it.

As mentioned in https://github.com/sourcegraph/scip-java/pull/610/files#r1262341973 I'm not sure this has to be a mutable thing at all, seems like appending one by one is not strictly necessary in this application.

List.newBuilder[String] is exactly the same thing as ListBuffer at runtime, and ListBuffer is used in the equivalent parts for Gradle/Maven.

keynmol · 2023-07-13T10:12:05Z

scip-java/src/main/scala/com/sourcegraph/scip_java/buildtools/BazelBuildTool.scala

+              "environment variable JAVA_HOME is not set. " +
+                "To fix this problem run `export JAVA_HOME=/path/to/java` and run scip-java index again."
+            )
+          return 1


This entire new logic feels very much like it's written in Java, with Scala syntax.

Would be better to

Remove the builder, replacing it with just a regular list, as I think all the places where you append to it are trivially replaced with

May be using a beefed up assert method that throws (and logs the error through index.app.error)? There's several early exists in this function that IMO doesn't have to be there

Agreed that this is written like Java/Go. I also agree the code in the method is messy.

I've refactored the code to improve clarity but I kept a few return 1 statements instead of adding deeply nested if/else expressions.

I also managed to replace the list builder for buildCommand with a single-expression List.

keynmol · 2023-07-13T10:14:58Z

scip-java/src/main/scala/com/sourcegraph/scip_java/buildtools/BazelBuildTool.scala

+  }
+
+  /**
+   * Callback to `os.ProcessOutput.Readlines` that processes the stderr output


For this and the various tree walking operations - is there a reason we can't just bring in os-lib? Seems like the cli has plenty of dependencies as it is, and it'd be a lot more readable in cases like this to just use os.walk

Unless I'm misreadingh what you mean by os.ProcessOutput.Readlines - this? https://github.com/com-lihaoyi/os-lib/blob/d0661198c915ca037591067ce8967275baca6aae/os/src-jvm/SubProcess.scala#L218

Wait, Moped already used os-lib :D

I'm confused about the use of file visitors then

I love os-lib in general but I'm not a fan os os.Path, which is required to use os.walk. I've never felt the need to use os.walk anyways because Files.walkFileTree works very reliably IME, even if it's a bit verbose.

For the record, the equivalent code (I believe) is

val bazelOutLink = Files.readSymbolicLink(bazelOut) val out = os.Path(index.finalOutput) os.walk( os.Path(bazelOutLink), skip = path => scipPattern.matches(path.toNIO) ) .foreach { p => os.write.append(out, os.read.stream(p)) }

Old habits die hard. I'll admit that looks very nice, esp. knowing how it uses generators under the hood

I tried running it and hit on

java.io.IOException: Is a directory at java.base/sun.nio.ch.FileDispatcherImpl.read0(Native Method)

because there are many directories with the .scip extension, and os.walk visits both files and directories. It's possible to skip = path => os.isFile(path) && ... but that requires an extra fsstat on every visited file

Apparently there's os.walk.attrs, which gives you StatInfo with path: https://sourcegraph.com/github.com/com-lihaoyi/os-lib/-/blob/os/src/ListOps.scala?L193

scip-java/src/main/scala/com/sourcegraph/scip_java/buildtools/BazelBuildTool.scala

scip-java/src/main/scala/com/sourcegraph/scip_java/buildtools/ScipBuildTool.scala

olafurpg

Thank you @keynmol for the review, lots of great feedback and I think the resulting code is much clearer now.

.github/workflows/ci.yml

olafurpg · 2023-07-13T12:23:52Z

scip-java/src/main/scala/com/sourcegraph/scip_java/buildtools/BazelBuildTool.scala

+      case None =>
+        1
+      case Some(aspectLabel) =>
+        val buildCommand = ListBuffer.empty[String]


List.newBuilder[String] is exactly the same thing as ListBuffer at runtime, and ListBuffer is used in the equivalent parts for Gradle/Maven.

olafurpg · 2023-07-13T12:50:55Z

scip-java/src/main/scala/com/sourcegraph/scip_java/buildtools/BazelBuildTool.scala

+              "environment variable JAVA_HOME is not set. " +
+                "To fix this problem run `export JAVA_HOME=/path/to/java` and run scip-java index again."
+            )
+          return 1


Agreed that this is written like Java/Go. I also agree the code in the method is messy.

I've refactored the code to improve clarity but I kept a few return 1 statements instead of adding deeply nested if/else expressions.

I also managed to replace the list builder for buildCommand with a single-expression List.

olafurpg · 2023-07-13T12:53:28Z

scip-java/src/main/scala/com/sourcegraph/scip_java/buildtools/BazelBuildTool.scala

+  }
+
+  /**
+   * Callback to `os.ProcessOutput.Readlines` that processes the stderr output


I love os-lib in general but I'm not a fan os os.Path, which is required to use os.walk. I've never felt the need to use os.walk anyways because Files.walkFileTree works very reliably IME, even if it's a bit verbose.

scip-java/src/main/scala/com/sourcegraph/scip_java/buildtools/BazelBuildTool.scala

scip-java/src/main/scala/com/sourcegraph/scip_java/buildtools/ScipBuildTool.scala

olafurpg · 2023-07-13T13:33:34Z

scip-java/src/main/scala/com/sourcegraph/scip_java/buildtools/ScipBuildTool.scala

-  val ConfigFileName = "lsif-java.json"
+  val ConfigFileNames = List("scip-java.json", "lsif-java.json")
+  val isIgnoredAnnotationProcessor = Set(
+    "",


I removed "", it wasn't needed.

olafurpg · 2023-07-13T13:43:03Z

FYI @sluongng Automatically indexing Bazel/Java codebases via aspects. It's still not fully hermetic but it still manages to index many Java codebases that I've tried it against (big improvement over requiring non-trivial manual configuration!). The aspect could become fully hermetic with a few more improvements that I don't have time to work on right now.

scip-java/src/main/scala/com/sourcegraph/scip_java/buildtools/BazelBuildTool.scala

scip-java/src/main/resources/scip-java/scip_java.bzl

keynmol

Excited for this!

Can't comment much on how idiomatic/correct the starlark script is, but seems fine for what it does.

Very very nice!

olafurpg force-pushed the olafurpg/aspect branch 4 times, most recently from 25b03f6 to 259062d Compare July 12, 2023 12:26

olafurpg marked this pull request as ready for review July 12, 2023 12:27

olafurpg force-pushed the olafurpg/aspect branch from 024a1d7 to d31f496 Compare July 13, 2023 07:48

olafurpg added 7 commits July 13, 2023 11:21

Debug CI failure

ab73d3b

More debugging

8f0be2e

Enable all CI tests

61041d8

Also index scip-java itself with Bazel aspect

d0f85e7

Scalafix

ea47c1a

Clean up code for review

928102e

olafurpg force-pushed the olafurpg/aspect branch from 340dbc0 to 928102e Compare July 13, 2023 09:21

olafurpg requested a review from keynmol July 13, 2023 09:21

keynmol reviewed Jul 13, 2023

View reviewed changes

.github/workflows/ci.yml Show resolved Hide resolved

keynmol reviewed Jul 13, 2023

View reviewed changes

scip-java/src/main/scala/com/sourcegraph/scip_java/buildtools/BazelBuildTool.scala Outdated Show resolved Hide resolved

keynmol reviewed Jul 13, 2023

View reviewed changes

scip-java/src/main/scala/com/sourcegraph/scip_java/buildtools/ScipBuildTool.scala Outdated Show resolved Hide resolved

keynmol reviewed Jul 13, 2023

View reviewed changes

scip-java/src/main/scala/com/sourcegraph/scip_java/buildtools/ScipBuildTool.scala Outdated Show resolved Hide resolved

olafurpg requested a review from jhchabran July 13, 2023 11:09

olafurpg changed the title ~~Implement an aspect to build a codebase with SemanticDB~~ Add support to auto-index Bazel builds with an aspect Jul 13, 2023

Address review comments

e690621

olafurpg commented Jul 13, 2023

View reviewed changes

keynmol reviewed Jul 13, 2023

View reviewed changes

scip-java/src/main/scala/com/sourcegraph/scip_java/buildtools/BazelBuildTool.scala Show resolved Hide resolved

keynmol reviewed Jul 13, 2023

View reviewed changes

scip-java/src/main/scala/com/sourcegraph/scip_java/buildtools/BazelBuildTool.scala Show resolved Hide resolved

keynmol reviewed Jul 13, 2023

View reviewed changes

scip-java/src/main/scala/com/sourcegraph/scip_java/buildtools/BazelBuildTool.scala Outdated Show resolved Hide resolved

keynmol reviewed Jul 13, 2023

View reviewed changes

scip-java/src/main/resources/scip-java/scip_java.bzl Show resolved Hide resolved

Remove unnecessary interpolation

72ff816

olafurpg enabled auto-merge July 13, 2023 14:09

keynmol approved these changes Jul 13, 2023

View reviewed changes

olafurpg merged commit 9dbad49 into main Jul 13, 2023

olafurpg deleted the olafurpg/aspect branch July 13, 2023 14:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support to auto-index Bazel builds with an aspect #610

Add support to auto-index Bazel builds with an aspect #610

olafurpg commented Jul 7, 2023 •

edited

Loading

olafurpg commented Jul 7, 2023

olafurpg commented Jul 12, 2023

keynmol Jul 13, 2023

olafurpg Jul 13, 2023

keynmol Jul 13, 2023

keynmol Jul 13, 2023

olafurpg Jul 13, 2023

keynmol Jul 13, 2023

olafurpg Jul 13, 2023

keynmol Jul 13, 2023

keynmol Jul 13, 2023

keynmol Jul 13, 2023

olafurpg Jul 13, 2023

keynmol Jul 13, 2023

olafurpg Jul 13, 2023

olafurpg Jul 13, 2023

keynmol Jul 13, 2023

olafurpg left a comment

olafurpg Jul 13, 2023

olafurpg Jul 13, 2023

olafurpg Jul 13, 2023

olafurpg Jul 13, 2023

olafurpg commented Jul 13, 2023

keynmol left a comment

Add support to auto-index Bazel builds with an aspect #610

Add support to auto-index Bazel builds with an aspect #610

Conversation

olafurpg commented Jul 7, 2023 • edited Loading

Test plan

olafurpg commented Jul 7, 2023

olafurpg commented Jul 12, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olafurpg left a 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

olafurpg commented Jul 13, 2023

keynmol left a comment

Choose a reason for hiding this comment

olafurpg commented Jul 7, 2023 •

edited

Loading