Bug 536207 (RHQ-582)

mazz, we had a very long conversation about this very topic, resulting in a few code changes.

ian s: 

The version attribute is currently optional in rhq-plugin.xsd. If not specified, the version ends up being null in the rhq_plugin table. in 1.0.1, I changed the algorithm that determines the version from a plugin jar to the following:

1) check MANIFEST.MF for Implementation-Version header
2) check the plugin descriptor for a version attribute
3) default to "1.0" 

I think the plugin version should be one in the same as the Maven pom version (i.e. the jar's Impl-Version). 

jay s:

Ah, OK. So it's quite OK if there is no version set in the plugin descriptor.  This makes sense to me, especially for plugins we don't write, but I would be in favor of us setting a version for all of our plugins.  

Actually, It seems odd to me that the version set in the descriptor is overridden by the MANIFEST.MF Implementation-Version header.  I would have expected an explicit version to take precedence.  If it stays like this then perhaps we should remove all <version> elements from the plugin descriptors. 

jay d:

+1

Everything else is in the plugin descriptor; I don't like the idea of telling plugin writers they have to manage the version of the plugin outside of it.

heiko r:

+1 

jay d:

If by some chance we do move away from the descriptor's version, we should remove it entirely. It's too easy to get these confused and botch it up having it in two places. An optional version in and of itself is usually a bad idea.

heiko r:

Yes. And two places to look for stuff is not good either. 

ian s:

The reason I wrote the code to favor the MANIFEST version over the descriptor version is because is for backwards compatibility with old/existing plugins, which had bogus versions in their plugin descriptors that didn't map to the jar version (e.g. jar version was 1.0.0.GA, but plugin version was 2.0). That is, I wanted to ensure that if one of the old plugin jars was dropped into an updated version, its jar version would get used instead of its bogus descriptor version, in case its version needed to be compared with the version of a newer version of the same jar.

However... since we are telling people to always install a new version of the Server to a fresh directory, rather than overlaying it on top of the previous version, we are assured there won't be old plugin jars kicking around in the rhq-plugins dir. A user would have to go out of their way to copy one of the old plugin jars into their new install. If we assume that will never happen, we are safe using only the plugin descriptor to determine the version (as long as we update all the descriptors to use version="${project.version}"). 

jay s:

Ian, this makes sense to me. I can see why it is like it is. I don't have a problem using the MANIFEST, it's easy because the version we want is set for us pretty much automatically, relieving us of having to set it in the plugin-descriptor.  I'm in favor of us removing <version> from all of our plugin descriptors and letting the version come from the MANIFEST.  I'm opposed to setting <version> to ${project.version} (and adding filtering so it gets replaced), just to end up with the same version.

I do think we may want to reverse the logic, though, to favor the plugin descriptor if, and when, it may make sense to override the MANIFEST setting.

Finally, I'm in favor or requiring a version be set in one of the two places, never defaulting to a null version.

I am making the change to remove <version> from our plugin descriptors and will check it in shortly unless there is some opposition. This change works regardless of the preference ordering because our MANIFEST settings are fine. 

ian s:

Before you do, let's wait for Jay and/or others to weigh in. Here's option B:

1) make the version attribute required in the plugin descriptor
2) only check the plugin descriptor
3) update all our plugin descriptors to use version="${project.version}"

Whether we go with option A or option B, I agree - let's abort deploy of the plugin if we can't determine a version. 

jay d:

Just going on record as saying I don't think we should remove it from the descriptor entirely. If anything, I'd be up for changing the priority logic to give it more weight than the MANIFEST, but that's just me.

jay s:

+1 or giving the plugin descriptor priority assuming we keep the MANIFEST version in the picture.  But I don't mind defaulting to the MANIFEST impl-version and leaving <version> out of the PD if all it would do is set the same version we have in the MANIFEST.  Obviously, if we just use PD then we should set it the <version> to ${project.version}.

joe m:

Have we considered what this means for external plugin developers?  What will their end-to-end experience with
platform look like when all is said and done for each of the various options at hand.  IMHO, the platform was written for them, not for us...so the best option is always going to be the one that gives the plugin developers the most power / best solution / etc.  In your guys' opinions, which option is that? 

ian s:

I think option A (what JayShaun suggested) gives the most ease of use and flexibility. The version attribute is optional in the descriptor. If not specified, the MANIFEST version will be used. But we keep it, to give plugin developers the ability to not use the MANIFEST version if they don't want to for some reason (or perhaps don't include MANIFEST's in their jars at all for some wonky reason). 

greg h:

+1 

jay d:

Ya, i thought this whole thread was talking about what would be easiest on the plugin writer, with things like confusion about the priority and consistency in where we ask them to provide information (i.e. all in the descriptor or elsewhere). By now people know where I stand   :) 

jay s:

r984/r10035 - Removing <version> from all plugin descriptors. This will default the version to that set in the MANIFEST impl-version.  If we decide to I'll change this such that all PDs have <version> set to ${project.version}, but I think we're leaning this way.  Either way we needed to change what is there, this is a step towards consistency. 

ian s:

r985 makes the code changes discussed. see my most recent comment in http://jira.rhq-project.org/browse/RHQ-412. 

This was actually fixed back in June.  Now it works as follows:

if the version in the descriptor is missing, then a DEBUG-level message is printed to the log and the 'implementation version' attribute from the manifest.mf file inside the plugin jar is used instead.  if that is also missing, then a DeploymentException is thrown.

With no version in either manifest or descriptor: 

--- Incompletely deployed packages ---
org.jboss.deployment.DeploymentInfo@a6cf1d62 { url=file:/usr/local/jon/server/jbossas/server/default/deploy/rhq.ear/rhq-downloads/rhq-plugins/rhq-iis-plugin-1.1.0-SNAPSHOT.jar }
  deployer: org.rhq.enterprise.server.core.plugin.ProductPluginDeployer@161d282
  status: Deployment FAILED reason: No version is defined for plugin jar '/usr/local/jon/server/jbossas/server/default/deploy/rhq.ear/rhq-downloads/rhq-plugins/rhq-iis-plugin-1.1.0-SNAPSHOT.jar'. A version must be defined either via the MANIFEST.MF 'Implementation-Version' attribute or via the plugin descriptor 'version' attribute.
  state: FAILED
  watch: file:/usr/local/jon/server/jbossas/server/default/deploy/rhq.ear/rhq-downloads/rhq-plugins/rhq-iis-plugin-1.1.0-SNAPSHOT.jar
  altDD: null
  lastDeployed: 1218560876143
  lastModified: 1218560876000
  mbeans:

With versions in both, it uses the descriptor version:
Version: 	jeff-descriptor-version1.2

With just the descriptor version, it uses that

With just the manifest version it uses that

All of these are the expected behavior.  

Tested on linux/postgres/qa build 6170.  used the IIS plugin for testing

This bug was previously known as http://jira.rhq-project.org/browse/RHQ-582