Migrate to bzlmod #5805
Migrate to bzlmod #5805
Conversation
github-actions
bot
added
product: CLion
aspect/testing/tests/src/com/google/idea/blaze/aspect/general/noide/NoIdeTest.java
Show resolved
Hide resolved
querysync/javatests/com/google/idea/blaze/qsync/QuerySyncTestUtils.java
Outdated
Show resolved
Hide resolved
# Conflicts: # WORKSPACE.bzlmod
| @@ -1,6 +1,6 @@ | |||
| workspace(name = "intellij_with_bazel") | |||
There was a problem hiding this comment.
Why not keep both WORKSPACE files, at least for one release? That way we have a fallback in case something goes wrong.
We might need to migrate the current WORKSPACE to rules_jvm_external, but you've already done it in this PR anyway.
I also see that some tests (esp. the aspect tests) will only work on bzlmod. I think this is fine, I don't think we need to add extra code to handle non-bzlmod cases.
There was a problem hiding this comment.
Why not keep both WORKSPACE files, at least for one release? That way we have a fallback in case something goes wrong.
Because then we will have to keep them in sync. If something goes wrong, we can just revert this PR, can't we?
There was a problem hiding this comment.
If something goes wrong, we can just revert this PR, can't we?
Given the number of files this PR touches, it's likely that someone will merge a change that makes the revert difficult at some point. But, you've already done most of the work for this and it's likely only going to impact us working on the plugin, so I'm not going to block the PR.
| @@ -191,7 +198,8 @@ public void getPendingExternalDeps_followJavaDeps_allBuilt() throws Exception { | |||
| assertThat( | |||
| dt.getPendingExternalDeps( | |||
| ImmutableSet.copyOf(TestData.JAVA_LIBRARY_EXTERNAL_DEP_QUERY.getAssumedLabels()))) | |||
| .isEmpty(); | |||
There was a problem hiding this comment.
Why has this test case changed?
There was a problem hiding this comment.
I'm not sure what this test case is supposed to check, but after my changes dt.getPendingExternalDeps( ImmutableSet.copyOf(TestData.JAVA_LIBRARY_EXTERNAL_DEP_QUERY.getAssumedLabels()))) started returning guava.
There was a problem hiding this comment.
@mai93 , do you know what this test case was supposed to check?
There was a problem hiding this comment.
this should be empty as expected. By debugging I found that this test returns the dependency @com_google_guava_guava//jar:jar in the getLiveCachedTargets() mock which is checked in getPendingExternalDeps() and only the dependencies not present in the cached targets are returned but since the name of this dependency with bzlmod enabled contains repo name with ~ the check fails and the dependency is returned. So what is added in line 197 here needs to be updated and this should assert that the result is empty.
querysync/javatests/com/google/idea/blaze/qsync/testdata/kotlinlib/BUILD
Show resolved
Hide resolved
# Conflicts: # .bazelrc # ext/BUILD # querysync/javatests/com/google/idea/blaze/qsync/BUILD
|
I just tried to import this on my machine (commit
❯ bazel test ... --define=ij_product=intellij-ue-under-dev
INFO: Invocation ID: 10dc268b-91d6-4f50-b4dd-4e2ea010edd0
WARNING: Build options --define and --java_runtime_version have changed, discarding analysis cache (this can be expensive, see https://bazel.build/advanced/performance/iteration-speed).
Analyzing: 791 targets (163 packages loaded, 607 targets configured)
Analyzing: 791 targets (163 packages loaded, 607 targets configured)
[1 / 1] checking cached actions
INFO: Repository rules_bazel_integration_test~0.20.0~bazel_binaries~bazel_binaries_bazelisk instantiated at:
<builtin>: in <toplevel>
Repository rule bazelisk_binary defined at:
/private/var/tmp/_bazel_blorente/99101ccc32544f31e060bfc1a1f5d4f8/external/rules_bazel_integration_test~0.20.0/bazel_integration_test/private/bazel_binaries.bzl:95:34: in <toplevel>
Analyzing: 791 targets (163 packages loaded, 607 targets configured)
[1 / 1] checking cached actions
ERROR: no such package '@@[unknown repo 'remoteapis' requested from @@]//': The repository '@@[unknown repo 'remoteapis' requested from @@]' could not be resolved: No repository visible as '@remoteapis' from main repository
Analyzing: 791 targets (163 packages loaded, 607 targets configured)
ERROR: /Volumes/code/github.com/bazelbuild/intellij/third_party/bazel/src/main/protobuf/BUILD:147:14: no such package '@@[unknown repo 'remoteapis' requested from @@]//': The repository '@@[unknown repo 'remoteapis' requested from @@]' could not be resolved: No repository visible as '@remoteapis' from main repository and referenced by '//third_party/bazel/src/main/protobuf:remote_execution_log_proto'
Analyzing: 791 targets (163 packages loaded, 607 targets configured)
Use --verbose_failures to see the command lines of failed build steps.
Analyzing: 791 targets (163 packages loaded, 607 targets configured)
ERROR: Analysis of target '//third_party/bazel/src/main/protobuf:remote_execution_log_proto' failed; build aborted: Analysis failed
Analyzing: 791 targets (163 packages loaded, 607 targets configured)
INFO: Elapsed time: 1.107s, Critical Path: 0.00s
Analyzing: 791 targets (163 packages loaded, 607 targets configured)
INFO: 1 process: 1 internal.
Analyzing: 791 targets (163 packages loaded, 607 targets configured)
ERROR: Build did NOT complete successfully
FAILED:
FAILED:
ERROR: No test targets were found, yet testing was requested
FAILED:
Fetching repository @@rules_java~7.1.0~toolchains~local_jdk; starting
Fetching https://github.com/bazelbuild/bazelisk/releases/download/v1.18.0/bazelisk-darwin-arm64 |
The first one is trivial, I forgot to leave an empty WORKSPACE file. I'm investigating the second one now |
|
@blorente has |
|
@agluszak Fair enough. This is a command line we run in our pre-submit CI, so I'd expect it to work: $ bazel clean --expunge && bazel test --define=ij_product=clion-latest //:clwb_tests
INFO: Repository rules_java~7.1.0~toolchains~remote_java_tools instantiated at:
<builtin>: in <toplevel>
Repository rule http_archive defined at:
/private/var/tmp/_bazel_blorente/99101ccc32544f31e060bfc1a1f5d4f8/external/bazel_tools/tools/build_defs/repo/http.bzl:384:31: in <toplevel>
ERROR: no such package '@@[unknown repo 'io_netty_netty_transport_classes_kqueue' requested from @@]//jar': The repository '@@[unknown repo 'io_netty_netty_transport_classes_kqueue' requested from @@]' could not be resolved: No repository visible as '@io_netty_netty_transport_classes_kqueue' from main repository
ERROR: /Volumes/code/github.com/bazelbuild/intellij/ext/BUILD:9:13: no such package '@@[unknown repo 'io_netty_netty_transport_classes_kqueue' requested from @@]//jar': The repository '@@[unknown repo 'io_netty_netty_transport_classes_kqueue' requested from @@]' could not be resolved: No repository visible as '@io_netty_netty_transport_classes_kqueue' from main repository and referenced by '//ext:intellijext'
ERROR: Analysis of target '//skylark:unit_tests' failed; build aborted: Analysis failed
INFO: Elapsed time: 5.183s, Critical Path: 0.00s
INFO: 1 process: 1 internal.
ERROR: Build did NOT complete successfully
ERROR: No test targets were found, yet testing was requested
FAILED:
Fetching repository @@bazel_tools~cc_configure_extension~local_config_cc; starting
Fetching repository @@rules_cc; starting
Fetching repository @@com_google_absl; starting
|
|
it's a macos vs linux thingy, because on my (linux) machine it works... let me know if the latest commit fixes it for you. kqueue is only used on macos |
|
Yeah, |
|
@agluszak Yep, commit I think this surfaces how nice it would be to have macOS CI, but that's for another day. Thanks for putting in this much effort. |
| "io.netty:netty-tcnative-boringssl-static:2.0.61.Final", | ||
| "io.netty:netty-tcnative-classes:2.0.61.Final", | ||
| "io.netty:netty-transport-classes-epoll:4.1.97.Final", | ||
| "io.netty:netty-transport-native-epoll:4.1.97.Final", |
There was a problem hiding this comment.
Not sure if this makes any difference, but previously it was x86_64 version (just a side note, don't care
| "//conditions:default": ["@io_netty_netty_transport_native_epoll//jar"], | ||
| "//conditions:default": [ | ||
| "@io_netty_netty_transport_classes_epoll//jar", | ||
| "@io_netty_netty_transport_native_epoll//jar", |
There was a problem hiding this comment.
Might be related to the missing x86_64 version of the library missed above? On the other hand thi looks better if we dont reference arch directly
There was a problem hiding this comment.
Ok, something was transitively loading com_google_protobuf, and probably invoking this transitievly 👍
| @@ -54,7 +54,8 @@ public enum TestData { | |||
| this.srcPaths = stream(paths).map(Path::of).collect(toImmutableList()); | |||
| } | |||
|
|
|||
| private static final String WORKSPACE_NAME = "intellij_with_bazel"; | |||
| // TODO: Probably there's a better way to do this. | |||
There was a problem hiding this comment.
Here's the answer, but let's do it later https://bazel.build/external/migration#specify-repo-name
There was a problem hiding this comment.
hmm, adding
module(
name = "intellij_with_bazel",
repo_name = "intellij_with_bazel",
)
to MODULE.bazel doesn't seem to affect it
There was a problem hiding this comment.
I think this is related to bazelbuild/bazel#18128
|
|
||
| load("@com_google_protobuf//:protobuf_deps.bzl", "protobuf_deps") | ||
|
|
||
| protobuf_deps() |
There was a problem hiding this comment.
That's the only new thing, compared to the old WORKSPACE, do you remember where it came from? If not, then let's merge it, and document later
There was a problem hiding this comment.
so previously something (I don't remember/didn't check) loaded com_google_protobuf as a transitive dependency and after I switched that something to bzlmod it became inaccessible
|
Oh no, I just noticed windows build doesn't work, but maybe we could address it later. It looks like I hit this problem protocolbuffers/protobuf#12947 EDIT: I was able to workaround it by just shortening the output_base path so NVM |
github-actions
bot
removed
the
awaiting-review
There was a problem hiding this comment.
I also tried using the plugin built with this change to import the simple java project defined in https://github.com/bazelbuild/intellij/blob/master/examples/java/greetings_project/hello/hello.bazelproject and the initial build failed with:
Error: Unable to initialize main class com.google.idea.blaze.aspect.PackageParser
Caused by: java.lang.UnsupportedClassVersionError: com/google/protobuf/MessageLite has been compiled by a more recent version of the Java Runtime (class file version 61.0), this version of the Java Runtime only recognizes class file versions up to 55.0
It is working fine on the commit right before this.
| @@ -191,7 +198,8 @@ public void getPendingExternalDeps_followJavaDeps_allBuilt() throws Exception { | |||
| assertThat( | |||
| dt.getPendingExternalDeps( | |||
| ImmutableSet.copyOf(TestData.JAVA_LIBRARY_EXTERNAL_DEP_QUERY.getAssumedLabels()))) | |||
| .isEmpty(); | |||
There was a problem hiding this comment.
this should be empty as expected. By debugging I found that this test returns the dependency @com_google_guava_guava//jar:jar in the getLiveCachedTargets() mock which is checked in getPendingExternalDeps() and only the dependencies not present in the cached targets are returned but since the name of this dependency with bzlmod enabled contains repo name with ~ the check fails and the dependency is returned. So what is added in line 197 here needs to be updated and this should assert that the result is empty.
| @@ -34,8 +34,8 @@ | |||
| public class BazelPluginProcessorJarTest extends BazelIntellijAspectTest { | |||
| // blaze and bazel have different annotation processor paths & names. | |||
| private static final String JAR_STR = | |||
| jarString("auto-value-1.6.2.jar", /*iJar=*/ null, /*sourceJar=*/ null); | |||
| private static final String OUTPUT_GROUP_FILES = "../auto_value/auto-value-1.6.2.jar"; | |||
| jarString("external/rules_jvm_external~5.3~maven~maven/com/google/auto/value/auto-value/1.10.2/processed_auto-value-1.10.2.jar", /*iJar=*/ null, /*sourceJar=*/ null); | |||
There was a problem hiding this comment.
I'm not sure if this is the right approach or we should update the aspect to correctly strip the new external dependencies path prefix. we need to look more into why it is needed.
@tpasternak is this considered a regression for using the plugin on windows or do you mean developing the plugin on windows? |
Unfortunately it looks like a general bzlmod issue. Luckily it only affects development on Windows (and other platforms), not using the plugin |
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:
<please reference the issue number or url here>Description of this change
Closes #5432.
Migration to bzlmod will be required at some point. I replaced all
jvm_maven_import_externalwithrules_jvm_external's mechanism so that we have a single point for downloading jar dependencies. I bumped versions of some rules - these which had no support for bzlmod.