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-41631, JENKINS-40088, JENKINS-43715, JENKINS-40979] - Update Stapler and Enforce upper bound deps #2956

Merged
merged 3 commits into from Aug 11, 2017

Conversation

5 participants
@jglick
Member

jglick commented Jul 31, 2017

Follow-up to jenkinsci/plugin-pom#67. Downstream of stapler/stapler#123.

Proposed changelog entries:

  • Cleanup of Maven dependencies in Jenkins core, allowing plugins depending on this version or later to build without “upper bound” dependency warnings.
  • Stapler library updated from 1.250 to 1.252:
    • order of search for web method overloads is now deterministic in case of ambiguity
    • deprecated HttpResponses.html and .plainText in favor of .literalHtml and .text
    • related to JENKINS-40088: configurable export API behavior
    • JENKINS-43715: NullPointerException fix
    • related to JENKINS-40979: NullPointerException fix
    • JENKINS-45903: Prevent file handle leak in LargeText.GzipAwareSession

@reviewbybees

@reviewbybees

This comment has been minimized.

Show comment
Hide comment
@reviewbybees

reviewbybees Jul 31, 2017

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 Jul 31, 2017

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.

@@ -95,6 +95,12 @@ THE SOFTWARE.
<dependency>
<groupId>com.google.inject</groupId>
<artifactId>guice</artifactId>
<exclusions>
<exclusion> <!-- TODO it seems to want Guava 16; apparently it manages to run against 11 -->

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Jul 31, 2017

Member

What could possibly go wrong? :(

@oleg-nenashev

oleg-nenashev Jul 31, 2017

Member

What could possibly go wrong? :(

This comment has been minimized.

@jglick

jglick Aug 1, 2017

Member

Well, we are already doing it—this just makes it apparent.

@jglick

jglick Aug 1, 2017

Member

Well, we are already doing it—this just makes it apparent.

<exclusions>
<exclusion> <!-- pick up from Stapler -->
<groupId>com.google.code.findbugs</groupId>
<artifactId>jsr305</artifactId>

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Jul 31, 2017

Member

Maybe it's safer to just define the newest version in this POM instead of adding exclusions

@oleg-nenashev

oleg-nenashev Jul 31, 2017

Member

Maybe it's safer to just define the newest version in this POM instead of adding exclusions

This comment has been minimized.

@jglick

jglick Aug 1, 2017

Member

Maybe. Not used at runtime anyway, so probably does not matter.

@jglick

jglick Aug 1, 2017

Member

Maybe. Not used at runtime anyway, so probably does not matter.

@@ -53,7 +53,7 @@ THE SOFTWARE.
<dependency>
<groupId>${project.groupId}</groupId>
<artifactId>jenkins-test-harness</artifactId>
<version>2.20</version>
<version>2.23</version>

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Jul 31, 2017

Member

Unrelated to this change, I'd guess

@oleg-nenashev

oleg-nenashev Jul 31, 2017

Member

Unrelated to this change, I'd guess

This comment has been minimized.

@batmat

batmat Aug 1, 2017

Member

It is, to catch jenkinsci/jenkins-test-harness#62 which indeed got released as 2.23.

@batmat

This comment has been minimized.

@jglick

jglick Aug 1, 2017

Member

Correct.

@jglick

jglick Aug 1, 2017

Member

Correct.

This comment has been minimized.

@oleg-nenashev
@oleg-nenashev
@@ -239,6 +263,23 @@ THE SOFTWARE.
<skip>true</skip>
</configuration>
</plugin>
<plugin> <!-- TODO pending JENKINS-45271 fix, would be best to finish moving MavenModuleSet-specific tests to maven-plugin and delete the test dep here -->

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Jul 31, 2017

Member

Yeah, needs fix asap. OTOH, how many MavenPlugin-dependent tests do we have in the core? Maybe we should just move them elsewhere and remove the dependency.

@oleg-nenashev

oleg-nenashev Jul 31, 2017

Member

Yeah, needs fix asap. OTOH, how many MavenPlugin-dependent tests do we have in the core? Maybe we should just move them elsewhere and remove the dependency.

This comment has been minimized.

@jglick

jglick Aug 1, 2017

Member

There are a bunch. I do think it would be wise to move them to maven-plugin and delete the dep. I just did not want to take it on in this PR; would confuse the issue too much.

@jglick

jglick Aug 1, 2017

Member

There are a bunch. I do think it would be wise to move them to maven-plugin and delete the dep. I just did not want to take it on in this PR; would confuse the issue too much.

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Aug 2, 2017

Member

Could you please create a follow-up ticket for it?

@oleg-nenashev

oleg-nenashev Aug 2, 2017

Member

Could you please create a follow-up ticket for it?

This comment has been minimized.

@jglick
@jglick
@batmat

batmat approved these changes Aug 1, 2017

A wee bit more unsure about the part in the test module, but as this is the test module, this is OK IMO. Worse case something is wrong and we fix it in the future, but it won't reach the war/production anyway.

@@ -87,7 +87,7 @@ THE SOFTWARE.
<patch.tracker.serverId>jenkins-jira</patch.tracker.serverId>
<guavaVersion>11.0.1</guavaVersion>
<slf4jVersion>1.7.7</slf4jVersion> <!-- < 1.6.x version didn't specify the license (MIT) -->
<slf4jVersion>1.7.25</slf4jVersion>

This comment has been minimized.

@batmat

batmat Aug 1, 2017

Member

👍 been hit by this recently and this improvement shows it's worth it to avoid JENKINS-41631 being too much of a nightmare when it starts reaching plugins (I think now it's still so recent that only very few ever tried, or reverted it seeing that new error).

@batmat

batmat Aug 1, 2017

Member

👍 been hit by this recently and this improvement shows it's worth it to avoid JENKINS-41631 being too much of a nightmare when it starts reaching plugins (I think now it's still so recent that only very few ever tried, or reverted it seeing that new error).

This comment has been minimized.

@jglick

jglick Aug 1, 2017

Member

Yes the intent here (together with the plugin-pom PR) was to allow a plugin using “newest everything” to build without warnings.

@jglick

jglick Aug 1, 2017

Member

Yes the intent here (together with the plugin-pom PR) was to allow a plugin using “newest everything” to build without warnings.

This comment has been minimized.

@oleg-nenashev
@oleg-nenashev
@@ -53,7 +53,7 @@ THE SOFTWARE.
<dependency>
<groupId>${project.groupId}</groupId>
<artifactId>jenkins-test-harness</artifactId>
<version>2.20</version>
<version>2.23</version>

This comment has been minimized.

@batmat

batmat Aug 1, 2017

Member

It is, to catch jenkinsci/jenkins-test-harness#62 which indeed got released as 2.23.

@batmat
@batmat

This comment has been minimized.

Show comment
Hide comment
@batmat

batmat Aug 1, 2017

Member

And so 🐝 for the tooling to catch up.

Member

batmat commented Aug 1, 2017

And so 🐝 for the tooling to catch up.

@oleg-nenashev

🐝 , a follow-up JIRA ticket for Maven Project tests is required. I or probably @aheritier will do it once we are around

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev
Member

oleg-nenashev commented Aug 2, 2017

@reviewbybees

This comment has been minimized.

Show comment
Hide comment
@reviewbybees

reviewbybees Aug 2, 2017

This pull request has completed our internal processes and we now respectfully request the maintainers of this repository to consider our proposal contained within this pull request for merging.

reviewbybees commented Aug 2, 2017

This pull request has completed our internal processes and we now respectfully request the maintainers of this repository to consider our proposal contained within this pull request for merging.

@jglick

This comment has been minimized.

Show comment
Hide comment
@jglick

jglick Aug 2, 2017

Member

Blocked pending @oleg-nenashev’s re-review of upstream PR.

Member

jglick commented Aug 2, 2017

Blocked pending @oleg-nenashev’s re-review of upstream PR.

@@ -39,7 +39,7 @@ THE SOFTWARE.
<properties>
<staplerFork>true</staplerFork>
<stapler.version>1.250</stapler.version>
<stapler.version>1.252</stapler.version>

This comment has been minimized.

@jglick
@jglick

This comment has been minimized.

@batmat

batmat Aug 20, 2017

Member

Now I wonder why I didn't ask about this when reviewing: was this related to Enforcer Upper Bound Deps? Shouldn't it have been a dedicated PR?

Anyway, it revealed stapler/stapler#128 for us, an internal plugin that does not compile anymore.

@batmat

batmat Aug 20, 2017

Member

Now I wonder why I didn't ask about this when reviewing: was this related to Enforcer Upper Bound Deps? Shouldn't it have been a dedicated PR?

Anyway, it revealed stapler/stapler#128 for us, an internal plugin that does not compile anymore.

@jglick

This comment has been minimized.

Show comment
Hide comment
@jglick

jglick Aug 3, 2017

Member

Ready for final review I think (assuming tests pass).

Member

jglick commented Aug 3, 2017

Ready for final review I think (assuming tests pass).

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev Aug 3, 2017

Member

The changelog entries should also refer Stapler upgrade, JENKINS-43715 and maybe JENKINS-40979

Member

oleg-nenashev commented Aug 3, 2017

The changelog entries should also refer Stapler upgrade, JENKINS-43715 and maybe JENKINS-40979

@@ -128,7 +142,7 @@ THE SOFTWARE.
<dependency><!-- we exclude this transient dependency from htmlunit, which we actually need in the test -->
<groupId>xalan</groupId>
<artifactId>xalan</artifactId>
<version>2.7.1</version>
<version>2.7.2</version>

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Aug 3, 2017

Member

So it's bundled into the core, right? Maybe requires a changelog entry as well

@oleg-nenashev

oleg-nenashev Aug 3, 2017

Member

So it's bundled into the core, right? Maybe requires a changelog entry as well

This comment has been minimized.

@jglick

jglick Aug 3, 2017

Member

No, this is the test POM.

@jglick

jglick Aug 3, 2017

Member

No, this is the test POM.

@jglick

This comment has been minimized.

Show comment
Hide comment
@jglick

jglick Aug 3, 2017

Member

Sigh, forgot to release the staging repo for 1.252. Did so now, but need to wait for Central synch and then trigger a new build.

Member

jglick commented Aug 3, 2017

Sigh, forgot to release the staging repo for 1.252. Did so now, but need to wait for Central synch and then trigger a new build.

@jglick

This comment has been minimized.

Show comment
Hide comment
@jglick
Member

jglick commented Aug 4, 2017

@jtnord

jtnord approved these changes Aug 5, 2017

🐝

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev Aug 7, 2017

Member

Will merge towards the next weekly if there is no negative feedback by Thursday

Member

oleg-nenashev commented Aug 7, 2017

Will merge towards the next weekly if there is no negative feedback by Thursday

@oleg-nenashev oleg-nenashev merged commit b8f6246 into jenkinsci:master Aug 11, 2017

1 check passed

continuous-integration/jenkins/pr-head This commit looks good
Details

@oleg-nenashev oleg-nenashev changed the title from [JENKINS-41631] Enforce upper bound deps on Jenkins core to [JENKINS-41631, JENKINS-40088, JENKINS-43715, JENKINS-40979] - Update Stapler and Enforce upper bound deps on Jenkins core Aug 11, 2017

@oleg-nenashev oleg-nenashev changed the title from [JENKINS-41631, JENKINS-40088, JENKINS-43715, JENKINS-40979] - Update Stapler and Enforce upper bound deps on Jenkins core to [JENKINS-41631, JENKINS-40088, JENKINS-43715, JENKINS-40979] - Update Stapler and Enforce upper bound deps Aug 11, 2017

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev Aug 13, 2017

Member

Added JENKINS-45903 to the changelog. We need to double-check changes in Stapler @daniel-beck

Member

oleg-nenashev commented Aug 13, 2017

Added JENKINS-45903 to the changelog. We need to double-check changes in Stapler @daniel-beck

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev
Member

oleg-nenashev commented Aug 13, 2017

@jglick jglick deleted the jglick:requireUpperBoundDeps-JENKINS-41631 branch Aug 14, 2017

<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-enforcer-plugin</artifactId>
<version>3.0.0-M1</version> <!-- TODO 3.0.0 when released -->

This comment has been minimized.

@jglick
@jglick
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment