Skip to content

Conversation

@kennknowles
Copy link
Member

This migrate dependencies from findbugs to spotbugs, with some fixes. There appear to be issues introduced in spotbugs.

  • The change to suppression of a volatile warning was necessary even though it was already suppressed. Something about how spotbugs scopes the suppression made it not work for a nested lambda.
  • There is still a failure claiming that the continuation parameter to SplittableProcessElementInvoker$Result is marked nullable, which is not the case. I have not yet decompiled to debug.

Follow this checklist to help us incorporate your contribution quickly and easily:

  • Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

It will help us expedite review of your Pull Request if you tag someone (e.g. @username) to look at it.

Post-Commit Tests Status (on master branch)

Lang SDK Apex Dataflow Flink Gearpump Samza Spark
Go Build Status --- --- --- --- --- ---
Java Build Status Build Status Build Status Build Status
Build Status
Build Status
Build Status Build Status Build Status
Python Build Status --- Build Status
Build Status
Build Status --- --- ---

@kennknowles kennknowles requested a review from iemejia February 1, 2019 19:33
@kennknowles kennknowles changed the title Migrate from findbugs to spotbugs [BEAM-6554] Migrate from findbugs to spotbugs Feb 1, 2019
@kennknowles
Copy link
Member Author

@iemejia @adude3141 opening this for discussion

reports {
html.enabled = false
xml.enabled = true
html { enabled = true }
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops. I'll revert this. I think we should default to settings that are good for a developer and have Jenkins jobs set things like -PxmlSpotbugs or some such. But later.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

definitely+1 on having decent developer defaults.

Unfortunately the Jenkins plugin requires xml output. And as findbugs/spotbugs support only single report format and I further was under the impression that findbugs should be removed, I simply switch to xml. Of course, this is a different story now.

Regarding that parameter, do we have other places where we have different configuration on jenkins? If so, would it make sense to extract some Jenkins-plugin which collects all this differences, so that we only either autodetect, we run on Jenkins (by checking some Jenkins env-var) or set only a single parameter, ciBuild=true or such.

@adude3141
Copy link
Contributor

LGTM

anything I could do to help pushing this forward?

@kennknowles
Copy link
Member Author

@adude3141 thanks! There's not much more to review. I noticed the same problem with the reports. I will set it to xml. Spotbugs itself in the build configuration has been no problem. I haven't had any minutes to spend on the second issue in the PR description. I think that is the main blocker, unless it is masking further errors.

@iemejia
Copy link
Member

iemejia commented Feb 7, 2019

This needs a rebase, in principle looks ok also to me, will LGTM once the xml change is done and green.

@kennknowles
Copy link
Member Author

OK. I got spotbugs happy. Now the blocker is adjusting the Jenkins job to either be flexible or to be OK with missing data.

@adude3141
Copy link
Contributor

adude3141 commented Feb 7, 2019

Now the blocker is adjusting the Jenkins job to either be flexible or to be OK with missing data.

I do not understand what is meant by that? Could you elaborate a bit?

@kennknowles
Copy link
Member Author

@adude3141 I got an error something about findbugs/**/ file not found. I thought the directory might be renamed to spotbugs for the output reports. But the jobs seem happy right now. If this goes green, then great! It could have just been something completely different.

@kennknowles
Copy link
Member Author

run java precommit

@adude3141
Copy link
Contributor

ah, yes.

we need to change https://github.com/apache/beam/blob/master/.test-infra/jenkins/job_PreCommit_Java.groovy#L44-L46
as well to something like spotBugs { pattern('...') }

@kennknowles kennknowles force-pushed the spotbugs branch 2 times, most recently from 15f558b to e43f0e6 Compare February 20, 2019 23:21
@kennknowles
Copy link
Member Author

Finally got the right stuff in place for the dependency analysis plugin. Looks like Jira was down/ or rebooting when the website checker ran.

@kennknowles kennknowles merged commit 086d037 into apache:master Mar 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants