-
Notifications
You must be signed in to change notification settings - Fork 78
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-40693, JENKINS-43713] upgrade to jetty 9.4.x #32
[JENKINS-40693, JENKINS-43713] upgrade to jetty 9.4.x #32
Conversation
+1 |
Is there a complete changelog for these versions in Jetty? |
@daniel-beck @aheritier @olamy jetty/jetty.project@jetty-9.2.15.v20160210...jetty-9.2.21.v20170120 There are many changes I'd say. But these changes include |
Summon @jenkinsci/code-reviewers since we need some extra eyes on it |
Why not go with 9.3.17.v20170317 since jenkins uses java 8 now. @daniel-beck the change log is here for 9.2 https://github.com/eclipse/jetty.project/blob/jetty-9.2.x/VERSION.txt and for 9.3 https://github.com/eclipse/jetty.project/blob/jetty-9.3.x/VERSION.txt |
@paladox Yes, I was going to create a PR for Jetty 9.3 on the next week. But this change may be still useful before we evaluate Jetty 9.3 |
Ok. Thanks. |
Created https://issues.jenkins-ci.org/browse/JENKINS-43501 for the upgrade to 9.3 |
@olamy Do you have any insights about possible compat issues between 9.2 and 9.3? |
@oleg-nenashev I don't think so (but as you know we never know :-) ) |
@olamy Please do. The main effort is to test the things (Jenkins core manual tests + ATH), I commit to do it within 1 week |
well 9.4.x will be a better option! I will test that. |
made some progress. (weird unit test fail on my osx but not on a linux box...). |
for the record, to fix unit test: |
so ATM this depends on jetty 9.4.5-SNAPSHOT version (but at least can be used for testing purpose) |
src/java/winstone/Launcher.java
Outdated
@@ -352,6 +352,7 @@ public static void main(String argv[]) throws IOException { | |||
new Launcher(args); | |||
} catch (Throwable err) { | |||
Logger.log(Logger.ERROR, RESOURCES, "Launcher.ContainerStartupError", err); | |||
System.exit(1); |
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.
Unrelated, but OK. Maybe deserves a separate changelog entry if there is a real issue with it
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.
Local tests consistently fail for me,
[ERROR] Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 76.273 s <<< FAILURE! - in winstone.LauncherTest
[ERROR] mimeType(winstone.LauncherTest) Time elapsed: 76.272 s <<< ERROR!
java.net.ConnectException: Operation timed out (Connection timed out)
at winstone.LauncherTest.mimeType(LauncherTest.java:22)
Caused by: java.net.ConnectException: Operation timed out (Connection timed out)
at winstone.LauncherTest.mimeType(LauncherTest.java:22)
INFO: Winstone shutdown successfully
[ERROR] Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 0.508 s <<< FAILURE! - in winstone.HttpConnectorFactoryTest
[ERROR] testListenAddress(winstone.HttpConnectorFactoryTest) Time elapsed: 0.508 s <<< ERROR!
java.io.IOException: Failed to start Jetty
at winstone.HttpConnectorFactoryTest.testListenAddress(HttpConnectorFactoryTest.java:20)
Caused by: java.net.BindException: Can't assign requested address
at winstone.HttpConnectorFactoryTest.testListenAddress(HttpConnectorFactoryTest.java:20)
OTOH manual testing of UI didn't discovered any issues. We will also need to update https://github.com/jenkinsci/maven-hudson-dev-plugin in order to merge it into Jenkins.
@olamy Any ETA regarding the 9.4.5 release?
pom.xml
Outdated
@@ -2,7 +2,7 @@ | |||
<modelVersion>4.0.0</modelVersion> | |||
<groupId>org.jenkins-ci</groupId> | |||
<artifactId>winstone</artifactId> | |||
<version>3.4-SNAPSHOT</version> | |||
<version>3.5-SNAPSHOT</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.
Better to use 4.0 IMHO (Java bump + major Jetty bump)
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.
LGTM I will do
@oleg-nenashev have setup |
Signed-off-by: olivier lamy <olamy@apache.org>
Signed-off-by: olivier lamy <olamy@apache.org>
Signed-off-by: olivier lamy <olamy@apache.org>
…that Signed-off-by: olivier lamy <olamy@apache.org>
Signed-off-by: olivier lamy <olamy@apache.org>
Signed-off-by: olivier lamy <olamy@apache.org>
Signed-off-by: olivier lamy <olamy@apache.org>
@oleg-nenashev I hope new Jetty release around end of next week. |
## Development | ||
If you have some unit test failures you may add an interface/ip alias such | ||
|
||
``` sudo ifconfig lo0 alias 127.0.0.2 ``` |
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.
OK, fine for me
Signed-off-by: olivier lamy <olamy@apache.org>
…ema :-) Signed-off-by: olivier lamy <olamy@apache.org>
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, releasing as 4.0 seems to be a right choice. This update seems to cause some binary incompatibilities, but we have to move forward at some point
pom.xml
Outdated
@@ -301,17 +308,19 @@ | |||
<version>${jetty.version}</version> | |||
</dependency> | |||
--> | |||
|
|||
<!-- | |||
spdy abandonned |
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.
Any potential impact on transient deps?
pom.xml
Outdated
@@ -228,6 +224,17 @@ | |||
<id>repo.jenkins-ci.org</id> | |||
<url>http://repo.jenkins-ci.org/public/</url> | |||
</repository> | |||
<!-- can be removed when jetty 9.4.5 released --> |
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.
Should be removed now I suppose
<web-app | ||
xmlns="http://java.sun.com/xml/ns/javaee" | ||
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" | ||
xsi:schemaLocation="http://java.sun.com/xml/ns/javaee http://java.sun.com/xml/ns/javaee/web-app_2_5.xsd" |
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.
Would be nice to have schema validation in the PR builder. Oh wait, PR builder first...
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 don't get it? well it's a test resource
pom.xml
Outdated
@@ -228,6 +224,17 @@ | |||
<id>repo.jenkins-ci.org</id> | |||
<url>http://repo.jenkins-ci.org/public/</url> | |||
</repository> | |||
<!-- can be removed when jetty 9.4.5 released --> |
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.
remove?
pom.xml
Outdated
@@ -301,17 +308,19 @@ | |||
<version>${jetty.version}</version> | |||
</dependency> | |||
--> | |||
|
|||
<!-- | |||
spdy abandonned |
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.
is there an impact on transient deps?
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.
Nope I don't think so.
FYI I have some test locally to enable http2 as well. But not fully test so will do an other pr later.
Signed-off-by: olivier lamy <olamy@apache.org>
ready to merge I guess? |
Could we perhaps use this opportunity to update https://github.com/jenkinsci/winstone/blob/master/src/java/winstone/LocalStrings.properties#L1 ? It should have been a filtered resource long ago… |
Signed-off-by: olivier lamy <olamy@apache.org>
Signed-off-by: olivier lamy <olamy@apache.org>
@jglick Could you please cut the release or approve https://github.com/jenkins-infra/repository-permissions-updater/pull/293/files ? |
Signed-off-by: olivier lamy olamy@apache.org
see https://issues.jenkins-ci.org/browse/JENKINS-40693