Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 511593

Summary: Issue resolving split packages
Product: [Eclipse Project] Equinox Reporter: Thomas Watson <tjwatson>
Component: FrameworkAssignee: Thomas Watson <tjwatson>
Status: RESOLVED FIXED QA Contact:
Severity: normal    
Priority: P3 CC: daniel_megert, Lars.Vogel, loskutov, markus.kell.r, nobody, tjwatson
Version: unspecified   
Target Milestone: Neon.3   
Hardware: All   
OS: All   
See Also: https://bugs.eclipse.org/bugs/show_bug.cgi?id=511599
https://git.eclipse.org/r/90289
https://git.eclipse.org/r/90288
https://git.eclipse.org/c/equinox/rt.equinox.framework.git/commit/?id=fcab3e35945c3d10db5b22838c1230c67afdaee2
https://git.eclipse.org/c/equinox/rt.equinox.framework.git/commit/?id=f9777863a046cb6516c5846f2d592ff6aa25fb15
https://git.eclipse.org/r/90635
https://git.eclipse.org/r/90634
https://git.eclipse.org/c/equinox/rt.equinox.framework.git/commit/?id=c8d441fca3a1343225d40663ccbc8b77807dcfda
https://git.eclipse.org/c/equinox/rt.equinox.framework.git/commit/?id=8e71dd54cca09480967bcc5f7984d2c49ffe40d1
https://git.eclipse.org/r/90743
https://git.eclipse.org/c/equinox/rt.equinox.framework.git/commit/?id=1bc36f585d4c1e6e329b0d584c9a649f0330416a
https://git.eclipse.org/r/90761
https://git.eclipse.org/c/equinox/rt.equinox.framework.git/commit/?id=71394a2cbca386ea5b4a2acc3d807be51336baca
https://git.eclipse.org/r/90766
https://git.eclipse.org/c/equinox/rt.equinox.framework.git/commit/?id=f5262c4a0110dcc27aeb553e720e07e67a7e6ae2
Whiteboard:
Bug Depends on:    
Bug Blocks: 511517    

Description Thomas Watson CLA 2017-02-02 12:14:41 EST
+++ This bug was initially created as a clone of Bug #511517 +++

http://download.eclipse.org/eclipse/downloads/drops4/I20170201-1030/testResults.php

On all platforms.

Starts with: SearchCheatsheet.testCheatSheetItemSearch

java.lang.AssertionError: expected:<1> but was:<0>
at org.eclipse.ua.tests.help.search.SearchCheatsheet.checkForCheatSheetMatch(SearchCheatsheet.java:102)
at org.eclipse.ua.tests.help.search.SearchCheatsheet.testCheatSheetItemSearch(SearchCheatsheet.java:52)
at org.eclipse.test.EclipseTestRunner.run(EclipseTestRunner.java:749)
at org.eclipse.test.EclipseTestRunner.run(EclipseTestRunner.java:350)
at org.eclipse.test.UITestApplication.lambda$0(UITestApplication.java:195)
at org.eclipse.swt.widgets.RunnableLock.run(RunnableLock.java:37)
at org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:182)
at org.eclipse.swt.widgets.Display.runAsyncMessages(Display.java:4468)

Initial investigation points to an issue resolving split packages which results in dropping optional requirements in order to find a consistent class space.  The solution  to make the requirement non-optional is a work around because it forces the resolver to "try-harder" instead of dropping the optional requirement.
Comment 1 Eclipse Genie CLA 2017-02-03 12:30:08 EST
New Gerrit change created: https://git.eclipse.org/r/90289
Comment 2 Eclipse Genie CLA 2017-02-03 12:30:14 EST
New Gerrit change created: https://git.eclipse.org/r/90288
Comment 3 Thomas Watson CLA 2017-02-03 16:51:39 EST
I opened felix issue https://issues.apache.org/jira/browse/FELIX-5514 to track getting this fixed at apache.  Meanwhile I have a fix I will release to Equinox shortly (see gerrit reviews)
Comment 6 Thomas Watson CLA 2017-02-03 17:10:57 EST
I have submitted the fixes to Equinox.
Comment 7 Eclipse Genie CLA 2017-02-08 09:15:06 EST
New Gerrit change created: https://git.eclipse.org/r/90635
Comment 8 Eclipse Genie CLA 2017-02-08 09:15:09 EST
New Gerrit change created: https://git.eclipse.org/r/90634
Comment 9 Thomas Watson CLA 2017-02-08 09:22:43 EST
This problem is severe and should be considered for Neon.3.
Comment 12 Dani Megert CLA 2017-02-08 10:40:11 EST
(In reply to Thomas Watson from comment #9)
> This problem is severe and should be considered for Neon.3.

+1
Comment 13 Thomas Watson CLA 2017-02-08 10:53:00 EST
Done for Neon.3.
Comment 14 Markus Keller CLA 2017-02-08 15:52:49 EST
org.eclipse.osgi.container.ModuleWiring#getSubstitutionWires() is a new API method that gave API Tools errors in R4_6_maintenance (bad @since 3.12, wants to increase minor version from 3.11.3).

The API addition looks OK, so I've added the necessary filters: http://git.eclipse.org/c/equinox/rt.equinox.framework.git/commit/?id=4fa9c28f322c3691a43ac71663a735b3a2790e85

I guess Dani will have to add an exclusion rule for the API change report.
Comment 15 Dani Megert CLA 2017-02-09 05:15:15 EST
(In reply to Markus Keller from comment #14)
> org.eclipse.osgi.container.ModuleWiring#getSubstitutionWires() is a new API
> method that gave API Tools errors in R4_6_maintenance (bad @since 3.12,
> wants to increase minor version from 3.11.3).
> 
> The API addition looks OK, so I've added the necessary filters:
> http://git.eclipse.org/c/equinox/rt.equinox.framework.git/commit/?id=4fa9c28f322c3691a43ac71663a735b3a2790e85
> 
> 
> I guess Dani will have to add an exclusion rule for the API change report.

Looks like we're not producing the freeze report for M-builds for quite some time. I've created bug 511963 as a reminder to do it for the Oxygen M-builds.
Comment 16 Thomas Watson CLA 2017-02-09 09:37:12 EST
(In reply to Markus Keller from comment #14)
> org.eclipse.osgi.container.ModuleWiring#getSubstitutionWires() is a new API
> method that gave API Tools errors in R4_6_maintenance (bad @since 3.12,
> wants to increase minor version from 3.11.3).
> 
> The API addition looks OK, so I've added the necessary filters:
> http://git.eclipse.org/c/equinox/rt.equinox.framework.git/commit/
> ?id=4fa9c28f322c3691a43ac71663a735b3a2790e85
> 
> I guess Dani will have to add an exclusion rule for the API change report.

I better get some javadoc in there.

One thing that concerns me.  Here we need to have ModuleWiring (considered API) implement FelixWiring (considered internal) so we can implement the getSubstitutionWires to improve performance of the resolver since the wiring implementation already has to keep track of this knowledge.

Is it a problem that our API implements a internal type, one that will hopefully go away in the future?  In the future (OSGi R7) I plan to propose the method getSubstitutionWires as a method on Wiring and then I would get rid of the FelixWiring interface altogether.
Comment 17 Eclipse Genie CLA 2017-02-09 10:10:47 EST
New Gerrit change created: https://git.eclipse.org/r/90743
Comment 19 Dani Megert CLA 2017-02-09 10:44:06 EST
(In reply to Thomas Watson from comment #16)
> One thing that concerns me.  Here we need to have ModuleWiring (considered
> API) implement FelixWiring (considered internal) so we can implement the
> getSubstitutionWires to improve performance of the resolver since the wiring
> implementation already has to keep track of this knowledge.
> 
> Is it a problem that our API implements a internal type, one that will
> hopefully go away in the future?  In the future (OSGi R7) I plan to propose
> the method getSubstitutionWires as a method on Wiring and then I would get
> rid of the FelixWiring interface altogether.

In general it is a considered an API leak and API Tools reports this.

As long as you can make sure this does not leak to the clients by adapting ModuleWiring to changes, it is fine, since clients are not allowed to reference FelixWiring.
Comment 20 Thomas Watson CLA 2017-02-09 10:59:47 EST
(In reply to Dani Megert from comment #19)
> (In reply to Thomas Watson from comment #16)
> > One thing that concerns me.  Here we need to have ModuleWiring (considered
> > API) implement FelixWiring (considered internal) so we can implement the
> > getSubstitutionWires to improve performance of the resolver since the wiring
> > implementation already has to keep track of this knowledge.
> > 
> > Is it a problem that our API implements a internal type, one that will
> > hopefully go away in the future?  In the future (OSGi R7) I plan to propose
> > the method getSubstitutionWires as a method on Wiring and then I would get
> > rid of the FelixWiring interface altogether.
> 
> In general it is a considered an API leak and API Tools reports this.
> 
> As long as you can make sure this does not leak to the clients by adapting
> ModuleWiring to changes, it is fine, since clients are not allowed to
> reference FelixWiring.

I was thinking about how I am going to propose this back to OSGi for R7 and I think I am going to take a different approach.  Instead of implementing this on the ModuleWiring/FelixWiring interface I'm going to add a method to the FelixResolveContext interface.  One issue is that I don't think we can add a new method to the Wiring interface in OSGi because it is a consumer type.  Instead I would add a method to the ResolveContext for OSGi R7 since that is an abstract class.  This will keep all of this solution out of the API altogether and allow us to internally give the information to the resolver.
Comment 21 Markus Keller CLA 2017-02-09 11:01:04 EST
(In reply to Thomas Watson from comment #16)
> I better get some javadoc in there.

API Javadocs for ModuleWiring are currently not generated; I filed bug 511984 for that.

If you're pretty sure that "Collection<Wire> getSubstitutionWires()" will be the API that eventually gets added to BundleWiring, or if you think it's necessary to allow other clients to refer to this method already now, then some Javadoc would be good.

Otherwise, you can also mark ModuleWiring#getSubstitutionWires() as @noreference for now.
Comment 22 Eclipse Genie CLA 2017-02-09 12:41:40 EST
New Gerrit change created: https://git.eclipse.org/r/90761
Comment 24 Eclipse Genie CLA 2017-02-09 13:46:11 EST
New Gerrit change created: https://git.eclipse.org/r/90766
Comment 26 Thomas Watson CLA 2017-02-09 15:50:51 EST
(In reply to Markus Keller from comment #14)
> org.eclipse.osgi.container.ModuleWiring#getSubstitutionWires() is a new API
> method that gave API Tools errors in R4_6_maintenance (bad @since 3.12,
> wants to increase minor version from 3.11.3).
> 
> The API addition looks OK, so I've added the necessary filters:
> http://git.eclipse.org/c/equinox/rt.equinox.framework.git/commit/
> ?id=4fa9c28f322c3691a43ac71663a735b3a2790e85
> 
> I guess Dani will have to add an exclusion rule for the API change report.

I reverted the filters and I released a solution that involves no added API.