Upgrade to Jetty 12 ee8 #341

olamy · 2023-08-16T09:08:01Z

Signed-off-by: Olivier Lamy olamy@apache.org

basil

Please take a moment and address the merge conflicts of your pull request. Thanks!

Signed-off-by: Olivier Lamy <olamy@apache.org>

olamy · 2024-05-07T01:10:08Z

fixed.

Jenkinsfile

pom.xml

src/main/java/winstone/HostConfiguration.java

basil · 2024-05-07T21:14:56Z

src/main/java/winstone/accesslog/SimpleAccessLogger.java


-        // mimic https://github.com/eclipse/jetty.project/blob/jetty-9.4.0.v20161208/jetty-server/src/main/java/org/eclipse/jetty/server/AbstractNCSARequestLog.java#L130


This old comment was valuable. Why delete it, rather than updating it? I think we should update it to:

// mimic https://github.com/jetty/jetty.project/blob/c5b2533fdecce21b54c6fbaf36f79bc3ba909775/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/CustomRequestLog.java#L1093-L1099

done with df6cfee

olamy · 2024-05-07T23:14:05Z

what about setup spotless for winstone?
Discussing and fixing extra spaces is an absolute waste of time.

olamy · 2024-05-07T23:16:25Z

spotless #380

basil · 2024-05-08T02:18:43Z

I have merged master into this branch and resolved the five issues related to unnecessary stylistic changes. The other feedback remains.

olamy · 2024-05-08T10:47:21Z

I have merged master into this branch and resolved the five issues related to unnecessary stylistic changes. The other feedback remains.

thanks for that!

a bit busy. So I will try to look at other comments, but probably not before the end of the week.

basil · 2024-05-08T18:03:06Z

Thanks @olamy! In the absence of an update next week, I will assume you are no longer interested in contributing to this project, either through code or through feedback.

Co-authored-by: Basil Crow <me@basilcrow.com>

Signed-off-by: Olivier Lamy <olamy@apache.org>

pom.xml

src/main/java/winstone/accesslog/SimpleAccessLogger.java

basil · 2024-05-09T17:43:36Z

src/main/java/winstone/accesslog/SimpleAccessLogger.java

-            remoteUser = null;
-        }
-
+        // https://github.com/jetty/jetty.project/blob/3632a57f2796a2ab4fcdbfe62837d494bcd4e94a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/CustomRequestLog.java#L307


Given the content the comment is referring to, the comment is placed above the wrong block of code. It should be placed above Request.AuthenticationState authenticationState.

the link is going to the NCSA format used by the Jetty.

In the old (currently shipping) code, there was a single comment above the copy-pasted code to set the remote user, showing where it was originally from. The new version still has copy-pasted code to set the remote user, but has lost any reference to the original code, which is technically a regression (in documentation). To fix this, the original link should be changed from a reference to the copy-pasted Jetty 10 code to the copy-pasted Jetty 12 code (https://github.com/jetty/jetty.project/blob/c5b2533fdecce21b54c6fbaf36f79bc3ba909775/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/CustomRequestLog.java#L1093-L1099).

Adding a link to the NCSA format is a valuable change, but it isn't strictly related to this PR (moving from Jetty 10 to Jetty 12), so I think if that change is desired then a separate PR should be opened to introduce it. That PR could be merged immediately and would reduce the size of this diff.

src/main/java/winstone/HostConfiguration.java

Signed-off-by: Olivier Lamy <olamy@apache.org>

Co-authored-by: Basil Crow <me@basilcrow.com>

olamy · 2024-05-09T20:58:45Z

my bad merge should be fixed. sorry for that.

basil

This is looking really close to finished! Very nice!

basil · 2024-05-15T18:01:01Z

pom.xml

    <changelist>-SNAPSHOT</changelist>
-    <jetty.version>10.0.20</jetty.version>
+    <jetty.version>12.0.9</jetty.version>


README.md should also be updated to replace 10.0.x with 12.0.x.

basil · 2024-05-15T18:04:46Z

pom.xml

    <slf4j.version>2.0.13</slf4j.version>
    <gitHubRepo>jenkinsci/winstone</gitHubRepo>
+    <maven.compiler.release>17</maven.compiler.release>


This is working around the fact that the parent POM doesn't (yet!) require Java 17 JENKINS-73121. Since we might forget to remove this workaround once JENKINS-73121 is resolved, let's leave a TODO comment here referencing JENKINS-73121.

Suggested change

<maven.compiler.release>17</maven.compiler.release>



<maven.compiler.release>17</maven.compiler.release>

basil · 2024-05-15T18:07:48Z

src/main/java/winstone/HostConfiguration.java

                }
            }
        }
-
+        server.setRequestLog(configureAccessLog("webapp"));


configureAccessLog can return null, so I think we should wrap this in a null check:

Suggested change

server.setRequestLog(configureAccessLog("webapp"));

RequestLog requestLog = configureAccessLog("webapp");

if (requestLog != null) {

server.setRequestLog(requestLog);

}

basil · 2024-05-15T18:11:04Z

src/main/java/winstone/HostConfiguration.java

    }

-    private WebAppContext create(File app, String prefix) {
+    private WebAppContext create(File app, String prefix, Server server) {


There is already a field named Server server. Shadowing this field in an argument variable is a confusing practice and Checkstyle has a HiddenField check that complains about this. I recommend simply reverting this hunk so that the field is used rather than the parameter:

Suggested change

private WebAppContext create(File app, String prefix, Server server) {

private WebAppContext create(File app, String prefix) {

basil · 2024-05-15T18:12:34Z

src/main/java/winstone/HostConfiguration.java

+                this.server
+                        .getMimeTypes()


A few lines above on line 96, you don't qualify the access to the server field (server.getMimeTypes().addMimeMapping(extension.toLowerCase(), mimeType);), so for consistency I don't think we should qualify the access here either:

Suggested change

this.server

.getMimeTypes()

server.getMimeTypes()

basil · 2024-05-15T18:12:54Z

src/main/java/winstone/accesslog/SimpleAccessLogger.java

@@ -56,7 +57,6 @@ public class SimpleAccessLogger extends AbstractLifeCycle implements RequestLog
            value = "PATH_TRAVERSAL_IN",
            justification = "false positive, webAppName come from command line")
    public SimpleAccessLogger(String webAppName, Map<String, String> startupArgs) throws IOException {
-


Unnecessary change?

basil · 2024-05-15T18:16:31Z

src/main/java/winstone/accesslog/SimpleAccessLogger.java

-            remoteUser = null;
-        }
-
+        // https://github.com/jetty/jetty.project/blob/3632a57f2796a2ab4fcdbfe62837d494bcd4e94a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/CustomRequestLog.java#L307


In the old (currently shipping) code, there was a single comment above the copy-pasted code to set the remote user, showing where it was originally from. The new version still has copy-pasted code to set the remote user, but has lost any reference to the original code, which is technically a regression (in documentation). To fix this, the original link should be changed from a reference to the copy-pasted Jetty 10 code to the copy-pasted Jetty 12 code (https://github.com/jetty/jetty.project/blob/c5b2533fdecce21b54c6fbaf36f79bc3ba909775/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/CustomRequestLog.java#L1093-L1099).

Adding a link to the NCSA format is a valuable change, but it isn't strictly related to this PR (moving from Jetty 10 to Jetty 12), so I think if that change is desired then a separate PR should be opened to introduce it. That PR could be merged immediately and would reduce the size of this diff.

olamy added work-in-progress enhancement labels Aug 16, 2023

olamy mentioned this pull request Aug 31, 2023

Jakarta EE 9 transition #180

Closed

olamy force-pushed the jetty-12 branch from 2183da8 to aa55edc Compare September 12, 2023 02:23

oleg-nenashev mentioned this pull request Nov 12, 2023

[Discussion] Update to Jetty version 11 jenkinsci/jenkinsfile-runner#699

Open

basil requested changes May 6, 2024

View reviewed changes

basil added the unresolved-merge-conflict label May 6, 2024

olamy added 3 commits May 7, 2024 10:28

Upgrade to Jetty 12 ee8

eac813c

Signed-off-by: Olivier Lamy <olamy@apache.org>

minimum is now Java 17 with Jetty 12

452d7af

Signed-off-by: Olivier Lamy <olamy@apache.org>

bump version to 7.x as it is a big change

bd7c683

Signed-off-by: Olivier Lamy <olamy@apache.org>

olamy force-pushed the jetty-12 branch from aa55edc to bd7c683 Compare May 7, 2024 00:29

basil reviewed May 7, 2024

View reviewed changes

basil added 2 commits May 7, 2024 19:10

Merge remote-tracking branch 'origin/master' into jetty-12

8761ffb

fix merge

f249adc

basil removed the unresolved-merge-conflict label May 8, 2024

olamy and others added 3 commits May 9, 2024 07:38

Update Jenkinsfile

9dd6e1a

Co-authored-by: Basil Crow <me@basilcrow.com>

after review changes

df6cfee

Signed-off-by: Olivier Lamy <olamy@apache.org>

Merge branch 'master' into jetty-12

56288e4

basil requested changes May 9, 2024

View reviewed changes

olamy and others added 3 commits May 10, 2024 06:55

fix my bad merge

cf2a6ad

Signed-off-by: Olivier Lamy <olamy@apache.org>

very important fix

4128588

Signed-off-by: Olivier Lamy <olamy@apache.org>

Update pom.xml

f5bc197

Co-authored-by: Basil Crow <me@basilcrow.com>

basil reviewed May 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to Jetty 12 ee8 #341

Upgrade to Jetty 12 ee8 #341

olamy commented Aug 16, 2023

basil left a comment

olamy commented May 7, 2024

basil May 7, 2024

olamy May 9, 2024

olamy commented May 7, 2024

olamy commented May 7, 2024 •

edited

basil commented May 8, 2024

olamy commented May 8, 2024

basil commented May 8, 2024

basil May 9, 2024

olamy May 9, 2024

basil May 15, 2024

olamy commented May 9, 2024

basil left a comment

basil May 15, 2024

basil May 15, 2024

basil May 15, 2024

basil May 15, 2024

basil May 15, 2024

basil May 15, 2024

basil May 15, 2024


		// mimic https://github.com/eclipse/jetty.project/blob/jetty-9.4.0.v20161208/jetty-server/src/main/java/org/eclipse/jetty/server/AbstractNCSARequestLog.java#L130

	<maven.compiler.release>17</maven.compiler.release>
	<!-- TODO JENKINS-73121 until in parent POM -->
	<maven.compiler.release>17</maven.compiler.release>

-        server.setRequestLog(configureAccessLog("webapp"));
+        RequestLog requestLog = configureAccessLog("webapp");
+        if (requestLog != null) {
+            server.setRequestLog(requestLog);
+        }

	private WebAppContext create(File app, String prefix, Server server) {
	private WebAppContext create(File app, String prefix) {

Upgrade to Jetty 12 ee8 #341

Are you sure you want to change the base?

Upgrade to Jetty 12 ee8 #341

Conversation

olamy commented Aug 16, 2023

basil left a comment

Choose a reason for hiding this comment

olamy commented May 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olamy commented May 7, 2024

olamy commented May 7, 2024 • edited

basil commented May 8, 2024

olamy commented May 8, 2024

basil commented May 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olamy commented May 9, 2024

basil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olamy commented May 7, 2024 •

edited