walkmod execution to remove dead code #1957

rpau · 2015-12-20T21:09:08Z

Hi,

I have executed walkmod, an open source tool to apply code conventions. In this case, I just tried to remove dead code in the core module. After executing it, the results are as follows:

Deletes of unused imports
Deletes of private variables and fields
Deletes of some private "readResolve" methods because classes are not Serializable.

Moreover, executing walkmod I have discovered some dependency conflicts that I have specified as "exclusions" and a missing runtime dependency.

Let me know if the results seem interesting to you :) and thank you for your time.

KostyaSha · 2015-12-20T21:16:57Z

👎 even if classes are not marked with Serializable they are serialised/deserialized with XStream, this change will break jenkins.
👎 for doing code clean-ups without understanding jenkins code and without having defined rules before to exclude next possible changes.
@rpau Please join discussion https://groups.google.com/d/topic/jenkinsci-dev/8fjvXGYbFJ4/discussion

rpau · 2015-12-20T22:08:47Z

Thank you for your comments

Then, there is a problem: Some classes should implement Serializable and they don't. Right? I can add it as a rule.
Sorry, but I don't understand the second issue. What do you mean "without having defined rules before to exclude next possible changes"? Do you prefer a PR just with the walkmod configuration and not the corrected code?
Thank you for the link. I will be a pleasure to participate :).

KostyaSha · 2015-12-21T09:33:36Z

Sorry, i wasn't clear enough.

for "2)" i mean coding style changes should go to email discussion. As there is still no agreement in project. Changes for imports * to single lines and again may happen always and coding style should be defined before.
walkmod usage discussion may happen here

imod · 2015-12-21T09:38:13Z

I'm not yet sure if I think this is a good thing or not, but its an interesting idea!
Definitively wrong is the remove of all readResolve() - this will break quite a bit of backward compatibility

KostyaSha · 2015-12-21T09:41:05Z

@imod @oleg-nenashev and i do periodic static analysing verifications with coverity (that is the most powerful as it closed source). Code styling under discussion. Dependency changes under question as it may change versions and break plugins. PR build fails because of license(?).

daniel-beck · 2015-12-21T09:42:51Z

Probably needs to be added to https://github.com/jenkinsci/jenkins/blob/master/licenseCompleter.groovy

rpau · 2015-12-21T10:01:31Z

Thank your for all your comments :) I really appreciate them.

What am I asking if there is an specific criteria to define if some classes should implement Serializable. What do you prefer? Add the implements Serializable or just ignore those readResolve? Two options are valid for me to adapt your walkmod configuration and plugins. Other practices such as add @Override can be added. If you are interested in the autofix of an specific Checkstyle config, I can do too.
This PR fails because there is a test that produces a null pointer, but this test also failed before I applied the walkmod changes.
Dependency changes are because there are multiple dependencies with different versions. However, the version choosen by maven is the same. The other one, is just a "runtime" dependency which may also appear in some of your executions. I have solved it because, walkmod uses Java reflection and with one of your classes, it produces an error because there is a dependency with a class of the introduced library.
Walkmod can be executed by developers before pushing code (and even just for your modified files), like PMD or CheckStyle from their local machine. Moreover, you could receive periodic PR with the suggested changes by walkmodhub (walkmod service) and then decide to accept or reject changes. Actually, I prefer to discuss what are your conventions and have a valid "execution" for this project :)

rpau · 2015-12-23T14:49:26Z

Hi, I have upgraded the walkmod configuration and the final result. Now, the readResolve methods are not removed. Can you take a look if the rest is ok for you?

Thanks

KostyaSha · 2015-12-23T14:53:17Z

core/src/main/java/hudson/model/Slave.java

-    /**
-     * Lazily computed set of labels from {@link #label}.
-     */
-    private transient volatile Set<Label> labels;


Should be protected instead of private. But probably may cause compatibility issues.

Perfect :-) ! I can change it

I fear you can't touch it because it may cause issues as i definitely remember that using labels cache for slaves. Feel free to investigate cloud plugins.

Jenkins allow that cloud plugins are able to access to this "private" field using reflection? Otherwise, they can't access to this field.

java code - not, groovy - whatever you want.

Ok ok, thanks

I have found plugins with groovy code that reference this property. I will add an exclude rule to avoid remove this property.

@rpau and now image that not all plugins under jenkinsci github account are hosted. Any clean-up change adds probability in compatibility breakage that jenkins is keeping so many years. You can propose such step for 2.0 as it will expect breakages. Removing unused imports imho relates to coding style (thread that i started in dev list).

Yes, I understand it, but I think that it is consequence of a direct binding of the plugins with the internal implementation details. I agree on applying these kind of changes in the 2.0 version.

Removing unused imports or variables is part of the coding style, but also any of the changes in the Java source file as well. However, in contrast with the field and method declarations, it can't break plugins.

KostyaSha · 2015-12-23T14:55:51Z

Any removed method can be accessed from groovy.

KostyaSha · 2015-12-23T14:59:21Z

Could you provide diff of dependencies before this PR and after?

rpau · 2015-12-23T15:13:20Z

So,

I need to check if the removed methods or fields are used by any cloud plugin, right?
What do you mean with the diff of dependencies - create a different PR ? You can compare the pom.xml in this PR.

KostyaSha · 2015-12-23T15:17:35Z

I need to check if the removed methods or fields are used by any cloud plugin, right?

Wait for other reviewers. Keeping in mind that some of already on vacation, it may happen that PR wouldn't be merged until january.

pom doesn't reflect transitive deps, text diff from mvn dependency tree before and after

rpau · 2015-12-23T15:34:52Z

Diff

It includes the new runtime dependency (javarebel-sdk) and the excluded rules for stappler that previously, sends a conflict message.

> [INFO] +- org.zeroturnaround:javarebel-sdk:jar:2.0.2:runtime
73d95
< [INFO] |  \- (org.kohsuke.stapler:stapler:jar:1.140:compile - omitted for conflict with 1.237)
75d96
< [INFO] |  \- (org.kohsuke.stapler:stapler:jar:1.140:compile - omitted for conflict with 1.237)
77d97
< [INFO] |  \- (org.kohsuke.stapler:stapler:jar:1.140:compile - omitted for conflict with 1.237)
79d98
< [INFO] |  \- (org.kohsuke.stapler:stapler:jar:1.140:test - omitted for conflict with 1.237)

KostyaSha · 2015-12-23T15:55:30Z

core/src/main/java/hudson/model/Job.java

@@ -674,7 +674,7 @@ public void renameTo(String newName) throws IOException {

    @Override
    public void movedTo(DirectlyModifiableTopLevelItemGroup destination, AbstractItem newItem, File destDir) throws IOException {
-        Job newJob = (Job) newItem; // Missing covariant parameters type here.


I thought this comment was related to removed line. I guess it used to check types even if it not used.

But do you think that is makes sense if the parameter is not used?

I don't see any reason to keep this line

Or to be on the safe side assert newItem instanceof Job

oleg-nenashev · 2015-12-27T13:40:24Z

core/pom.xml

+       <artifactId>javarebel-sdk</artifactId>
+       <version>2.0.2</version>
+       <scope>runtime</scope>
+    </dependency>


CC @bsideup

daniel-beck · 2016-01-08T13:49:23Z

@KostyaSha

Any removed method can be accessed from groovy.

WDYM? (Is this comment obsolete?)

rpau · 2016-01-12T16:29:58Z

Hi, is going to be accepted this pull request? What else is necessary to correct?

Thanks

KostyaSha · 2016-01-12T16:31:42Z

@rpau get agreement between contributors, that may end in PR delayed for years.

KostyaSha · 2016-01-12T16:35:11Z

WDYM? (Is this comment obsolete?)

Not really, but i'm against removing some code just for analyser until core will have some defined rules. I also don't think that @rpau would be ready to work on any regressions caused but this changes.

Also this PR still doesn't pass verification.

KostyaSha · 2016-01-12T16:37:07Z

core/src/main/java/hudson/init/package-info.java

@@ -44,4 +44,3 @@
 */
 package hudson.init;

-import org.jvnet.hudson.reactor.Task;


Is javadoc navigable after this change?

Yes, because the javadoc only uses a full qualified name

Not all docs using FQDN that depends on IDE settings.

daniel-beck · 2016-07-21T12:00:38Z

@KostyaSha Any Groovy script wasn't supposed to access private methods in the first place. There has never been a reasonable expectation that we keep compatibility for those.

oleg-nenashev · 2016-08-16T06:39:22Z

Any plans to finalize it?

rpau · 2016-08-16T06:41:58Z

@oleg-nenashev To finalize the review?

daniel-beck · 2018-01-23T17:03:12Z

PR appears to have been abandoned, closing.

rpau added 2 commits December 20, 2015 22:02

dependency conflicts and missing runtime dependencies

98aefbc

walkmod config

17beee9

walkmod config with exclude rules

8f3812d

KostyaSha reviewed Dec 23, 2015
View reviewed changes

rpau added 2 commits December 27, 2015 13:04

excluding rules

0631bac

walkmod execlution with excluding rules over model fields

355dc6b

oleg-nenashev reviewed Dec 27, 2015
View reviewed changes

core/pom.xml

<artifactId>javarebel-sdk</artifactId>

<version>2.0.2</version>

<scope>runtime</scope>

</dependency>

Copy link

Member

oleg-nenashev Dec 27, 2015

Choose a reason for hiding this comment

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

CC @bsideup

KostyaSha reviewed Jan 12, 2016
View reviewed changes

oleg-nenashev added the needs-comments-resolved Comments in the PR need to be addressed before it can be considered for merging label Jul 2, 2016

oleg-nenashev added the unresolved-merge-conflict There is a merge conflict with the target branch. label Jul 2, 2016

daniel-beck closed this Jan 23, 2018

walkmod execution to remove dead code #1957

walkmod execution to remove dead code #1957

Conversation

rpau commented Dec 20, 2015

KostyaSha commented Dec 20, 2015

rpau commented Dec 20, 2015

KostyaSha commented Dec 21, 2015

imod commented Dec 21, 2015

KostyaSha commented Dec 21, 2015

daniel-beck commented Dec 21, 2015

rpau commented Dec 21, 2015

rpau commented Dec 23, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KostyaSha commented Dec 23, 2015

KostyaSha commented Dec 23, 2015

rpau commented Dec 23, 2015

KostyaSha commented Dec 23, 2015

rpau commented Dec 23, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniel-beck commented Jan 8, 2016

rpau commented Jan 12, 2016

KostyaSha commented Jan 12, 2016

KostyaSha commented Jan 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniel-beck commented Jul 21, 2016

oleg-nenashev commented Aug 16, 2016

rpau commented Aug 16, 2016

daniel-beck commented Jan 23, 2018