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

Bump to v1.105 #36

Merged
merged 5 commits into from
Jan 27, 2020
Merged

Bump to v1.105 #36

merged 5 commits into from
Jan 27, 2020

Conversation

bitwiseman
Copy link
Contributor

@bitwiseman bitwiseman commented Jan 22, 2020

@kshultzCB @dwnusbaum @jglick @oleg-nenashev

The change adds what I've learned about shading. In the next version of github-api we can produce a shaded jar with it's own pom, but for now this gets us the shaded jar with the project pom, from which we can exclude all dependencies.

The change adds what I've learned about shading - don't do it. Instead I cleaned up the dependencies to make this safe without shading.

Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

exclusions looks suspicious, but LGTM if tested locally

pom.xml Outdated Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
@dwnusbaum
Copy link
Member

The test failures looks somewhat similar to those in JENKINS-60694, although I am not sure if the socket read that is timing out in the tests here is connecting to Jetty or something else. I would try updating the parent POM to 3.56 to pick up jenkinsci/jenkins-test-harness#193 through jenkinsci/plugin-pom#283 to see if that helps.

@bitwiseman
Copy link
Contributor Author

@dwnusbaum Test failure was transitory.

@bitwiseman
Copy link
Contributor Author

@dwnusbaum @oleg-nenashev
Okay it looks like shading is not going to work - or it will work but it is a breaking change.

Any exceptions defined in one of the shaded dependencies will now be in a new package. This means any consumer of this library that catches wants to specifically catch com.fasterxml.jackson.databind.JsonMappingException from the shaded library (as opposed to a general java.io.IOException) would would need to be rewritten to catch hidden.com.fasterxml.jackson.databind.JsonMappingException (where hidden is the package name classes were relocated into by shading).

So, given that is not going to work, I'll have to figure something else out.

@dwnusbaum
Copy link
Member

@bitwiseman Are the jackson exceptions the only problematic ones? If so, you could shade everything but jackson upstream in github-api, and pull in the jackson2-api plugin in compile scope here.

@dwnusbaum
Copy link
Member

Also, is the shading necessary because of changes upstream (newly added dependencies or something), or were you just trying to make it so that other plugins that depend on this one didn't need to worry about handling <exclusions> or upper bound mismatches?

@bitwiseman bitwiseman force-pushed the task/shaded branch 2 times, most recently from 548da52 to c4bb5b8 Compare January 23, 2020 22:31
@bitwiseman
Copy link
Contributor Author

It was requested due to changes upstream in 1.99.
In 1.103, I've removed the dependency on commons-codec, so shading looks much less needed.

Per our discussion offline, I've updated the minimum Jenkins version for this plugin to 2.164.3, so that commons-io is at least 2.6 the same version as github-api. commons-lang3 is not a dependency of Jenkins, so were safe there.

github-api uses commons-io 2.6 so also bumping jenkins version to 2.164.3
@bitwiseman
Copy link
Contributor Author

@oleg-nenashev
I'd like to merge and release this. I've tested against github-branch-source locally and it works.

Copy link
Member

@dwnusbaum dwnusbaum left a comment

Choose a reason for hiding this comment

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

If you take a look at the jars bundled in the plugin, you'll see that jackson is not bundled, which is good, because we want to pick it up through the jackson2-api plugin, but commons-io is bundled, which is bad, because the way classloading works means we are actually going to use commons-io from core, so bundling the library increases the plugin's size for no reason. I think you should exclude commons-io from the github-api dependency to fix this:

$ jar -tf target/github-api.hpi
META-INF/
META-INF/MANIFEST.MF
WEB-INF/
WEB-INF/lib/
WEB-INF/licenses.xml
WEB-INF/lib/github-api-1.103.jar (the jar for the library)
WEB-INF/lib/commons-lang3-3.9.jar
WEB-INF/lib/okio-1.6.0.jar
WEB-INF/lib/okhttp-2.7.5.jar
WEB-INF/lib/commons-io-2.6.jar (we don't want to bundle this because it is in core)
WEB-INF/lib/okhttp-urlconnection-2.7.5.jar
WEB-INF/lib/github-api.jar (the jar for the plugin itself)

@@ -56,6 +56,11 @@
</build>

<dependencies>
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>jackson2-api</artifactId>
Copy link
Member

@dwnusbaum dwnusbaum Jan 24, 2020

Choose a reason for hiding this comment

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

From a quick look at mvn dependency:tree, it looks like the Jackson libraries are picked up through the plugin, which is what you want, but I'm not 100% sure why they come through this dependency and not github-api, since Jackson is a one-level deep transitive dependency in both cases, so maybe it's just a matter of ordering or something. I would explicitly exclude Jackson from the github-api dependency in case an update to that library might change the behavior of Maven.

Copy link
Member

@dwnusbaum dwnusbaum left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@bitwiseman
Copy link
Contributor Author

@dwnusbaum

$ jar -tf target/github-api.hpi
META-INF/
META-INF/MANIFEST.MF
WEB-INF/
WEB-INF/lib/
WEB-INF/licenses.xml
WEB-INF/lib/github-api-1.103.jar
WEB-INF/lib/commons-lang3-3.9.jar
WEB-INF/lib/okio-1.6.0.jar
WEB-INF/lib/okhttp-2.7.5.jar
WEB-INF/lib/okhttp-urlconnection-2.7.5.jar
WEB-INF/lib/github-api.jar

@dwnusbaum
Copy link
Member

Also just while it is on my mind, the behavior of what jars are bundled into the .hpi is controlled by maven-hpi-plugin, which has had some bugs in the past where it included jars unnecessarily (e.g. jenkinsci/maven-hpi-plugin#140), so it never hurts to take a look at the hpi when making significant modifications to dependencies or exclusions just to make sure everything looks ok.

No dependency changes
@bitwiseman bitwiseman changed the title Bump to v1.103 Bump to v1.105 Jan 27, 2020
@bitwiseman bitwiseman merged commit de74489 into jenkinsci:master Jan 27, 2020
@bitwiseman bitwiseman deleted the task/shaded branch January 27, 2020 08:25
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.

4 participants