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

Allow artifacts from aspects to be excluded #898

Merged
merged 5 commits into from
May 24, 2023

Conversation

shs96c
Copy link
Collaborator

@shs96c shs96c commented May 11, 2023

When using a java_proto_library as a dependency of a java_export the generated jar contains classes from protobuf, which is unexpected. This PR allows users to specify a list of workspaces to be excluded from the generated jar, and that allows us to filter the class files generated by these aspects.

@sitaktif
Copy link
Contributor

@shs96c in our discussions, you mentioned an alternate implementation using a { "@repository": "maven:coordinates" } mapping so we can e.g. add protobuf as a maven dependency when the aspect hits the corresponding workspace. Having thought about it a bit more, it seems like a pretty good idea. WDYT? 🙂

@jin
Copy link
Collaborator

jin commented May 22, 2023

Isn't this what java_binary.deploy_env was supposed to do? Have you considered using that?

@shs96c
Copy link
Collaborator Author

shs96c commented May 22, 2023

This isn't quite the same. The problem this PR address is where someone uses an aspect which uses a toolchain, which adds a dependency that the has_maven_deps aspect can't see, and, even if it could, wouldn't handle correctly because it lacks a tag containing maven_coordinates=.

One example is in the test: java_proto_library creates an aspect per proto_library rule, and this both generates the source and then compiles the code using a toolchain with references to the com_google_protobuf workspace. However, the java_export in the test can't see that: it just sees the JavaInfo of the java_proto_library rule, and can't traverse further up the graph, since the jars making up that JavaInfo come from aspects. Even if we did allow the has_maven_deps aspect to walk other aspects, we'd still not be able to spot that the com_google_protobuf workspace typically maps to the maven com.google.protobuf:protobuf-java coordinates.

However, we can't blindly assume that when we see the com_google_protobuf workspace we should add the dep on the maven coordinates, since people do weird things, and there's no guarantee that they're even using the default maven workspace name. This is the reason we add the ability to map workspace names to coordinates (via a Label), but by default we just drop the items coming from com_google_protobuf

deploy_env is slightly different, in that it doesn't allow one to add a dependency to the generated maven artifacts, which we do need to do in this case.

@shs96c
Copy link
Collaborator Author

shs96c commented May 22, 2023

Put another way, the deploy_env expresses what will be in the environment your code will be running in. This PR allows one to either express or mask dependencies that your artifact should depend on, which aren't provided by the environment your code will be running in, in a way that the rest of the java ecosystem will honour properly.

Copy link
Collaborator

@jin jin left a comment

Choose a reason for hiding this comment

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

Thanks for the explanation.

private/rules/maven_project_jar.bzl Outdated Show resolved Hide resolved
When using a `java_proto_library` as a dependency of a `java_export`
the generated jar contains classes from protobuf, which is
unexpected. This PR allows users to specify a list of workspaces to
be excluded from the generated jar, and that allows us to filter the
class files generated by these aspects.
@shs96c shs96c merged commit 0cb0da3 into bazel-contrib:master May 24, 2023
@shs96c shs96c deleted the exclude-aspects branch May 24, 2023 13:27
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.

3 participants