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

Unrevert #8093 and fix bug. #8125

Merged
merged 3 commits into from Jul 30, 2019

Conversation

@hrfuller
Copy link
Contributor

commented Jul 29, 2019

Problem

Previous jdeps PR 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 stuhood added this to the 1.19.x milestone Jul 29, 2019

@stuhood
Copy link
Member

left a comment

Thanks! Since CI will fail right now anyway due to #8123, would you mind adding a test for the no-deps case?

@hrfuller

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2019

Sure. Feel free to cancel this run then.

@stuhood
Copy link
Member

left a comment

Thanks!

hrfuller added some commits Jul 22, 2019

Reapply changes made in #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
Fix bug where -classpath flag is passed without an argument when target
does not have any deps. Be more careful with regex matching, don't
assume every line will match.

@stuhood stuhood force-pushed the twitter:hfuller/jdeps-usage branch from e8b09c7 to cfcfbdf Jul 29, 2019

@stuhood

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

Interesting. Twice in a row now we've seen pants_test.backend.jvm.tasks.jvm_compile.zinc.test_zinc_compile_integration.ZincCompileIntegration test_consecutive_compiler_option_sets fail with:

Fatal Python error: Bus error

(full gist: https://gist.github.com/stuhood/35e7eb81832fc35eacc6229a2007078e)

I haven't tried reproing it: @hrfuller : do you mind trying? To run just that test you'd do something like:

./pants --owner-of=tests/python/pants_test/backend/jvm/tasks/jvm_compile/zinc/test_zinc_compile_integration.py test -- -k test_consecutive_compiler_option_sets

I'll clear the cache and re-run.

@stuhood stuhood merged commit 86fcbab into pantsbuild:master Jul 30, 2019

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

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

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.
@hrfuller

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2019

I just tried to repro and I wasn't able to, even from a clean state.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.