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

          Matthieu Morel created issue -
          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.
          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.
          Tony Stevenson made changes -
          Field Original Value New Value
          Workflow jira [ 12708781 ] no-reopen-closed, patch-avail [ 12711351 ]
          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).
          Matthieu Morel made changes -
          Fix Version/s 0.6 [ 12321702 ]
          Fix Version/s 0.5 [ 12318653 ]
          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 -

          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
          Matthieu Morel made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Assignee Matthieu Morel [ mmorel ]
          Matthieu Morel made changes -
          Link This issue is duplicated by S4-79 [ S4-79 ]
          Aimee Cheng made changes -
          Link This issue is duplicated by S4-118 [ S4-118 ]
          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.
          Aimee Cheng made changes -
          Attachment 0001-check-whether-defined-app-class-exists-and-extends-A.patch [ 12571805 ]
          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?
          Aimee Cheng made changes -
          Attachment 0001-check-whether-defined-app-class-exists-and-extends-A.patch [ 12571805 ]
          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.
          Aimee Cheng made changes -
          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
          Daniel Gómez Ferro made changes -
          Status Patch Available [ 10002 ] Resolved [ 5 ]
          Resolution Fixed [ 1 ]
          Transition Time In Source Status Execution Times Last Executer Last Execution Date
          Open Open Patch Available Patch Available
          245d 8h 51m 1 Matthieu Morel 27/Feb/13 18:45
          Patch Available Patch Available Resolved Resolved
          10d 1h 42m 1 Daniel Gómez Ferro 09/Mar/13 20:27

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development