bazelbuild / intellij

IntelliJ plugin for Bazel projects
https://ij.bazel.build/
Apache License 2.0
760 stars 303 forks source link

Revert "Automatic code cleanup." - Remove dependency on rules_java #6476

Closed LeFrosch closed 3 months ago

LeFrosch commented 3 months ago

This reverts commit 697b9a9a9fe4d0b3c0672a694d8bbd3a5e5a5d4e.

Checklist

Please note that the maintainers will not be reviewing this change until all checkboxes are ticked. See the Contributions section in the README for more details.

Discussion thread for this change

Issue number: #6474

Description of this change

It seems to be the case that if a project:

The project can no longer be synced because of an error like this:

ERROR: Traceback (most recent call last):
    File "<redacted>/external/intellij_aspect/intellij_info_impl_bundled.bzl", line 3, column 37, in <toplevel>
        load("@rules_java//java:defs.bzl", "JavaInfo", "java_common")
Error: file '@rules_java//java:defs.bzl' does not contain symbol 'JavaInfo'

I would suggest reverting this commit for now and to release a hotfix.

mai93 commented 3 months ago

We added a note about this breaking change to the release notes https://github.com/bazelbuild/intellij/releases/tag/v2024.05.21-stable, do you think it is not enough?

ujohnny commented 3 months ago

We added a note about this breaking change to the release notes https://github.com/bazelbuild/intellij/releases/tag/v2024.05.21-stable, do you think it is not enough?

We've received several reports also to JetBrains YouTrack about this issue, I assume we have to take some action based on this feedback. It's partially confusing to require rules_java in C++ projects, for example old rules_java version is coming from googletest dependencies IIUC.

mai93 commented 3 months ago

@hvadehra will this load be mandatory in Bazel 8?

is there a way we can refactor the aspect code to not have this load for C++ workspaces? Until we figure this out we can rollback this commit

hvadehra commented 3 months ago

@hvadehra will this load be mandatory in Bazel 8?

The plan currently is Bazel 8 will not contain these native rules/symbols, so yes, the loads will become necessary then.

Rollback SGTM for now.

is there a way we can refactor the aspect code to not have this load

load() statements unfortunately can't be conditional. If we have different plugin releases by Bazel versions, we can to handle this in the packaging/release step for the plugin. Otherwise, ISTM the plugin may need to use different root bzl files (i.e. one with loads and one without) for the aspect(s) that will be conditioned on the Bazel version.