Compute entrypoint for a standard jar in exploded mode. #2862

mpeddada1 · 2020-10-24T01:25:22Z

For #2845
This PR also contains some minor refactorings.

jib-cli/src/main/java/com/google/cloud/tools/jib/cli/jar/JarProcessor.java

chanseokoh · 2020-10-26T14:09:45Z

jib-cli/src/test/java/com/google/cloud/tools/jib/cli/jar/JarProcessorTest.java

+      throws IOException, URISyntaxException {
+    Path standardJar =
+        Paths.get(Resources.getResource(STANDARD_JAR_WITH_CLASS_PATH_MANIFEST).toURI());
+    Path destDir = temporaryFolder.newFolder().toPath();


Just so you know, if you don't need a nested directory or multiple directories, you can use the root: temporaryFolder.getRoot().toPath().

chanseokoh · 2020-10-26T14:11:11Z

jib-cli/src/main/java/com/google/cloud/tools/jib/cli/jar/JarProcessor.java

+   *     provided doesn't exist
+   * @throws IllegalArgumentException if main class is not found in the jar manifest
+   */
+  public static ImmutableList<String> computeEntrypoint_explodedStandard(


Using _ is only accepted in test method names. computeEntrypointForExplodedStandard()?

And I don't know who will require this method, but you should always double-checking if a method may have a narrower access scope. (Just don't habitually make everything public until necessary.)

oops I keep mixing up the naming with python convention

Oh good point. Changed this method and explodeStandardJar to protected since they will be accessed from within the same package (specifically in the class that will be used to take a jar and convert it into a JibContainerBuilder -- next pr).

jib-cli/src/main/java/com/google/cloud/tools/jib/cli/jar/JarProcessor.java

chanseokoh · 2020-10-26T14:20:19Z

jib-cli/src/main/java/com/google/cloud/tools/jib/cli/jar/JarProcessor.java

+      mainClass = jarFile.getManifest().getMainAttributes().getValue(Attributes.Name.MAIN_CLASS);
+    }
+    if (mainClass == null) {
+      throw new IllegalArgumentException("Main-Class not found in jar's manifest.");


The CLI will crash with this message. And providing a clear message implying an actionable fix increases the chance of the user understanding and fixing the issue on their own, resulting in less frustrated getting-started experience (which is important to increase the chance of adoption). It also reduces our workload by reducing the likelyhood of multiple user opening the same issue.

Suggested change

throw new IllegalArgumentException("Main-Class not found in jar's manifest.");

throw new IllegalArgumentException("'Main-Class:' attribute to define an application main class not defined in the input JAR's manifest ('META-INF/MANIFEST.MF` in the JAR).");

Ah you're right. Better error messaging is certainly helpful especially with something that causes the code to crash. Thanks.

chanseokoh · 2020-10-26T14:29:44Z

jib-cli/src/main/java/com/google/cloud/tools/jib/cli/jar/JarProcessor.java

+    List<String> classpath = new ArrayList<>();
+    for (FileEntriesLayer layer : layers) {
+      if (layer.getName().equals(DEPENDENCIES) || layer.getName().equals(SNAPSHOT_DEPENDENCIES)) {
+        classpath.add(APP_ROOT.resolve("dependencies").toString());


At least this should be

Suggested change

classpath.add(APP_ROOT.resolve("dependencies").toString());

classpath.add(APP_ROOT + "/dependencies/*");

Just so you know, there's an issue with the order of loading jars (like #1871 for Jib build plugins), but I think there's nothing we can do about it for a standard jar.

is the ordering issue not a problem for a standard jar atm?

jib-cli/src/main/java/com/google/cloud/tools/jib/cli/jar/JarProcessor.java

chanseokoh · 2020-10-26T22:29:15Z

jib-cli/src/main/java/com/google/cloud/tools/jib/cli/jar/JarProcessor.java

@@ -76,7 +81,7 @@ public static JarType determineJarType(Path jarPath) throws IOException {
   * @throws IOException if I/O error occurs when opening the jar file or if temporary directory
   *     provided doesn't exist
   */
-  public static List<FileEntriesLayer> explodeStandardJar(Path jarPath, Path tempDirPath)
+  protected static List<FileEntriesLayer> explodeStandardJar(Path jarPath, Path tempDirPath)


Perhaps this can be package-private?

Suggested change

protected static List<FileEntriesLayer> explodeStandardJar(Path jarPath, Path tempDirPath)

static List<FileEntriesLayer> explodeStandardJar(Path jarPath, Path tempDirPath)

And if this needs to be package-private only to be able to call from tests (i.e., it should have been private if not for the tests), add @VisibleForTesting.

chanseokoh · 2020-10-26T22:31:23Z

jib-cli/src/main/java/com/google/cloud/tools/jib/cli/jar/JarProcessor.java

+    }
+    if (mainClass == null) {
+      throw new IllegalArgumentException(
+          "`Main-Class:` attribute to define an application main class not defined in the input Jar's manifest (`META-INF/MANIFEST.MF` in the JAR).");


Sorry, I think this reads better:

Suggested change

"`Main-Class:` attribute to define an application main class not defined in the input Jar's manifest (`META-INF/MANIFEST.MF` in the JAR).");

"`Main-Class:` attribute for an application main class not defined in the input Jar's manifest (`META-INF/MANIFEST.MF` in the Jar).");

chanseokoh · 2020-10-26T22:33:11Z

jib-cli/src/main/java/com/google/cloud/tools/jib/cli/jar/JarProcessor.java

+  protected static ImmutableList<String> computeEntrypointForExplodedStandard(
+      Path jarPath, Path tempDirPath, List<FileEntriesLayer> layers) throws IOException {
+    Path localExplodedJarRoot = tempDirPath;
+    ZipUtil.unzip(jarPath, localExplodedJarRoot);


Oh, actually, I remember unzipping a jar somewhere else too. Are we unzipping it multiple times? We should unzip only once for efficiency.

Actually, I think there is no need to unzip a jar?

Cleaned this up a bit.

chanseokoh · 2020-10-26T22:34:43Z

jib-cli/src/main/java/com/google/cloud/tools/jib/cli/jar/JarProcessor.java

+    try (JarFile jarFile = new JarFile(jarPath.toFile())) {
+      mainClass = jarFile.getManifest().getMainAttributes().getValue(Attributes.Name.MAIN_CLASS);
+    }


Nit: I think this code block is super fast, so it's good to put everything in to the try-resource block.

Suggested change

try (JarFile jarFile = new JarFile(jarPath.toFile())) {

mainClass = jarFile.getManifest().getMainAttributes().getValue(Attributes.Name.MAIN_CLASS);

}

try (JarFile jarFile = new JarFile(jarPath.toFile())) {

String mainClass = jarFile.getManifest().getMainAttributes().getValue(Attributes.Name.MAIN_CLASS);

if (mainClass == null)

...

return ...

}

Add method to compute entrypoint for standard jar in exploded mode

7f4e83f

mpeddada1 requested a review from a team October 24, 2020 01:25

google-cla bot added the cla: yes label Oct 24, 2020

chanseokoh reviewed Oct 26, 2020

View reviewed changes

mpeddada1 added 2 commits October 26, 2020 18:18

improve error messaging, narrow down scope of method

fd9300c

use getRoot instead of newFolder in test

f61293e

chanseokoh reviewed Oct 26, 2020

View reviewed changes

mpeddada1 added 5 commits October 27, 2020 10:24

fix parameters

037672a

making methods package-private

3dcc8f0

modify try-resource block

188717e

minor formatting

5cbcd62

modifying tests

e8f1968

chanseokoh approved these changes Oct 27, 2020

View reviewed changes

mpeddada1 merged commit dbba0c6 into master Oct 27, 2020

mpeddada1 deleted the cli-jar-compute-entrypoint branch October 27, 2020 20:12

mpeddada1 mentioned this pull request Nov 2, 2020

[jib-cli-jar] Compute entrypoint for standard jar. #2845

Closed

mpeddada1 added the jib-cli-jar label Feb 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compute entrypoint for a standard jar in exploded mode. #2862

Compute entrypoint for a standard jar in exploded mode. #2862

mpeddada1 commented Oct 24, 2020

chanseokoh Oct 26, 2020

chanseokoh Oct 26, 2020

mpeddada1 Oct 26, 2020 •

edited

Loading

chanseokoh Oct 26, 2020

mpeddada1 Oct 26, 2020

chanseokoh Oct 26, 2020

mpeddada1 Oct 26, 2020

chanseokoh Oct 26, 2020

chanseokoh Oct 26, 2020

chanseokoh Oct 26, 2020

chanseokoh Oct 27, 2020

mpeddada1 Oct 27, 2020

chanseokoh Oct 26, 2020

mpeddada1 Oct 27, 2020

	throw new IllegalArgumentException("Main-Class not found in jar's manifest.");
	throw new IllegalArgumentException("'Main-Class:' attribute to define an application main class not defined in the input JAR's manifest ('META-INF/MANIFEST.MF` in the JAR).");

	classpath.add(APP_ROOT.resolve("dependencies").toString());
	classpath.add(APP_ROOT + "/dependencies/*");

	protected static List<FileEntriesLayer> explodeStandardJar(Path jarPath, Path tempDirPath)
	static List<FileEntriesLayer> explodeStandardJar(Path jarPath, Path tempDirPath)

	"`Main-Class:` attribute to define an application main class not defined in the input Jar's manifest (`META-INF/MANIFEST.MF` in the JAR).");
	"`Main-Class:` attribute for an application main class not defined in the input Jar's manifest (`META-INF/MANIFEST.MF` in the Jar).");

Compute entrypoint for a standard jar in exploded mode. #2862

Compute entrypoint for a standard jar in exploded mode. #2862

Conversation

mpeddada1 commented Oct 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpeddada1 Oct 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpeddada1 Oct 26, 2020 •

edited

Loading