Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix things after bzlmod migration #5888

Merged
merged 29 commits into from
Feb 27, 2024
Merged

Fix things after bzlmod migration #5888

merged 29 commits into from
Feb 27, 2024

Conversation

agluszak
Copy link
Collaborator

@agluszak agluszak commented Dec 21, 2023

Second attempt at bzlmod migration. This time there's almost no naming changes and I verified that it works in IntelliJ 2023.3 on Linux.

Fixes #5432.

TODO:

  • Fix test com.google.idea.blaze.base.qsync.DependencyTrackerImplTest.getPendingExternalDeps_followJavaDeps_allBuilt(DependencyTrackerImplTest.java:201)
  • Figure out how to regenerate the module lockfile for all systems/configurations at once (cc @tpasternak)
  • [] Test on non-Linux OSes

@agluszak agluszak mentioned this pull request Dec 21, 2023
3 tasks
# Conflicts:
#	MODULE.bazel
#	MODULE.bazel.lock
#	WORKSPACE.bzlmod
#	aspect/testing/tests/src/com/google/idea/blaze/aspect/java/pluginprocessorjars/BazelPluginProcessorJarTest.java
#	aspect/tools/BUILD
#	ext/proto/BUILD
#	intellij_platform_sdk/BUILD
#	maven_install.json
#	querysync/javatests/com/google/idea/blaze/qsync/BUILD
#	querysync/javatests/com/google/idea/blaze/qsync/testdata/grpc/BUILD
@agluszak
Copy link
Collaborator Author

agluszak commented Jan 4, 2024

bazel-contrib/rules_jvm_external#1016 the problem with grpc-java dependency names seems to have been fixed now, so I can revert that part if you consider it problematic @mai93

@mai93
Copy link
Collaborator

mai93 commented Jan 8, 2024

Yes, I think it will be better if we removed the extra files: non_module_deps.bzl, tools/BUILD and tools/java_grpc_library.bzl.

For getPendingExternalDeps_followJavaDeps_allBuilt(DependencyTrackerImplTest.java:201), we need to change the dependency label added here with the full label with the dependency repo name including the ~ to keep it testing the same thing and the expected result should be empty.

@agluszak
Copy link
Collaborator Author

bazelbuild/rules_jvm_external#1016 the problem with grpc-java dependency names seems to have been fixed now, so I can revert that part if you consider it problematic @mai93

Unfortunately as I mentioned in Slack it turned out that grpc-java dependency names are still incompatible with names used in this repository. So we have to either

  1. change the names used in this repo (so we'd be back to 100 BUILD file changes)
  2. use this solution which removes the source dependency on grpc-java
  3. wait until grpc-java migrates to bzlmod (which can take a lot of time)

@mai93
Copy link
Collaborator

mai93 commented Jan 16, 2024

@Wyverald we are trying to migrate the plugin to use bzlmod and we have a problem with the lock file. the plugin has a dependency on rules_go which adds an entry in the lock file that is specific to the architecture on which it is created, this causes a problem when the plugin is built on a different OS/architecture. Is there a way around this? @tpasternak for more details

@Wyverald
Copy link
Member

this causes a problem when the plugin is built on a different OS/architecture

What is the problem? If you use the latest rules_go, then the lockfile would contain different sections for rules_go depending on which platform you're building on. You'd need to update the lockfile from each of those different platforms, but ultimately they shouldn't step on each other's toes.

cc @SalmaSamy

@SalmaSamy
Copy link

@mai93 As @Wyverald mentioned the lockfile should handle this you just have to make sure:

  1. The rules_go version you're using contains this change
  2. The bazel version you're using contains this change

Basically, using the latest in both should result in a lockfile with sections for each platform like the example here

@mai93
Copy link
Collaborator

mai93 commented Jan 17, 2024

@tpasternak FYI, the response from bazel team about bzlmod lock file question

Copy link
Contributor

@tpasternak tpasternak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some loose thoughts:

  1. I'm getting this warning when trying to build the project:
DEBUG: /home/t/.cache/bazel/_bazel_t/9f891cf4952c58dbb7dca5b940b19431/external/rules_jvm_external~override/private/extensions/maven.bzl:139:14: The maven repository 'maven' is used in two different bazel modules, originally in '' and now in 'protobuf'
  1. I'm still getting some modifications to MODULE.bazel after build, like this one:
     "@@bazel_tools//tools/cpp:cc_configure.bzl%cc_configure_extension": {
       "general": {
-        "bzlTransitiveDigest": "O9sf6ilKWU9Veed02jG9o2HM/xgV/UAyciuFBuxrFRY=",
+        "bzlTransitiveDigest": "mcsWHq3xORJexV5/4eCvNOLxFOQKV6eli3fkr+tEaqE=",
         "accumulatedFileDigests": {},
         "envVariables": {},
         "generatedRepoSpecs": {
@@ -1894,7 +1903,14 @@
               "name": "bazel_tools~cc_configure_extension~local_config_cc_toolchains"
             }
           }
-        }
+        },
+        "recordedRepoMappingEntries": [
+          [
+            "bazel_tools",
+            "bazel_tools",
+            "bazel_tools"
+          ]
+        ]
       }
     },
  1. I'm not sure what was the conclusion from the last meeting/chats, but we've been talking about keeping rules_go in as a non-bzlmod dep

  2. Same for grpc - this might be quite hard to maintain, so personally I would prefer to keep in the WORKSPACE def, too, but it's not a strong opinion, current is also ok.

MODULE.bazel Outdated Show resolved Hide resolved
MODULE.bazel Outdated Show resolved Hide resolved
aswb/BUILD Show resolved Hide resolved
build_defs/build_defs.bzl Show resolved Hide resolved
non_module_deps.bzl Outdated Show resolved Hide resolved
non_module_deps.bzl Outdated Show resolved Hide resolved
tools/java_grpc_library.bzl Outdated Show resolved Hide resolved
tools/BUILD Outdated Show resolved Hide resolved
# Conflicts:
#	.bazelrc
#	base/tests/unittests/com/google/idea/blaze/base/qsync/DependencyTrackerImplTest.java
#	querysync/javatests/com/google/idea/blaze/qsync/testdata/TestData.java
@agluszak agluszak marked this pull request as ready for review February 22, 2024 18:27
@agluszak agluszak requested a review from jastice as a code owner February 22, 2024 18:27
@github-actions github-actions bot added product: CLion CLion plugin product: IntelliJ IntelliJ plugin product: GoLand GoLand plugin awaiting-review Awaiting review from Bazel team on PRs labels Feb 22, 2024
MODULE.bazel Outdated
bazel_binaries,
"bazel_binaries",
"bazel_binaries_bazelisk",
"build_bazel_bazel_4_2_4",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the test for version 4.2.4 is missing: https://buildkite.com/bazel/intellij-plugin-aspect/builds/21280#018dd20e-4bca-481d-8d9b-060f884523f5.

We actually need to test with a version before 6 and a version after it because based on that we change the label of the aspect repo in the sync invocation.

MODULE.bazel Show resolved Hide resolved
@mai93
Copy link
Collaborator

mai93 commented Feb 27, 2024

I addressed the comments I had and manually tested it. I will merge it tomorrow if no one has a problem.

@mai93 mai93 merged commit 3963380 into master Feb 27, 2024
8 checks passed
@github-actions github-actions bot removed the awaiting-review Awaiting review from Bazel team on PRs label Feb 27, 2024
@agluszak
Copy link
Collaborator Author

Thanks @mai93! Sorry for not handling it, I don't work on Mondays and Tuesdays

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product: CLion CLion plugin product: GoLand GoLand plugin product: IntelliJ IntelliJ plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate to Bzlmod for managing external dependencies
6 participants