Skip to content

Conversation

vinnybod
Copy link
Contributor

We are migrating a Gradle project to Bazel and can't break compatibility with other Java repos that import the Maven artifacts.

Gradle has a concept of implementation dependencies. These dependencies are available at compile time when building the library, but are given a runtime scope in the generated pom.xml file for consumers of the library to avoid leaking the dependency to the consumers at compile time.

This adds a list to java_export called implementation_deps. The targets passed here and in deps will be given a runtime scope on the generated pom.xml, but will still be available at compile time to the java_library like normal deps. It only impacts pom generation.

This is different than runtime_deps because we still need the dependencies at compile time.

* add bazel equivalent of gradle's 'implementation' scope

* update get_implementation_coordinates

* Update private/rules/maven_utils.bzl

* Update private/rules/pom_file.bzl
@shs96c
Copy link
Collaborator

shs96c commented Apr 26, 2024

Thank you for the PR. I like the idea, but Bazel already allows us to model runtime deps using the runtime_deps attributes. I think the correct approach here is to gather the set of runtime deps and expose the difference of that and the compile time deps with the runtime scope in the generated pom.

@vinnybod
Copy link
Contributor Author

vinnybod commented Apr 26, 2024

Are you suggesting that this would still require changes, just not a new field? Or are you suggesting that I should just be able to pass the deps in as runtime_deps?

I did try duplicating all my deps as runtime_deps and I did not quite get the desired result. This was a while ago, so I don't exactly remember what the issue was. I can revisit it, especially now that I have been in the code a bit more now.

@shs96c
Copy link
Collaborator

shs96c commented Apr 26, 2024

I’m saying we don’t need the new field because the Java rules already allow us to express the concept of a runtime dependency. We need to change how we generate the pom file to reflect this.

@shs96c
Copy link
Collaborator

shs96c commented Apr 26, 2024

We don’t currently draw a distinction between the two kinds of deps in the pom_file rule. We should :)

@vinnybod
Copy link
Contributor Author

I see, thanks! I'll come back to this on Monday and try to do it that way.

@vinnybod
Copy link
Contributor Author

vinnybod commented Apr 27, 2024

@shs96c Maybe you could give me a bit of guidance on where the changes are needed? I've been looking through has_maven_deps.bzl and pom_file.bzl to try to piece it together but am a bit lost on some of the processing steps.

My first thought was just to get the scope into the expanded_maven_deps strings by inserting it into the _extract_from function but that doesn't seem quite right.

@shs96c
Copy link
Collaborator

shs96c commented Apr 28, 2024

In theory, this is relatively straight-forward. In practice, it might well be fiddly.

The way that we figure out which Maven dependencies belong to a maven_project_jar (and therefore used by pom_file) is via the has_maven_deps aspect. You can imagine this being applied to the "bottom" of the build graph (that is, to something which has no dependencies) all the way up to the rule that applied the aspect. As it's applied it does one of two things:

  1. If the target has a maven_coordinates tag, it makes a note of the coordinates, and adds them to a MavenInfo provider.
  2. Otherwise, it accumulates the target's JavaInfo ready for the next rule in the chain to handle.

This means that once we reach the target we're dealing with, the MavenInfo contains the set of dependencies and all the jars to include in maven_project_jar. In order to support the runtime scope, we'll need to do a few things:

  1. Extend MavenInfo to keep track of runtime_deps.
  2. Modify the attributes we apply the has_maven_deps aspect to so that we separate out collection of the runtime_deps (which will likely start by modifying _ASPECT_ATTRS in has_maven_deps.bzl)
  3. When we come to generate the pom.xml, take the difference of the set of runtime and "all" maven dependencies and present them as runtime scoped in the generated file.

We need to take the difference of the sets of dependencies, since one java_libraray might declare a dep on target which another java_library declares as a runtime_dep. In this case, we want to use the default scope for the dependency in the generated pom.xml.

It sounds like a lot, but I think most of the infrastructure for this is already present, since we needed to write some of it to support the deploy_env attribute.

@vinnybod
Copy link
Contributor Author

vinnybod commented Apr 30, 2024

This update adds a new field for tracking runtime_deps in the MavenInfo provider and doesn't require any new parameter on java_export. When testing in our private repository, it generated equivalent poms to the original pull request. I mostly just followed the same pattern as maven_deps, I'm not sure if its the best way to do it, so I look forward to any feedback!

This method didn't require using a diff of any sets, but maybe I am missing the use case.


Working on this does have me thinking of an alternative approach, though it may be considered a breaking change for rules_jvm_external.

It seems like we could take a page out of Gradle's methodology for generated pom files. Using compile scope on the generated pom leaks the dependencies to the consuming maven project, while runtime doesn't.
We could take the approach of putting every dep and runtime_dep as a runtime scoped dependency in the pom, and only use compile as the scope for dependencies that are exports, sort of mirroring how deps, runtime_deps, and export work in Bazel.

Copy link
Collaborator

@shs96c shs96c 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 making these changes. We can add a test case in a follow-up PR, but some manual checks show that this is working as I would expect it. This is a really nice feature, so thank you! LGTM.

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