-
Notifications
You must be signed in to change notification settings - Fork 27
Allow patterns to match classes under META-INF #34
Conversation
Is this speculative, or do you guys actually have a usecase for this? I wonder if it could have unintended consequences... |
Now that I've tracked down the real issue it was a workaround for pantsbuild/pants#5671. So I could take it or leave it? |
4c6dcd7
to
3bf6406
Compare
Ok, I updated this with a better test case. I don't think you would want to map classes under META-INF but you might want to remove them from the shaded 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.
One minor concern, but I might not be understanding the usecase.
@@ -122,6 +122,10 @@ private static boolean checkIdentifierChars(String expr, String extra) { | |||
if (expr.endsWith("package-info")) { | |||
expr = expr.substring(0, expr.length() - "package-info".length()); | |||
} | |||
// Allow patterns under META-INF if you want to match multi-release class files. |
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 this strips the prefix off, is the result that it will match classes with that name outside META-INF
? Would a test that confirms that a META-INF/
pattern doesn't accidentally match something outside META-INF
be good?
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.
Since this method is only returning true or false if the identifier is valid it shouldn't affect what pattern is matched but I'll add a test anyway.
3bf6406
to
2163608
Compare
With Java9 you can have module classes under the META-INF directory of a jar. It will probably have unintended consequences to shade those classes in a jar but this change allows you to shade those classes.