Skip to content
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

Addendum to #11566. #11722

Merged

Conversation

sbordet
Copy link
Contributor

@sbordet sbordet commented Apr 30, 2024

#11566
Restored methods that were removed in WebAppClassLoader.Context.

Fixed method signatures for deprecated method -- they must take the deprecated ClassMatcher, not the newly introduced one.

Restored methods that were removed in WebAppClassLoader.Context.

Fixed method signatures for deprecated method -- they must take the deprecated ClassMatcher, not the newly introduced one.

Signed-off-by: Simone Bordet <[email protected]>
@joakime
Copy link
Contributor

joakime commented Apr 30, 2024

Test failures, all in ee8 layer

Signed-off-by: Simone Bordet <[email protected]>
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the work in core/EE10, but I think we should leave EE8/EE9 as much as possible with the old naming in the APIs and just use the new names in the implementation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intention was to not do the public API renaming in EE8/EE9 APIs. i.e. leave as system/server rather than change to protected/hidden. The new vocab will appear to some extent in ClassMatchers returned from the APIs, but there is no need to add it to the EE8/EE9 APIs if they were not already there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.
I have un-deprecated ee9's ClassMatcher because it is used in every non-deprecated APIs (since ee9 does not have the new APIs, the old APIs cannot be deprecated, and so ee9's ClassMatcher should also not deprecated).

@sbordet sbordet requested a review from gregw May 1, 2024 18:22
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
Re-running CI to see if the failures are flakes.

@sbordet sbordet merged commit 42ba415 into jetty-12.0.x May 2, 2024
3 of 8 checks passed
@sbordet sbordet deleted the fix/jetty-12/11566/fix-classmatcher-signatures branch May 2, 2024 15:40
@joakime
Copy link
Contributor

joakime commented May 2, 2024

This is still broken.

2024-05-02 11:43:37.351:WARN :oejx.XmlConfiguration:main: Config error java.lang.IllegalStateException: No Method: <Call name="addServerClassMatcher"><Arg>|      <New class="org.eclipse.jetty.util.ClassMatcher"><Arg>|         <Array type="java.lang.String"><Item>-org.eclipse.jetty.util.Decorator</Item><Item>-org.eclipse.jetty.util.DecoratedObjectFactory</Item><Item>-org.eclipse.jetty.server.handler.ContextHandler.</Item><Item>-org.eclipse.jetty.server.handler.ContextHandler</Item><Item>-org.eclipse.jetty.ee10.servlet.ServletContextHandler</Item></Array>|        </Arg></New>|    </Arg></Call> on class org.eclipse.jetty.ee10.maven.plugin.MavenWebAppContext at <Call name="addServerClassMatcher"><Arg>|      <New class="org.eclipse.jetty.util.ClassMatcher"><Arg>|         <Array type="java.lang.String"><Item>-org.eclipse.jetty.util.Decorator</Item><Item>-org.eclipse.jetty.util.DecoratedObjectFactory</Item><Item>-org.eclipse.jetty.server.handler.ContextHandler.</Item><Item>-org.eclipse.jetty.server.handler.ContextHandler</Item><Item>-org.eclipse.jetty.ee10.servlet.ServletContextHandler</Item></Array>|        </Arg></New>|    </Arg></Call> in file:///home/joakim/code/jetty/12.0.x/jetty-ee10/jetty-ee10-maven-plugin/target/it/jetty-cdi-start-forked/src/main/jetty/jetty-context.xml
Exception in thread "main" java.lang.IllegalStateException: No Method: <Call name="addServerClassMatcher"><Arg>
      <New class="org.eclipse.jetty.util.ClassMatcher"><Arg>
         <Array type="java.lang.String"><Item>-org.eclipse.jetty.util.Decorator</Item><Item>-org.eclipse.jetty.util.DecoratedObjectFactory</Item><Item>-org.eclipse.jetty.server.handler.ContextHandler.</Item><Item>-org.eclipse.jetty.server.handler.ContextHandler</Item><Item>-org.eclipse.jetty.ee10.servlet.ServletContextHandler</Item></Array>
        </Arg></New>
    </Arg></Call> on class org.eclipse.jetty.ee10.maven.plugin.MavenWebAppContext
	at org.eclipse.jetty.xml.XmlConfiguration$JettyXmlConfiguration.call(XmlConfiguration.java:979)
	at org.eclipse.jetty.xml.XmlConfiguration$JettyXmlConfiguration.configure(XmlConfiguration.java:529)
	at org.eclipse.jetty.xml.XmlConfiguration$JettyXmlConfiguration.configure(XmlConfiguration.java:445)
	at org.eclipse.jetty.xml.XmlConfiguration.configure(XmlConfiguration.java:368)
	at org.eclipse.jetty.ee10.maven.plugin.WebAppPropertyConverter.fromProperties(WebAppPropertyConverter.java:301)
	at org.eclipse.jetty.ee10.maven.plugin.JettyEmbedder.applyWebAppProperties(JettyEmbedder.java:101)
	at org.eclipse.jetty.ee10.maven.plugin.JettyEmbedder.configureWebApp(JettyEmbedder.java:83)
	at org.eclipse.jetty.maven.AbstractJettyEmbedder.configure(AbstractJettyEmbedder.java:231)
	at org.eclipse.jetty.maven.AbstractJettyEmbedder.doStart(AbstractJettyEmbedder.java:204)
	at org.eclipse.jetty.util.component.AbstractLifeCycle.start(AbstractLifeCycle.java:93)
	at org.eclipse.jetty.maven.AbstractForkedChild.doStart(AbstractForkedChild.java:193)
	at org.eclipse.jetty.util.component.AbstractLifeCycle.start(AbstractLifeCycle.java:93)
	at org.eclipse.jetty.ee10.maven.plugin.JettyForkedChild.main(JettyForkedChild.java:69)
Caused by: java.lang.NoSuchMethodException: addServerClassMatcher
	at org.eclipse.jetty.xml.XmlConfiguration$JettyXmlConfiguration.call(XmlConfiguration.java:1014)
	at org.eclipse.jetty.xml.XmlConfiguration$JettyXmlConfiguration.call(XmlConfiguration.java:971)
	... 12 more

@sbordet
Copy link
Contributor Author

sbordet commented May 2, 2024

Thanks, on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

3 participants