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

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

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?

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?

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.

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

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

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."

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?

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

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?

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

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

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?

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

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.

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

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

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?

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.

Attaching a patch for review.

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

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.

The patch looks good to me

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)

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

Fixed.

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.

I don't exactly understand why you do

+ if(cl instanceof URLClassLoader)

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);

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

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.

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.

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.

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?)

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.

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.*

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().

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.

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

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.

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.

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.