-
Notifications
You must be signed in to change notification settings - Fork 1.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
Issue #11092 - Allow MetaInfConfiguration parsing of java.class.path
to support globs
#12287
Issue #11092 - Allow MetaInfConfiguration parsing of java.class.path
to support globs
#12287
Conversation
…to support globs
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.
If you could add the extra term to the glob string as per my comment, then I think we're ok.
@Test | ||
public void testSplitOnPathSeparatorWithGlob() throws IOException | ||
{ | ||
try (ResourceFactory.Closeable resourceFactory = ResourceFactory.closeable()) |
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.
Could you add another term to the glob string that is just a single jar file, to mimic a classpath that has both glob and individual jar files on 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.
This test kind has that already, 2 non globs, and 1 glob.
Also the existing test testSplitOnPipeWithGlob()
has the same.
Do you want the non-glob to be specifically a jar?
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.
Followup, do you want the extra non-globs to be before or after the glob?
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 really mind the position of the jar, just so long as the test is a combination of dirs, a dir with glob and at least one explicit jar.
Stream.of(classPath.split(File.pathSeparator)) | ||
.map(resourceFactory::newResource) | ||
.filter(r -> uriPatternPredicate.test(r.getURI())) | ||
resourceFactory.split(classPath, File.pathSeparator) |
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.
Heh, technically we shouldn't fix ee8 and ee9 because they should be bug-for-bug compatible with jetty 10/11. But ok.
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.
In Jetty 10/11 we ignore globs here.
The raw string is created a File
and then if it doesn't exist, it's ignored.
The thing is, a File
with a *
glob always produces a File
instance that doesn't exist (or can be opened).
But a Path cannot even be created with a *
glob, it throws an Exception.
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.
understood.
@joakime some test failures to address here as well. |
@joakime is this one ready for re-review? |
Yes. Review away. |
@janbartel bump |
Use ResourceFactory.split(String) to properly parse the
java.class.path
so that globs can be supported in MetaInfConfiguration as well.Fixes #11092
Fixes #12283