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-37062] incorporate changes from stapler 1.253 (servlet 3.1) #3033

Merged
merged 8 commits into from Oct 27, 2017

Conversation

6 participants
@jtnord
Member

jtnord commented Sep 22, 2017

Downstream updates from stapler 1.253

The stapler API was built using servlet 2.5 and yet Jenkins uses 3.1.
This meant some of the code that stapler used was missing new methods
that where part of the updated spec and if a plugin happened to call
them you would end up with a LinkageError or some other crazyness.

This also make Jenkins depend on a dummy version of the old serlet-api
maven co-ordinates such that if any plugin gets aa dependency on it
transitivly (e.g. Jenkins test harness) they will get a version with no
code so the classpath should always be clean (this was more an issue for
eclipse than an mvn command)

See JENKINS-37062.

Testing performed

  • All unit tests ran - verified no new failures introduced.
  • checked the war does not include the servlet-api.jar (or javax.servlet-api.jar)
  • started Jenkins with java -jar jenkins.war and ran the install wizard

Upstream changes

Changelog and diffs.

Also Commons Codec changelog.

Proposed changelog entries

  • Stapler library upgraded to 1.253:
    • Improved performance for some Blue Ocean operations.
    • Various changes which could be of interest to plugin developers, for which see component changelog.
  • Commons Codec library upgraded to 1.9.

Submitter checklist

  • JIRA issue is well described
  • Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change). Examples
    * Use the Internal: prefix if the change has no user-visible impact (API, test frameworks, etc.)
  • Appropriate autotests or explanation to why this change has no tests
  • For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@reviewbybees
@jenkinsci/code-reviewers

[JENKINS-37062] incorporate changes from stapler servlet 3.1
Downstream updates from stapler/stapler#131

The stapler API was built using servlet 2.5 and yet Jenkins uses 3.1.
This meant some of the code that stapler used was missing new methods
that where part of the updated spec and if a plugin happened to call
them you would end up with a LinkageError or some other crazyness.

This also make Jenkins depend on a dummy version of the old serlet-api
maven co-ordinates such that if any plugin gets aa dependency on it
transitivly (e.g. Jenkins test harness) they will get a version with no
code so the classpath should always be clean (this was more an issue for
eclipse than an mvn command)

@jtnord jtnord referenced this pull request Sep 22, 2017

Merged

[stapler#114] Update to Servlet API 3.1.0 #131

2 of 2 tasks complete
pom.xml Outdated
<groupId>javax.servlet</groupId>
<!-- the old artifactID for the servlet API -->
<artifactId>servlet-api</artifactId>
<version>[0-SNAPSHOT]</version>

This comment has been minimized.

@jtnord

jtnord Sep 22, 2017

Member

this needs releasing before this can be merged... Not sure how to go about making that happen.
@daniel-beck is this just a case of a new PR to the repository permissions?

This comment has been minimized.

@daniel-beck

daniel-beck Sep 22, 2017

Member

As I wrote on the dev list, yes.

This comment has been minimized.

@jtnord

jtnord Sep 25, 2017

Member
@reviewbybees

This comment has been minimized.

reviewbybees commented Sep 22, 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.

<groupId>javax.servlet</groupId>
<artifactId>jstl</artifactId>
<version>1.1.0</version>
<groupId>javax.servlet.jsp.jstl</groupId>

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Sep 24, 2017

Member

Any risk of compatibility issues in plugins?

This comment has been minimized.

@jtnord

jtnord Sep 25, 2017

Member

there is always a risk - but 1.2 has been part of the Java EE spec since Java EE 5 (may 2006) that contained Servlet 2.5 so you likely got this at run time anyway. Whilst 1.2.1 is not called out in any spec it is an old version (also of the 2006 era in the JCP)
Maybe we should ask the spec lead who is now involved in the Jenkins community :-)
/cc @pelegri

@daniel-beck daniel-beck requested a review from stephenc Sep 25, 2017

@daniel-beck

This comment has been minimized.

Member

daniel-beck commented Sep 25, 2017

I think a review by Maven maven @stephenc would be helpful here.

@jglick jglick self-requested a review Sep 26, 2017

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

This comment has been minimized.

@jglick

jglick Sep 26, 2017

Member

Please use timestamped snapshots.

This comment has been minimized.

@jglick

jglick Oct 25, 2017

Member

Now just use the release.

pom.xml Outdated
<groupId>javax.servlet</groupId>
<!-- the old artifactID for the servlet API -->
<artifactId>servlet-api</artifactId>
<version>[0]</version>

This comment has been minimized.

@jglick

jglick Sep 26, 2017

Member

What does this mean, if not 0? A comment would be in order.

This comment has been minimized.

@stephenc

stephenc Sep 27, 2017

Member

This is a hard version range dependency that can only be satisfied by one and only one version, 0

When you do <version>0</version> you are telling Maven

Use any version you like, but I think 0 would be a good idea

The "closest" suggestion wins.

As soon as you have some version ranges specified, then Maven starts collating the intersection set... so if we had <version>[0,1.2)</version> and a dependency had <version>[1,1.3]</version> then Maven will conclude that the selected version should be in the range [1,1.2)... now if the closest suggestion was 1.0.1 and that suggestion is valid for the required range... Maven should use that suggestion, but otherwise it just picks from the range (in a way that may be documented but I cannot recall)

Keep in mind, that <optional> dependencies are allowed to contribute to range requirements... they just do not force their way onto the classpath.

By specifying [0] we force this version over anything else.

  • If somebody has specified a version in the normal way... requirements always beat suggestions and 0 wins.
  • If somebody has specified a version range... if there is no intersection their build will fail... if there is an intersection including 0 then that wins.

This comment has been minimized.

@jglick

jglick Oct 25, 2017

Member

OK…anyway, my comment stands: a comment in the source code explaining this would be appropriate.

This comment has been minimized.

@jtnord

jtnord Oct 25, 2017

Member

@jglick comment left

pom.xml Outdated
@@ -230,6 +230,19 @@ THE SOFTWARE.
<artifactId>access-modifier-annotation</artifactId>
<version>${access-modifier-annotation.version}</version>
</dependency>
<!-- make sure these plugins are never used by us or by any plugins which end up depening on this version -->

This comment has been minimized.

@jglick

jglick Sep 26, 2017

Member

Plugins?

Also, typo.

@@ -61,12 +61,8 @@ THE SOFTWARE.
jars that are not needed in war. most of the exclusions should happen in the core, to make IDEs happy, not here.
-->
<exclusion>
<groupId>javax.servlet</groupId>
<artifactId>servlet-api</artifactId>

This comment has been minimized.

@jglick

jglick Sep 26, 2017

Member

Do you not need to exclude javax.servlet:javax.servlet-api?

This comment has been minimized.

@stephenc

stephenc Sep 27, 2017

Member

Nope... <version>[0]</version> globally excludes it for us!

This comment has been minimized.

@jtnord

jtnord Oct 10, 2017

Member

well it doesn't exclude it - it just means we get an empty jar, but then its provided and optional so we end up not using it.

This comment has been minimized.

@jglick

jglick Oct 25, 2017

Member

Doing some nonobvious tricks here with dependencies. Have you tried building a core snapshot and then selecting that as a jenkins.version from some plugin’s POM to verify that you get the right mvn dependency:tree and that mvn hpi:run works as expected and so on?

This comment has been minimized.

@jtnord

jtnord Oct 25, 2017

Member

Have you tried building a core snapshot and then selecting that as a jenkins.version from some plugin’s POM to verify that you get the right mvn dependency:tree and that mvn hpi:run works as expected and so on?

No but I have been doing that trick on an internal plugin so that it gets the correct servlet API.

I can give it a go..

This comment has been minimized.

@jtnord

jtnord Oct 25, 2017

Member

in jenkinsci/workflow-step-api-plugin.git (sha 8856adca44c476938eb18d62d5a8a5986f60334c)

mvn -Denforcer.skip -Djenkins.version=2.80-20171025.175859-1 -Djenkins-war.version=2.80-20171025.180130-1 clean install
mvn -Denforcer.skip -Djenkins.version=2.80-20171025.175859-1 -Djenkins-war.version=2.80-20171025.180130-1 hpi:run

install passes all tests and builds ok
hpi:run Jenkins starts up plugin is installed, created a pipeline job with echo "hello" build it verified the console output of the run.

enforcer was skipped as project was configured for java7 and obviously has bytecode for java8.
using java.level=8 was not OK as there where syntax errors in some of the generic bounds matching that made it past the java7 compiler

@stephenc

Just a query on the way you have implemented depMgmt, if you have tested and you have confidence that it works, fine by me.

Otherwise 👍

pom.xml Outdated
<!-- the old artifactID for the servlet API -->
<artifactId>servlet-api</artifactId>
<version>[0]</version>
<scope>provided</scope>

This comment has been minimized.

@stephenc

stephenc Sep 27, 2017

Member

IIRC This does not do what you think it does when in dependency management... I recommend removing the <scope> and <optional> from depMgmt

This comment has been minimized.

@jtnord

jtnord Oct 10, 2017

Member

scope is[1] inherited when you declare the dependency (groupId:ArtifactId), so I am not sure what you are meaning here? (I have not checked this with transitive dependedcies but given we declare the dependency below it should be correct?)

[1] (Which is why you almost should never put a scope in depMgmt in a parent pom because you at some time someone will want that junit dependency not in test scope!).

<!-- make sure our dependency tree and all others are clean of the legacy servlet api. -->
<groupId>javax.servlet</groupId>
<!-- the old artifactID for the servlet API -->
<artifactId>servlet-api</artifactId>

This comment has been minimized.

@stephenc

stephenc Sep 27, 2017

Member

IIRC this will inherit the version as there is no <scope> element or <optional> element... but if there was a <scope> or a <optional> then this would not match your current depMgmt so the version would not be inferred... (though TBH this is an aspect of depMgmt that is poorly documented)

This comment has been minimized.

@jtnord

jtnord Oct 10, 2017

Member

in which case (if correct and scope/optionality was added) the version should be undefined and you should get a build error (as the dependency has no version).

@jtnord

oop forgot to submit my responses - sorry for the delay.

pom.xml Outdated
<!-- the old artifactID for the servlet API -->
<artifactId>servlet-api</artifactId>
<version>[0]</version>
<scope>provided</scope>

This comment has been minimized.

@jtnord

jtnord Oct 10, 2017

Member

scope is[1] inherited when you declare the dependency (groupId:ArtifactId), so I am not sure what you are meaning here? (I have not checked this with transitive dependedcies but given we declare the dependency below it should be correct?)

[1] (Which is why you almost should never put a scope in depMgmt in a parent pom because you at some time someone will want that junit dependency not in test scope!).

<!-- make sure our dependency tree and all others are clean of the legacy servlet api. -->
<groupId>javax.servlet</groupId>
<!-- the old artifactID for the servlet API -->
<artifactId>servlet-api</artifactId>

This comment has been minimized.

@jtnord

jtnord Oct 10, 2017

Member

in which case (if correct and scope/optionality was added) the version should be undefined and you should get a build error (as the dependency has no version).

@@ -61,12 +61,8 @@ THE SOFTWARE.
jars that are not needed in war. most of the exclusions should happen in the core, to make IDEs happy, not here.
-->
<exclusion>
<groupId>javax.servlet</groupId>
<artifactId>servlet-api</artifactId>

This comment has been minimized.

@jtnord

jtnord Oct 10, 2017

Member

well it doesn't exclude it - it just means we get an empty jar, but then its provided and optional so we end up not using it.

@jtnord jtnord referenced this pull request Oct 25, 2017

Closed

Stapler 1.253 #3103

@jglick

You will probably also need to port the commons-codec fix from #3103.

pom.xml Outdated
<groupId>javax.servlet</groupId>
<!-- the old artifactID for the servlet API -->
<artifactId>servlet-api</artifactId>
<version>[0]</version>

This comment has been minimized.

@jglick

jglick Oct 25, 2017

Member

OK…anyway, my comment stands: a comment in the source code explaining this would be appropriate.

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

This comment has been minimized.

@jglick

jglick Oct 25, 2017

Member

Now just use the release.

@jglick jglick self-requested a review Oct 25, 2017

@jglick jglick requested a review from oleg-nenashev Oct 25, 2017

@jtnord jtnord changed the title from [JENKINS-37062] incorporate changes from stapler servlet 3.1 to [JENKINS-37062] incorporate changes from stapler 1.253 (servlet 3.1) Oct 25, 2017

@jglick

jglick approved these changes Oct 25, 2017

A few questions but basically sounds fine.

RequestDispatcher view = req.getView(it, viewName);
if ("true".equals(req.getParameter("encrypt"))) {
final ByteArrayOutputStream baos = new ByteArrayOutputStream();
final CapturingServletOutputStream csos = new CapturingServletOutputStream();

This comment has been minimized.

@jglick

jglick Oct 25, 2017

Member

Or we could just remove this old hack and directly render the JNLP template into a memory buffer using raw Jelly calls or something, not using a mock response.

This comment has been minimized.

@jtnord

jtnord Oct 25, 2017

Member

I went for the conservative approach here as the code was not super easy to understand what it was doing to begin with, If someone wants to do a better thing then go for it!

public void setWriteListener(WriteListener writeListener) {
// we are always ready to write so we just call once to say we are ready.
try {
// should we do this on a separate thread to avoid deadlocaks?

This comment has been minimized.

@jglick

This comment has been minimized.

@jtnord

jtnord Oct 25, 2017

Member

fixed

pom.xml Outdated
@@ -230,6 +230,22 @@ THE SOFTWARE.
<artifactId>access-modifier-annotation</artifactId>
<version>${access-modifier-annotation.version}</version>
</dependency>
<dependency>
<!-- make sure these old servlet versions are never used by us or by any plugins which end up depening on this version -->

This comment has been minimized.

@jglick

This comment has been minimized.

@jtnord

jtnord Oct 25, 2017

Member

fixed

pom.xml Outdated
@@ -230,6 +230,22 @@ THE SOFTWARE.
<artifactId>access-modifier-annotation</artifactId>
<version>${access-modifier-annotation.version}</version>
</dependency>
<dependency>
<!-- make sure these old servlet versions are never used by us or by any plugins which end up depening on this version -->
<!-- Jenkins test Harness tries to fudge this, causes issues with some IDEs -->

This comment has been minimized.

@jglick

jglick Oct 25, 2017

Member

Is some matching PR to jenkins-test-harness needed here then?

This comment has been minimized.

@jtnord

jtnord Oct 25, 2017

Member

JTH already uses servlet 3.x so that should be OK.

The plugin POM should have the hack for servlet 2.x vs 3.x removed but that will mean it will no longer work correctly with old cores (ones where we originally added 3.1 support) as plugin authors may end up with servlet 3.1 on the classpath for compilation but end up using deploying to a jenkins that uses 2.4

I'm ok with creating a new plugin-pom and removing support for any jenkins in the pre 3.1 servlet era. (basically pre 2.0 (2016-04-20) ) but using <version>[0]</version> should workaround this.

So the comment is misleading its the plugin-pom....

@@ -61,12 +61,8 @@ THE SOFTWARE.
jars that are not needed in war. most of the exclusions should happen in the core, to make IDEs happy, not here.
-->
<exclusion>
<groupId>javax.servlet</groupId>
<artifactId>servlet-api</artifactId>

This comment has been minimized.

@jglick

jglick Oct 25, 2017

Member

Doing some nonobvious tricks here with dependencies. Have you tried building a core snapshot and then selecting that as a jenkins.version from some plugin’s POM to verify that you get the right mvn dependency:tree and that mvn hpi:run works as expected and so on?

@jglick jglick added the needs-review label Oct 25, 2017

jtnord added some commits Oct 25, 2017

[JENKINS-37062] add a comment for those not familiar with version ranges
Add a comment about what <version>[x]</version> means as not everyone is
familiar with its syntax.
[JENKINS-37062] fix some typos in comments and update others.
There was a few typos in comments that have been addressed.
Also update the comment about the servlet fudge in plugin-pom
@jglick

jglick approved these changes Oct 25, 2017

pom.xml Outdated
<!-- make sure these old servlet versions are never used by us or by any plugins which end up depening on this version -->
<!-- Jenkins test Harness tries to fudge this, causes issues with some IDEs -->
<!-- make sure these old servlet versions are never used by us or by any plugins which end up depending on this version -->
<!-- plugin-pom tries to fudge servlet support to be compatible with cores < 2.0 and JTH which needs 3.x for jetty,

This comment has been minimized.

@jglick

jglick Oct 25, 2017

Member

IIUC this comment will become misleading if & when jenkinsci/plugin-pom#83 is merged and becomes established.

pom.xml Outdated
<groupId>javax.servlet</groupId>
<!-- the old artifactID for the servlet API -->
<artifactId>servlet-api</artifactId>
<version>[0]</version>
<!--
"[0]" is a range that must be exaclty 0

This comment has been minimized.

@jglick
this is different to "0" which is hint to use version 0.
therefore unless anyone else uses ranges (they should not) this version will always win
We have deployed a version 0 to jenkins repo which has an empty jar
This prevents conflicts between the old Servet API and the new Servlet API as the groupIDs have changed

This comment has been minimized.

@jglick
@jtnord

This comment has been minimized.

Member

jtnord commented Oct 26, 2017

@oleg-nenashev

🐝 let's finally do something about that. Seems to be a bit YOLO, but it the worst case we can just pick previous weekly as LTS

@jglick jglick added ready-for-merge and removed needs-review labels Oct 26, 2017

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Oct 26, 2017

@jglick recommends doing some manual testing and ATH before merging

@jtnord

This comment has been minimized.

Member

jtnord commented Oct 27, 2017

@oleg-nenashev see inline for the manual testing I have already performed.

checked the war does not include the servlet-api.jar (or javax.servlet-api.jar)
started Jenkins with java -jar jenkins.war and ran the install wizard

hpi:run Jenkins starts up plugin is installed, created a pipeline job with echo "hello" build it verified the console output of the run.
enforcer was skipped as project was configured for java7 and obviously has bytecode for java8.
using java.level=8 was not OK as there where syntax errors in some of the generic bounds matching that made it past the java7 compiler

If someone wants to kick the ATH with this, that's fine but from my point of view this is ready to go.
alternatively merge it and if there are ATH failures ping me (but I want to get this into the weekly ASAP)

@jglick

This comment has been minimized.

Member

jglick commented Oct 27, 2017

The only other things I could think of that use unusual HTTP features and so are worth special manual testing (neither of which is covered by the ATH anyway AFAIK):

  • HTTP-based CLI
  • something using ProgressiveRendering, like /asynchPeople
@jtnord

This comment has been minimized.

Member

jtnord commented Oct 27, 2017

@jglick verified both worked.

for the CLI command I used java -jar "C:\Users\jnord\Downloads\jenkins-cli (1).jar" -s http://localhost:8080/ -http -noKeyAuth -auth admin:admin build myjob

@jtnord jtnord merged commit 96b0169 into jenkinsci:master Oct 27, 2017

1 check passed

continuous-integration/jenkins/pr-head This commit looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment