Implement zinc `unused deps` check

Review Request #3635 — Created March 31, 2016 and submitted

stuhood
pants
3115
pants-reviews
benjyw, gmalmquist, nhoward_tw, peiyu

This is an implementation of an "unused deps" check for zinc. See https://docs.google.com/document/d/1WfdcP6GE_4eDUZKGfaMcMJonhXxbUQtaNUMxz_7Gi5c/edit# for more information.

  • Add a forced scope to indicate that a particular target is in the default scope, but is not eligible for unused dep checks.
  • Separate JvmDependencyAnalyzer out of the Task hierarchy into a standalone class.
  • Implement the check and return a dict of suggested (ie, more specific) replacements.
  • Move strict/non-strict dep calculation into CompileContext.
  • Remove spurious deps in the examples code.
  • Remove strict_deps=False in a few places where scope=forced on a particular dep would be preferable.
  • Formalize JavacPlugin's tools.jar dep as an injected target provided by a task during bootstrap.
  • In case of an unused dep (which is eligible to be marked that way due to its scope), declare the dependency_type of the edge unused in the dep-usage.jvm task.

This review also cleans up all warnings/errors for:

./pants compile.zinc --unused-deps=fatal examples/{src,tests}/{java,scala}/::

When an unused dep is encountered, it is logged like so:

compile(examples/tests/scala/org/pantsbuild/example/hello/welcome:welcome) failed: unused dependencies:
  'examples/src/scala/org/pantsbuild/example/hello/exe:exe',
Suggested replacements:
  'examples/src/scala/org/pantsbuild/example/hello/welcome:welcome',
(If you're seeing this message in error, you might need to change the `scope` of the dependencies.)

... with suggested replacements coming from the transitive deps of the unused deps that were used.

http://jenkins.pantsbuild.org/job/pantsbuild/job/pants/job/PR-3115/11/

  • 0
  • 0
  • 3
  • 1
  • 4
Description From Last Updated
  1. (note to self: will need to ensure --[no-]dep-usage-jvm-internal-only is working on this review... currently it doesn't.)

  2. 
      
  1. 
      
  2. Took me a bit to parse this (I wasn't previously aware you could chain generator comprehensions like this. Could maybe use a comment? Not sure.

    Also, you should probably just use a set comprehension here {p for ... for p in paths} instead of instantiating a set from a generator comprehension, and you can also just use set literals for {dep} above.

  3. Why dict() instead of just {}?

  4. Do we also want to skip any targets with the codegen label? I don't think codegen targets will ever be used directly, only the synthetic targets derived from them will be used.

    1. That's handled below by the expansion of the derived_from_chain.

    1. There is no "empty dict" literal... {} is a set literal (yea, wacky).

    2. ...argh. I had this backwards. Thanks python.

    3. Yeah, it's a bit sneaky.

      {} is an empty dict literal.
      {'foobar'} is a set literal containing a single string.
      {'foo': 'bar'} is a dict literal again.

  5. 
      
  1. We've been working on rolling out an offline version of this check to strip all unused deps before we turn it on, and one of the expected reasons that things need to be "whitelisted" is that they are actually runtime dependencies. So things marked scope='runtime' should not trigger this check.

  2. 
      
  1. I've drafted a proposal for how to mark these: https://docs.google.com/document/d/1WfdcP6GE_4eDUZKGfaMcMJonhXxbUQtaNUMxz_7Gi5c/edit#

  2. 
      
  1. Ship It!
  2. 
      
  1. The tools.jar part LGTM.

  2. 
      
  1. Have tested for a few weeks now in twitter repo

  2. super(GoThriftGen,..) here?

    1. Good eye! Just fixed.

  3. as opposed to target's actual scope?

    1. This filters to only the deps of the target with scope==default

  4. makes more sense to be part of the _dep_type logic, move `len(products_used) > 0 inside

  5. 
      
Review request changed

Status: Closed (submitted)

Change Summary:

Merged as 5b7d317160b69e4ec0077cbfc2380e6df8217717

Loading...