Bugzilla – Full Text Bug Listing
| Summary: | Adopt R3.4 Attribute and Directive name syntax (Was: Deployer fails with split packages) | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [RT] Virgo | Reporter: | Benjamin Muskalla <b.muskalla> | ||||||||||||
| Component: | unknown | Assignee: | Steve Powell <zteve.powell> | ||||||||||||
| Status: | CLOSED FIXED | QA Contact: | |||||||||||||
| Severity: | enhancement | ||||||||||||||
| Priority: | P3 | CC: | glyn.normington, holger.staudacher, jeffmcaffer, tjwatson, zteve.powell | ||||||||||||
| Version: | 2.1.0.M01 | ||||||||||||||
| Target Milestone: | --- | ||||||||||||||
| Hardware: | All | ||||||||||||||
| OS: | All | ||||||||||||||
| Whiteboard: | |||||||||||||||
| Bug Depends on: | |||||||||||||||
| Bug Blocks: | 324445 | ||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Benjamin Muskalla
Created attachment 173403 [details]
example
Two bundles using split packages to reproduce the exception
The diagnostics could certainly be better, but the root cause of this failure is a syntax error in the manifest. The attribute name "nasty.split" does not conform to the syntax in the OSGi core specification which allows only alphanumeric, '_' and '-' in attribute names (tokens). Refer to section 1.3.2 of the core spec for the correct syntax. Please note that a split package need not have attributes. Typically a split package is exported by two or more bundles and gathered together by another bundle using require-bundle. You're right, according to the spec the attribute name is illegal. The problem is that Equinox accepts "nasty.split" as a valid split package and RCP enforces this with its splits (ui.workbench and ui.split). I know it feels wrong but do you think Virgo could support this false notation? On the RCP/RAP side we cannot change the attribute as we would break all clients out there as most of them use Import-Package. I'm very uncomfortable with making Virgo tolerate erroneous bundles. The clients using the invalid syntax in import-bundle are *already* broken, which will become apparent if those bundles are installed in an OSGi framework which is faithful to the spec. for attribute names. I suggest renaming attribute names like "nasty.split" to something valid like "nasty-split" and providing a conversion utility to help clients perform the necessary renaming. I know this suggestion seems very costly, but it is a one-time hit and it only affects users of split packages who are already living on a knife-edge. Interesting that this has gone undetected for some time. Must not be any OSGi TCK tests for this particular part of the spec... Some thoughts: - changing the name of the split attributes would be a breaking API change. Anyone importing the packages explicitly will be broken. This is not widespread but does happen so we'd have to assess the impact. - It would be interesting to see how many people are spec'ing exports incorrectly. The UI guys got it wrong but did others copy or make the same mistake? The runtime/equinox splits do not have '.'s. If it is just the UI guys and few people use import with the attribute (as I suspect), then we may be able to just change the attribute names. - In the mean time I suggest the RAP team change their attribute names accordingly. There should not be any impacts in the immediate term/scope. - there is really no need for a conversion tool. Change the '.' to a '-' in the imports. I'm not keen on hacking the framework to convert or some such for free. - we could potentially change the framework matching to treat . and - synonymously so we'd be able to change the exports without breaking the importers (perhaps flag them or something). Gag but possible. - I'm not convinced that people are "living on a knife-edge" (Glyn's been reading too many suspense novels ;-) Fact is that most people pick a framework and use it with no intention of switching. Clearly this is true since no one seems to have noticed this problem for the last 5 years. The point being, breaking them to make them correct is unlikely to be viewed favorably. - Changing Virgo to be a tad permissive to an existing (though incorrect) community does not seem beyond reason. - We can seek to update the spec to allow '.'. Its not clear why . should be omitted. Based on Jeff's comments, it would seem reasonable to address the problem at source and not modify Virgo. If there is still a strong requirement for Virgo to tolerate this syntax, perhaps it could be done using a manifest transformer which would convert the relevant '.' to a '-' on all imports and exports. This may be a little involved as the transformation would need to occur before the manifest parser runs into the '.' and fails. If someone would like to have a go at this, they should take a look at the ManifestUpgrader: http://git.eclipse.org/c/virgo/org.eclipse.virgo.kernel.git/tree/org.eclipse.virgo.kernel.deployer/src/main/java/org/eclipse/virgo/kernel/deployer/core/internal/ManifestUpgrader.java PS. The knife-edge I was referring to was the general problem of coping with split packages. ;-) A while back I opened a bug against the specification to allow '.' in attributes and directives (for folks that have access https://www.osgi.org/members/bugzilla/show_bug.cgi?id=1634) Basically the syntax in section 1.3.2 of the specification will be updated to: directive ::= extended ’:=’ argument attribute ::= extended ’=’ argument extended ::= ( alphanum | ’_’ | ’-’ | ’.’ )+ was this change approved? when will it take effect? If it is going forward then perhaps Virgo can just go ahead now and update their code to allow '.' (In reply to comment #8) > was this change approved? when will it take effect? If it is going forward > then perhaps Virgo can just go ahead now and update their code to allow '.' It has been approved. I am 99% confident it will be in the R4.3 specification when it is released next year. (In reply to comment #9) > (In reply to comment #8) > > was this change approved? when will it take effect? If it is going forward > > then perhaps Virgo can just go ahead now and update their code to allow '.' > > It has been approved. I am 99% confident it will be in the R4.3 specification > when it is released next year. Yes, I'm happy to see this change made based on the direction of the standard. However, it's nowhere near the top of my team's priorities, so I'm hoping someone is sufficiently motivated to develop a patch. If someone expresses an interest, I'll help them locate the relevant code. Created attachment 175550 [details]
1/4
I confirm that i have written 100% of the attached virgoise.sh patch and that
i'm allowed to contribute the patch to eclipse.org.
Created attachment 175551 [details]
2/4
I confirm that i have written 100% of the attached patch and that
i'm allowed to contribute the patch to eclipse.org.
Created attachment 175552 [details]
3/4
I confirm that i have written 100% of the attached virgoise.sh patch and that
i'm allowed to contribute the patch to eclipse.org.
Created attachment 175553 [details]
4/4
I confirm that i have written 100% of the attached patch and that
i'm allowed to contribute the patch to eclipse.org.
Hi all, i digged into the StandardHeaderLexer and created a patch. Now Virgo accepts the provided test bundles with the "nasty" header: a.split.pkg;nasty.split="split" [2010-07-30 08:54:18.990] fs-watcher org.eclipse.virgo.medic.eventlog.default HD0001I Hot deployer processing 'CREATED' event for file 'split.provider_1.0.0.201007051158.jar'. [2010-07-30 08:54:19.014] fs-watcher org.eclipse.virgo.medic.eventlog.default DE0000I Installing bundle 'split.provider' version '1.0.0.201007051158'. [2010-07-30 08:54:19.027] fs-watcher org.eclipse.virgo.medic.eventlog.default DE0001I Installed bundle 'split.provider' version '1.0.0.201007051158'. [2010-07-30 08:54:19.037] fs-watcher org.eclipse.virgo.medic.eventlog.default DE0004I Starting bundle 'split.provider' version '1.0.0.201007051158'. [2010-07-30 08:54:19.043] start-signalling-3 org.eclipse.virgo.medic.eventlog.default DE0005I Started bundle 'split.provider' version '1.0.0.201007051158'. [2010-07-30 08:54:37.056] fs-watcher org.eclipse.virgo.medic.eventlog.default HD0001I Hot deployer processing 'CREATED' event for file 'split.consumer_1.0.0.201007051158.jar'. [2010-07-30 08:54:37.092] fs-watcher org.eclipse.virgo.medic.eventlog.default DE0000I Installing bundle 'split.consumer' version '1.0.0.201007051158'. [2010-07-30 08:54:37.104] fs-watcher org.eclipse.virgo.medic.eventlog.default DE0001I Installed bundle 'split.consumer' version '1.0.0.201007051158'. [2010-07-30 08:54:37.113] fs-watcher org.eclipse.virgo.medic.eventlog.default DE0004I Starting bundle 'split.consumer' version '1.0.0.201007051158'. [2010-07-30 08:54:37.118] start-signalling-3 org.eclipse.virgo.medic.eventlog.default DE0005I Started bundle 'split.consumer' version '1.0.0.201007051158'. Regards, florian Currently reviewing patches for contribution. Contributions flagged IPLOG. Commits in util are: SHA: 185b36ba4e4094b42f13ce89c3d83588df83f6e0 Author: Florian Waibel <fwaibel@eclipsesource.com> Date: Thu Aug 05 2010 19:19:05 GMT+0100 (BST) Committer: Steve Powell <spowell@vmware.com> Commit Date: Thu Aug 05 2010 19:24:41 GMT+0100 (BST) Subject: Add tests for package attribute names. and SHA: ff688582407992157d85eda5a7def780afa9c840 Author: Florian Waibel <fwaibel@eclipsesource.com> Date: Thu Aug 05 2010 19:42:46 GMT+0100 (BST) Committer: Steve Powell <spowell@vmware.com> Commit Date: Thu Aug 05 2010 19:42:46 GMT+0100 (BST) Subject: Add tests and lexer for package attribute names. Only modifications I made was to change the names of the test methods to more precisely indicate what they were testing. Thank you Florian. Closing after silent integration tests. |