Community
Participate
Working Groups
The fix in https://bugs.eclipse.org/bugs/show_bug.cgi?id=575125 resolves fragments using host.getDependents(). However now the resulting order of the fragments relies on the order of ResolverImpl.bundleMapping.values(), which is different every time Eclipse starts up. This leads to reordering class path changes in JDT, which in turn can lead to full builds in Xtext projects --> https://bugs.eclipse.org/bugs/show_bug.cgi?id=560822. Before the fix StateImpl.bundleDescriptions was used to resolve fragments. This is also a HashMap. However, it uses bundle ids as keys. ResolverImpl.bundleMapping has system hashcodes as keys, because BundleDescriptionImpl doesn't override hashCode(). Because of this issue we experience full builds in our projects every time Eclipse starts up.
(In reply to Nils Ruhr from comment #0) > However now the resulting order of the > fragments relies on the order of ResolverImpl.bundleMapping.values(), which > is different every time Eclipse starts up. Do you mean the iteration in org.eclipse.osgi.internal.module.ResolverImpl.rewireBundles() ? @Hannes: could you please check that? Ideally we could use LinkedHashMap (or sort, whatever appropriate) to preserve order of fragments, even if the order is probably not guaranteed to be stable in the contract. But permanent rebuilds is surely a major regression.
This seems like a PDE issue vs some issue in the old ResolverImpl. I presume we are observing a change in behavior (order) of org.eclipse.osgi.service.resolver.BundleDescription.getFragments() If PDE needs a consistent order here (which is not prescribed in the javadoc of BundleDescription.getFragments()) then PDE should sort the result however they see fit to get a consistent order from one launch to the next.
(In reply to Andrey Loskutov from comment #1) > (In reply to Nils Ruhr from comment #0) > > However now the resulting order of the > > fragments relies on the order of ResolverImpl.bundleMapping.values(), which > > is different every time Eclipse starts up. > > Do you mean the iteration in > org.eclipse.osgi.internal.module.ResolverImpl.rewireBundles() ? > > @Hannes: could you please check that? I tried to check it but I could not yet identify the path between BundleDescriptionImpl.addDependent() and ResolverImpl.bundleMapping (or it has to be quite long). However if it is ResolverImpl.bundleMapping, the location Andrey suggested seems to be the only possible one. A minimal reproducer would help here. @Nils could you please provide one? > > Ideally we could use LinkedHashMap (or sort, whatever appropriate) to > preserve order of fragments, even if the order is probably not guaranteed to > be stable in the contract. But permanent rebuilds is surely a major > regression. Using a LinkedHashMap would be one solution. Another solution, that only affects getFragments(), would be to use in StateImpl.getFragments() either a TreeSet for fragments or create a stream and sort the elements before creating the array of BundleDescriptions. In both cases the BundleDescriptions could be sorted according to their Bundle-Id. (In reply to Thomas Watson from comment #2) > This seems like a PDE issue vs some issue in the old ResolverImpl. I > presume we are observing a change in behavior (order) of > org.eclipse.osgi.service.resolver.BundleDescription.getFragments() That's right, no guarantee is made about any stable ordering of fragments in the javadoc. But if it has been stable before I could understand that somebody expect a stable ordering now. > > If PDE needs a consistent order here (which is not prescribed in the javadoc > of BundleDescription.getFragments()) then PDE should sort the result however > they see fit to get a consistent order from one launch to the next. This would be possible as well. In PDE I think this has to be handled in RequiredPluginsClasspathContainer. All three calls to BundleDescription.getFragments() would have to sort the descriptions first before further processing. A small static function getSortedFragments(BundleDescription) could do the job here. I would prefer solution 2 or 3, because its effect is only local.
Hi. Here's the relevant part of the call stack (I'm using org.eclipse.osgi.compatibility.state_1.2.500.v20210730-0750): BundleDescriptionImpl.addDependent(BundleDescription) line: 616 BundleDescriptionImpl.addDependency(BaseDescriptionImpl, boolean) line: 587 BundleDescriptionImpl.addDependencies(BaseDescription[], boolean) line: 574 UserState(StateImpl).resolveConstraints(BundleDescriptionImpl, BundleDescription[], ExportPackageDescription[], ExportPackageDescription[], GenericDescription[], BundleDescription[], ExportPackageDescription[], GenericDescription[], Map<String,List<StateWire>>) line: 496 UserState(StateImpl).resolveBundle(BundleDescription, boolean, BundleDescription[], ExportPackageDescription[], ExportPackageDescription[], GenericDescription[], BundleDescription[], ExportPackageDescription[], GenericDescription[], Map<String,List<StateWire>>) line: 463 ResolverImpl.stateResolveBundle(ResolverBundle) line: 2015 ResolverImpl.stateResolveBundles(ResolverBundle[]) line: 1839 ResolverImpl.resolve(BundleDescription[], Dictionary<Object,Object>[]) line: 530 UserState(StateImpl).resolve(boolean, BundleDescription[], BundleDescription[]) line: 601 UserState(StateImpl).resolve(boolean) line: 661 PDEState(MinimalState).internalResolveState(boolean) line: 245 PDEState(MinimalState).resolveState(boolean) line: 222 PluginModelManager.initializeTable(IProgressMonitor) line: 631 PluginModelManager.findModel(IProject) line: 1053 RequiredPluginsInitializer.initialize(IPath, IJavaProject) line: 50 JavaModelManager.initializeContainer(IJavaProject, IPath) line: 3144 JavaModelManager.getClasspathContainer(IPath, IJavaProject) line: 2092 JavaCore.getClasspathContainer(IPath, IJavaProject) line: 3783 (...) Current bundle is "org.elipse.osgi" and the dependent is "com.diffplug.osgi.extension.sun.misc".
This is where bundleMapping.values() is called: ResolverImpl.resolve(BundleDescription[], Dictionary<Object,Object>[]) line: 530
(In reply to Hannes Wellmann from comment #3) > > I would prefer solution 2 or 3, because its effect is only local. I'm not clear what the proposed solutions 1, 2 or 3 are. If others want to get change the Equinox State impl to have a consistent ordering of BundleDescription.getFragments() I'm not going to block that and I can review the changes. But I do think if PDE needs some consistent ordering then it likely is not only BundleDescription.getFragments() that needs to be consistent but the other getResolved methods need to be consistent also: BundleDescription.getResolvedImports() BundleDescription.getResolvedRequires() IIRC these should go in the order they are specified in the Import-Package and Require-Bundle header but that is not clear by the javadoc. It would be good to confirm that stays consistent also. If such guarantees are important then I suggest some testcases be added to org.eclipse.osgi.tests.services.resolver.AllTests to ensure this consistency remains.
(In reply to Nils Ruhr from comment #4) > Hi. Here's the relevant part of the call stack (I'm using > org.eclipse.osgi.compatibility.state_1.2.500.v20210730-0750): > Thank you, with the provided information and the linked Bug 560822 I was able to reproduce/confirm the issue and that a stable fragments order resolves it. (In reply to Thomas Watson from comment #6) > (In reply to Hannes Wellmann from comment #3) > > > > I would prefer solution 2 or 3, because its effect is only local. > > I'm not clear what the proposed solutions 1, 2 or 3 are. If others want to > get change the Equinox State impl to have a consistent ordering of > BundleDescription.getFragments() I'm not going to block that and I can > review the changes. Sorry for being not clear. Option 1: Assign StateImpl.bundleDescriptions a LinkedHashMap Option 2: Sort the array returned by StateImpl.getFragments() (e.g. by their bundle-id) Option 3: sort the array returned by BundleDescription.getFragments() in PDE's RequiredPluginsClasspathContainer and other locations > But I do think if PDE needs some consistent ordering then it likely is not > only BundleDescription.getFragments() that needs to be consistent but the > other getResolved methods need to be consistent also: > > BundleDescription.getResolvedImports() > BundleDescription.getResolvedRequires() > > IIRC these should go in the order they are specified in the Import-Package > and Require-Bundle header but that is not clear by the javadoc. It would be > good to confirm that stays consistent also. If such guarantees are > important then I suggest some testcases be added to > org.eclipse.osgi.tests.services.resolver.AllTests to ensure this consistency > remains. I agree. And probably you are right since the issue did not appear before getFragment() was change. But after going through PDE's calls to getFragments() I suggest to resolve this with option-2. There are multiple other locations in PDE besides RequiredPluginsClasspathContainer that might be affected by a changing fragment-order (for example the API-tools in BundleComponent.createApiDescription()) and I cannot tell if they are sensible or not. If I wouldn't know it now I couldn't tell that a changing order in getFragments() causes full rebuilds on start up, because cause and effect are very far apparat here, even in different Eclipse projects. Therefore I think for now the fastest way to fix this is option-2 to restore the behaviour as it was before (even if it was not specified). But when PDE moves off the resolver as suggested in Bug 570198 this has to be considered again.
New Gerrit change created: https://git.eclipse.org/r/c/equinox/rt.equinox.framework/+/190483
Gerrit change https://git.eclipse.org/r/c/equinox/rt.equinox.framework/+/190483 was merged to [master]. Commit: http://git.eclipse.org/c/equinox/rt.equinox.framework.git/commit/?id=ea3839e51c85b5b84d8ec5ef007c667978f3a165
@Hannes: are you done with the bug, or is something missing? If we are done, please update the bug status.
(In reply to Andrey Loskutov from comment #10) > @Hannes: are you done with the bug, or is something missing? If we are done, > please update the bug status. Yes I'm done here. The behaviour should now be as it before. A user that switches from an older version to the up-comming one will then experience at most one rebuild in case the order based on the bundle-ids is different as it was before. But there should not follow more due to fragment order change, since the bundle-id is stable.
(In reply to Hannes Wellmann from comment #11) > Yes I'm done here. Thanks Hannes.
*** Bug 578968 has been marked as a duplicate of this bug. ***
