Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
use jdeps to run dep-usage task #8093
use jdeps to run dep-usage task #8093
Changes from all commits
32fb086
76acff6
76dbd5e
0d0755d
2388ae2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, I think rather than passing in this entire collection, we should only be aliasing the valid classpath entries for the target. This is a correctness issue because as much as we try to avoid it, it is possible for multiple classpath entries to provide a particular class, and that would represent a weird false positive here.
In this case, that would look like using the Zinc subsystem to compute the deps per-target we are going to invoke jdeps on: https://github.com/pantsbuild/pants/blob/master/src/python/pants/backend/jvm/subsystems/zinc.py#L436
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand the false positive issue, but isn't the point of this task to remove reliance on zinc? Maybe we could sort the classpath entries so the ones that come from explicit deps of the target, and their transients, are resolved first? One benefit of using all the classpaths is you can find out if some deps are being used implicitly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The goal is to remove reliance on usage of the zinc tool: but this classpath-computation codepath is our code, and will likely just be renamed at some point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I must be missing something. If we compute the deps directly from the dependency context, what new information do we learn by apply jdeps to a target? It seems like the only thing we can figure out about a target is whether it has missing or extra deps. Maybe I just answered my own question but I had assumed a big part of what we wanted was to resolve what those extra undeclared deps were, which we can't do unless they are on the classpath.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dependency context computes deps based on what is declared in BUILD files (ie, declared target dependencies).
jdeps
computes actual usage of deps (at the classlevel). It's entirely possible to declare a dependency in your BUILD file without using it, and that's exactly what the./pants dep-usage
task is supposed to help you fix.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And with regard to undeclared deps: those are only possible in the case of
strict_deps: False
, and it refers to things that are in your transitive dependencies (ie, they were passed to the compiler), but which were not listed in your declared dependencies (ie, in your BUILD file).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I preemptively made this change because I figured there was a good explanation. So when CI is green again we should be good to ship. Also, noted that the main purpose of dep-usage in unused deps. The jvm-dep-check linting task on the other hand does compare the actual deps with declared deps. Based on the strict_dependencies docs it sounds like transient strict deps that are "exported" are captured in the dependent targets list of the root. If that isn't true then the dep check could fail in some places. It would manifest as a classpath called
not found
being unresolvable to a target.