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-46053, JENKINS-45271, JENKINS-46210, JENKINS-46148] - Update HttpClient libraries and Fix Parent POM #102
Conversation
…ns of Apache HttpClient plugin and Maven Interceptors
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems OK. Is there a quick summary of what this practically changes in terms of bundled libraries?
jar tf target/maven-plugin.hpi | fgrep WEB-INF/lib | cut -c13- | sort
before and after the patch and diff
them.
pom.xml
Outdated
@@ -44,8 +44,8 @@ THE SOFTWARE. | |||
<properties> | |||
<jenkins.version>1.625.3</jenkins.version> | |||
<java.level>7</java.level> | |||
<mavenInterceptorsVersion>1.11</mavenInterceptorsVersion> | |||
<mavenVersion>3.1.0</mavenVersion> | |||
<mavenInterceptorsVersion>1.12-SNAPSHOT</mavenInterceptorsVersion> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use a timestamped snapshot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree but it depends how many days the PR will leave. SNAPSHOTs are quickly removed and thus it makes the build KO each time a new SNAPSHOT is published ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SNAPSHOTs are quickly removed
Not that quickly.
pom.xml
Outdated
<plugin> | ||
<groupId>org.apache.maven.plugins</groupId> | ||
<artifactId>maven-enforcer-plugin</artifactId> | ||
<version>3.0.0-M1</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we not get a version for free from plugin-pom
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, at least maven-enforcer-plugin.version
should be used
<basePackage>**</basePackage> | ||
<includeTestCode>true</includeTestCode> | ||
<bannedImports> | ||
<bannedImport>org.apache.commons.httpclient.**</bannedImport> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is this better than bannedDependencies
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jglick It is a transient dependency from the Jenkins core, so it will blow up if I just ban it. bannedImports
rather says that this lib usage is deprecated across the Maven Plugin. jenkinsci/maven-interceptors#13
<rules> | ||
<restrictImports implementation="de.skuzzle.enforcer.restrictimports.RestrictImports"> | ||
<basePackage>**</basePackage> | ||
<includeTestCode>true</includeTestCode> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary.
W.r.t. JENKINS-45271, did you actually verify that a plugin may depend on |
pom.xml
Outdated
@@ -44,8 +44,8 @@ THE SOFTWARE. | |||
<properties> | |||
<jenkins.version>1.625.3</jenkins.version> | |||
<java.level>7</java.level> | |||
<mavenInterceptorsVersion>1.11</mavenInterceptorsVersion> | |||
<mavenVersion>3.1.0</mavenVersion> | |||
<mavenInterceptorsVersion>1.12-SNAPSHOT</mavenInterceptorsVersion> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree but it depends how many days the PR will leave. SNAPSHOTs are quickly removed and thus it makes the build KO each time a new SNAPSHOT is published ...
pom.xml
Outdated
@@ -223,7 +204,7 @@ THE SOFTWARE. | |||
<dependency> | |||
<groupId>org.apache.maven</groupId> | |||
<artifactId>maven-aether-provider</artifactId> | |||
<version>${mavenVersion}</version> | |||
<version>3.3.9</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we will quickly have a nightmare to resolve with the 3 versions of aether (Maven 3.5.0 added a new lib) cc @olamy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The released version of Maven plugin already depends on Maven Core 3.5.0, but Maven Plugin bundles core 3.1.0. So we are already in the YOLO mode :(
maven-aether-provider 3.5.0 does not exist. If the new library uses different package names, there should be no issue. We could bundle both. What could possibly go wrong? (c)
OK, here the fun begins....
|
So, my best guess that Maven agents uses libs from Maven (it does) and classloads the rest from Jenkins. The "rest" on Jenkins is newer than the stuff supported by Maven, hence the test blows up with flying colors. I'd guess the first step would be to pass Maven Embedder in classpath, where it is not mentioned. If it does not help, I think. If it does not help either, I drink as @jglick suggested.
|
Curious thing is that calls from Maven plugin happen within Maven Embedder:
|
OTOH I definitely see other requests like Maven Embedder version is required on the Agent/Master side, so we cannot just specify custom version in the classpath from what I see |
CI didn't even start for the last push |
I will move exclusions to the Maven Interceptors repo later. |
Look @reviewbybees @aheritier @olamy , the build is green! If everybody is fine with the approach, maybe we could integrate the PR |
Current libs included into the Maven Plugin bundle...
I definitely need to exclude HTTPCore from Jackrabbit. |
…-webdav-jackrabbit
Deployed the build as maven-plugin-2.18-20170921.105941-1.hpi |
@jglick As requested, full library diff:
|
Things to review:
Attached current deps.txt |
For something like this, pending JENKINS-41827 you just cannot trust |
really good job @oleg-nenashev The code review is ok too. I agree with your choices and I think that in the future we should just mark all maven dependencies as "provided" in interceptor modules. |
@reviewbybees done, I'd guess. ATH passed locally as well today. |
It should better to have an alpha in the experimental update center but to
be honest I think we’ll have 0 testers. It’s not if like we were about to
deliver a super cool new feature
I think we can do a real release but we monitor jira and if something goes
wrong we stand on the starting blocks to fix/release. WDYT? Cc @olamy
Le lun. 25 sept. 2017 à 20:21, Oleg Nenashev <notifications@github.com> a
écrit :
@reviewbybees <https://github.com/reviewbybees> done, I'd guess. ATH
passed locally as well today.
@aheritier <https://github.com/aheritier> Should we release the Maven
Plugin with alpha version of Interceptors? Or should we respin a new
release there?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#102 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAKqCFKGgN8X2SWzZ81JeGYMNkxlpx5oks5sl-8OgaJpZM4O3u-R>
.
--
-----
Arnaud Héritier
http://aheritier.net
Mail/GTalk: aheritier AT gmail DOT com
Twitter/Skype : aheritier
|
CC @martinda who referenced the Guava conflict in Http Request Plugin caused by the dependency on the Maven plugin. If there is an issue reported for it, I will add it to the changelog |
Here is the http-request plugin dependency on Guava 11 Ranges: CC @aheritier |
@martinda which particular version are required by the plugin? @aheritier @olamy What about Maven 2.18-rc-1 or 3.0-rc-1 release? The current request is probably incompatible because it stops bundling libraries available in 2.17 (especially Maven 3.5.0 deps), so 3.0 may be justtified enough |
@oleg-nenashev fine for me to do a 3.0-rc-1 cc @olamy |
Works for me. Starting from tomorrow I am available to do ad-hoc fixes if the release crash-lands in production. 2.17 already feels bad due to 3.5.0 dependencies, so I doubt rc-1 may make it worse |
@oleg-nenashev the plugin depends on |
The fix has been reverted in 3.0.0-rc1 by jenkinsci#102. It was just a mistake I made when patching the dependencies, but unfortunately nobody noticed it during the code review.
Caused https://issues.jenkins-ci.org/browse/JENKINS-47233. I should have noticed it during the dependency review |
Downstream PRs:
@reviewbybees @jtnord @rysteboe @jglick @aheritier @olamy (seems there is a nice party in the CC list)