-
-
Notifications
You must be signed in to change notification settings - Fork 9k
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
Web fragments (attempt #2) #10227
Web fragments (attempt #2) #10227
Conversation
…` under core (jenkinsci#10185)" (jenkinsci#10225) This reverts commit 6109053.
* Remove core classes from extra classpath, as when running `mvn -pl war jetty:run` it automatically gets added.
<!-- Allows resources to be reloaded, and enable nicer console logging. --> | ||
<extraClasspath>${project.basedir}/../core/src/main/resources,${project.basedir}/../core/target/classes,${project.build.directory}/support-log-formatter.jar</extraClasspath> | ||
<!-- Enable nicer console logging. --> | ||
<extraClasspath>${project.build.directory}/support-log-formatter.jar</extraClasspath> |
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.
While investigating some warnings during the startup, I realised that the jetty-maven-plugin already include core classes and resources from the module itself and not from the local maven repository (as long as mvn -pl war jetty:run
is used).
Which means these extra classpath entries shouldn't be necessary to get live updates through hot reload.
Unfortunately, hot reload currently fails with
java.nio.file.ClosedFileSystemException
at jdk.nio.zipfs.ZipFileSystem.ensureOpen (ZipFileSystem.java:1635)
at jdk.nio.zipfs.ZipFileSystem.exists (ZipFileSystem.java:657)
at jdk.nio.zipfs.ZipPath.exists (ZipPath.java:905)
at jdk.nio.zipfs.ZipFileSystemProvider.exists (ZipFileSystemProvider.java:197)
at java.nio.file.Files.exists (Files.java:2515)
at org.eclipse.jetty.util.resource.PathResource.exists (PathResource.java:187)
at org.eclipse.jetty.ee9.webapp.WebAppClassLoader.addClassPath (WebAppClassLoader.java:225)
at org.eclipse.jetty.ee9.webapp.WebAppClassLoader.<init> (WebAppClassLoader.java:192)
at org.eclipse.jetty.ee9.webapp.WebAppContext.configureClassLoader (WebAppContext.java:504)
at org.eclipse.jetty.ee9.webapp.WebAppContext.preConfigure (WebAppContext.java:477)
at org.eclipse.jetty.ee9.webapp.WebAppContext.doStart (WebAppContext.java:527)
at org.eclipse.jetty.ee9.maven.plugin.MavenWebAppContext.doStart (MavenWebAppContext.java:313)
at org.eclipse.jetty.util.component.AbstractLifeCycle.start (AbstractLifeCycle.java:93)
at org.eclipse.jetty.ee9.maven.plugin.JettyEmbedder.redeployWebApp (JettyEmbedder.java:64)
at org.eclipse.jetty.ee9.maven.plugin.JettyRunMojo.restartWebApp (JettyRunMojo.java:362)
at org.eclipse.jetty.ee9.maven.plugin.JettyRunMojo$2.consoleEvent (JettyRunMojo.java:198)
at org.eclipse.jetty.maven.ConsoleReader.signalEvent (ConsoleReader.java:64)
at org.eclipse.jetty.maven.ConsoleReader.run (ConsoleReader.java:57)
at java.lang.Thread.run (Thread.java:1583)
that seems to be caused by a JDK bug? (see jetty/jetty.project#11548)
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 have had problems with the JDK ZIP file code for most of my adult life.
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.
This broke live updates of resources like Jelly files. What motivated this change?
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.
Ah, indeed I shouldn't have removed ${project.basedir}/../core/src/main/resources
, only ${project.basedir}/../core/target/classes
. Sorry for that.
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.
Thanks, I tested jetty:run
locally
<extension>xml</extension> | ||
<mime-type>application/xml</mime-type> | ||
</mime-mapping> | ||
<!--mime-mapping> commenting out until this works out of the box with JOnAS. See http://www.nabble.com/Error-with-mime-type%2D-%27application-xslt%2Bxml%27-when-deploying-hudson-1.316-in-jonas-td24740489.html |
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.
You have to wonder how much of this stuff actually still does anything useful…
/label ready-for-merge This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback. Thanks! |
Caused JENKINS-75280. |
Fix a regression introduced in jenkinsci#10227 This removed both classes and resources from the extra classpath, whereas only the classes should have been removed.
Fix a regression introduced in jenkinsci#10227 This removed both classes and resources from the extra classpath, whereas only the classes should have been removed.
web.xml
toweb-fragment.xml
under core ([JENKINS-75174] Move existingweb.xml
toweb-fragment.xml
under core #10185)" (Revert "[JENKINS-75174] Move existingweb.xml
toweb-fragment.xml
under core (#10185)" #10225)For running plugins with
mvn hpi:run
, It is necessary to update parent pom to 5.7 or later (requires hpi plugin 3.61 or later)See JENKINS-XXXXX.
Testing done
JenkinsRule
RealJenkinsRule
mvn -pl war jetty:run
mvn hpi:run
Proposed changelog entries
Proposed upgrade guidelines
N/A
Submitter checklist
Desired reviewers
@mention
Before the changes are marked as
ready-for-merge
:Maintainer checklist