Bugzilla – Full Text Bug Listing
| Summary: | Service impls should use a tight version range for service imports. | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Equinox | Reporter: | John Ross <jwross> | ||||||||||||||||||||||
| Component: | Compendium | Assignee: | John Ross <jwross> | ||||||||||||||||||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||||||||||||||||||
| Severity: | normal | ||||||||||||||||||||||||
| Priority: | P3 | CC: | hargrave, tjwatson | ||||||||||||||||||||||
| Version: | unspecified | ||||||||||||||||||||||||
| Target Milestone: | 3.7 M4 | ||||||||||||||||||||||||
| Hardware: | All | ||||||||||||||||||||||||
| OS: | All | ||||||||||||||||||||||||
| Whiteboard: | |||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||
|
Description
John Ross
Once this is patched, released, and included in a build, we should update the DS, EventAdmin, and MetaType RIs as well as the org.eclipse.equinox.util JAR. This change is a good thing, but it will make it harder for these bundles to resolve since they require a more narrow version range. We should also package the service packages in these bundles as requested by bug328653. This will ensure these bundle do not fail to resolve (because of the service package dependency). Created attachment 182488 [details]
CM Patch
I decided to do these separately for clarity and to reduce the possibility of error. Let me know if you prefer a single patch. Also, let me know if there are any other service impls I should update.
This one is for Config Admin.
Created attachment 182489 [details]
DS Patch
Declarative Services
Created attachment 182490 [details]
Event Patch
Event Admin
Created attachment 182491 [details]
MetaType Patch
MetaType Service
org.eclipse.equinox.app includes the org.osgi.service.application API as part of the bundle but does not include that package as part of the Import-Package header. Should it? (In reply to comment #7) > org.eclipse.equinox.app includes the org.osgi.service.application API as part > of the bundle but does not include that package as part of the Import-Package > header. Should it? If it exports the package, it should also import it with the narrow (minor version) range to allow for substitution. (In reply to comment #8) > (In reply to comment #7) > > org.eclipse.equinox.app includes the org.osgi.service.application API as part > > of the bundle but does not include that package as part of the Import-Package > > header. Should it? > > If it exports the package, it should also import it with the narrow (minor > version) range to allow for substitution. Need to be careful with this one. This is one of those packages where the implementation can modify the source to add in their own impl. I think equinox.app would fail if it imported the application API from another bundle that was not tied into the application container's implementation. (In reply to comment #9) > Need to be careful with this one. This is one of those packages where the > implementation can modify the source to add in their own impl. I think > equinox.app would fail if it imported the application API from another bundle > that was not tied into the application container's implementation. Sigh... Regarding the patches, I would not put a space after the comma in the version range. Spaces should be avoided to handle version range parsers which dislike them. Also, version ranges should be used for *all* imported packages. For packages which are for API which are not being implemented by the bundle, then ceiling is the next major version. e.g. "[1.3,2.0)" So while we are fixing the imports for the implemented API, lets fix all the imports. org.eclipse.equinox.http currently imports version 1.1 of org.osgi.service.http while org.eclipse.equinox.http.servlet imports the latest version 1.2 does org.eclipse.equinox.http need to stay at 1.1? (In reply to comment #11) > Regarding the patches, I would not put a space after the comma in the version > range. Spaces should be avoided to handle version range parsers which dislike > them. > Also, version ranges should be used for *all* imported packages. For packages > which are for API which are not being implemented by the bundle, then ceiling > is the next major version. e.g. "[1.3,2.0)" So while we are fixing the imports > for the implemented API, lets fix all the imports. For package imports that do not currently specify a version at all, should I simply default to 1.0 as the floor? Created attachment 182502 [details]
App Patch
Application Admin
(In reply to comment #14) > Created an attachment (id=182502) [details] > App Patch > Application Admin Note I did not add org.eclipse.equinox.app to the Import-Package header based on comment 9. (In reply to comment #15) > (In reply to comment #14) > > Created an attachment (id=182502) [details] [details] > > App Patch > > Application Admin > Note I did not add org.eclipse.equinox.app to the Import-Package header based > on comment 9. meant to say org.osgi.service.application (In reply to comment #13) > For package imports that do not currently specify a version at all, should I > simply default to 1.0 as the floor? When no version is specified, the floor is 0. What packages are these? If they are expected to come from the JRE (via system bundle), then we can leave the version off. If they come from some other bundle, we need to consult that bundle for the proper floor and ceiling. Created attachment 182504 [details] Updated CM Patch 1 See comment 11 (In reply to comment #17) > When no version is specified, the floor is 0. What packages are these? If they > are expected to come from the JRE (via system bundle), then we can leave the > version off. If they come from some other bundle, we need to consult that > bundle for the proper floor and ceiling. As a specific example, see import org.osgi.service.condpermadmin within org.eclipse.equinox.app. I set the floor to 1.0. The most recent version is 1.1.1. Assigning to John to avoid inbox spam. (In reply to comment #19) > As a specific example, see import org.osgi.service.condpermadmin within > org.eclipse.equinox.app. I set the floor to 1.0. The most recent version is > 1.1.1. Check to make sure what version of the package the code is compiling against. That is the proper floor. If you pick a lower floor, you risk runtime errors if the code actually uses types/members introduced after the floor version. Spoke with Tom, and we decided to take a more granular (a.k.a. wimpy ;) approach in order to keep the patch, release, build cycles (particularly in terms of being able to identify why the build broke) more straightforward. This bug will be used for the originally stated purpose of tightening up the version ranges of imported services that are also implemented. Subsequent stages will include the following. (1) Tightening up the version ranges of other service imports (new bug I will open). (2) Including the service API within the implementing bundle and exporting it (see https://bugs.eclipse.org/bugs/show_bug.cgi?id=328653). (3) Establishing the lowest valid floor within each version range (new bug I will open). Would it be better to open up separate bugs for each service? Comment on attachment 182502 [details] App Patch Obsoleting this patch because it's not relevant to this bug. The Application Admin implementation does not import the service API it implements. See comment 9. Created attachment 182517 [details] CM Patch 2 Original patch with the space after the comma removed. See comment 11. Created attachment 182518 [details] DS Patch 1 Removes space after comma. See comment 11. Created attachment 182528 [details] Patch for all services Finally got tired of the one patch per service approach. This includes the following services. CM Device DS Event HTTP HTTP Servlet IO IP Log MetaType User Admin Wire Admin The following services were intentionally left out because neither imports the service API they implement. Application Admin (see comment 9) Preferences (what's the story here?) Let me know if I missed any. (In reply to comment #22) > Spoke with Tom, and we decided to take a more granular (a.k.a. wimpy ;) > approach in order to keep the patch, release, build cycles (particularly in > terms of being able to identify why the build broke) more straightforward. OK. > Would it be better to open up separate bugs for each service? Only if you think we need to release them at separate times so that we need different tracking bugs. (In reply to comment #26) > Preferences (what's the story here?) The Preferences Service impl should also import the service API. Created attachment 182770 [details] Patch for all services 2 Same content as in comment 26 but adds Preferences Service. I released the latest patch with a couple of changes: 1) I updated the bundle-versions where appropriate 2) I used a floor of 1.2 for the http packages for the http service implementation bundles. Comment on attachment 182770 [details]
Patch for all services 2
Thanks for the patch.
|