diff options
author | Thomas Watson | 2020-01-13 15:53:33 +0000 |
---|---|---|
committer | Thomas Watson | 2020-01-14 22:32:15 +0000 |
commit | b61cf1d96b3e7ef8da2e506f0393d91e422dbcfd (patch) | |
tree | 7bbfc8eb49ce1693969e2609cfdfa691556861c5 | |
parent | 4d9b3a851bc877af780577fa4f096976399c121b (diff) | |
download | rt.equinox.framework-b61cf1d96b3e7ef8da2e506f0393d91e422dbcfd.tar.gz rt.equinox.framework-b61cf1d96b3e7ef8da2e506f0393d91e422dbcfd.tar.xz rt.equinox.framework-b61cf1d96b3e7ef8da2e506f0393d91e422dbcfd.zip |
Bug 559118 - Add testcase and fix reexport uses constraint checkI20200116-1800I20200116-0930I20200116-0330I20200116-0310I20200115-1800
This is a testcase that reproduces the issue found in bug 558895. When
an exporter of a split package requires multiple bundles that also
export the split package and that bundle does a reexport on one of the
parts it can cause resolution issues for uses constraints.
The actual problem is with reexport itself. The reexporting bundle does
not have to also export the package to cause the issue. The problem is
that each part of the package pulled in from the require-reexport is
checked in isolation with the using bundles wire to the same package
name. This is incorrect because that is only a subset of the actual
used package from the perspective of the exported package that is using
the split package.
The fix is to record all the package parts for the split used package
from the wiring of the bundle exporting the package that uses the split
package. That way during the compatibility check we can accurately use
the set of sources for the split package that the exporting bundle is
using.
Change-Id: I5d6194adabc7c04fe990d663ad1dd6bb77f2ac39
Signed-off-by: Thomas Watson <tjwatson@us.ibm.com>
-rw-r--r-- | bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/container/TestModuleContainer.java | 67 | ||||
-rwxr-xr-x | bundles/org.eclipse.osgi/felix/src/org/apache/felix/resolver/ResolverImpl.java | 96 |
2 files changed, 122 insertions, 41 deletions
diff --git a/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/container/TestModuleContainer.java b/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/container/TestModuleContainer.java index bb48c72b4..161841eaf 100644 --- a/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/container/TestModuleContainer.java +++ b/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/container/TestModuleContainer.java @@ -3571,6 +3571,73 @@ public class TestModuleContainer extends AbstractTest { stopError.printStackTrace(); } + @Test + public void testUsesWithRequireReexport() throws BundleException, IOException { + DummyContainerAdaptor adaptor = createDummyAdaptor(); + ModuleContainer container = adaptor.getContainer(); + + // install the system.bundle + Module systemBundle = installDummyModule("system.bundle.MF", Constants.SYSTEM_BUNDLE_LOCATION, Constants.SYSTEM_BUNDLE_SYMBOLICNAME, null, null, container); + ResolutionReport report = container.resolve(Arrays.asList(systemBundle), true); + Assert.assertNull("Failed to resolve system.bundle.", report.getResolutionException()); + + // install part 1 (ui.workbench) + Map<String, String> split1Manifest = new HashMap<String, String>(); + split1Manifest.put(Constants.BUNDLE_MANIFESTVERSION, "2"); + split1Manifest.put(Constants.BUNDLE_SYMBOLICNAME, "split1"); + split1Manifest.put(Constants.EXPORT_PACKAGE, "split.pkg"); + Module moduleSplit1 = installDummyModule(split1Manifest, "split1", container); + + // install part 2 (e4.ui.ide) + Map<String, String> split2Manifest = new HashMap<String, String>(); + split2Manifest.put(Constants.BUNDLE_MANIFESTVERSION, "2"); + split2Manifest.put(Constants.BUNDLE_SYMBOLICNAME, "split2"); + split2Manifest.put(Constants.EXPORT_PACKAGE, "split.pkg"); + Module moduleSplit2 = installDummyModule(split2Manifest, "split2", container); + + // install part 3 which requires part 1 and 2, reexports 1 and 2 (ui.ide) + Map<String, String> split3Manifest = new HashMap<String, String>(); + split3Manifest.put(Constants.BUNDLE_MANIFESTVERSION, "2"); + split3Manifest.put(Constants.BUNDLE_SYMBOLICNAME, "split3"); + split3Manifest.put(Constants.EXPORT_PACKAGE, "split.pkg"); + // the reexport here are not necessary; but cause issues for the resolver + split3Manifest.put(Constants.REQUIRE_BUNDLE, "split1; visibility:=reexport, split2; visibility:=reexport"); + Module moduleSplit3 = installDummyModule(split3Manifest, "split3", container); + + // install reexporter of part1 (ui) + Map<String, String> reexporterPart1Manifest = new HashMap<String, String>(); + reexporterPart1Manifest.put(Constants.BUNDLE_MANIFESTVERSION, "2"); + reexporterPart1Manifest.put(Constants.BUNDLE_SYMBOLICNAME, "reexport1"); + reexporterPart1Manifest.put(Constants.REQUIRE_BUNDLE, "split1; visibility:=reexport"); + Module moduleReexport1 = installDummyModule(reexporterPart1Manifest, "reexport1", container); + + // install reexporter of split3 + Map<String, String> reexporterSplit3Manifest = new HashMap<String, String>(); + reexporterSplit3Manifest.put(Constants.BUNDLE_MANIFESTVERSION, "2"); + reexporterSplit3Manifest.put(Constants.BUNDLE_SYMBOLICNAME, "reexportSplit3"); + reexporterSplit3Manifest.put(Constants.REQUIRE_BUNDLE, "split3; visibility:=reexport"); + Module moduleReexportSplit3 = installDummyModule(reexporterSplit3Manifest, "reexportSplit3", container); + + // install test export that requires reexportSplit3 (should get access to all 3 parts) + Map<String, String> testExporterUses = new HashMap<String, String>(); + testExporterUses.put(Constants.BUNDLE_MANIFESTVERSION, "2"); + testExporterUses.put(Constants.BUNDLE_SYMBOLICNAME, "test.exporter"); + testExporterUses.put(Constants.REQUIRE_BUNDLE, "reexportSplit3"); + testExporterUses.put(Constants.EXPORT_PACKAGE, "export.pkg; uses:=split.pkg"); + Module testExporter = installDummyModule(testExporterUses, "test.exporter", container); + + // install test requirer that requires the exporter and reexport1 (should get access to only part 1) + // part 1 is a subset of what the exporter has access to so it should resolve + Map<String, String> testRequireUses = new HashMap<String, String>(); + testRequireUses.put(Constants.BUNDLE_MANIFESTVERSION, "2"); + testRequireUses.put(Constants.BUNDLE_SYMBOLICNAME, "test.requirer"); + testRequireUses.put(Constants.REQUIRE_BUNDLE, "test.exporter, reexport1"); + Module testRequirer = installDummyModule(testRequireUses, "test.requirer", container); + + report = container.resolve(Arrays.asList(moduleSplit1, moduleSplit2, moduleSplit3, moduleReexport1, moduleReexportSplit3, testExporter, testRequirer), true); + Assert.assertNull("Failed to resolve", report.getResolutionException()); + } + private static void assertWires(List<ModuleWire> required, List<ModuleWire>... provided) { for (ModuleWire requiredWire : required) { for (List<ModuleWire> providedList : provided) { diff --git a/bundles/org.eclipse.osgi/felix/src/org/apache/felix/resolver/ResolverImpl.java b/bundles/org.eclipse.osgi/felix/src/org/apache/felix/resolver/ResolverImpl.java index e9a781e5e..b06d43227 100755 --- a/bundles/org.eclipse.osgi/felix/src/org/apache/felix/resolver/ResolverImpl.java +++ b/bundles/org.eclipse.osgi/felix/src/org/apache/felix/resolver/ResolverImpl.java @@ -22,7 +22,6 @@ import java.security.*; import java.util.*; import java.util.Map.Entry; import java.util.concurrent.*; -import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; import org.apache.felix.resolver.reason.ReasonException; @@ -1094,26 +1093,30 @@ public class ResolverImpl implements Resolver continue; } - ArrayMap<Capability, UsedBlames> usedPkgBlames = currentPkgs.m_usedPkgs.getOrCompute(usedPkgName); + ArrayMap<Set<Capability>, UsedBlames> usedPkgBlames = currentPkgs.m_usedPkgs.getOrCompute(usedPkgName); + List<Blame> newBlames = new ArrayList<Blame>(); for (Blame blame : candSourceBlames) { + List<Requirement> newBlameReqs; if (blame.m_reqs != null) { - List<Requirement> blameReqs2 = new ArrayList<Requirement>(blameReqs.size() + 1); - blameReqs2.addAll(blameReqs); + newBlameReqs = new ArrayList<Requirement>(blameReqs.size() + 1); + newBlameReqs.addAll(blameReqs); // Only add the last requirement in blame chain because // that is the requirement wired to the blamed capability - blameReqs2.add(blame.m_reqs.get(blame.m_reqs.size() - 1)); - addUsedBlame(usedPkgBlames, blame.m_cap, blameReqs2, matchingCap); - mergeUses(session, current, currentPkgs, blame.m_cap, blameReqs2, matchingCap, - resourcePkgMap, cycleMap); + newBlameReqs.add(blame.m_reqs.get(blame.m_reqs.size() - 1)); } else { - addUsedBlame(usedPkgBlames, blame.m_cap, blameReqs, matchingCap); - mergeUses(session, current, currentPkgs, blame.m_cap, blameReqs, matchingCap, - resourcePkgMap, cycleMap); + newBlameReqs = blameReqs; } + newBlames.add(new Blame(blame.m_cap, newBlameReqs)); + } + addUsedBlames(usedPkgBlames, newBlames, matchingCap, resourcePkgMap); + for (Blame newBlame : newBlames) + { + mergeUses(session, current, currentPkgs, newBlame.m_cap, newBlame.m_reqs, matchingCap, + resourcePkgMap, cycleMap); } } } @@ -1276,18 +1279,31 @@ public class ResolverImpl implements Resolver return uses; } - private static void addUsedBlame( - ArrayMap<Capability, UsedBlames> usedBlames, Capability usedCap, - List<Requirement> blameReqs, Capability matchingCap) + private static void addUsedBlames( + ArrayMap<Set<Capability>, UsedBlames> usedBlames, Collection<Blame> blames, Capability matchingCap, Map<Resource, Packages> resourcePkgMap) { - // Create a new Blame based off the used capability and the - // blame chain requirements. - Blame newBlame = new Blame(usedCap, blameReqs); - // Find UsedBlame that uses the same capablity as the new blame. - UsedBlames addToBlame = usedBlames.getOrCompute(usedCap); - // Add the new Blame and record the matching capability cause + Set<Capability> usedCaps; + if (blames.size() == 1) + { + usedCaps = getPackageSources(blames.iterator().next().m_cap, resourcePkgMap); + } + else + { + usedCaps = new HashSet<Capability>(); + for (Blame blame : blames) + { + usedCaps.addAll(getPackageSources(blame.m_cap, resourcePkgMap)); + } + } + + // Find UsedBlame that uses the same capability as the new blame. + UsedBlames addToBlame = usedBlames.getOrCompute(usedCaps); + // Add the new Blames and record the matching capability cause // in case the root requirement has multiple cardinality. - addToBlame.addBlame(newBlame, matchingCap); + for (Blame blame : blames) + { + addToBlame.addBlame(blame, matchingCap); + } } private ResolutionError checkPackageSpaceConsistency( @@ -1368,14 +1384,14 @@ public class ResolverImpl implements Resolver { String pkgName = entry.getKey(); Blame exportBlame = entry.getValue(); - ArrayMap<Capability, UsedBlames> pkgBlames = pkgs.m_usedPkgs.get(pkgName); + ArrayMap<Set<Capability>, UsedBlames> pkgBlames = pkgs.m_usedPkgs.get(pkgName); if (pkgBlames == null) { continue; } for (UsedBlames usedBlames : pkgBlames.values()) { - if (!isCompatible(exportBlame, usedBlames.m_cap, resourcePkgMap)) + if (!isCompatible(exportBlame, usedBlames.m_caps, resourcePkgMap)) { mutated = (mutated != null) ? mutated @@ -1421,7 +1437,7 @@ public class ResolverImpl implements Resolver for (Entry<String, List<Blame>> entry : allImportRequirePkgs.fast()) { String pkgName = entry.getKey(); - ArrayMap<Capability, UsedBlames> pkgBlames = pkgs.m_usedPkgs.get(pkgName); + ArrayMap<Set<Capability>, UsedBlames> pkgBlames = pkgs.m_usedPkgs.get(pkgName); if (pkgBlames == null) { continue; @@ -1430,7 +1446,7 @@ public class ResolverImpl implements Resolver for (UsedBlames usedBlames : pkgBlames.values()) { - if (!isCompatible(requirementBlames, usedBlames.m_cap, resourcePkgMap)) + if (!isCompatible(requirementBlames, usedBlames.m_caps, resourcePkgMap)) { mutated = (mutated != null) ? mutated @@ -1686,21 +1702,20 @@ public class ResolverImpl implements Resolver } private static boolean isCompatible( - Blame currentBlame, Capability candCap, + Blame currentBlame, Set<Capability> candSources, Map<Resource, Packages> resourcePkgMap) { - if (currentBlame.m_cap.equals(candCap)) + if (candSources.contains(currentBlame.m_cap)) { return true; } - Set<Capability> candSources = getPackageSources(candCap, resourcePkgMap); Set<Capability> currentSources = getPackageSources(currentBlame.m_cap, resourcePkgMap); return currentSources.containsAll(candSources) || candSources.containsAll(currentSources); } private static boolean isCompatible( - List<Blame> currentBlames, Capability candCap, + List<Blame> currentBlames, Set<Capability> candSources, Map<Resource, Packages> resourcePkgMap) { int size = currentBlames.size(); @@ -1709,7 +1724,7 @@ public class ResolverImpl implements Resolver case 0: return true; case 1: - return isCompatible(currentBlames.get(0), candCap, resourcePkgMap); + return isCompatible(currentBlames.get(0), candSources, resourcePkgMap); default: Set<Capability> currentSources = new HashSet<Capability>(currentBlames.size()); for (Blame currentBlame : currentBlames) @@ -1717,7 +1732,6 @@ public class ResolverImpl implements Resolver Set<Capability> blameSources = getPackageSources(currentBlame.m_cap, resourcePkgMap); currentSources.addAll(blameSources); } - Set<Capability> candSources = getPackageSources(candCap, resourcePkgMap); return currentSources.containsAll(candSources) || candSources.containsAll(currentSources); } @@ -2073,7 +2087,7 @@ public class ResolverImpl implements Resolver System.out.println(" " + entry.getKey() + " - " + entry.getValue()); } System.out.println(" USED"); - for (Entry<String, ArrayMap<Capability, UsedBlames>> entry : packages.m_usedPkgs.entrySet()) + for (Entry<String, ArrayMap<Set<Capability>, UsedBlames>> entry : packages.m_usedPkgs.entrySet()) { System.out.println(" " + entry.getKey() + " - " + entry.getValue().values()); } @@ -2097,7 +2111,7 @@ public class ResolverImpl implements Resolver public final OpenHashMap<String, Blame> m_substitePkgs; public final OpenHashMap<String, List<Blame>> m_importedPkgs; public final OpenHashMap<String, List<Blame>> m_requiredPkgs; - public final OpenHashMap<String, ArrayMap<Capability, UsedBlames>> m_usedPkgs; + public final OpenHashMap<String, ArrayMap<Set<Capability>, UsedBlames>> m_usedPkgs; public final OpenHashMap<Capability, Set<Capability>> m_sources; @SuppressWarnings("serial") @@ -2118,12 +2132,12 @@ public class ResolverImpl implements Resolver return new ArrayList<Blame>(); } }; - m_usedPkgs = new OpenHashMap<String, ArrayMap<Capability, UsedBlames>>(128) { + m_usedPkgs = new OpenHashMap<String, ArrayMap<Set<Capability>, UsedBlames>>(128) { @Override - protected ArrayMap<Capability, UsedBlames> compute(String s) { - return new ArrayMap<Capability, UsedBlames>() { + protected ArrayMap<Set<Capability>, UsedBlames> compute(String s) { + return new ArrayMap<Set<Capability>, UsedBlames>() { @Override - protected UsedBlames compute(Capability key) { + protected UsedBlames compute(Set<Capability> key) { return new UsedBlames(key); } }; @@ -2177,18 +2191,18 @@ public class ResolverImpl implements Resolver */ private static class UsedBlames { - public final Capability m_cap; + public final Set<Capability> m_caps; public final List<Blame> m_blames = new ArrayList<ResolverImpl.Blame>(); private Map<Requirement, Set<Capability>> m_rootCauses; - public UsedBlames(Capability cap) + public UsedBlames(Set<Capability> caps) { - m_cap = cap; + m_caps = caps; } public void addBlame(Blame blame, Capability matchingRootCause) { - if (!m_cap.equals(blame.m_cap)) + if (!m_caps.contains(blame.m_cap)) { throw new IllegalArgumentException( "Attempt to add a blame with a different used capability: " |