-
Notifications
You must be signed in to change notification settings - Fork 24.7k
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
Packaging: Remove classpath ordering hack #23596
Conversation
After the removal of the joda time hack we used to have, we can cleanup the codebase handling in security, jarhell and plugins to be more picky about uniqueness. This was originally in elastic#18959 which was never merged. closes elastic#18959
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.
LGTM. I left two minor comments, treat them as you wish.
@@ -114,7 +112,7 @@ public void testDirAndJar() throws Exception { | |||
public void testWithinSingleJar() throws Exception { | |||
// the java api for zip file does not allow creating duplicate entries (good!) so | |||
// this bogus jar had to be constructed with ant |
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 comment is out of date; we produce these with https://github.com/jasontedor/duplicate-classes/ now (the previous mechanism for building the bogus jars produced invalid jars which blew up animal-sniffer).
@@ -198,7 +203,7 @@ public static void checkJarHell(URL urls[]) throws URISyntaxException, IOExcepti | |||
public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IOException { | |||
String entry = root.relativize(file).toString(); | |||
if (entry.endsWith(".class")) { | |||
// normalize with the os separator | |||
// normalize with the os separator, remove '.class' | |||
entry = entry.replace(sep, ".").substring(0, entry.length() - 6); |
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 wonder if replacing entry.length() - 6
with entry.length() - ".class".length()
would be clearer?
@jasontedor I addressed your comments before merging. Thanks for the review. |
* master: [API] change wait_for_completion defaults according to docs (elastic#23672) Share XContent rendering code in terms aggs (elastic#23680) Update ingest-node.asciidoc [DOCS] Update the docs about the fact that global ordinals for _parent field are loaded eagerly instead of lazily by default. Build: remove progress logger hack for gradle 2.13 (elastic#23679) Test: Add dump of integ test cluster logs on failure (elastic#23688) Plugins: Add plugin cli specific exit codes (elastic#23599) Plugins: Output better error message when existing plugin is incompatible (elastic#23562) Reindex: wait for cleanup before responding (elastic#23677) Packaging: Remove classpath ordering hack (elastic#23596) Docs: Add note about updating plugins requiring removal and reinstallation (elastic#23597) Build: Make plugin list for smoke tester dynamic (elastic#23601) [TEST] Propertly cleans up failing restore test
After the removal of the joda time hack we used to have, we can cleanup
the codebase handling in security, jarhell and plugins to be more picky
about uniqueness. This was originally in #18959 which was never merged.
closes #18959