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

[JENKINS-36717] - FindBugs cleanup #2459

Merged
merged 26 commits into from Aug 7, 2016

Conversation

4 participants
@oleg-nenashev
Member

oleg-nenashev commented Jul 15, 2016

https://issues.jenkins-ci.org/browse/JENKINS-36717

The change addresses FindBugs warnings.
The fixes need some investigation, real issues like 8c73550 may need a separate bug and Pull request

@daniel-beck

This comment has been minimized.

Show comment
Hide comment
@daniel-beck

daniel-beck Jul 20, 2016

Member

Pretty cool, thanks for this effort!

Member

daniel-beck commented Jul 20, 2016

Pretty cool, thanks for this effort!

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev Jul 20, 2016

Member

@daniel-beck It should become helpful once I finalize #2458, because we may get a kind of the "clean" FindBugs run for Jenkins master. This clean run will actually contain hundreds of low- and medium- priority warnings, but it's something which can unblock integrating FB into the PR builder

Member

oleg-nenashev commented Jul 20, 2016

@daniel-beck It should become helpful once I finalize #2458, because we may get a kind of the "clean" FindBugs run for Jenkins master. This clean run will actually contain hundreds of low- and medium- priority warnings, but it's something which can unblock integrating FB into the PR builder

for (Descriptor<ListViewColumn> d : ListViewColumn.all())
try {
if (d instanceof ListViewColumnDescriptor) {
ListViewColumnDescriptor ld = (ListViewColumnDescriptor) d;
if (!ld.shownByDefault()) continue; // skip this
}
ListViewColumn lvc = d.newInstance(null, null);
ListViewColumn lvc = d.newInstance(null, emptyJSON);

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Jul 27, 2016

Member

This one is potentially serious (violation of safe-defined API for plugins), but I do not see related issues

@oleg-nenashev

oleg-nenashev Jul 27, 2016

Member

This one is potentially serious (violation of safe-defined API for plugins), but I do not see related issues

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev Aug 1, 2016

Member

Bugs left:

[INFO] hudson.cli.CliManagerImpl$1 stored into non-transient field CliManagerImpl.authenticationFilter [hudson.cli.CliManagerImpl] At CliManagerImpl.java:[line 56] SE_BAD_FIELD_STORE
[INFO] Potentially ambiguous invocation of either an outer or inherited method java.util.ArrayList.clear() in hudson.model.Queue$ItemList.cancelAll() [hudson.model.Queue$ItemList] At Queue.java:[line 2776] IA_AMBIGUOUS_INVOCATION_OF_INHERITED_OR_OUTER_METHOD
[INFO] String.format(String, Object[]) needs printf-style format but called with MessageFormat [hudson.model.UpdateCenter] At UpdateCenter.java:[line 281] VA_FORMAT_STRING_EXPECTED_MESSAGE_FORMAT_SUPPLIED
[INFO] String.format(String, Object[]) needs printf-style format but called with MessageFormat [hudson.model.User] At User.java:[line 450] VA_FORMAT_STRING_EXPECTED_MESSAGE_FORMAT_SUPPLIED
[INFO] Redundant nullcheck of e, which is known to be non-null in hudson.model.queue.Executables.getEstimatedDurationFor(Queue$Executable) [hudson.model.queue.Executables] Redundant null check at Executables.java:[line 77] RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE
[INFO] Null passed for non-null parameter of hudson.model.Descriptor.newInstance(StaplerRequest, JSONObject) in hudson.scm.RepositoryBrowsers.createInstance(Class, StaplerRequest, String) [hudson.scm.RepositoryBrowsers] At RepositoryBrowsers.java:[line 77] NP_NONNULL_PARAM_VIOLATION
[INFO] req must be non-null but is marked as nullable [hudson.security.HudsonPrivateSecurityRealm$Details$DescriptorImpl] At HudsonPrivateSecurityRealm.java:[lines 573-585] NP_PARAMETER_MUST_BE_NONNULL_BUT_MARKED_AS_NULLABLE
[INFO] Return value of String.trim() ignored in hudson.security.SecurityRealm.getFrom() [hudson.security.SecurityRealm] At SecurityRealm.java:[line 529] RV_RETURN_VALUE_IGNORED
[INFO] Boxing/unboxing to parse a primitive new hudson.slaves.ChannelPinger() [hudson.slaves.ChannelPinger] At ChannelPinger.java:[line 64] DM_BOXED_PRIMITIVE_FOR_PARSING
[INFO] Impossible downcast from Object[] to hudson.tools.ToolInstallation[] in hudson.tools.ToolDescriptor.getInstallations() [hudson.tools.ToolDescriptor] At ToolDescriptor.java:[line 75] BC_IMPOSSIBLE_DOWNCAST
[INFO] Write to static field jenkins.util.SystemProperties.theContext from instance method jenkins.util.SystemProperties.contextInitialized(ServletContextEvent) [jenkins.util.SystemProperties] At SystemProperties.java:[line 91] ST_WRITE_TO_STATIC_FROM_INSTANCE_METHOD
[INFO] jenkins.util.xml.XMLUtils.setDocumentBuilderFactoryFeature(DocumentBuilderFactory, String, boolean) might ignore java.lang.Exception [jenkins.util.xml.XMLUtils, jenkins.util.xml.XMLUtils] At XMLUtils.java:[line 236]At XMLUtils.java:[line 236] DE_MIGHT_IGNORE
[INFO] 
Member

oleg-nenashev commented Aug 1, 2016

Bugs left:

[INFO] hudson.cli.CliManagerImpl$1 stored into non-transient field CliManagerImpl.authenticationFilter [hudson.cli.CliManagerImpl] At CliManagerImpl.java:[line 56] SE_BAD_FIELD_STORE
[INFO] Potentially ambiguous invocation of either an outer or inherited method java.util.ArrayList.clear() in hudson.model.Queue$ItemList.cancelAll() [hudson.model.Queue$ItemList] At Queue.java:[line 2776] IA_AMBIGUOUS_INVOCATION_OF_INHERITED_OR_OUTER_METHOD
[INFO] String.format(String, Object[]) needs printf-style format but called with MessageFormat [hudson.model.UpdateCenter] At UpdateCenter.java:[line 281] VA_FORMAT_STRING_EXPECTED_MESSAGE_FORMAT_SUPPLIED
[INFO] String.format(String, Object[]) needs printf-style format but called with MessageFormat [hudson.model.User] At User.java:[line 450] VA_FORMAT_STRING_EXPECTED_MESSAGE_FORMAT_SUPPLIED
[INFO] Redundant nullcheck of e, which is known to be non-null in hudson.model.queue.Executables.getEstimatedDurationFor(Queue$Executable) [hudson.model.queue.Executables] Redundant null check at Executables.java:[line 77] RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE
[INFO] Null passed for non-null parameter of hudson.model.Descriptor.newInstance(StaplerRequest, JSONObject) in hudson.scm.RepositoryBrowsers.createInstance(Class, StaplerRequest, String) [hudson.scm.RepositoryBrowsers] At RepositoryBrowsers.java:[line 77] NP_NONNULL_PARAM_VIOLATION
[INFO] req must be non-null but is marked as nullable [hudson.security.HudsonPrivateSecurityRealm$Details$DescriptorImpl] At HudsonPrivateSecurityRealm.java:[lines 573-585] NP_PARAMETER_MUST_BE_NONNULL_BUT_MARKED_AS_NULLABLE
[INFO] Return value of String.trim() ignored in hudson.security.SecurityRealm.getFrom() [hudson.security.SecurityRealm] At SecurityRealm.java:[line 529] RV_RETURN_VALUE_IGNORED
[INFO] Boxing/unboxing to parse a primitive new hudson.slaves.ChannelPinger() [hudson.slaves.ChannelPinger] At ChannelPinger.java:[line 64] DM_BOXED_PRIMITIVE_FOR_PARSING
[INFO] Impossible downcast from Object[] to hudson.tools.ToolInstallation[] in hudson.tools.ToolDescriptor.getInstallations() [hudson.tools.ToolDescriptor] At ToolDescriptor.java:[line 75] BC_IMPOSSIBLE_DOWNCAST
[INFO] Write to static field jenkins.util.SystemProperties.theContext from instance method jenkins.util.SystemProperties.contextInitialized(ServletContextEvent) [jenkins.util.SystemProperties] At SystemProperties.java:[line 91] ST_WRITE_TO_STATIC_FROM_INSTANCE_METHOD
[INFO] jenkins.util.xml.XMLUtils.setDocumentBuilderFactoryFeature(DocumentBuilderFactory, String, boolean) might ignore java.lang.Exception [jenkins.util.xml.XMLUtils, jenkins.util.xml.XMLUtils] At XMLUtils.java:[line 236]At XMLUtils.java:[line 236] DE_MIGHT_IGNORE
[INFO] 

oleg-nenashev added some commits Aug 1, 2016

Queue.ItemList: Be sure we cleanup the list instead of the entire Que…
…ue (IA_AMBIGUOUS_INVOCATION_OF_INHERITED_OR_OUTER_METHOD)
FindBugs: Optimize logic of ChannelPinger#pingInterval initialization…
… in order to fix the Integer unboxing issue in the original code
FindBugs: Violation of Descriptor newInstance() contract in hudson.sc…
…m.RepositoryBrowsers#createInstance deprecated method
@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev Aug 1, 2016

Member

@daniel-beck @jenkinsci/code-reviewers With this change the master branch will be "clean" at least in the case of high-priority issues.

Do we want to backport anything from the fixes? If yes, I can create JIRA issues and move fixes to separate PRs

Member

oleg-nenashev commented Aug 1, 2016

@daniel-beck @jenkinsci/code-reviewers With this change the master branch will be "clean" at least in the case of high-priority issues.

Do we want to backport anything from the fixes? If yes, I can create JIRA issues and move fixes to separate PRs

@@ -50,10 +50,11 @@
private Authentication transportAuth;
//TODO: Migrate the code to Callable decorator

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Aug 1, 2016

Member

CalllableFilter is deprecated in remoting. Ideally the code here should be rewritten

@oleg-nenashev

oleg-nenashev Aug 1, 2016

Member

CalllableFilter is deprecated in remoting. Ideally the code here should be rewritten

@@ -137,6 +137,11 @@ public String getDisplayName() {
@Override
public DefaultCrumbIssuer newInstance(StaplerRequest req, JSONObject formData) throws FormException {
if (req == null) {

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Aug 1, 2016

Member

According to the discussion with @amuniz it may be to somehow handle this case instead of just failing. But FormException is better than NPE

@oleg-nenashev

oleg-nenashev Aug 1, 2016

Member

According to the discussion with @amuniz it may be to somehow handle this case instead of just failing. But FormException is better than NPE

@daniel-beck

This comment has been minimized.

Show comment
Hide comment
@daniel-beck

daniel-beck Aug 1, 2016

Member

Do we want to backport anything from the fixes? If yes, I can create JIRA issues and move fixes to separate PRs

Doubt it, unless we know of an actual issue this fixes. CC @olivergondza

Member

daniel-beck commented Aug 1, 2016

Do we want to backport anything from the fixes? If yes, I can create JIRA issues and move fixes to separate PRs

Doubt it, unless we know of an actual issue this fixes. CC @olivergondza

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev Aug 1, 2016

Member

Particular issues like broken log messages (5a93762 and e5a728b) are definitely reproducible, but I'm not sure they are important enough to spend time on backporting.

Member

oleg-nenashev commented Aug 1, 2016

Particular issues like broken log messages (5a93762 and e5a728b) are definitely reproducible, but I'm not sure they are important enough to spend time on backporting.

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
Member

oleg-nenashev commented Aug 4, 2016

@reviewbybees

This comment has been minimized.

Show comment
Hide comment
@reviewbybees

reviewbybees Aug 4, 2016

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

reviewbybees commented Aug 4, 2016

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev Aug 6, 2016

Member

Retriggering the build to check the current master state

Member

oleg-nenashev commented Aug 6, 2016

Retriggering the build to check the current master state

@stephenc

This comment has been minimized.

Show comment
Hide comment
@stephenc

stephenc Aug 6, 2016

Member

@oleg-nenashev I'm getting ticked off with the build failures on the PR builder!

Member

stephenc commented Aug 6, 2016

@oleg-nenashev I'm getting ticked off with the build failures on the PR builder!

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev Aug 7, 2016

Member

Got +1 from @daniel-beck in IRC. As we discussed, I commit to monitor Jenkins core builds and to fix the code if anything blows up after merges of pending PRs.

I'm going to merge PR without squash, because several fixes require backporting according to our discussion with @jtnord. The commits are consistent enough. In JENKINS-36717 I'll provide a list of commits requiring backporting

Member

oleg-nenashev commented Aug 7, 2016

Got +1 from @daniel-beck in IRC. As we discussed, I commit to monitor Jenkins core builds and to fix the code if anything blows up after merges of pending PRs.

I'm going to merge PR without squash, because several fixes require backporting according to our discussion with @jtnord. The commits are consistent enough. In JENKINS-36717 I'll provide a list of commits requiring backporting

@oleg-nenashev oleg-nenashev merged commit ba83b3d into jenkinsci:master Aug 7, 2016

1 check passed

Jenkins This pull request looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment