Uploaded image for project: 'Apache Twill'
  1. Apache Twill
  2. TWILL-182

ApplicationBundler will overwrite dependencies with identical names

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 0.7.0-incubating
    • Fix Version/s: 0.8.0
    • Component/s: core
    • Labels:
      None

      Description

      If two jars obtained from different classpath locations have the same name but different contents, one will overwrite the other. The dependency code correctly finds the jars (uses the full path in the HashSet which accumulates the deps) but when the bundle is created the jars are written to /lib under their name. This results in one overwriting the other.

      While this is not a likely occurrence, it occurs for us in our development environment because our published jar names are built up from their project hierarchy. For example the model project for our sdk is in .../sdk/model and will be on the classpath as .../sdk/model.jar and published as sdk-model.jar.

      In practice however this could occur with any jar name and would be more likely over time.

      The ApplicationBundler could detect this and re-write the name with some part of the path or suffix to ensure the name is unique.

        Issue Links

          Activity

          Hide
          chtyim Terence Yim added a comment -

          Thanks for the contribution. Changes merged.

          Show
          chtyim Terence Yim added a comment - Thanks for the contribution. Changes merged.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/twill/pull/1

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/twill/pull/1
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user serranom commented on a diff in the pull request:

          https://github.com/apache/twill/pull/1#discussion_r75601503

          — Diff: twill-core/src/test/java/org/apache/twill/internal/utils/ApplicationBundlerTest.java —
          @@ -68,6 +71,56 @@ public void testFindDependencies() throws IOException, ClassNotFoundException

          { Assert.assertNotSame(classLoader, clz.getClassLoader()); }

          + @Test
          + public void testSameJar() throws IOException, ClassNotFoundException {
          + File dir1 = tmpDir.newFolder();
          + File dir2 = tmpDir.newFolder();
          + File j1 = new File(dir1, "samename.jar");
          + File j2 = new File(dir2, "samename.jar");
          +
          + createJar(Class1.class, j1);
          + createJar(Class2.class, j2);
          +
          + ClassLoader currentClassLoader = Thread.currentThread().getContextClassLoader();
          + Location location;
          +
          + try {
          + URL[] urls = new URL[]

          { j1.toURI().toURL(), j2.toURI().toURL() }

          ;
          + Thread.currentThread().setContextClassLoader(new URLClassLoader(urls, null));
          +
          + // create bundle
          + location = new LocalLocationFactory(tmpDir.newFolder()).create("test.jar");
          + ApplicationBundler bundler = new ApplicationBundler(ImmutableList.<String> of());
          + bundler.createBundle(location, Class1.class, Class2.class);
          +
          + } finally

          { + Thread.currentThread().setContextClassLoader(currentClassLoader); + }

          +
          + File targetDir = tmpDir.newFolder();
          + unjar(new File(location.toURI()), targetDir);
          +
          + // should be able to load both classes
          + ClassLoader classLoader = createClassLoader(targetDir);
          + Class<?> c1 = classLoader.loadClass(Class1.class.getName());
          + Class<?> c2 = classLoader.loadClass(Class2.class.getName());
          +
          + // make sure we are loading them from the correct class loader (not defaulting back to some classloader
          + // from the test
          + Assert.assertSame(classLoader, c1.getClassLoader());
          + Assert.assertSame(classLoader, c2.getClassLoader());
          + }
          +
          + private void createJar(Class<?> clazz, File jarFile) throws IOException {
          + try (JarOutputStream jos = new JarOutputStream(new FileOutputStream(jarFile))) {
          + String pathname = clazz.getName().replace(".", "/") + ".class";
          + JarEntry entry = new JarEntry(pathname);
          + jos.putNextEntry(entry);
          + IOUtils.copy(clazz.getClassLoader().getResourceAsStream(pathname), jos);
          — End diff –

          I will switch to guava. And fix the resource stream.

          Show
          githubbot ASF GitHub Bot added a comment - Github user serranom commented on a diff in the pull request: https://github.com/apache/twill/pull/1#discussion_r75601503 — Diff: twill-core/src/test/java/org/apache/twill/internal/utils/ApplicationBundlerTest.java — @@ -68,6 +71,56 @@ public void testFindDependencies() throws IOException, ClassNotFoundException { Assert.assertNotSame(classLoader, clz.getClassLoader()); } + @Test + public void testSameJar() throws IOException, ClassNotFoundException { + File dir1 = tmpDir.newFolder(); + File dir2 = tmpDir.newFolder(); + File j1 = new File(dir1, "samename.jar"); + File j2 = new File(dir2, "samename.jar"); + + createJar(Class1.class, j1); + createJar(Class2.class, j2); + + ClassLoader currentClassLoader = Thread.currentThread().getContextClassLoader(); + Location location; + + try { + URL[] urls = new URL[] { j1.toURI().toURL(), j2.toURI().toURL() } ; + Thread.currentThread().setContextClassLoader(new URLClassLoader(urls, null)); + + // create bundle + location = new LocalLocationFactory(tmpDir.newFolder()).create("test.jar"); + ApplicationBundler bundler = new ApplicationBundler(ImmutableList.<String> of()); + bundler.createBundle(location, Class1.class, Class2.class); + + } finally { + Thread.currentThread().setContextClassLoader(currentClassLoader); + } + + File targetDir = tmpDir.newFolder(); + unjar(new File(location.toURI()), targetDir); + + // should be able to load both classes + ClassLoader classLoader = createClassLoader(targetDir); + Class<?> c1 = classLoader.loadClass(Class1.class.getName()); + Class<?> c2 = classLoader.loadClass(Class2.class.getName()); + + // make sure we are loading them from the correct class loader (not defaulting back to some classloader + // from the test + Assert.assertSame(classLoader, c1.getClassLoader()); + Assert.assertSame(classLoader, c2.getClassLoader()); + } + + private void createJar(Class<?> clazz, File jarFile) throws IOException { + try (JarOutputStream jos = new JarOutputStream(new FileOutputStream(jarFile))) { + String pathname = clazz.getName().replace(".", "/") + ".class"; + JarEntry entry = new JarEntry(pathname); + jos.putNextEntry(entry); + IOUtils.copy(clazz.getClassLoader().getResourceAsStream(pathname), jos); — End diff – I will switch to guava. And fix the resource stream.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user chtyim commented on a diff in the pull request:

          https://github.com/apache/twill/pull/1#discussion_r75600610

          — Diff: twill-core/src/test/java/org/apache/twill/internal/utils/ApplicationBundlerTest.java —
          @@ -68,6 +71,56 @@ public void testFindDependencies() throws IOException, ClassNotFoundException

          { Assert.assertNotSame(classLoader, clz.getClassLoader()); }

          + @Test
          + public void testSameJar() throws IOException, ClassNotFoundException {
          + File dir1 = tmpDir.newFolder();
          + File dir2 = tmpDir.newFolder();
          + File j1 = new File(dir1, "samename.jar");
          + File j2 = new File(dir2, "samename.jar");
          +
          + createJar(Class1.class, j1);
          + createJar(Class2.class, j2);
          +
          + ClassLoader currentClassLoader = Thread.currentThread().getContextClassLoader();
          + Location location;
          +
          + try {
          + URL[] urls = new URL[]

          { j1.toURI().toURL(), j2.toURI().toURL() }

          ;
          + Thread.currentThread().setContextClassLoader(new URLClassLoader(urls, null));
          +
          + // create bundle
          + location = new LocalLocationFactory(tmpDir.newFolder()).create("test.jar");
          + ApplicationBundler bundler = new ApplicationBundler(ImmutableList.<String> of());
          + bundler.createBundle(location, Class1.class, Class2.class);
          +
          + } finally

          { + Thread.currentThread().setContextClassLoader(currentClassLoader); + }

          +
          + File targetDir = tmpDir.newFolder();
          + unjar(new File(location.toURI()), targetDir);
          +
          + // should be able to load both classes
          + ClassLoader classLoader = createClassLoader(targetDir);
          + Class<?> c1 = classLoader.loadClass(Class1.class.getName());
          + Class<?> c2 = classLoader.loadClass(Class2.class.getName());
          +
          + // make sure we are loading them from the correct class loader (not defaulting back to some classloader
          + // from the test
          + Assert.assertSame(classLoader, c1.getClassLoader());
          + Assert.assertSame(classLoader, c2.getClassLoader());
          + }
          +
          + private void createJar(Class<?> clazz, File jarFile) throws IOException {
          + try (JarOutputStream jos = new JarOutputStream(new FileOutputStream(jarFile))) {
          + String pathname = clazz.getName().replace(".", "/") + ".class";
          + JarEntry entry = new JarEntry(pathname);
          + jos.putNextEntry(entry);
          + IOUtils.copy(clazz.getClassLoader().getResourceAsStream(pathname), jos);
          — End diff –

          And that also make sure the proper close of the resource stream.

          Show
          githubbot ASF GitHub Bot added a comment - Github user chtyim commented on a diff in the pull request: https://github.com/apache/twill/pull/1#discussion_r75600610 — Diff: twill-core/src/test/java/org/apache/twill/internal/utils/ApplicationBundlerTest.java — @@ -68,6 +71,56 @@ public void testFindDependencies() throws IOException, ClassNotFoundException { Assert.assertNotSame(classLoader, clz.getClassLoader()); } + @Test + public void testSameJar() throws IOException, ClassNotFoundException { + File dir1 = tmpDir.newFolder(); + File dir2 = tmpDir.newFolder(); + File j1 = new File(dir1, "samename.jar"); + File j2 = new File(dir2, "samename.jar"); + + createJar(Class1.class, j1); + createJar(Class2.class, j2); + + ClassLoader currentClassLoader = Thread.currentThread().getContextClassLoader(); + Location location; + + try { + URL[] urls = new URL[] { j1.toURI().toURL(), j2.toURI().toURL() } ; + Thread.currentThread().setContextClassLoader(new URLClassLoader(urls, null)); + + // create bundle + location = new LocalLocationFactory(tmpDir.newFolder()).create("test.jar"); + ApplicationBundler bundler = new ApplicationBundler(ImmutableList.<String> of()); + bundler.createBundle(location, Class1.class, Class2.class); + + } finally { + Thread.currentThread().setContextClassLoader(currentClassLoader); + } + + File targetDir = tmpDir.newFolder(); + unjar(new File(location.toURI()), targetDir); + + // should be able to load both classes + ClassLoader classLoader = createClassLoader(targetDir); + Class<?> c1 = classLoader.loadClass(Class1.class.getName()); + Class<?> c2 = classLoader.loadClass(Class2.class.getName()); + + // make sure we are loading them from the correct class loader (not defaulting back to some classloader + // from the test + Assert.assertSame(classLoader, c1.getClassLoader()); + Assert.assertSame(classLoader, c2.getClassLoader()); + } + + private void createJar(Class<?> clazz, File jarFile) throws IOException { + try (JarOutputStream jos = new JarOutputStream(new FileOutputStream(jarFile))) { + String pathname = clazz.getName().replace(".", "/") + ".class"; + JarEntry entry = new JarEntry(pathname); + jos.putNextEntry(entry); + IOUtils.copy(clazz.getClassLoader().getResourceAsStream(pathname), jos); — End diff – And that also make sure the proper close of the resource stream.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user chtyim commented on a diff in the pull request:

          https://github.com/apache/twill/pull/1#discussion_r75600604

          — Diff: twill-core/src/test/java/org/apache/twill/internal/utils/ApplicationBundlerTest.java —
          @@ -68,6 +71,56 @@ public void testFindDependencies() throws IOException, ClassNotFoundException

          { Assert.assertNotSame(classLoader, clz.getClassLoader()); }

          + @Test
          + public void testSameJar() throws IOException, ClassNotFoundException {
          + File dir1 = tmpDir.newFolder();
          + File dir2 = tmpDir.newFolder();
          + File j1 = new File(dir1, "samename.jar");
          + File j2 = new File(dir2, "samename.jar");
          +
          + createJar(Class1.class, j1);
          + createJar(Class2.class, j2);
          +
          + ClassLoader currentClassLoader = Thread.currentThread().getContextClassLoader();
          + Location location;
          +
          + try {
          + URL[] urls = new URL[]

          { j1.toURI().toURL(), j2.toURI().toURL() }

          ;
          + Thread.currentThread().setContextClassLoader(new URLClassLoader(urls, null));
          +
          + // create bundle
          + location = new LocalLocationFactory(tmpDir.newFolder()).create("test.jar");
          + ApplicationBundler bundler = new ApplicationBundler(ImmutableList.<String> of());
          + bundler.createBundle(location, Class1.class, Class2.class);
          +
          + } finally

          { + Thread.currentThread().setContextClassLoader(currentClassLoader); + }

          +
          + File targetDir = tmpDir.newFolder();
          + unjar(new File(location.toURI()), targetDir);
          +
          + // should be able to load both classes
          + ClassLoader classLoader = createClassLoader(targetDir);
          + Class<?> c1 = classLoader.loadClass(Class1.class.getName());
          + Class<?> c2 = classLoader.loadClass(Class2.class.getName());
          +
          + // make sure we are loading them from the correct class loader (not defaulting back to some classloader
          + // from the test
          + Assert.assertSame(classLoader, c1.getClassLoader());
          + Assert.assertSame(classLoader, c2.getClassLoader());
          + }
          +
          + private void createJar(Class<?> clazz, File jarFile) throws IOException {
          + try (JarOutputStream jos = new JarOutputStream(new FileOutputStream(jarFile))) {
          + String pathname = clazz.getName().replace(".", "/") + ".class";
          + JarEntry entry = new JarEntry(pathname);
          + jos.putNextEntry(entry);
          + IOUtils.copy(clazz.getClassLoader().getResourceAsStream(pathname), jos);
          — End diff –

          Sorry I missed this in last review. Usually we use guava in twill instead of apache common. For this, can you use `Resources.copy(clazz.getClassLoader().getResource(pathname), jos);` instead? Thanks.

          Show
          githubbot ASF GitHub Bot added a comment - Github user chtyim commented on a diff in the pull request: https://github.com/apache/twill/pull/1#discussion_r75600604 — Diff: twill-core/src/test/java/org/apache/twill/internal/utils/ApplicationBundlerTest.java — @@ -68,6 +71,56 @@ public void testFindDependencies() throws IOException, ClassNotFoundException { Assert.assertNotSame(classLoader, clz.getClassLoader()); } + @Test + public void testSameJar() throws IOException, ClassNotFoundException { + File dir1 = tmpDir.newFolder(); + File dir2 = tmpDir.newFolder(); + File j1 = new File(dir1, "samename.jar"); + File j2 = new File(dir2, "samename.jar"); + + createJar(Class1.class, j1); + createJar(Class2.class, j2); + + ClassLoader currentClassLoader = Thread.currentThread().getContextClassLoader(); + Location location; + + try { + URL[] urls = new URL[] { j1.toURI().toURL(), j2.toURI().toURL() } ; + Thread.currentThread().setContextClassLoader(new URLClassLoader(urls, null)); + + // create bundle + location = new LocalLocationFactory(tmpDir.newFolder()).create("test.jar"); + ApplicationBundler bundler = new ApplicationBundler(ImmutableList.<String> of()); + bundler.createBundle(location, Class1.class, Class2.class); + + } finally { + Thread.currentThread().setContextClassLoader(currentClassLoader); + } + + File targetDir = tmpDir.newFolder(); + unjar(new File(location.toURI()), targetDir); + + // should be able to load both classes + ClassLoader classLoader = createClassLoader(targetDir); + Class<?> c1 = classLoader.loadClass(Class1.class.getName()); + Class<?> c2 = classLoader.loadClass(Class2.class.getName()); + + // make sure we are loading them from the correct class loader (not defaulting back to some classloader + // from the test + Assert.assertSame(classLoader, c1.getClassLoader()); + Assert.assertSame(classLoader, c2.getClassLoader()); + } + + private void createJar(Class<?> clazz, File jarFile) throws IOException { + try (JarOutputStream jos = new JarOutputStream(new FileOutputStream(jarFile))) { + String pathname = clazz.getName().replace(".", "/") + ".class"; + JarEntry entry = new JarEntry(pathname); + jos.putNextEntry(entry); + IOUtils.copy(clazz.getClassLoader().getResourceAsStream(pathname), jos); — End diff – Sorry I missed this in last review. Usually we use guava in twill instead of apache common. For this, can you use `Resources.copy(clazz.getClassLoader().getResource(pathname), jos);` instead? Thanks.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user serranom commented on the issue:

          https://github.com/apache/twill/pull/1

          Fixed these and some other checkstyle related parts. I was running full builds but was not seeing the checkstyle failures. I'm not sure what mistake I made. Anyway, these issues are now fixed. Thanks for your patience as I get to know the project. Also, twill-core has these checkstyle warnings but a failure is not triggered:

          [INFO] — maven-checkstyle-plugin:2.12.1:check (validate) @ twill-core —
          [INFO] Starting audit...
          /home/martin/dev/other/twill/twill- core/src/main/java/../java/org/apache/twill/internal/state/SystemMessages.java:22: warning: Wrong order for 'com.google.common.base.Preconditions' import.
          /home/martin/dev/other/twill/twill-core/src/test/java/org/apache/twill/internal/DebugOptionsTest.java:26: warning: First sentence should end with a period.
          Audit done.

          Show
          githubbot ASF GitHub Bot added a comment - Github user serranom commented on the issue: https://github.com/apache/twill/pull/1 Fixed these and some other checkstyle related parts. I was running full builds but was not seeing the checkstyle failures. I'm not sure what mistake I made. Anyway, these issues are now fixed. Thanks for your patience as I get to know the project. Also, twill-core has these checkstyle warnings but a failure is not triggered: [INFO] — maven-checkstyle-plugin:2.12.1:check (validate) @ twill-core — [INFO] Starting audit... /home/martin/dev/other/twill/twill- core/src/main/java/../java/org/apache/twill/internal/state/SystemMessages.java:22: warning: Wrong order for 'com.google.common.base.Preconditions' import. /home/martin/dev/other/twill/twill-core/src/test/java/org/apache/twill/internal/DebugOptionsTest.java:26: warning: First sentence should end with a period. Audit done.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user hsaputra commented on a diff in the pull request:

          https://github.com/apache/twill/pull/1#discussion_r75589275

          — Diff: twill-core/src/main/java/org/apache/twill/internal/ApplicationBundler.java —
          @@ -51,6 +34,24 @@
          import java.util.zip.CRC32;
          import java.util.zip.CheckedOutputStream;

          +import org.apache.twill.api.ClassAcceptor;
          — End diff –

          Ah yes, all tests fail due to checkstyle

          Show
          githubbot ASF GitHub Bot added a comment - Github user hsaputra commented on a diff in the pull request: https://github.com/apache/twill/pull/1#discussion_r75589275 — Diff: twill-core/src/main/java/org/apache/twill/internal/ApplicationBundler.java — @@ -51,6 +34,24 @@ import java.util.zip.CRC32; import java.util.zip.CheckedOutputStream; +import org.apache.twill.api.ClassAcceptor; — End diff – Ah yes, all tests fail due to checkstyle
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user hsaputra commented on a diff in the pull request:

          https://github.com/apache/twill/pull/1#discussion_r75587290

          — Diff: twill-core/src/main/java/org/apache/twill/internal/ApplicationBundler.java —
          @@ -51,6 +34,24 @@
          import java.util.zip.CRC32;
          import java.util.zip.CheckedOutputStream;

          +import org.apache.twill.api.ClassAcceptor;
          — End diff –

          Shouldn't this trigger the style check error?

          Show
          githubbot ASF GitHub Bot added a comment - Github user hsaputra commented on a diff in the pull request: https://github.com/apache/twill/pull/1#discussion_r75587290 — Diff: twill-core/src/main/java/org/apache/twill/internal/ApplicationBundler.java — @@ -51,6 +34,24 @@ import java.util.zip.CRC32; import java.util.zip.CheckedOutputStream; +import org.apache.twill.api.ClassAcceptor; — End diff – Shouldn't this trigger the style check error?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user chtyim commented on the issue:

          https://github.com/apache/twill/pull/1

          Other than the import arrangement, the code changes LGTM.

          Show
          githubbot ASF GitHub Bot added a comment - Github user chtyim commented on the issue: https://github.com/apache/twill/pull/1 Other than the import arrangement, the code changes LGTM.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user chtyim commented on a diff in the pull request:

          https://github.com/apache/twill/pull/1#discussion_r75587121

          — Diff: twill-core/src/test/java/org/apache/twill/internal/utils/ApplicationBundlerTest.java —
          @@ -17,27 +17,32 @@
          */
          package org.apache.twill.internal.utils;

          -import com.google.common.collect.ImmutableList;
          -import com.google.common.collect.Lists;
          -import com.google.common.io.ByteStreams;
          -import com.google.common.io.Files;
          -import org.apache.twill.filesystem.LocalLocationFactory;
          -import org.apache.twill.filesystem.Location;
          -import org.apache.twill.internal.ApplicationBundler;
          -import org.junit.Assert;
          -import org.junit.Rule;
          -import org.junit.Test;
          -import org.junit.rules.TemporaryFolder;
          -
          import java.io.File;
          import java.io.FileInputStream;
          +import java.io.FileOutputStream;
          import java.io.IOException;
          import java.net.MalformedURLException;
          import java.net.URL;
          import java.net.URLClassLoader;
          +import java.util.ArrayList;
          import java.util.List;
          import java.util.jar.JarEntry;
          import java.util.jar.JarInputStream;
          +import java.util.jar.JarOutputStream;
          +
          +import org.apache.commons.compress.utils.IOUtils;
          — End diff –

          Same here. The profile that we use have all JDK classes come after other imports.

          Show
          githubbot ASF GitHub Bot added a comment - Github user chtyim commented on a diff in the pull request: https://github.com/apache/twill/pull/1#discussion_r75587121 — Diff: twill-core/src/test/java/org/apache/twill/internal/utils/ApplicationBundlerTest.java — @@ -17,27 +17,32 @@ */ package org.apache.twill.internal.utils; -import com.google.common.collect.ImmutableList; -import com.google.common.collect.Lists; -import com.google.common.io.ByteStreams; -import com.google.common.io.Files; -import org.apache.twill.filesystem.LocalLocationFactory; -import org.apache.twill.filesystem.Location; -import org.apache.twill.internal.ApplicationBundler; -import org.junit.Assert; -import org.junit.Rule; -import org.junit.Test; -import org.junit.rules.TemporaryFolder; - import java.io.File; import java.io.FileInputStream; +import java.io.FileOutputStream; import java.io.IOException; import java.net.MalformedURLException; import java.net.URL; import java.net.URLClassLoader; +import java.util.ArrayList; import java.util.List; import java.util.jar.JarEntry; import java.util.jar.JarInputStream; +import java.util.jar.JarOutputStream; + +import org.apache.commons.compress.utils.IOUtils; — End diff – Same here. The profile that we use have all JDK classes come after other imports.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user chtyim commented on a diff in the pull request:

          https://github.com/apache/twill/pull/1#discussion_r75587072

          — Diff: twill-core/src/main/java/org/apache/twill/internal/ApplicationBundler.java —
          @@ -51,6 +34,24 @@
          import java.util.zip.CRC32;
          import java.util.zip.CheckedOutputStream;

          +import org.apache.twill.api.ClassAcceptor;
          — End diff –

          Can you not re-arranging the imports?

          Show
          githubbot ASF GitHub Bot added a comment - Github user chtyim commented on a diff in the pull request: https://github.com/apache/twill/pull/1#discussion_r75587072 — Diff: twill-core/src/main/java/org/apache/twill/internal/ApplicationBundler.java — @@ -51,6 +34,24 @@ import java.util.zip.CRC32; import java.util.zip.CheckedOutputStream; +import org.apache.twill.api.ClassAcceptor; — End diff – Can you not re-arranging the imports?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user serranom commented on the issue:

          https://github.com/apache/twill/pull/1

          @chtyim I assume you mean create those jars at runtime? This is what I had difficulty doing within the test, I think because the classes were getting loaded incorrectly. I will give that another go.

          Show
          githubbot ASF GitHub Bot added a comment - Github user serranom commented on the issue: https://github.com/apache/twill/pull/1 @chtyim I assume you mean create those jars at runtime? This is what I had difficulty doing within the test, I think because the classes were getting loaded incorrectly. I will give that another go.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user serranom commented on a diff in the pull request:

          https://github.com/apache/twill/pull/1#discussion_r75528764

          — Diff: twill-core/src/main/java/org/apache/twill/internal/ApplicationBundler.java —
          @@ -215,8 +215,19 @@ public boolean accept(String className, URL classUrl, URL classPathUrl) {
          private void putEntry(String className, URL classUrl, URL classPathUrl, Set<String> entries, JarOutputStream jarOut) {
          String classPath = classPathUrl.getFile();
          if (classPath.endsWith(".jar")) {
          + String entryName = classPath.substring(classPath.lastIndexOf('/') + 1);
          + // need unique name or else we lose classes (TWILL-181)
          + if (entries.contains(SUBDIR_LIB + entryName)) {
          + String[] parts = classPath.split("/");
          — End diff –

          In this context, we know the classPath urls are all unique because they come from a set. What this logic does is to prepending the entry name with enough elements of this path to make it unique. I presumed there was some desire to keep the entry names short for other reasons. By using the elements from the path we get something guaranteed to be unique while keeping some meaning to the name.

          Show
          githubbot ASF GitHub Bot added a comment - Github user serranom commented on a diff in the pull request: https://github.com/apache/twill/pull/1#discussion_r75528764 — Diff: twill-core/src/main/java/org/apache/twill/internal/ApplicationBundler.java — @@ -215,8 +215,19 @@ public boolean accept(String className, URL classUrl, URL classPathUrl) { private void putEntry(String className, URL classUrl, URL classPathUrl, Set<String> entries, JarOutputStream jarOut) { String classPath = classPathUrl.getFile(); if (classPath.endsWith(".jar")) { + String entryName = classPath.substring(classPath.lastIndexOf('/') + 1); + // need unique name or else we lose classes ( TWILL-181 ) + if (entries.contains(SUBDIR_LIB + entryName)) { + String[] parts = classPath.split("/"); — End diff – In this context, we know the classPath urls are all unique because they come from a set. What this logic does is to prepending the entry name with enough elements of this path to make it unique. I presumed there was some desire to keep the entry names short for other reasons. By using the elements from the path we get something guaranteed to be unique while keeping some meaning to the name.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user chtyim commented on the issue:

          https://github.com/apache/twill/pull/1

          Actually you don't need to use ASM. You can just create two jars, one contains `org/apache/twill/internal/utils/Class1.class`, the other has `org/apache/twill/internal/utils/Class2.class`. Then create a `URLClassLoader` from that two jars, with `null` as the parent. Then load `Class1` and `Class2` from that classloader and provide the both classes to the `ApplicationBundler.createBundle` method.

          Show
          githubbot ASF GitHub Bot added a comment - Github user chtyim commented on the issue: https://github.com/apache/twill/pull/1 Actually you don't need to use ASM. You can just create two jars, one contains `org/apache/twill/internal/utils/Class1.class`, the other has `org/apache/twill/internal/utils/Class2.class`. Then create a `URLClassLoader` from that two jars, with `null` as the parent. Then load `Class1` and `Class2` from that classloader and provide the both classes to the `ApplicationBundler.createBundle` method.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user chtyim commented on the issue:

          https://github.com/apache/twill/pull/1

          +1 for @hsaputra idea of not checking in the binary jar. To not mess with the classpath, you can simply generate an empty class using ASM and write it to a jar (shouldn't be too complicate to use `ClassWriter` to do so, probably about 20 lines of code). I can help with that if needed

          Show
          githubbot ASF GitHub Bot added a comment - Github user chtyim commented on the issue: https://github.com/apache/twill/pull/1 +1 for @hsaputra idea of not checking in the binary jar. To not mess with the classpath, you can simply generate an empty class using ASM and write it to a jar (shouldn't be too complicate to use `ClassWriter` to do so, probably about 20 lines of code). I can help with that if needed
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user chtyim commented on a diff in the pull request:

          https://github.com/apache/twill/pull/1#discussion_r75149838

          — Diff: twill-core/src/main/java/org/apache/twill/internal/ApplicationBundler.java —
          @@ -215,8 +215,19 @@ public boolean accept(String className, URL classUrl, URL classPathUrl) {
          private void putEntry(String className, URL classUrl, URL classPathUrl, Set<String> entries, JarOutputStream jarOut) {
          String classPath = classPathUrl.getFile();
          if (classPath.endsWith(".jar")) {
          + String entryName = classPath.substring(classPath.lastIndexOf('/') + 1);
          + // need unique name or else we lose classes (TWILL-181)
          + if (entries.contains(SUBDIR_LIB + entryName)) {
          + String[] parts = classPath.split("/");
          — End diff –

          I don't quite understand this part of the logic. Is it trying to use some parent directory name as the entryName? Seems like it would lost the original name. Also, if the jar is already at the root directory, then it will be using the original "entryName", which would still result in name conflict? I would suggest simply use package name (derived from `className`) and a random uuid to make it unique if there is conflict. Something like:

          ```java
          String entryName = classPath.substring(classPath.lastIndexOf('/') + 1);
          if (entries.contains(SUBDIR_LIB + entryName))

          { entryName = entryName.substring(0, entryName.lastIndexOf('.')) + "-" + className.substring(0, className.lastIndexOf('.')) + "-" + UUID.randomUUID() + ".jar"; }

          ```

          Show
          githubbot ASF GitHub Bot added a comment - Github user chtyim commented on a diff in the pull request: https://github.com/apache/twill/pull/1#discussion_r75149838 — Diff: twill-core/src/main/java/org/apache/twill/internal/ApplicationBundler.java — @@ -215,8 +215,19 @@ public boolean accept(String className, URL classUrl, URL classPathUrl) { private void putEntry(String className, URL classUrl, URL classPathUrl, Set<String> entries, JarOutputStream jarOut) { String classPath = classPathUrl.getFile(); if (classPath.endsWith(".jar")) { + String entryName = classPath.substring(classPath.lastIndexOf('/') + 1); + // need unique name or else we lose classes ( TWILL-181 ) + if (entries.contains(SUBDIR_LIB + entryName)) { + String[] parts = classPath.split("/"); — End diff – I don't quite understand this part of the logic. Is it trying to use some parent directory name as the entryName? Seems like it would lost the original name. Also, if the jar is already at the root directory, then it will be using the original "entryName", which would still result in name conflict? I would suggest simply use package name (derived from `className`) and a random uuid to make it unique if there is conflict. Something like: ```java String entryName = classPath.substring(classPath.lastIndexOf('/') + 1); if (entries.contains(SUBDIR_LIB + entryName)) { entryName = entryName.substring(0, entryName.lastIndexOf('.')) + "-" + className.substring(0, className.lastIndexOf('.')) + "-" + UUID.randomUUID() + ".jar"; } ```
          Hide
          mserrano Martin Serrano added a comment -

          will do.

          Show
          mserrano Martin Serrano added a comment - will do.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user hsaputra commented on the issue:

          https://github.com/apache/incubator-twill/pull/83

          I am sorry for the trouble, but could you move the PR to our new Github mirror: https://github.com/apache/twill

          The mirror will go away in the next ASF infra mirror refresh.

          I will send email to dev@ list for announcement. Thanks!

          Show
          githubbot ASF GitHub Bot added a comment - Github user hsaputra commented on the issue: https://github.com/apache/incubator-twill/pull/83 I am sorry for the trouble, but could you move the PR to our new Github mirror: https://github.com/apache/twill The mirror will go away in the next ASF infra mirror refresh. I will send email to dev@ list for announcement. Thanks!
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user serranom opened a pull request:

          https://github.com/apache/incubator-twill/pull/83

          TWILL-182, make jar name unique when creating bundle

          These code changes add a test for the same jar issue and update the jar name saved to the bundle if it is no longer unique. To uniquify the name, parts of the path are progressively prefixed onto the original name. We know at some point the full path is unique (because it is present in a set) so eventually the prepending must end up with a unique jar name.

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/serranom/incubator-twill master

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/incubator-twill/pull/83.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #83


          commit 0ae5df6cb21d1eede4482615fb07cc4870df5c73
          Author: martin <martin@attivio.com>
          Date: 2016-08-09T19:21:30Z

          TWILL-182, failing test which demonstrates issue

          commit 2208baa4ece8c947e0e6b9320d635376ecbed7ff
          Author: martin <martin@attivio.com>
          Date: 2016-08-10T00:10:56Z

          TWILL-181, uniquify included jars by using dir names

          commit 01623e81137215db2d188a1fb6f7c98acdce483f
          Author: martin <martin@attivio.com>
          Date: 2016-08-10T00:15:43Z

          TWILL-181, fix warnings for style

          commit 1bc367b459cf97f5d92391b89164c4f0baeac7ee
          Author: martin <martin@attivio.com>
          Date: 2016-08-10T00:18:54Z

          Merge commit '0ae5df6'

          commit 421e0f44f592a992a374500537b4c42d9c32a180
          Author: martin <martin@attivio.com>
          Date: 2016-08-10T00:20:06Z

          Merge commit '2208baa'

          commit a6cfceb6c4db6e5e8daab953f4dae9ae5995db3a
          Author: martin <martin@attivio.com>
          Date: 2016-08-10T00:20:20Z

          Merge commit '01623e8'


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user serranom opened a pull request: https://github.com/apache/incubator-twill/pull/83 TWILL-182 , make jar name unique when creating bundle These code changes add a test for the same jar issue and update the jar name saved to the bundle if it is no longer unique. To uniquify the name, parts of the path are progressively prefixed onto the original name. We know at some point the full path is unique (because it is present in a set) so eventually the prepending must end up with a unique jar name. You can merge this pull request into a Git repository by running: $ git pull https://github.com/serranom/incubator-twill master Alternatively you can review and apply these changes as the patch at: https://github.com/apache/incubator-twill/pull/83.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #83 commit 0ae5df6cb21d1eede4482615fb07cc4870df5c73 Author: martin <martin@attivio.com> Date: 2016-08-09T19:21:30Z TWILL-182 , failing test which demonstrates issue commit 2208baa4ece8c947e0e6b9320d635376ecbed7ff Author: martin <martin@attivio.com> Date: 2016-08-10T00:10:56Z TWILL-181 , uniquify included jars by using dir names commit 01623e81137215db2d188a1fb6f7c98acdce483f Author: martin <martin@attivio.com> Date: 2016-08-10T00:15:43Z TWILL-181 , fix warnings for style commit 1bc367b459cf97f5d92391b89164c4f0baeac7ee Author: martin <martin@attivio.com> Date: 2016-08-10T00:18:54Z Merge commit '0ae5df6' commit 421e0f44f592a992a374500537b4c42d9c32a180 Author: martin <martin@attivio.com> Date: 2016-08-10T00:20:06Z Merge commit '2208baa' commit a6cfceb6c4db6e5e8daab953f4dae9ae5995db3a Author: martin <martin@attivio.com> Date: 2016-08-10T00:20:20Z Merge commit '01623e8'

            People

            • Assignee:
              mserrano Martin Serrano
              Reporter:
              mserrano Martin Serrano
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development