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

Don't export via deps, only via exports #563

Merged
merged 4 commits into from
Jul 7, 2021

Conversation

illicitonion
Copy link
Contributor

#557 replaced using
JavaInfo.transitive_exports by instead using
HasMavenDeps.transitive_exports but it changed how this was computed.

The JavaInfo traversal code:
https://github.com/bazelbuild/bazel/blob/5a647bb234ba9817f51fdb2b4b5973a0602f51a4/src/main/java/com/google/devtools/build/lib/rules/java/JavaCommon.java#L341-L357
only follows the exports attribute, and merges together each of the
transitive exports it finds that way.

The replacement starlark code was traversing each of deps, exports,
and runtime_deps, looking for anything which may have been exported
to those targets, which exports significantly more targets.

This adds a unit test to prevent future regressions.

bazelbuild#557 replaced using
`JavaInfo.transitive_exports` by instead using
`HasMavenDeps.transitive_exports` but it changed how this was computed.

The JavaInfo traversal code:
https://github.com/bazelbuild/bazel/blob/5a647bb234ba9817f51fdb2b4b5973a0602f51a4/src/main/java/com/google/devtools/build/lib/rules/java/JavaCommon.java#L341-L357
only follows the `exports` attribute, and merges together each of the
transitive exports it finds that way.

The replacement starlark code was traversing each of `deps`, `exports`,
and `runtime_deps`, looking for anything which may have been exported
_to_ those targets, which exports significantly more targets.

This adds a unit test to prevent future regressions.
@illicitonion
Copy link
Contributor Author

/cc @comius in case the change in behaviour was intended

@comius
Copy link
Contributor

comius commented May 25, 2021

/cc @comius in case the change in behaviour was intended

No, it was unintentional; I assume you have a case that is broken/different now. Thanks for the fix and the test!

Didn't the old code here https://github.com/bazelbuild/rules_jvm_external/pull/557/files#diff-4c35294998684c53cec071ff25a96119a3545b55579a7a21233c83870ed37c6cL56-L58 also collect transitive_exports from deps and runtime_deps? Because artifact_infos are gathered over _ASPECT_ATTRS?

Do collect transitive exports, but only via a direct exports attribute
somewhere in the tree.
@illicitonion
Copy link
Contributor Author

illicitonion commented Jun 21, 2021

Sorry for the delay - I've been out on PTO :)

/cc @comius in case the change in behaviour was intended

I assume you have a case that is broken/different now.

I do indeed!

Didn't the old code here https://github.com/bazelbuild/rules_jvm_external/pull/557/files#diff-4c35294998684c53cec071ff25a96119a3545b55579a7a21233c83870ed37c6cL56-L58 also collect transitive_exports from deps and runtime_deps? Because artifact_infos are gathered over _ASPECT_ATTRS?

I have spent quite a while staring at this code, and I think I now understand the key difference. JavaInfo itself does filtering for us to only follow exports attributes when computing transitive_exports - so transitive_exports is arguably a misleading name - it's really transitive_exports_via_my_exports - target X only has something appear in its transitive_exports if the route from X to that target is in the exports attribute of X.

Your change had the effect of removing this "only via the exports of X" filtering for maven_project_jar targets - before we consulted its target's target[JavaInfo].transitive_exports which performed the "only via my exports" filtering, and now we don't because we're reading its target[MavenInfo].transitive_exports which includes transitive_exports from its target's deps attribute.

I've just pushed a commit which I think makes this more clear - when traversing the tree to gather, we only accumulate the transitive_exports if the attribute we went via to get to the target we're accumulating from was itself exports. I also added a test on the transitive_runtime_deps the maven_project_jar sees, as this is the root cause of my failure in a private repo.

Note that this is still a slightly change in behaviour, but based on non-public examples I think it's closer to the original behaviour.

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.

Sorry it took so long to review. Thank you @illicitonion!

@shs96c shs96c merged commit d7c7de4 into bazelbuild:master Jul 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants