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

use jdeps to run dep-usage task #8093

Merged
merged 5 commits into from Jul 26, 2019

Conversation

@hrfuller
Copy link
Contributor

commented Jul 22, 2019

Problem

closes #7995

The current implementation of the dep-usage goal uses analysis files which are output from zinc compiles to track dependencies between java and scala targets. We want to replace zinc with rsc eventually so we need to disconnect the dep usage and dep linting tasks from zinc output.

Solution

We replace using zinc analysis files to track target dependencies with the the jdk tool jdeps. We also change the format for the product produced by the AnalysisExecution task from a nested {target: {src_file: [dependencies]}} to a single `{target: [dependencies]} which removes an intermediate layer of granularity, simplifying the shape of the analysis product. We do this because jdeps does not have any information about which source files compiled to which class files, and it cannot because that information is not contained in .class files.

Result

Changes are transparent to the user with the exception of the format of one error message when the user requests pants lint with the --jvm-dep-check-missing-direct-deps={warn,fatal} previously the error message would reference the source file that had an implicit dependency, and what the dependency is, but now we just reference the target address which has the implicit dependency. This is still enough information to fix the linting issue, because dependencies are specified on a target level anyway.

@stuhood
Copy link
Member

left a comment

Thanks, looks good!

We'll need to add deprecations for the removed products, and find a cleaner way to locate the jdeps binary, but should be able to save that for when CI is greener.

invalidate_dependents=True) as invalidation_check:
for vt in invalidation_check.all_vts:
# class paths for the target we are computing deps for
target_cps = [cp[1] for cp in classpath_product.get_for_target(vt.target)]

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 23, 2019

Member

Rather than grabbing things out of tuples with indexes, I'd recommend "destructuring" them to give the relevant members of the tuple names. ie:

[entry for _, entry in classpath_product.get_for_target(vt.target)]

(here and elsewhere)

@stuhood

This comment has been minimized.

Copy link
Member

commented Jul 23, 2019

You had a bootstrap shard flake: have restarted it. Sorry for the trouble!

@stuhood

This comment has been minimized.

Copy link
Member

commented Jul 24, 2019

Regarding deprecation: what you can do is claim to continue to export these products in product_types, and then add a:

deprecated_conditional(
  lambda: self.context.products.is_required_data('classes_by_source'),
  '1.20.0.dev2',
  'The `classes_by_source` product depends on internal compiler details and is no longer produced.'
)
@stuhood
Copy link
Member

left a comment

This looks great! One correctness tweak, and then I think we can land this.

for vt in invalidation_check.all_vts:
summary_json_file = self._summary_json_file(vt)
cp_entry, _, analysis_file = zinc_analysis[vt.target]
# class paths for the artifacts created by the target we are computing deps for

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 24, 2019

Member

Nit: A convention in the codebase is for comments to be complete sentences: https://www.pantsbuild.org/styleguide.html

This comment has been minimized.

Copy link
@hrfuller

hrfuller Jul 25, 2019

Author Contributor

Yeah will fix. I think this was just a note for myself while working.

return re.compile(r"^.+\s->\s(.+)$")

def _run_jdeps_analysis(self, target, target_artifact_classpaths, potential_deps_classpaths, jdeps_output_json):
with self.aliased_classpaths(potential_deps_classpaths) as classpaths_by_alias:

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 24, 2019

Member

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

This comment has been minimized.

Copy link
@hrfuller

hrfuller Jul 25, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 25, 2019

Member

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.

This comment has been minimized.

Copy link
@hrfuller

hrfuller Jul 25, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 25, 2019

Member

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.

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 25, 2019

Member

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).

This comment has been minimized.

Copy link
@hrfuller

hrfuller Jul 25, 2019

Author Contributor

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.

@hrfuller

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2019

@hrfuller hrfuller force-pushed the twitter:hfuller/jdeps-usage branch from 5eced80 to 2388ae2 Jul 26, 2019

@stuhood
Copy link
Member

left a comment

Thanks a lot!

@stuhood stuhood merged commit b5d41da into pantsbuild:master Jul 26, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@stuhood stuhood deleted the twitter:hfuller/jdeps-usage branch Jul 26, 2019

@illicitonion

This comment has been minimized.

Copy link
Contributor

commented Jul 29, 2019

Apologies, I'm going to revert this because @stuhood found that it causes some issues in the pantsbuild repo:

$ ./pants dep-usage src/scala/org/pantsbuild/zinc/::
[...]
               FAILURE
timestamp: 2019-07-29T11:58:02.506241
Exception caught: (builtins.AttributeError)
  File "/Users/dwagnerhall/src/github.com/pantsbuild/pants/src/python/pants/bin/pants_loader.py", line 82, in <module>
    main()
  File "/Users/dwagnerhall/src/github.com/pantsbuild/pants/src/python/pants/bin/pants_loader.py", line 78, in main
    PantsLoader.run()
  File "/Users/dwagnerhall/src/github.com/pantsbuild/pants/src/python/pants/bin/pants_loader.py", line 74, in run
    cls.load_and_execute(entrypoint)
  File "/Users/dwagnerhall/src/github.com/pantsbuild/pants/src/python/pants/bin/pants_loader.py", line 67, in load_and_execute
    entrypoint_main()
  File "/Users/dwagnerhall/src/github.com/pantsbuild/pants/src/python/pants/bin/pants_exe.py", line 32, in main
    PantsRunner(start_time=start_time).run()
  File "/Users/dwagnerhall/src/github.com/pantsbuild/pants/src/python/pants/bin/pants_runner.py", line 83, in run
    return runner.run()
  File "/Users/dwagnerhall/src/github.com/pantsbuild/pants/src/python/pants/bin/local_pants_runner.py", line 231, in run
    self._run()
  File "/Users/dwagnerhall/src/github.com/pantsbuild/pants/src/python/pants/bin/local_pants_runner.py", line 299, in _run
    goal_runner_result = self._maybe_run_v1()
  File "/Users/dwagnerhall/src/github.com/pantsbuild/pants/src/python/pants/bin/local_pants_runner.py", line 259, in _maybe_run_v1
    return goal_runner_factory.create().run()
  File "/Users/dwagnerhall/src/github.com/pantsbuild/pants/src/python/pants/bin/goal_runner.py", line 205, in run
    return self._run_goals()
  File "/Users/dwagnerhall/src/github.com/pantsbuild/pants/src/python/pants/bin/goal_runner.py", line 176, in _run_goals
    result = self._execute_engine()
  File "/Users/dwagnerhall/src/github.com/pantsbuild/pants/src/python/pants/bin/goal_runner.py", line 164, in _execute_engine
    result = engine.execute(self._context, self._goals)
  File "/Users/dwagnerhall/src/github.com/pantsbuild/pants/src/python/pants/engine/legacy_engine.py", line 21, in execute
    self.attempt(context, goals)
  File "/Users/dwagnerhall/src/github.com/pantsbuild/pants/src/python/pants/engine/round_engine.py", line 229, in attempt
    goal_executor.attempt(explain)
  File "/Users/dwagnerhall/src/github.com/pantsbuild/pants/src/python/pants/engine/round_engine.py", line 45, in attempt
    task.execute()
  File "/Users/dwagnerhall/src/github.com/pantsbuild/pants/src/python/pants/backend/jvm/tasks/analysis_extraction.py", line 127, in execute
    jdeps_output_json
  File "/Users/dwagnerhall/src/github.com/pantsbuild/pants/src/python/pants/backend/jvm/tasks/analysis_extraction.py", line 146, in _run_jdeps_analysis
    match = self._jdeps_summary_line_regex.fullmatch(line.strip()).group(1)

Exception message: 'NoneType' object has no attribute 'group'

Hopefully we can get it re-landed soon! :)

illicitonion added a commit that referenced this pull request Jul 29, 2019

hrfuller added a commit to twitter/pants that referenced this pull request Jul 29, 2019

Reapply changes made in pantsbuild#8093 which were reverted due to a …
…bug.

use jdeps to run dep-usage task

fix jdeps regex. filter jdeps targets to those with a classpath product

Use jvm SubprocessExecutor to create and spawn jdeps main.
deprecate old analysis execution products.
fix jvm-dep-check target selection to only pick jvm targets.

tidy up comments and deprecation help

use zinc config compile classpath for potential deps discovery

stuhood added a commit to twitter/pants that referenced this pull request Jul 29, 2019

Reapply changes made in pantsbuild#8093 which were reverted due to a …
…bug.

use jdeps to run dep-usage task

fix jdeps regex. filter jdeps targets to those with a classpath product

Use jvm SubprocessExecutor to create and spawn jdeps main.
deprecate old analysis execution products.
fix jvm-dep-check target selection to only pick jvm targets.

tidy up comments and deprecation help

use zinc config compile classpath for potential deps discovery

stuhood added a commit that referenced this pull request Jul 30, 2019

Unrevert #8093 and fix jdeps parsing. (#8125)
### Problem

Previous jdeps PR (#8093) was reverted due to a bug where regex parsing of jdeps output failed.

### Solution

Mitigate root cause where incorrect arguments were passed to jdeps when targets had no deps with classpaths. Add extra layer of safety checking when matching individual lines of the summary output.

### Result

dep-usage task functions more reliably.

stuhood added a commit that referenced this pull request Jul 30, 2019

Unrevert #8093 and fix jdeps parsing. (#8125)
### Problem

Previous jdeps PR (#8093) was reverted due to a bug where regex parsing of jdeps output failed.

### Solution

Mitigate root cause where incorrect arguments were passed to jdeps when targets had no deps with classpaths. Add extra layer of safety checking when matching individual lines of the summary output.

### Result

dep-usage task functions more reliably.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.