Apache S4
  1. Apache S4
  2. S4-66

S4R packaging: improve app class resolution

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.5.0
    • Fix Version/s: 0.6
    • Labels:
      None

      Description

      Currently, when creating an S4R package, the app name added to the manifest is extracted from the source code through a very brittle process ( see line 152 https://git-wip-us.apache.org/repos/asf?p=incubator-s4.git;a=blob;f=subprojects/s4-tools/src/main/resources/templates/build.gradle;h=a1e50e330c4de47928af38a57da17b050c0c62b5;hb=049a6b00b285039f36c2276079bfc2d037ca856b#l152 )

      We should either use a proper Java class to extract that name, or require the name of the app class to be passed as a parameter

        Issue Links

          Activity

          Hide
          Daniel Gómez Ferro added a comment -

          Thanks Aimee for the patch, +1

          I merged it in dev, commit 042431338a51c4481e0cad706de0b66deafe26ca

          Show
          Daniel Gómez Ferro added a comment - Thanks Aimee for the patch, +1 I merged it in dev, commit 042431338a51c4481e0cad706de0b66deafe26ca
          Hide
          Aimee Cheng added a comment -

          Hi Matthieu, I updated the patch. Now use the class loader to load the app Class and check in gradle script. And I removed the getAppClassName Please help to check it.

          Show
          Aimee Cheng added a comment - Hi Matthieu, I updated the patch. Now use the class loader to load the app Class and check in gradle script. And I removed the getAppClassName Please help to check it.
          Hide
          Matthieu Morel added a comment -

          Thanks for pointing this out Aimee.

          Actually we should get rid of the getAppClassName method in the gradle build, since its implementation is too weak (see the TODO). I forgot to remove the method apparently!

          If we want to check the existence of the app class, a better option would be to simply do use the class name passed as a required parameter, and try to resolve it with a Class.forName() in the gradle script. If that fails, fail the build. I'm not exactly sure how to achieve this in gradle however, maybe you have an idea?

          Show
          Matthieu Morel added a comment - Thanks for pointing this out Aimee. Actually we should get rid of the getAppClassName method in the gradle build, since its implementation is too weak (see the TODO). I forgot to remove the method apparently! If we want to check the existence of the app class, a better option would be to simply do use the class name passed as a required parameter, and try to resolve it with a Class.forName() in the gradle script. If that fails, fail the build. I'm not exactly sure how to achieve this in gradle however, maybe you have an idea?
          Hide
          Aimee Cheng added a comment -

          Seems S4-118 duplicated with this ticket.

          But the latest commit in S4-66 still doesn't check whether the user defined appClass exists or extends from App. If not exists or not extends from App, the build process need be stopped. So I think we still need code in build.gradle to check this.

          Show
          Aimee Cheng added a comment - Seems S4-118 duplicated with this ticket. But the latest commit in S4-66 still doesn't check whether the user defined appClass exists or extends from App. If not exists or not extends from App, the build process need be stopped. So I think we still need code in build.gradle to check this.
          Hide
          Matthieu Morel added a comment -

          Uploaded patch in branch S4-66 commit 60a75a078603125a28395ae0b4e99c751e18b5b8

          1. Got rid of the code for automatically finding the app class in the s4r
          2. Forced appClass parameter to be specified in s4r command
          3. Removed -generatedS4R option from deploy command, in order to simplify the commands and clearly separate tasks

          Show
          Matthieu Morel added a comment - Uploaded patch in branch S4-66 commit 60a75a078603125a28395ae0b4e99c751e18b5b8 1. Got rid of the code for automatically finding the app class in the s4r 2. Forced appClass parameter to be specified in s4r command 3. Removed -generatedS4R option from deploy command, in order to simplify the commands and clearly separate tasks
          Hide
          Matthieu Morel added a comment -

          Forcing the appClass to be specified sounds like the safest option

          Show
          Matthieu Morel added a comment - Forcing the appClass to be specified sounds like the safest option
          Hide
          Matthieu Morel added a comment -

          Postponing to 0.6

          In the meantime, -appClass parameter allows to explicitly specify the application class (though it's not checked yet).

          Show
          Matthieu Morel added a comment - Postponing to 0.6 In the meantime, -appClass parameter allows to explicitly specify the application class (though it's not checked yet).
          Hide
          Matthieu Morel added a comment -

          Actually I'm more leaning towards a tool that would work with compiled classes, probably easier and safer.

          Show
          Matthieu Morel added a comment - Actually I'm more leaning towards a tool that would work with compiled classes, probably easier and safer.
          Hide
          Mariano Valles added a comment -

          Is the idea to include a simple Java class for parsing? or to use some kind of already made parser?
          (e.g. http://code.google.com/p/javaparser/)

          If it's the first one: How can that be better than improving the Groovy function to exclude comments and corner cases? It would still be parsing the Java source code in any case.

          Show
          Mariano Valles added a comment - Is the idea to include a simple Java class for parsing? or to use some kind of already made parser? (e.g. http://code.google.com/p/javaparser/ ) If it's the first one: How can that be better than improving the Groovy function to exclude comments and corner cases? It would still be parsing the Java source code in any case.

            People

            • Assignee:
              Matthieu Morel
              Reporter:
              Matthieu Morel
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development