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.

Sign up for GitHub

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

Jump to bottom

Workaround #61 #66

Merged
merged 1 commit into from
Nov 8, 2018
Merged

Workaround #61 #66

merged 1 commit into from
Nov 8, 2018

Conversation

DPUkyle
Copy link
Contributor

@DPUkyle DPUkyle commented Oct 26, 2018

By bailing when the SpotBugsTask is not configured.

  • This shouldn't ever happen; it might be related to the way Kotlin buildscripts apply plugins?
  • Also added a Kotlin buildscript-based simple sanity test

Signed-off-by: Kyle Moore github@kylemoore.com

@DPUkyle
Fix spotbugs#61 by doing a null-safety check and bailing when the Spo…
9ddaf0d 
…tBugsTask is not configured

 - This shouldn't ever happen; it might be related to the way Kotlin buildscripts apply plugins?
 - Also added a Kotlin buildscript-based simple sanity test

Signed-off-by: Kyle Moore <github@kylemoore.com>
KengoTODA
KengoTODA requested changes Nov 5, 2018
View reviewed changes
Copy link
Member

@KengoTODA KengoTODA left a comment

Choose a reason for hiding this comment

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

I build your change on local. It seems that these test cases does not reproduce that problem, because I can pass ./gradlew build without your change in src/main/java. Could you make sure that your test really reproduces that problem?

@DPUkyle
Copy link
Contributor Author

DPUkyle commented Nov 7, 2018

Yes, I could not reproduce the root cause of #61. Sorry for not being more clear in the comments. So I am ok to close and not merge this PR.

However I am concerned about #68 having basically the same symptom; that is, SpotBugsReportsInternal#getFirstEnabled() is being called before the container is initialized or when all reports are explicitly disabled.

@KengoTODA
Copy link
Member

I agree with your opinion so let me merge this PR first. We still don't know why Kotlin build disable all reports by default, so let's keep discussion about it at #61.

KengoTODA
KengoTODA approved these changes Nov 8, 2018
View reviewed changes
Copy link
Member

@KengoTODA KengoTODA left a comment

Choose a reason for hiding this comment

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

This test cannot reproduce reported problem, however it works as smoke test for Kotlin build script. We'll keep discussion in #61 for related issues.

@KengoTODA KengoTODA merged commit 4fe0530 into spotbugs:master Nov 8, 2018
@KengoTODA KengoTODA mentioned this pull request Nov 8, 2018
Closed
@KengoTODA
Copy link
Member

@DPUkyle sorry but I forgot to ask you to add an entry to CHANGELOG.md. Could you propose separated PR to add it?

@KengoTODA KengoTODA added the bug label Nov 8, 2018
@KengoTODA KengoTODA added this to the 1.6.6 milestone Nov 8, 2018
@DPUkyle DPUkyle mentioned this pull request Nov 8, 2018
Merged
@DPUkyle DPUkyle mentioned this pull request Feb 7, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KengoTODA KengoTODA KengoTODA approved these changes

Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
1.6.6
Development

Successfully merging this pull request may close these issues.
2 participants
@DPUkyle @KengoTODA