BEF/BJLS: support Bazel target scoped classpaths

[guw]

Sample project: https://github.com/salesforce/bazel-jdt-java-toolchain

//builder/src/main/java

java_library(
    name = "jdt_java_builder_lib",
    srcs = [
        "com/salesforce/bazel/jdt/toolchain/builder/InvalidCommandLineException.java",
        "com/salesforce/bazel/jdt/toolchain/builder/JdtJavaBuilder.java",
        "com/salesforce/bazel/jdt/toolchain/builder/SimpleOptionsParser.java",
    ],
    deps = [
        "//builder/src/main/java/com/salesforce/bazel/jdt/toolchain/builder/jarhelper",
        "//builder/src/main/protobuf:deps_java_proto",
        "//builder/src/main/protobuf:java_compilation_java_proto",
        "//builder/src/main/protobuf:worker_protocol_java_proto",
        "@rules_jdt_guava",
        "//builder/third_party/ecj",
    ],
    visibility = ["//visibility:public"],
)

//builder/src/main/com/salesforce/bazel/jdt/toolchain/builder/jarhelper

java_library(
    name = "jarhelper",
    srcs = [
        "JarCreator.java",
        "JarHelper.java",
    ],
    visibility = ["//visibility:public"],
)

Coming up with project names and representation is not easy.

Should we use target names? Should we use project names inherited from the full folder structure? Can we do something else?

[guw]

@plaird Can you share some context why the current logic is implemented this way: https://github.com/salesforce/bazel-eclipse/blob/c8c20c22a155774b304d5a05edf426e87bfe13e9/bundles/com.salesforce.bazel.eclipse.common/src/main/java/com/salesforce/bazel/eclipse/project/EclipseProjectUtils.java#L82-L87

Given that there can be multiple java targets in a single package - each with entirely different classpaths - shouldn't the target name become a qualifier for the project name (if not a dominator)?

To follow the example above: two projects will be created in Eclipse: jdt_java_builder_lib and jarhelper

Alternatively I can think of:

1. builder - jdt_java_builder_lib
2. java - jdt_java_builder_lib
3. builder-src-main-java (jdt_java_builder_lib)
4. jdt_java_builder_lib (builder-src-main-java)

Or other variations of it.

3+4 will have the benefit of being unique in the Eclipse workspace. The number suffix would disappear. However, they are very verbose and there could be special cases with overlap (eg., //package-special/somedir vs. //package/special/somedir). One could argue those should be clean-ed up in the Bazel workspace, though.

[plaird]

Good question, this is a design decision that I should document somewhere. It is discussed in docs here and there and discussed in some Issues. Remember that we map the whole Bazel package into JDT main/test classpath construct using a union classpath of all java targets in the package:

https://github.com/salesforce/bazel-eclipse/blob/master/docs/bef/using_the_feature_classpath.md#classpaths-are-scoped-to-each-eclipse-project

There are a few reasons for this...

1. With target scoped projects there would be an explosion of Eclipse projects inside the IDE. Personally, I don't like to have more than 10-20 projects open in Eclipse at any time. I typically develop with about 10 Bazel packages at a time in our monorepo (use case dependent, but for what I work on this is normal). If you import packages with 5 targets each, the experience gets bloated really quickly.

2. Even if we implemented this, JDT would be awkward anyway because a single java source file can be consumed by multiple java targets within the package...with different classpaths. Right now if you do this in BEF you can add an import that is valid in the Java editor but breaks the command line build, which isn't great. If we had target scoped classpaths, this would appear in JDT as the same source file being linked to multiple Eclipse projects. But then code completion would be context sensitive - how did you open the file? Not only is this confusing, but again you could add something that broke the command line build for another target. I don't think this is great user experience either. If we did all this work to implement this, I would expect a better experience.

3. In Bazel, each test class has its own classpath. So to be correct, every test would need to be a distinct Eclipse project as well. With some of our packages having 200 test classes, this is nasty.

4. Level of effort and complexity of the BEF code. This would be a major change and require quite a bit of work to get right. Test coverage of this would be a big project by itself. Just consider all the variations of the above (test classpaths, launchers, etc).

Given what else we need to work on to make BEF better, it just hasn't been a priority for me to provide a finer grained classpath. The package scope of classpath is a compromise between usability and correctness, and requires a lower level of effort and testing. As a user myself, I personally would not welcome having target scoped Eclipse projects, as for our monorepo that would be a step backwards in usability.

As a comparison, the IJ plugin puts the entire Bazel workspace into a single project and classpath. So BEF is far better than that with package scoped classpaths. If your team wanted to do this, we could make it an import option for users that want that level of granularity. But I honestly don't think most users would prefer it.

So back to the above computeEclipseProjectNameForBazelPackage() method, appending a target name would actually be confusing, because that would imply the classpath was scoped to the target in the IDE.

Other things:

1. We do plan to support selective target import, that is in fact the theme of the next minor release: https://github.com/salesforce/bazel-eclipse/projects/12. Having that would allow the user to reduce the project explosion problem. But this feature makes most sense if users exclude entire types of targets (springboot) as hand picking packages for import is tedious already. Project Views help with this, but....
2. Having target scoped classpaths would solve the circular reference problem in #197 which is nice. But in practice has been easily solved by just changing the BUILD files so hasn't been a priority for us.

[guw]

@plaird Thanks for sharing. I have a few follow up question/comments.

• Why is "Bazel Workspace (name)" mostly empty, i.e. only links? Wouldn't it be better if the full workspace is made accessible from a file browsing perspective? It could be a simple project without any Java.

• Problem for shared sources between targets is a valid concern. Would it work to exclude that case and ask developers to restructure their projects for better granularity and dependency management? How does IntelliJ deals with this?

• In our mono-repo, developers would need to work with 50-150 targets at a time. They are used to have this as projects. I'm not sure if this is the best approach, though.

• I'm not a big fan of linking. It takes away the ability to use Eclipse tools such as EGit for Git management. Have there been experiments with creating projects inside the source tree and using filtering capabilities?

[plaird]

@guw good questions, responses....

Why is "Bazel Workspace (name)" mostly empty, i.e. only links? Wouldn't it be better if the full workspace is made accessible from a file browsing perspective? It could be a simple project without any Java.

I am open to this. I last worked on this in #180 but in a limited way. BTW, this is changing for some use cases due to the work in #404

Problem for shared sources between targets is a valid concern. Would it work to exclude that case and ask developers to restructure their projects for better granularity and dependency management?

Yes, but this is why I get worried about the mental complexity of target scoped Eclipse projects. It just feels like it will be hard to convey the sharp edges to users. "I just tried to import this workspace, and BEF vomited up all these weird complaints. I just want to edit some Java code". So I wouldn't want it to be the default option, if we do it.

How does IntelliJ deals with this?

IJ builds a single union classpath for the entire Bazel workspace. They talked about fixing this at one point.

In our mono-repo, developers would need to work with 50-150 targets at a time. They are used to have this as projects. I'm not sure if this is the best approach, though.

OK. That is why I think target scoped projects is still worthwhile option for some use cases. Just don't like it as the default behavior. BTW, the way I see users working with your monorepo is your team adds a Project View creator to your setup CLI (obfuscating internal SFDC details here, that you and I both know). So you will ask the users what packages they will work with up-front, and it generates a Project View file. The Project View file would need to signal that the Eclipse projects should use target scoped import.

I'm not a big fan of linking. It takes away the ability to use Eclipse tools such as EGit for Git management. Have there been experiments with creating projects inside the source tree and using filtering capabilities?

Agreed, this would be good to support. No, we haven't investigated it.