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.

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

Remove commons-io as it comes from Jenkins core #153

Merged
merged 1 commit into from
Oct 13, 2020

Conversation

timja
Copy link
Member

@timja timja commented Oct 9, 2020

Part of fixing jenkinsci/junit-plugin#176

https://github.com/jenkinsci/jenkins/blob/dd134ef84e642e851c1fcc8946bebef5cce840ca/bom/pom.xml#L69

Tested in bootstrap4-api-plugin, the Jenkins bom overrides the version coming from codingstyle but not the explicit dependency here.

@timja
Copy link
Member Author

timja commented Oct 10, 2020

cc @uhafner

@uhafner
Copy link
Member

uhafner commented Oct 10, 2020

Hmm, this change (and the other ones) shift only the place where one needs to fix the enforcer bounds (I do not like the enforcer, it just creates so many extra work without fixing anything). If I merge this change I need to add a dependency constraint on all my plugins that use a library that depends on a newer commons-io library. That was the reason I added it here: then I have one place where I can fix those enforcer messages.

@timja
Copy link
Member Author

timja commented Oct 11, 2020

As far as I know if a library is used in Jenkins core then that’s the version that will be actually used and putting a newer version here does nothing.

Using a bom managed dep (which this is) should handling conflicting does, and if it doesn’t then you should exclude it from anywhere that’s pulling it in.

cc @jglick if any of what I’m saying is rubbish

Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

Right, unless you are doing complex things with class loaders somewhere, the commons-io you are actually using is the one from jenkins-core. (Which was only recently upgraded to 2.8.0 from 2.6.)

@uhafner
Copy link
Member

uhafner commented Oct 13, 2020

Ok. I will do the extra work (for the sake of the enforcer) in my plugins that use the library... Again another hour lost just to make the enforcer happy...

@uhafner uhafner merged commit c239427 into jenkinsci:master Oct 13, 2020
@timja timja deleted the exclude-commons-io branch October 13, 2020 13:16
@jglick
Copy link
Member

jglick commented Oct 13, 2020

The Enforcer plugin is doing the right thing here—catching a genuine mistake.

@uhafner
Copy link
Member

uhafner commented Oct 13, 2020

Can you elaborate? Why is the enforcer catching a mistake? It is a false positive (it might be a problem with a chance of let's say < 1%), otherwise something will not work when the plugin is deployed in Jenkins. While I am using the latest commons-io library (and several others like SpotBugs annotations) I never got a bug report from a user because of a missing method or a similar error. In my plugins I am using several libraries that depend on up-to-date commons-io (and SpotBugs) versions. So why is this a problem? Seems that these dependencies do not use methods that are not also part of the Jenkins versions (otherwise the plugin would not work anyway and then we have a problem).

So what is the idea if the enforcer does report such an issue? Should we ban dependencies (libraries) that depend on libraries that are not linked in Jenkins core with the same version? This means we will loose a lot of new features of those libraries! (even if the likelihood of such a failure is so low)

@jglick
Copy link
Member

jglick commented Oct 13, 2020

Seems that these dependencies do not use methods that are not also part of the Jenkins versions

Well then you are lucky. The point of Enforcer is to prevent accidents, just like strong typing in a compiler prevents you from referring to a nonexistent method.

Should we ban dependencies (libraries) that depend on libraries that are not linked in Jenkins core with the same version?

Not necessarily—it is OK if the dependency does not really need that new a version of commons-io or whatever. But making the test classpath according to Maven match reality ensures that any reference to a new method in commons-io would throw an error at test time. It also ensures you do not accidentally compile your own code against the wrong version.

@uhafner uhafner added the dependencies Update of dependencies label Oct 13, 2020
@uhafner
Copy link
Member

uhafner commented Oct 14, 2020

Ok, thanks for clarifying!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Update of dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants