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

Suggest missing dependencies from target's transitive dependencies #4171

Merged
merged 20 commits into from Jan 10, 2017

Conversation

Projects
None yet
2 participants
@peiyuwang
Contributor

peiyuwang commented Jan 9, 2017

Problem

Switching from non-strict_deps to strict-deps may result missing deps compile errors if deps used are from target's transitive deps but not declared as direct deps.

Solution

This PR extracts the class names that are reported as not found from failure compile log and locate them in target's transitive classpath.

This approach is on a best-effort bsis:

  • Compiler may not always report the not found class name, even though the error is due to missing deps. Work to improve scalac coverage is undergoing
  • Newly introduced deps that are not in the transitive deps are not covered.

Result

./pants compile testprojects/src/java/org/pantsbuild/testproject/missingjardepswhitelist:missingjardepswhitelist --compile-zinc-suggest-missing-deps
...
                       [info] Compiling 1 Java source to /Users/peiyu/github/pants/.pants.d/compile/zinc/66ac1c9326be/testprojects.src.java.org.pantsbuild.testproject.missingjardepswhitelist.missingjardepswhitelist/current/classes...
                       [error] /Users/peiyu/github/pants/testprojects/src/java/org/pantsbuild/testproject/missingjardepswhitelist/MissingJarDepsWhitelist.java:4:1: package com.google.common.io does not exist
                       [error] import com.google.common.io.Closer;
                       [error] /Users/peiyu/github/pants/testprojects/src/java/org/pantsbuild/testproject/missingjardepswhitelist/MissingJarDepsWhitelist.java:8:1: cannot find symbol
                       [error]   symbol:   class Closer
                       [error]   location: class org.pantsbuild.testproject.missingjardepswhitelist.MissingJarDepsWhitelist
                       [error]     Closer c = Closer.create();
                       [error] /Users/peiyu/github/pants/testprojects/src/java/org/pantsbuild/testproject/missingjardepswhitelist/MissingJarDepsWhitelist.java:8:1: cannot find symbol
                       [error]   symbol:   variable Closer
                       [error]   location: class org.pantsbuild.testproject.missingjardepswhitelist.MissingJarDepsWhitelist
                       [error]     Closer c = Closer.create();
                       [error] Compile failed at Jan 10, 2017 11:23:09 AM [0.058s]
                       
11:23:09 00:03         [missing-deps-suggest]
                       
                       Found the following deps from target's transitive dependencies that provide the missing classes:
                         com.google.common.io.Closer: 3rdparty:guava
                       
                       If the above information is correct, please add the following to the dependencies of (testprojects/src/java/org/pantsbuild/testproject/missingjardepswhitelist:missingjardepswhitelist):
                         3rdparty:guava
                       
                   compile(testprojects/src/java/org/pantsbuild/testproject/missingjardepswhitelist:missingjardepswhitelist) failed: Zinc compile failed.
@stuhood

Looks good! A few minor issues.

Show outdated Hide outdated ...on/pants/backend/jvm/tasks/jvm_compile/class_not_found_error_patterns.py
import re
_CLASS_NOT_FOUND_ERROR_PATTERNS = [

This comment has been minimized.

@stuhood

stuhood Jan 10, 2017

Member

Would it be crazy to have these in a pants list option, so that users could add/remove them via configuration?

Also, having these as module-level constants will cause the regexes to be compiled during pants' startup... if you're not going to move them to an option, you should still make them members of a class so that they will be compiled only if they will be used.

@stuhood

stuhood Jan 10, 2017

Member

Would it be crazy to have these in a pants list option, so that users could add/remove them via configuration?

Also, having these as module-level constants will cause the regexes to be compiled during pants' startup... if you're not going to move them to an option, you should still make them members of a class so that they will be compiled only if they will be used.

This comment has been minimized.

@peiyuwang

peiyuwang Jan 10, 2017

Contributor

not crazy at all! since the patterns aren't going to be complete, versions of compiler may change, it's more flexible to allow override from config.

changed.

@peiyuwang

peiyuwang Jan 10, 2017

Contributor

not crazy at all! since the patterns aren't going to be complete, versions of compiler may change, it's more flexible to allow override from config.

changed.

Show outdated Hide outdated src/python/pants/backend/jvm/tasks/jvm_compile/jvm_compile.py
@@ -163,6 +165,10 @@ def register_options(cls, register):
help='Controls whether unused deps are checked, and whether they cause warnings or '
'errors.')
register('--suggest-missing-deps', advanced=False, type=bool,
help='Suggest missing dependencies best-effort from target\'s transitive deps '

This comment has been minimized.

@stuhood

stuhood Jan 10, 2017

Member

s/best-effort/on a best-effort basis/

@stuhood

stuhood Jan 10, 2017

Member

s/best-effort/on a best-effort basis/

This comment has been minimized.

@peiyuwang

peiyuwang Jan 10, 2017

Contributor

done

@peiyuwang

peiyuwang Jan 10, 2017

Contributor

done

Show outdated Hide outdated src/python/pants/backend/jvm/tasks/jvm_compile/jvm_compile.py
log_file, settings, fatal_warnings, zinc_file_manager,
javac_plugins_to_exclude)
except TaskError:
def find_failed_compile_log(workunits):

This comment has been minimized.

@stuhood

stuhood Jan 10, 2017

Member

Defining this inside the except block makes this more challenging to read. Would prefer to see the function defined outside (class level, or otherwise) so that the except is small.

@stuhood

stuhood Jan 10, 2017

Member

Defining this inside the except block makes this more challenging to read. Would prefer to see the function defined outside (class level, or otherwise) so that the except is small.

This comment has been minimized.

@peiyuwang

peiyuwang Jan 10, 2017

Contributor

moved _find_failed_compile_log to outside except block

@peiyuwang

peiyuwang Jan 10, 2017

Contributor

moved _find_failed_compile_log to outside except block

Show outdated Hide outdated src/python/pants/backend/jvm/tasks/jvm_compile/jvm_compile.py
for workunit in workunits:
for output_name, outpath in workunit.output_paths().items():
# Locate failure log from child compilation workunit's stdout
if (workunit.name == self.name() and output_name == 'stdout'

This comment has been minimized.

@stuhood

stuhood Jan 10, 2017

Member

Matching self.name is a bit strange... a comment on why this is the appropriate thing to match would be good. Or maybe having an explicit property that matches the tool name?

@stuhood

stuhood Jan 10, 2017

Member

Matching self.name is a bit strange... a comment on why this is the appropriate thing to match would be good. Or maybe having an explicit property that matches the tool name?

This comment has been minimized.

@peiyuwang

peiyuwang Jan 10, 2017

Contributor

Added comments

@peiyuwang

peiyuwang Jan 10, 2017

Contributor

Added comments

Show outdated Hide outdated src/python/pants/backend/jvm/tasks/jvm_compile/jvm_compile.py
if missing_dep_suggestions:
self.context.log.info('Found the following deps from target\'s transitive '
'dependencies that contain the not found classes:')

This comment has been minimized.

@stuhood

stuhood Jan 10, 2017

Member

s/that contain the not found classes/that provide the missing classes/

@stuhood

stuhood Jan 10, 2017

Member

s/that contain the not found classes/that provide the missing classes/

This comment has been minimized.

@peiyuwang

peiyuwang Jan 10, 2017

Contributor

Fixed

@peiyuwang

peiyuwang Jan 10, 2017

Contributor

Fixed

Show outdated Hide outdated src/python/pants/backend/jvm/tasks/jvm_compile/jvm_compile.py
)
self.context.log.info(suggestion_msg)
if no_suggestions:

This comment has been minimized.

@stuhood

stuhood Jan 10, 2017

Member

IMO, I don't think it makes sense to log the case where you don't find anything (that seems obvious enough). Maybe you could do it at debug level though?

@stuhood

stuhood Jan 10, 2017

Member

IMO, I don't think it makes sense to log the case where you don't find anything (that seems obvious enough). Maybe you could do it at debug level though?

This comment has been minimized.

@peiyuwang

peiyuwang Jan 10, 2017

Contributor

I think there is still value. when there is no suggestion found, there are two possibilities, either error extraction pattern is incomplete or classpath is incomplete. would be good to know which one of the two causes the miss.

change it to debug level.

@peiyuwang

peiyuwang Jan 10, 2017

Contributor

I think there is still value. when there is no suggestion found, there are two possibilities, either error extraction pattern is incomplete or classpath is incomplete. would be good to know which one of the two causes the miss.

change it to debug level.

Show outdated Hide outdated src/python/pants/backend/jvm/tasks/jvm_dependency_analyzer.py
"""Search which targets from `target`'s transitive dependencies contain `classname`."""
return self._targets_by_class(tuple(target.closure())).get(classname, set())
@memoized_method

This comment has been minimized.

@stuhood

stuhood Jan 10, 2017

Member

This is a very expensive thing to memoize, afaict. Remember that memoizing a method treats the arguments to the method as a cache key. So you're using a very large (unsorted) tuple as a cache key here.

Instead, I think you'd want to break up the memoization so that you have per-target memoization of classnames: ie, mark a method that takes a single target memoized_method instead.

@stuhood

stuhood Jan 10, 2017

Member

This is a very expensive thing to memoize, afaict. Remember that memoizing a method treats the arguments to the method as a cache key. So you're using a very large (unsorted) tuple as a cache key here.

Instead, I think you'd want to break up the memoization so that you have per-target memoization of classnames: ie, mark a method that takes a single target memoized_method instead.

This comment has been minimized.

@peiyuwang

peiyuwang Jan 10, 2017

Contributor

changed to memoize at target level. Thanks!

@peiyuwang

peiyuwang Jan 10, 2017

Contributor

changed to memoize at target level. Thanks!

@stuhood

Looks good, except for one big typo.

Show outdated Hide outdated src/python/pants/backend/jvm/tasks/jvm_compile/jvm_compile.py
@@ -133,7 +138,7 @@ def register_options(cls, register):
help='Extra compiler args to use when warnings are disabled.')
register('--fatal-warnings-enabled-args', advanced=True, type=list, fingerprint=True,
default=list(cls.get_fatal_warnings_enabled_args_default()),
default=CLASS_NOT_FOUND_ERROR_PATTERNS,

This comment has been minimized.

@stuhood

stuhood Jan 10, 2017

Member

xx: wrong option.

@stuhood

stuhood Jan 10, 2017

Member

xx: wrong option.

This comment has been minimized.

@peiyuwang

peiyuwang Jan 10, 2017

Contributor

Fixed. :( late night edits, remember there was sth odd. Thanks for catching this

@peiyuwang

peiyuwang Jan 10, 2017

Contributor

Fixed. :( late night edits, remember there was sth odd. Thanks for catching this

Show outdated Hide outdated src/python/pants/backend/jvm/tasks/jvm_compile/jvm_compile.py
raise
def _find_failed_compile_log(self, compile_workunit):
'''One of the compile child workunits actually calls compiler, locate its stdout.'''

This comment has been minimized.

@stuhood

stuhood Jan 10, 2017

Member

" rather than '

@stuhood

stuhood Jan 10, 2017

Member

" rather than '

This comment has been minimized.

@peiyuwang

peiyuwang Jan 10, 2017

Contributor

fixed

@peiyuwang

peiyuwang Jan 10, 2017

Contributor

fixed

@stuhood

Thanks Peiyu!

@peiyuwang peiyuwang merged commit 3d9f891 into pantsbuild:master Jan 10, 2017

1 check passed

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

@peiyuwang peiyuwang deleted the peiyuwang:peiyu/suggest-missing-deps branch Jan 18, 2017

lenucksi added a commit to lenucksi/pants that referenced this pull request Apr 25, 2017

Suggest missing dependencies from target's transitive dependencies (#…
…4171)

### Problem

Switching from `non-strict_deps` to `strict-deps` may result missing deps compile errors if deps used are from target's transitive deps but not declared as direct deps.

### Solution

This PR extracts the class names that are reported as `not found` from failure compile log and locate them in target's transitive classpath.

This approach is on a best-effort bsis:

* Compiler may not always report the not found class name, even though the error is due to missing deps. Work to improve scalac coverage is undergoing
* Newly introduced deps that are not in the transitive deps are not covered.

### Result
```
./pants compile testprojects/src/java/org/pantsbuild/testproject/missingjardepswhitelist:missingjardepswhitelist --compile-zinc-suggest-missing-deps
...
                       [info] Compiling 1 Java source to /Users/peiyu/github/pants/.pants.d/compile/zinc/66ac1c9326be/testprojects.src.java.org.pantsbuild.testproject.missingjardepswhitelist.missingjardepswhitelist/current/classes...
                       [error] /Users/peiyu/github/pants/testprojects/src/java/org/pantsbuild/testproject/missingjardepswhitelist/MissingJarDepsWhitelist.java:4:1: package com.google.common.io does not exist
                       [error] import com.google.common.io.Closer;
                       [error] /Users/peiyu/github/pants/testprojects/src/java/org/pantsbuild/testproject/missingjardepswhitelist/MissingJarDepsWhitelist.java:8:1: cannot find symbol
                       [error]   symbol:   class Closer
                       [error]   location: class org.pantsbuild.testproject.missingjardepswhitelist.MissingJarDepsWhitelist
                       [error]     Closer c = Closer.create();
                       [error] /Users/peiyu/github/pants/testprojects/src/java/org/pantsbuild/testproject/missingjardepswhitelist/MissingJarDepsWhitelist.java:8:1: cannot find symbol
                       [error]   symbol:   variable Closer
                       [error]   location: class org.pantsbuild.testproject.missingjardepswhitelist.MissingJarDepsWhitelist
                       [error]     Closer c = Closer.create();
                       [error] Compile failed at Jan 10, 2017 11:23:09 AM [0.058s]
                       
11:23:09 00:03         [missing-deps-suggest]
                       
                       Found the following deps from target's transitive dependencies that provide the missing classes:
                         com.google.common.io.Closer: 3rdparty:guava
                       
                       If the above information is correct, please add the following to the dependencies of (testprojects/src/java/org/pantsbuild/testproject/missingjardepswhitelist:missingjardepswhitelist):
                         3rdparty:guava
                       
                   compile(testprojects/src/java/org/pantsbuild/testproject/missingjardepswhitelist:missingjardepswhitelist) failed: Zinc compile failed.
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment