groovy
  1. groovy
  2. GROOVY-3884

InvokerHelper.getVersion() should also work on Google App Engine

    Details

    • Type: Improvement Improvement
    • Status: Closed Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.7-beta-2
    • Fix Version/s: 1.6.6, 1.7-rc-1
    • Component/s: None
    • Labels:
      None
    • Number of attachments :
      4

      Description

      Right now, InvokerHelper.getVersion() does nothing on GAE.
      Talking about version numbers, I suggest to create a GroovyVersion class that is focused on version numbers and is clearly part of the public API. People shouldn't have to use InvokerHelper to retrieve the version number.

      1. 3884_v17x_GroovyVersion_Patch.txt
        6 kB
        Roshan Dawrani
      2. 3884_v17x_v2.txt
        6 kB
        Roshan Dawrani
      3. 3884_v17x_v3.txt
        9 kB
        Roshan Dawrani
      4. 3884_v17x.txt
        5 kB
        Roshan Dawrani

        Activity

        Hide
        blackdrag blackdrag added a comment -

        which means the version number should be available through groovy.lang.GroovySystem then I would say

        Show
        blackdrag blackdrag added a comment - which means the version number should be available through groovy.lang.GroovySystem then I would say
        Hide
        Roshan Dawrani added a comment - - edited

        Why doesn't InvokerHelper.getVersion() do anything on GAE? Are there any restrictions on classloader usage there (which the version getting implementation is based on)? Is that why?

        Show
        Roshan Dawrani added a comment - - edited Why doesn't InvokerHelper.getVersion() do anything on GAE? Are there any restrictions on classloader usage there (which the version getting implementation is based on)? Is that why?
        Hide
        Roshan Dawrani added a comment - - edited

        To try out getting the groovy version on GAE, I uploaded a test app today on GAE with groovy-1.7-rc-1 lib and tried the following:

        println "${GroovyObject.class}" // == 'interface groovy.lang.GroovyObject'
        println "${GroovyObject.class.package}" // == 'package groovy.lang'
        println "${GroovyObject.class.package.implementationVersion}" // == null
        

        If I locally try the same output, then I get:

        println "${GroovyObject.class}" // == 'interface groovy.lang.GroovyObject'
        println "${GroovyObject.class.package}" // == 'package groovy.lang, Groovy: a powerful, dynamic language for the JVM, version 1.7-rc-1-SNAPSHOT'
        println "${GroovyObject.class.package.implementationVersion}" // == '1.7-rc-1-SNAPSHOT'
        

        I think the difference is made by the classloader used by GAE. Locally, when a class from "groovy.lang" package is first accessed, I see that the classloader defines the package "groovy.lang" (used later to get groovy version) with the various details from groovy jar's MANIFEST.MF and stuff like vendor name, version, title, etc get correctly set on the package name.

        However, if the GAE classloader doesn't do that (doesn't define the packages with details from MANIFEST.MF), then I don't think groovy can do anything about it, can it?

        Show
        Roshan Dawrani added a comment - - edited To try out getting the groovy version on GAE, I uploaded a test app today on GAE with groovy-1.7-rc-1 lib and tried the following: println "${GroovyObject.class}" // == ' interface groovy.lang.GroovyObject' println "${GroovyObject.class. package }" // == ' package groovy.lang' println "${GroovyObject.class. package .implementationVersion}" // == null If I locally try the same output, then I get: println "${GroovyObject.class}" // == ' interface groovy.lang.GroovyObject' println "${GroovyObject.class. package }" // == ' package groovy.lang, Groovy: a powerful, dynamic language for the JVM, version 1.7-rc-1-SNAPSHOT' println "${GroovyObject.class. package .implementationVersion}" // == '1.7-rc-1-SNAPSHOT' I think the difference is made by the classloader used by GAE. Locally, when a class from "groovy.lang" package is first accessed, I see that the classloader defines the package "groovy.lang" (used later to get groovy version) with the various details from groovy jar's MANIFEST.MF and stuff like vendor name, version, title, etc get correctly set on the package name. However, if the GAE classloader doesn't do that (doesn't define the packages with details from MANIFEST.MF), then I don't think groovy can do anything about it, can it?
        Hide
        Peter Niederwieser added a comment -

        What Groovy could do is to use a different way to store and/or retrieve its version number. But first we should ask the GAE folks if what we are seeing matches their expectations.

        Show
        Peter Niederwieser added a comment - What Groovy could do is to use a different way to store and/or retrieve its version number. But first we should ask the GAE folks if what we are seeing matches their expectations.
        Hide
        Roshan Dawrani added a comment -

        Ask GAE folks about it? That is a good idea. I will try that and post back when I get something more on it.

        Show
        Roshan Dawrani added a comment - Ask GAE folks about it? That is a good idea. I will try that and post back when I get something more on it.
        Hide
        Guillaume Laforge added a comment -

        I sent an email to one of my contacts in the GAE team. We'll see if he can shed some light on this.

        Show
        Guillaume Laforge added a comment - I sent an email to one of my contacts in the GAE team. We'll see if he can shed some light on this.
        Hide
        Guillaume Laforge added a comment -

        Answer I got from the GAE team:

        "Currently, GAE ignores custom implementations of ClassLoader.definePackage. If you can rely on URLClassLoader to define the package for you (which it does from findClass), then properties like implementation version, etc... will be set correctly from a manifest.

        It might be possible for us to loosen this restriction down the road, but it's not currently a high priority."

        Show
        Guillaume Laforge added a comment - Answer I got from the GAE team: "Currently, GAE ignores custom implementations of ClassLoader.definePackage. If you can rely on URLClassLoader to define the package for you (which it does from findClass), then properties like implementation version, etc... will be set correctly from a manifest. It might be possible for us to loosen this restriction down the road, but it's not currently a high priority."
        Hide
        Roshan Dawrani added a comment -

        Thanks for getting the info, Guillaume. We at least a confirmation now that it is due to classloader restrictions of GAE.

        In standalone groovy execution, it is sun.misc.Launcher$AppClassLoader (extends URLClassLoader),
        which is defining the package with all manifest details. So, I don't think relying on URLClassLoader is an option.

        As Peter suggested, we should probably switch to a classloader-free way of getting the groovy version. Shall I proceed
        on those lines and give it a try?

        Show
        Roshan Dawrani added a comment - Thanks for getting the info, Guillaume. We at least a confirmation now that it is due to classloader restrictions of GAE. In standalone groovy execution, it is sun.misc.Launcher$AppClassLoader (extends URLClassLoader), which is defining the package with all manifest details. So, I don't think relying on URLClassLoader is an option. As Peter suggested, we should probably switch to a classloader-free way of getting the groovy version. Shall I proceed on those lines and give it a try?
        Hide
        Guillaume Laforge added a comment -

        Yup, you can proceed with a version that fits the constraints.

        Show
        Guillaume Laforge added a comment - Yup, you can proceed with a version that fits the constraints.
        Hide
        blackdrag blackdrag added a comment -

        but what is a way that does not depend on the classloader? A file that we read in and still have to find through the class loader? A hard coded version information?

        Show
        blackdrag blackdrag added a comment - but what is a way that does not depend on the classloader? A file that we read in and still have to find through the class loader? A hard coded version information?
        Hide
        Peter Niederwieser added a comment -

        I suggest to read the version number from a properties file (through the class loader). If necessary, the properties file could be generated by the Ant build. Might be a hassle when building with IDE though.
        Interesting discussion on this topic: http://stackoverflow.com/questions/690419/build-and-version-numbering-for-java-projects-ant-cvs-hudson

        Show
        Peter Niederwieser added a comment - I suggest to read the version number from a properties file (through the class loader). If necessary, the properties file could be generated by the Ant build. Might be a hassle when building with IDE though. Interesting discussion on this topic: http://stackoverflow.com/questions/690419/build-and-version-numbering-for-java-projects-ant-cvs-hudson
        Hide
        Guillaume Laforge added a comment -

        Can't we use the manifest file for that purpose?
        Isn't that done more or less automatically?
        http://java.sun.com/docs/books/tutorial/deployment/jar/packageman.html

        Show
        Guillaume Laforge added a comment - Can't we use the manifest file for that purpose? Isn't that done more or less automatically? http://java.sun.com/docs/books/tutorial/deployment/jar/packageman.html
        Hide
        Peter Niederwieser added a comment -

        The manifest certainly contains all the information we need. But how do we find it at runtime? Search through all manifests found on the class path?

        Show
        Peter Niederwieser added a comment - The manifest certainly contains all the information we need. But how do we find it at runtime? Search through all manifests found on the class path?
        Hide
        Peter Niederwieser added a comment -

        With a custom properties file, we could also include things like SVN revision number and build time (if we wanted to).

        Show
        Peter Niederwieser added a comment - With a custom properties file, we could also include things like SVN revision number and build time (if we wanted to).
        Hide
        Guillaume Laforge added a comment -

        What I meant is that I thought there was a convention followed by Java that automatically picks up the package version from the JAR manifest.

        Show
        Guillaume Laforge added a comment - What I meant is that I thought there was a convention followed by Java that automatically picks up the package version from the JAR manifest.
        Hide
        Peter Niederwieser added a comment -

        Yes, there is Package.getImplementationVersion(). That's what InvokerHelper uses right now.

        Show
        Peter Niederwieser added a comment - Yes, there is Package.getImplementationVersion(). That's what InvokerHelper uses right now.
        Hide
        Peter Niederwieser added a comment -

        By now I believe we might be hitting a GAE bug. Will investigate further. See http://groups.google.com/group/google-appengine-java/browse_thread/thread/27a558895036b982

        Show
        Peter Niederwieser added a comment - By now I believe we might be hitting a GAE bug. Will investigate further. See http://groups.google.com/group/google-appengine-java/browse_thread/thread/27a558895036b982
        Hide
        Roshan Dawrani added a comment -

        But even if it turns out to be a GAE bug, shouldn't it still change to some version-info.properties kind of thing?

        If it is on the mercy of classloader's definePackage() behavior, if not in GAE, some other container may again have that issue.

        It's quite common for these container's to use their custom class loaders, isn't it? Why depend on their differences in treating definePackage() call?

        Show
        Roshan Dawrani added a comment - But even if it turns out to be a GAE bug, shouldn't it still change to some version-info.properties kind of thing? If it is on the mercy of classloader's definePackage() behavior, if not in GAE, some other container may again have that issue. It's quite common for these container's to use their custom class loaders, isn't it? Why depend on their differences in treating definePackage() call?
        Hide
        Peter Niederwieser added a comment -

        It's probably true that Package.getImplementationVersion() won't work everywhere. JavaDoc of class SpringVersion, which essentially uses the same approach, says:

        Note that some ClassLoaders do not expose the package metadata, hence this class might not be able to determine the Spring version in all environments.

        I'm certainly OK with a solution based on a properties file, in particular because it could convey other useful information like the build date/time.

        Show
        Peter Niederwieser added a comment - It's probably true that Package.getImplementationVersion() won't work everywhere. JavaDoc of class SpringVersion, which essentially uses the same approach, says: Note that some ClassLoaders do not expose the package metadata, hence this class might not be able to determine the Spring version in all environments. I'm certainly OK with a solution based on a properties file, in particular because it could convey other useful information like the build date/time.
        Hide
        Roshan Dawrani added a comment -

        Attaching a patch for review.

        I have tested it locally and on GAE (http://roshandawrani.appspot.com/).

        Show
        Roshan Dawrani added a comment - Attaching a patch for review. I have tested it locally and on GAE ( http://roshandawrani.appspot.com/ ).
        Hide
        Roshan Dawrani added a comment -

        Attaching V2 of the patch. I have just re-organized a bit and moved all the release-info code from InvokerHelper to a small ReleaseInfo class.

        Show
        Roshan Dawrani added a comment - Attaching V2 of the patch. I have just re-organized a bit and moved all the release-info code from InvokerHelper to a small ReleaseInfo class.
        Hide
        Guillaume Laforge added a comment -

        The patch looks good to me

        Show
        Guillaume Laforge added a comment - The patch looks good to me
        Hide
        Roshan Dawrani added a comment -

        2 small questions:

        1) It can also be ported to 1.6.x, right?

        2) Does InvokerHelper.getVersion() need to be marked deprecated? On 1.6.x? On 1.7.x? (need to know for both)

        Show
        Roshan Dawrani added a comment - 2 small questions: 1) It can also be ported to 1.6.x, right? 2) Does InvokerHelper.getVersion() need to be marked deprecated? On 1.6.x? On 1.7.x? (need to know for both)
        Hide
        Guillaume Laforge added a comment -

        1) Yes
        2) You can mark it as deprecated on 1.7, but I would let it non-deprecated on 1.6.

        Show
        Guillaume Laforge added a comment - 1) Yes 2) You can mark it as deprecated on 1.7, but I would let it non-deprecated on 1.6.
        Hide
        Roshan Dawrani added a comment -

        Fixed.

        Show
        Roshan Dawrani added a comment - Fixed.
        Hide
        Roshan Dawrani added a comment -

        Attaching a small patch that is incorporating some feedback from the dev mailing list.

        • Use findResource() instead of getResourceAsStream() to avoid potential clashes with parent class-loaders and bootstrap one.
        • Rename the custom properties to make more groovy specific - again to reduce the chances of possible clashes.
        • Minor changes to internally switch from the deprecated InvokerHelper#getVersion() to new GroovySystem#getVersion()

        I have tested this patch locally and on GAE.

        Show
        Roshan Dawrani added a comment - Attaching a small patch that is incorporating some feedback from the dev mailing list. Use findResource() instead of getResourceAsStream() to avoid potential clashes with parent class-loaders and bootstrap one. Rename the custom properties to make more groovy specific - again to reduce the chances of possible clashes. Minor changes to internally switch from the deprecated InvokerHelper#getVersion() to new GroovySystem#getVersion() I have tested this patch locally and on GAE.
        Hide
        blackdrag blackdrag added a comment -

        I don't exactly understand why you do

        + if(cl instanceof URLClassLoader)

        { + // this avoids going through the parent classloaders/bootstarp + url = ((URLClassLoader) cl).findResource(RELEASE_INFO_FILE); + }

        findResource is available on a normal ClassLoader as well. Instead I suggest not doing the instanceof check and calling it on cl, but in a try-catch. And if that fails we use

        + // fallback option as ClassLoader#findResource() is protected
        + url = cl.getResource(RELEASE_INFO_FILE);

        Show
        blackdrag blackdrag added a comment - I don't exactly understand why you do + if(cl instanceof URLClassLoader) { + // this avoids going through the parent classloaders/bootstarp + url = ((URLClassLoader) cl).findResource(RELEASE_INFO_FILE); + } findResource is available on a normal ClassLoader as well. Instead I suggest not doing the instanceof check and calling it on cl, but in a try-catch. And if that fails we use + // fallback option as ClassLoader#findResource() is protected + url = cl.getResource(RELEASE_INFO_FILE);
        Hide
        Roshan Dawrani added a comment -

        findResource() on the normal ClassLoader is protected. I mentioned that in the comments too (that u have copied above).

        How can I call that ClassLoader#findResource() from org.codehaus.groovy.util.ReleaseInfo that has no inheritance relation
        with java.lang.ClassLoader?

        Show
        Roshan Dawrani added a comment - findResource() on the normal ClassLoader is protected. I mentioned that in the comments too (that u have copied above). How can I call that ClassLoader#findResource() from org.codehaus.groovy.util.ReleaseInfo that has no inheritance relation with java.lang.ClassLoader?
        Hide
        blackdrag blackdrag added a comment -

        I didn't read the comment it seems, bad me. Then leave it as it is.

        Show
        blackdrag blackdrag added a comment - I didn't read the comment it seems, bad me. Then leave it as it is.
        Hide
        Peter Niederwieser added a comment -

        I still think it would be much nicer to have a GroovyVersion class, implementing Comparable<GroovyVersion> and with methods like getMajor(), getMinor(), toString(), parse(), etc.

        Show
        Peter Niederwieser added a comment - I still think it would be much nicer to have a GroovyVersion class, implementing Comparable<GroovyVersion> and with methods like getMajor(), getMinor(), toString(), parse(), etc.
        Hide
        Roshan Dawrani added a comment -

        That part of the JIRA got a bit lost in the GAE/classloader related discussions, I guess.

        Say, is there an urgent need for that (the releases being today)?

        If "String GroovySystem.getVersion()" can remain as is and we can change "String ReleaseInfo.getVersion()" to "GroovyVersion ReleaseInfo.getVersion()", then it is something that can be done a bit further down.

        But if it makes more sense to change "String GroovySystem.getVersion()" to "GroovyVersion GroovySystem.getVersion()", then
        the best time to do it is today, before it officially becomes part of the public API.

        Show
        Roshan Dawrani added a comment - That part of the JIRA got a bit lost in the GAE/classloader related discussions, I guess. Say, is there an urgent need for that (the releases being today)? If "String GroovySystem.getVersion()" can remain as is and we can change "String ReleaseInfo.getVersion()" to "GroovyVersion ReleaseInfo.getVersion()", then it is something that can be done a bit further down. But if it makes more sense to change "String GroovySystem.getVersion()" to "GroovyVersion GroovySystem.getVersion()", then the best time to do it is today, before it officially becomes part of the public API.
        Hide
        Roshan Dawrani added a comment -

        Ok, attaching a patch for the GroovyVersion requirement.

        The groovy version string is assumed to be of the format <major>.<minor>[.<micro>][-<qulifier>] - I have added tests for various version strings based on this format.

        I'd like to push the change in today before the release, so kindly take out a few minutes to share the feedback.

        Show
        Roshan Dawrani added a comment - Ok, attaching a patch for the GroovyVersion requirement. The groovy version string is assumed to be of the format <major>.<minor> [.<micro>] [-<qulifier>] - I have added tests for various version strings based on this format. I'd like to push the change in today before the release, so kindly take out a few minutes to share the feedback.
        Hide
        Guillaume Laforge added a comment -

        Looks good to me, even if I think following Peter's idea may be going a bit too far on the over-engineering branch... (do we really need that?)

        Show
        Guillaume Laforge added a comment - Looks good to me, even if I think following Peter's idea may be going a bit too far on the over-engineering branch... (do we really need that?)
        Hide
        Roshan Dawrani added a comment -

        If we are not looking at changing "String GroovySystem.getVersion()" signature, then it really becomes an internal
        implementation details and then there is really no rush to do it today itself. I just made it ready anyway as
        getting time later in the day was not an option.

        Show
        Roshan Dawrani added a comment - If we are not looking at changing "String GroovySystem.getVersion()" signature, then it really becomes an internal implementation details and then there is really no rush to do it today itself. I just made it ready anyway as getting time later in the day was not an option.
        Hide
        blackdrag blackdrag added a comment -

        One part you guys are missing is that changing "String GroovySystem.getVersion()" to "GroovyVersion GroovySystem.getVersion()", then it is a breaking change to be done either today or not at all, since it is in groovy.*

        Show
        blackdrag blackdrag added a comment - One part you guys are missing is that changing "String GroovySystem.getVersion()" to "GroovyVersion GroovySystem.getVersion()", then it is a breaking change to be done either today or not at all, since it is in groovy.*
        Hide
        Peter Niederwieser added a comment -

        It's not a must in my book (Roshan is too fast ), but I think it would help to compare actual version against a range of supported versions (e.g. for Spock).
        If we add GroovyVersion, I wouldn't want to get it from GroovySystem (why couple these two classes?). Instead, I'd go for something like GroovyVersion.getCurrentVersion().

        Show
        Peter Niederwieser added a comment - It's not a must in my book (Roshan is too fast ), but I think it would help to compare actual version against a range of supported versions (e.g. for Spock). If we add GroovyVersion, I wouldn't want to get it from GroovySystem (why couple these two classes?). Instead, I'd go for something like GroovyVersion.getCurrentVersion().
        Hide
        Roshan Dawrani added a comment -

        Most of the suggestions from you guys make sense - it's just that one is slightly better than the other.

        While GroovySystem.getVersion() seemed fine, GroovyVersion.getCurrentVersion() seems even better.

        In the current patch, GroovyVersion is in org.codehaus.groovy.*, if we want the version from GroovyVersion, it should
        come out on the public API.

        Most of the things are in place now - we just need to freeze its face on the public API. Otherwise, by default, it will
        remain as GroovySystem.getVersion() as that is what is in the codebase right now.

        Please let me know if GroovyVersion and its functionality in today's patch are needed.

        Show
        Roshan Dawrani added a comment - Most of the suggestions from you guys make sense - it's just that one is slightly better than the other. While GroovySystem.getVersion() seemed fine, GroovyVersion.getCurrentVersion() seems even better. In the current patch, GroovyVersion is in org.codehaus.groovy.*, if we want the version from GroovyVersion, it should come out on the public API. Most of the things are in place now - we just need to freeze its face on the public API. Otherwise, by default, it will remain as GroovySystem.getVersion() as that is what is in the codebase right now. Please let me know if GroovyVersion and its functionality in today's patch are needed.
        Hide
        blackdrag blackdrag added a comment -

        how about doing now: GroovySystem.getVersionString() and decide for doing its own class later? Then we can still add a getVersion() method, a new class, or whatever

        Show
        blackdrag blackdrag added a comment - how about doing now: GroovySystem.getVersionString() and decide for doing its own class later? Then we can still add a getVersion() method, a new class, or whatever
        Hide
        Peter Niederwieser added a comment -

        If we postpone GroovyVersion, I suggest to stick to InvokerHelper.getVersion() for now, and just change the implementation. Merely "moving" the method to GroovySystem doesn't add much value. Better introduce GroovyVersion at some point.

        Show
        Peter Niederwieser added a comment - If we postpone GroovyVersion, I suggest to stick to InvokerHelper.getVersion() for now, and just change the implementation. Merely "moving" the method to GroovySystem doesn't add much value. Better introduce GroovyVersion at some point.
        Hide
        Roshan Dawrani added a comment -

        Every individual piece of it is ready.

        We can have GroovyVersion.getCurrentVersion() on the public API (with InvokerHelper.getVersion() deprecated for 1.7.x). With 1.7-rc-1 also, we should not have people access internal classes to access the version.

        One way or another, access point should be a public API class, I think.

        Show
        Roshan Dawrani added a comment - Every individual piece of it is ready. We can have GroovyVersion.getCurrentVersion() on the public API (with InvokerHelper.getVersion() deprecated for 1.7.x). With 1.7-rc-1 also, we should not have people access internal classes to access the version. One way or another, access point should be a public API class, I think.
        Hide
        Roshan Dawrani added a comment -

        I will not make any further changes on it unless a clear message comes out on this - which means that "String GroovySystem.getVersion()" will remain in API.

        Show
        Roshan Dawrani added a comment - I will not make any further changes on it unless a clear message comes out on this - which means that "String GroovySystem.getVersion()" will remain in API.

          People

          • Assignee:
            Roshan Dawrani
            Reporter:
            Peter Niederwieser
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: