Infer main class if it isn't configured anywhere #278

TadCordle · 2018-05-16T19:35:36Z

Searches the build output for .class files with a main method. If more than one is found, or if one isn't found, it throws an error.

Fixes #189.

…gration tests

…er_main_class

coollog

Good stuff!

coollog · 2018-05-16T20:45:50Z

jib-core/src/main/java/com/google/cloud/tools/jib/frontend/MainClassFinder.java

+import java.util.stream.Stream;
+import javax.annotation.Nullable;
+
+/** Class used for inferring the main class in an application. */


nit: usually can omit phrases like Class used for so it's like Infers the main...

coollog · 2018-05-16T20:47:31Z

jib-core/src/main/java/com/google/cloud/tools/jib/frontend/MainClassFinder.java

+  /** Helper class for loading a .class file. */
+  private static class ClassFileLoader extends ClassLoader {
+
+    private Path classFile;


coollog · 2018-05-16T20:54:27Z

jib-core/src/main/java/com/google/cloud/tools/jib/frontend/MainClassFinder.java

+  }
+
+  /**
+   * Searches for a class containing a main method given an absolute root directory.


Should prob have a note that this is searching for .class files in the directory.

coollog · 2018-05-16T20:56:44Z

jib-core/src/main/java/com/google/cloud/tools/jib/frontend/MainClassFinder.java

+    }
+
+    String className = null;
+    try (Stream<Path> pathStream = Files.walk(Paths.get(rootDirectory))) {


Can use DirectoryWalker here.

Is this much of an improvement? Looks like implementing a DirectoryWalker would be more complicated.

It's mostly as a way to abstract away the strategy for walking into a testable class. The use would like something like:

new DirectoryWalker(rootDirectory).filter(Files::isRegularFile).filter(path -> path.toString().endsWith(".class")).walk(classFile -> {...});

Oh I see, the one you implemented. I was thinking you meant something like this https://commons.apache.org/proper/commons-io/javadocs/api-1.4/org/apache/commons/io/DirectoryWalker.html

coollog · 2018-05-16T20:57:34Z

jib-core/src/main/java/com/google/cloud/tools/jib/frontend/MainClassFinder.java

+      List<Path> classFiles =
+          pathStream
+              .filter(Files::isRegularFile)
+              .filter(path -> path.toString().endsWith(".class"))


Path has endsWith

http://saltnlight5.blogspot.com/2014/04/be-careful-with-java-pathendswithstring.html

Ah right that makes sense. Looks like we used Filesystems#getPathMatcher in MavenSourceFilesConfiguration.

coollog · 2018-05-16T21:18:09Z

jib-gradle-plugin/src/integration-test/resources/projects/empty/build.gradle

@@ -15,7 +15,6 @@ jib {
        image = 'gcr.io/jib-integration-testing/emptyimage:gradle'
        credHelper = 'gcr'
    }
-    mainClass = 'com.test.Empty'


coollog · 2018-05-16T21:20:19Z

jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/ProjectProperties.java

-        throw new GradleException(
-            HelpfulSuggestionsProvider.get("Could not find main class specified in a 'jar' task")
-                .suggest("add a `mainClass` configuration to jib"));
+        gradleBuildLogger.info(


This could prob just be at the debug level.

coollog · 2018-05-16T21:22:28Z

jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/ProjectProperties.java

+        try {
+          mainClass =
+              MainClassFinder.findMainClass(
+                  project


This should use GradleSourceConfiguration#getClassesFiles, which will be the files that go into the container.

This returns a list of Paths, how would I use it to get the root directory?

coollog · 2018-05-16T21:24:10Z

jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/ProjectProperties.java

+        } catch (MultipleClassesFoundException | IOException ex) {
+          throw new GradleException(
+              HelpfulSuggestionsProvider.get("Failed to get main class: " + ex.getMessage())
+                  .suggest("add a `mainClass` configuration to jib"));


The ex should go as a second argument to GradleException rather than a string addition so there is stack hierarchy.

coollog · 2018-05-16T21:24:34Z

jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/ProjectProperties.java

+        } catch (MultipleClassesFoundException | IOException ex) {
+          throw new GradleException(
+              HelpfulSuggestionsProvider.get("Failed to get main class: " + ex.getMessage())
+                  .suggest("add a `mainClass` configuration to jib"));


This suggestion can be a constant together with below.

coollog · 2018-05-16T22:27:53Z

It would need to do the findMainClass for all the paths since there could be multiple sourcesets.

…

On Wed, May 16, 2018 at 6:25 PM Tad Cordle ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/ProjectProperties.java <#278 (comment)> : > @@ -46,21 +48,38 @@ String ***@***.*** String mainClass) { if (mainClass == null) { mainClass = getMainClassFromJarTask(); if (mainClass == null) { - throw new GradleException( - HelpfulSuggestionsProvider.get("Could not find main class specified in a 'jar' task") - .suggest("add a `mainClass` configuration to jib")); + gradleBuildLogger.info( + "Could not find main class specified in a 'jar' task; attempting to " + + "infer main class."); + try { + mainClass = + MainClassFinder.findMainClass( + project This returns a list of Paths, how would I use it to get the root directory? — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#278 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEoLKvBO-ZtYuQWH90MK318HSTR6gLfIks5tzKd1gaJpZM4UB6PN> .

…er_main_class

coollog · 2018-05-17T14:49:06Z

jib-core/src/main/java/com/google/cloud/tools/jib/frontend/MainClassFinder.java

-  public static String findMainClass(String rootDirectory)
-      throws MultipleClassesFoundException, IOException {
+  public static List<String> findMainClass(Path rootDirectory) throws IOException {
+    List<String> results = new ArrayList<>();


A more descriptive variable name like classNames

coollog · 2018-05-17T14:50:15Z

jib-core/src/main/java/com/google/cloud/tools/jib/frontend/MainClassFinder.java

-          // Got an invalid class file, keep searching
-          continue;
-        }
+    // Get all .class files


s/Get/Searches (or Looks at)

coollog · 2018-05-17T14:53:29Z

jib-core/src/main/java/com/google/cloud/tools/jib/frontend/MainClassFinder.java

+        .filter(Files::isRegularFile)
+        .filter(path -> path.toString().endsWith(".class"))
+        .walk(
+            classFile -> {


Might be good to put this in a separate method since it is getting to almost 6 levels of tabbing around line 91.

coollog · 2018-05-17T14:58:55Z

jib-core/src/main/java/com/google/cloud/tools/jib/frontend/MainClassFinder.java

+                      && main.getReturnType() == void.class
+                      && Modifier.isStatic(main.getModifiers())
+                      && Modifier.isPublic(main.getModifiers())) {
+                    results.add(name);


Hmm, is it possible to not have to do the path-to-class conversion and instead just get the class from the file via reflection?

Doesn't look like it, I think I need to pass the full name into the class loader.

I believe if you pass in null to defineClass it would resolve the class name from the file.

coollog · 2018-05-17T15:03:24Z

jib-core/src/main/java/com/google/cloud/tools/jib/frontend/MainClassFinder.java

-      }
-    }
+              Class<?> fileClass = new ClassFileLoader(classFile).findClass(name);
+              if (fileClass != null) {


An early return here would help reduce the tabbing.

coollog · 2018-05-17T15:06:45Z

jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/ProjectProperties.java

            "Could not find main class specified in a 'jar' task; attempting to "
                + "infer main class.");
+
+        final String mainClassSuggestion = "add a `mainClass` configuration to jib";


Oh by constant, I meant like a private static final field.

coollog · 2018-05-17T15:10:05Z

jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/ProjectProperties.java

-        }
-        if (mainClass == null) {
+          // Adds each file in each classes output directory to the classes files list.
+          JavaPluginConvention javaPluginConvention =


Oh I meant here to just directly use the classes files from SourceFilesConfiguration

It was either this or something like getSourceFilesConfiguration().getClassesFiles().get(0).getParent() since I need the root directory to resolve the class names correctly, and getClassesFiles seems to return a list of the files in the root directory. Plus using getSourceFilesConfiguration() was weird in testing, since I would either need to refactor a bit, or mock a bunch of the GradleSourceFilesConfiguration internals to avoid null pointer exceptions.

Okay, though we do need some way to guarantee that the classes we are searching in are the classes that will be in the container.

coollog · 2018-05-17T15:10:34Z

jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/ProjectProperties.java

+              project.getConvention().getPlugin(JavaPluginConvention.class);
+          SourceSet mainSourceSet = javaPluginConvention.getSourceSets().getByName("main");
+          Path classesDirs = Paths.get(mainSourceSet.getOutput().getClassesDirs().getAsPath());
+          List<String> mainClasses = new ArrayList<>(MainClassFinder.findMainClass(classesDirs));


The new ArrayList<> doesn't seem to be necessary.

coollog · 2018-05-17T15:12:11Z

jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/ProjectProperties.java

+            mainClass = mainClasses.get(0);
+          } else if (mainClasses.size() == 0) {
+            throw new GradleException(
+                HelpfulSuggestionsProvider.get("Main class was not found")


Since these messages are the same as in Maven, they can be added to HelpfulSuggestions.

coollog · 2018-05-17T15:18:19Z

jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/ProjectProperties.java

@@ -48,29 +51,36 @@ String getMainClass(@Nullable String mainClass) {
    if (mainClass == null) {


Since the logic is getting a bit more complex, it'd be good to add a description to the Javadoc like:

If {@code mainClass} is {@code null}, tries to infer main class in this order: 1. Looks in a {@code jar} task. 2. Searches for a class defined with a main method. Warns if main class is not valid.

coollog

Almost there!

coollog · 2018-05-17T17:03:40Z

jib-core/src/main/java/com/google/cloud/tools/jib/frontend/MainClassFinder.java

+                    && main.getReturnType() == void.class
+                    && Modifier.isStatic(main.getModifiers())
+                    && Modifier.isPublic(main.getModifiers())) {
+                  classNames.add(fileClass.getCanonicalName());


I believe you would want getName since the canonical name would replace a subclass like package.Hello$Main with package.Hello.Main

https://stackoverflow.com/questions/15202997/what-is-the-difference-between-canonical-name-simple-name-and-class-name-in-jav

K, wasn't sure which one we wanted.

coollog · 2018-05-17T17:06:48Z

jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/ProjectProperties.java


  ProjectProperties(Project project, GradleBuildLogger gradleBuildLogger) {
    this.project = project;
    this.gradleBuildLogger = gradleBuildLogger;
+    try {


Looks good!

This should prob go in a static method like ProjectProperties#getForProject since constructors generally should be lightweight.

coollog · 2018-05-17T17:12:10Z

jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/ProjectProperties.java

@@ -54,32 +78,29 @@ String getMainClass(@Nullable String mainClass) {
        gradleBuildLogger.debug(


I was talking to @patflynn and Patrick mentioned that it might be better to log something like Searching for main class... Add a 'mainClass' configuration to 'jib' to improve build speed. at the info level to help users in case they have a ton of classes to search.

coollog · 2018-05-17T17:15:14Z

jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/ProjectProperties.java

        try {
          List<String> mainClasses =
-              MainClassFinder.findMainClass(Paths.get(project.getBuild().getOutputDirectory()));
+              MainClassFinder.findMainClasses(Paths.get(project.getBuild().getOutputDirectory()));


This should use MavenSourceFilesConfiguration like you did in Gradle.

coollog · 2018-05-17T18:31:10Z

jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/ProjectProperties.java

-            HelpfulSuggestionsProvider.get("Could not find main class specified in a 'jar' task")
-                .suggest("add a `mainClass` configuration to jib"));
+        gradleBuildLogger.debug(
+            "Could not find main class specified in a 'jar' task; attempting to "


Let's add a todo to consolidate this code with the Maven one.

Already added it to #262, but I can add a TODO in the code as well if you want.

Okay that's fine then.

TadCordle added 5 commits May 15, 2018 16:36

Add core functionality

1aa6cdb

Add recursive search, add tests

8ac2af5

Fix ProjectProperties unit tests

b05a9a9

Use ClassLoader instead of ClassVisitory; use class inference in inte…

49b9e89

…gration tests

Comments and cleanup

0389ae2

TadCordle requested review from loosebazooka and coollog May 16, 2018 19:35

TadCordle added 3 commits May 16, 2018 15:52

Merge branch 'master' of github.com:GoogleContainerTools/jib into inf…

01a9824

…er_main_class

Merge branch 'master' of github.com:GoogleContainerTools/jib into inf…

daaf5d2

…er_main_class

Remove getLogger() from gradle ProjectProperties

60f3fc5

coollog reviewed May 16, 2018

View reviewed changes

TadCordle added 2 commits May 17, 2018 10:43

Feedback

eeeed9a

Merge branch 'master' of github.com:GoogleContainerTools/jib into inf…

21c1dfa

…er_main_class

coollog reviewed May 17, 2018

View reviewed changes

Feedback

bca760b

coollog reviewed May 17, 2018

View reviewed changes

Feedback

9d1c3f6

coollog approved these changes May 17, 2018

View reviewed changes

TadCordle merged commit ce40f40 into master May 17, 2018

TadCordle deleted the infer_main_class branch May 17, 2018 18:43

		@@ -48,29 +51,36 @@ String getMainClass(@Nullable String mainClass) {
		if (mainClass == null) {

		@@ -54,32 +78,29 @@ String getMainClass(@Nullable String mainClass) {
		gradleBuildLogger.debug(

Infer main class if it isn't configured anywhere #278

Infer main class if it isn't configured anywhere #278

Conversation

TadCordle commented May 16, 2018

coollog left a 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

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

coollog commented May 16, 2018 via email

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

TadCordle May 17, 2018 • 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

coollog left a 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

TadCordle May 17, 2018 •

edited

Loading