Skip to content

Conversation

@agluszak
Copy link

No description provided.

@agluszak
Copy link
Author

Need #423 to be taken care of first anyway

Copy link
Contributor

@ted-xie ted-xie left a comment

Choose a reason for hiding this comment

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

Thank you for the PR! I left a few comments, but overall we can't accept submissions which change the ordering for things like attributes and starlark load()s, since the original ordering of the loads reflects the reality of our primary source of truth: the Google monorepo.

Other changes in this PR such as updating the dep versions and adding missing loads are greatly appreciated.

MODULE.bazel Outdated
# Shared maven repository for bazel_worker_java and protobuf dependencies
maven.install(
artifacts = [
"com.google.protobuf:protobuf-java:4.33.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't be necessary, right? In my mental model, if project A depends on project B that has a maven install, then project B's maven should be namespaced to that project, and project A shouldn't have to declare its deps.

maven = use_extension("@rules_jvm_external//:extensions.bzl", "maven")
maven.install(
artifacts = [
"com.google.protobuf:protobuf-java:4.33.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

This definitely should not go here. We should not expect user projects to declare deps for deps of deps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants