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

Update errorprone to 2.3.0 and findbugs to spotbugs 3.1.3 #5725

Merged
merged 1 commit into from Apr 24, 2018

Conversation

Projects
None yet
3 participants
@cheister
Copy link
Contributor

cheister commented Apr 20, 2018

Problem

Pants was on older versions of Errorprone and Findbugs (now SpotBugs) that did not run on Java 9+

Solution

Update errorprone and findbugs so they work on Java 9 and 10.

Result

Both are updated. I didn't think the noise of renaming findbugs to spotbugs made sense in this change. I also assumed it is ok that they require Java 8 as a minimum runtime even though Pants still supports Java 7.

@cheister cheister requested a review from stuhood Apr 20, 2018

@illicitonion

This comment has been minimized.

Copy link
Contributor

illicitonion commented Apr 20, 2018

Errorprone-specific tests are failing on CI, please take a look @cheister :)

@cheister cheister force-pushed the cheister:update-errorprone-findbugs branch 3 times, most recently from 684f9fd to db64055 Apr 20, 2018

@cheister

This comment has been minimized.

Copy link
Contributor

cheister commented Apr 24, 2018

Not sure what is going on in Travis.

./pants test contrib/errorprone/tests/python/pants_test/contrib/errorprone/tasks:integration

passes locally.

I thought maybe there was test pollution in the shard but running the whole shard locally also passed.

export SHARD="Python contrib tests - shard 1"
export PYTEST_PASSTHRU_ARGS="-v --duration=3"
./build-support/bin/ci.sh -x -efkmrcjlpt -y 0/2 "${SHARD}"

Any ideas? I could comment out the test from CI but that seems odd considering it's passing locally.

@cheister cheister force-pushed the cheister:update-errorprone-findbugs branch 2 times, most recently from 2c76785 to 04ac362 Apr 24, 2018

@illicitonion
Copy link
Contributor

illicitonion left a comment

Looks good to me :)

@wisechengyi Is there a nice abstraction that can be used to make the ivy portion of this generic over ivy and coursier?

@stuhood stuhood requested a review from benjyw Apr 24, 2018

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Apr 24, 2018

@benjyw : Could you take a look at this? You're more familiar with javac plugins.


# Unfortunately using the IvyTaskMixin to resolve the javac dependency causes the cache_manager.py

This comment has been minimized.

@stuhood

stuhood Apr 24, 2018

Member

If it's failing in CI due to a caching issue, it seems very likely to cause you issues in production.

It looks like you're only using IvyTaskMixin to get a list of the entries in the classpath... there are other helper methods to do that.

@@ -115,6 +117,12 @@ def calculate_sources(self, target):
return {source for source in target.sources_relative_to_buildroot()
if source.endswith(self._JAVA_SOURCE_EXTENSION)}

@memoized_property
def _javac_jar_path(self):
targets = list(self.context.resolve('//:errorprone'))

This comment has been minimized.

@stuhood

stuhood Apr 24, 2018

Member

Given that you've registered this tool, you should be able to get its classpath by calling self.tool_classpath('errorprone'), and then you can filter out entries from the classpath directly in that list?

Alternatively, having two tool registrations (one for errorprone, and one for errorprone's compiler plugin) would also be cleaner.

This comment has been minimized.

@cheister

cheister Apr 24, 2018

Contributor

self.tool_classpath('errorprone') only returns the shaded jar.

Adding a separate registration for errorprone-javac should be easier though. I'll give that a try.

This comment has been minimized.

@stuhood

stuhood Apr 24, 2018

Member

@cheister : If you disable shading (ie, don't pass a main method) it will give you a list of classpath entries.

This comment has been minimized.

@cheister

cheister Apr 24, 2018

Contributor

Yeah, I wasn't sure if I wanted to disable shading just to get the errorprone-javac jar. Adding errorprone-javac as it's own jvm_tool also means we don't have to include the IvyTaskMixin which seems better anyway.

@cheister cheister force-pushed the cheister:update-errorprone-findbugs branch from 04ac362 to 93d71c5 Apr 24, 2018

@stuhood
Copy link
Member

stuhood left a comment

Thanks!

@@ -2,6 +2,7 @@
# Licensed under the Apache License, Version 2.0 (see LICENSE).

junit_tests(
name='test',

This comment has been minimized.

@stuhood

stuhood Apr 24, 2018

Member

This change doesn't look necessary?

This comment has been minimized.

@cheister

cheister Apr 24, 2018

Contributor

Unfortunately it's needed to avoid google/error-prone#1007

@cheister cheister merged commit a0072d3 into pantsbuild:master Apr 24, 2018

1 check passed

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

@cheister cheister deleted the cheister:update-errorprone-findbugs branch Apr 24, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment