From 42ebff65742f76a4785b05539664ee6e3d31fbc6 Mon Sep 17 00:00:00 2001 From: Hannes Wellmann Date: Sat, 18 Nov 2023 21:16:49 +0100 Subject: [PATCH] Support no version and version ranges in target definitions Fixes https://github.com/eclipse-pde/eclipse.pde/issues/757 TODO: search for more issues?! --- .../core/target/IUBundleContainer.java | 128 ++++++++++++++---- .../core/target/IULocationFactory.java | 11 +- .../internal/core/target/P2TargetUtils.java | 18 ++- .../core/target/TargetPlatformService.java | 10 +- 4 files changed, 127 insertions(+), 40 deletions(-) diff --git a/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/IUBundleContainer.java b/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/IUBundleContainer.java index b140f2a75a..fa41e0c7ea 100644 --- a/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/IUBundleContainer.java +++ b/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/IUBundleContainer.java @@ -53,9 +53,8 @@ import org.eclipse.equinox.p2.engine.IProfile; import org.eclipse.equinox.p2.metadata.IArtifactKey; import org.eclipse.equinox.p2.metadata.IInstallableUnit; -import org.eclipse.equinox.p2.metadata.IVersionedId; import org.eclipse.equinox.p2.metadata.Version; -import org.eclipse.equinox.p2.metadata.VersionedId; +import org.eclipse.equinox.p2.metadata.VersionRange; import org.eclipse.equinox.p2.query.IQuery; import org.eclipse.equinox.p2.query.IQueryResult; import org.eclipse.equinox.p2.query.IQueryable; @@ -78,6 +77,56 @@ */ public class IUBundleContainer extends AbstractBundleContainer { + record UnitDescription(String id, VersionRange version) { + + static UnitDescription parse(String id, String version) { + VersionRange range; + if (Version.emptyVersion.toString().equals(version)) { + range = VersionRange.emptyRange; + } else if (version.contains(",")) { //$NON-NLS-1$ + range = VersionRange.create(version); + } else { + range = VersionRange.create('[' + version + ',' + version + ']'); + } + return new UnitDescription(id, range); + } + + static UnitDescription create(String id, Version version) { + VersionRange range = new VersionRange(version, true, version, true); + return new UnitDescription(id, range); + } + + UnitDescription { + Objects.requireNonNull(id); + Objects.requireNonNull(version); + } + + private boolean isSingleVersion() { + // TODO: also check bounds? if not both open, then it would be empty + // anyways... + return version.equals(VersionRange.emptyRange) || version.getMaximum().equals(version.getMinimum()); + } + + Optional getSingleVersion() { + return isSingleVersion() ? Optional.of(version.getMinimum()) : Optional.empty(); + } + + IQuery createIUQuery() { + return isSingleVersion() // + ? QueryUtil.createIUQuery(id, version.getMinimum()) + : QueryUtil.createIUQuery(id, version); + } + + String versionString() { + return getSingleVersion().map(Version::toString).orElseGet(version()::toString); + } + + @Override + public String toString() { + return VersionRange.emptyRange.equals(version) ? id : id + '/' + versionString(); + } + } + /** * Constant describing the type of bundle container */ @@ -114,7 +163,7 @@ public class IUBundleContainer extends AbstractBundleContainer { public static final int INCLUDE_CONFIGURE_PHASE = 1 << 3; /** The list of id+version of all root units declared in this container. */ - private List fIUs; + private List fIUs; /** * Cached IU's referenced by this bundle container, or null if not @@ -151,7 +200,7 @@ public class IUBundleContainer extends AbstractBundleContainer { * @param repositories metadata repositories used to search for IU's or null for default set * @param resolutionFlags bitmask of flags to control IU resolution, possible flags are {@link IUBundleContainer#INCLUDE_ALL_ENVIRONMENTS}, {@link IUBundleContainer#INCLUDE_REQUIRED}, {@link IUBundleContainer#INCLUDE_SOURCE}, {@link IUBundleContainer#INCLUDE_CONFIGURE_PHASE} */ - IUBundleContainer(Collection units, List repositories, int resolutionFlags) { + IUBundleContainer(Collection units, List repositories, int resolutionFlags) { fIUs = new ArrayList<>(units); fFlags = resolutionFlags; fRepos = List.copyOf(repositories); @@ -251,8 +300,8 @@ private void cacheIUs() throws CoreException { IProfile profile = fSynchronizer.getProfile(); List result = new ArrayList<>(); MultiStatus status = new MultiStatus(PDECore.PLUGIN_ID, 0, Messages.IUBundleContainer_ProblemsLoadingRepositories, null); - for (IVersionedId unit : fIUs) { - IQuery query = QueryUtil.createIUQuery(unit); + for (UnitDescription unit : fIUs) { + IQuery query = unit.createIUQuery(); addElement(profile, query, unit, result, status); } if (!status.isOK()) { @@ -262,8 +311,8 @@ private void cacheIUs() throws CoreException { fUnits = List.copyOf(result); } - private void addElement(IQueryable queryable, IQuery query, IVersionedId iu, - List result, MultiStatus status) { + private void addElement(IQueryable queryable, IQuery query, + UnitDescription iu, List result, MultiStatus status) { Optional queryResult = queryFirst(queryable, query, null); if (queryResult.isEmpty()) { status.add(Status.error(NLS.bind(Messages.IUBundleContainer_1, iu))); @@ -367,25 +416,36 @@ public synchronized IUBundleContainer update(Set toUpdate, IProgressMoni SubMonitor progress = SubMonitor.convert(monitor, 100); IQueryable source = P2TargetUtils.getQueryableMetadata(fRepos, progress.split(30)); boolean updated = false; - List updatedUnits = new ArrayList<>(fIUs); + List updatedUnits = new ArrayList<>(fIUs); SubMonitor loopProgress = progress.split(70).setWorkRemaining(updatedUnits.size()); for (int i = 0; i < updatedUnits.size(); i++) { - IVersionedId unit = updatedUnits.get(i); - if (!toUpdate.isEmpty() && !toUpdate.contains(unit.getId())) { + UnitDescription unit = updatedUnits.get(i); + if (!toUpdate.isEmpty() && !toUpdate.contains(unit.id())) { continue; } - IQuery query = QueryUtil.createLatestQuery(QueryUtil.createIUQuery(unit.getId())); + // TODO: why is createLatestQuery() used here but not in the other + // caller ? + IQuery query = QueryUtil.createLatestQuery(QueryUtil.createIUQuery(unit.id())); Optional queryResult = queryFirst(source, query, loopProgress.split(1)); Version updatedVersion = queryResult.map(IInstallableUnit::getVersion) // bail if the feature is no longer available. .orElseThrow(() -> new CoreException(Status.error(NLS.bind(Messages.IUBundleContainer_1, unit)))); // if the version is different from the spec (up or down), record the change. - if (!updatedVersion.equals(unit.getVersion())) { + // FIXME: check the new logic here again?! + Optional singleVersion = unit.getSingleVersion(); + + if (singleVersion.isPresent() && !updatedVersion.equals(singleVersion.get())) { updated = true; // if the spec was not specific (e.g., 0.0.0) the target def itself has changed. - if (!unit.getVersion().equals(Version.emptyVersion)) { - updatedUnits.set(i, new VersionedId(unit.getId(), updatedVersion)); + if (!singleVersion.get().equals(Version.emptyVersion)) { + updatedUnits.set(i, UnitDescription.create(unit.id(), updatedVersion)); } + } else { + // TODO: how to update version ranges properly. There are + // different strategies possible. + // E.g. compute its 'span' and apply it to the updated version + // as lower bound + throw new UnsupportedOperationException(); } } if (updated) { @@ -488,7 +548,14 @@ public List getRepositories() { * @param unit unit to remove from the list of root IUs */ public synchronized void removeInstallableUnit(IInstallableUnit unit) { - fIUs.remove(new VersionedId(unit.getId(), unit.getVersion())); + // TODO: TODO: maybe the units list can be converted to a map and map + // each unit to the declaring IU (if its not a dependency). Then we get + // an exact matched version for the range. Also consider this for the + // update method. + Optional toRemove = fIUs.stream() + .filter(iu -> iu.id().equals(unit.getId()) && iu.version().isIncluded(unit.getVersion())).findFirst(); + toRemove.ifPresent(fIUs::remove); + // fIUs.remove(new VersionedId(unit.getId(), unit.getVersion())); // Need to mark the container as unresolved clearResolutionStatus(); } @@ -565,7 +632,7 @@ public List getInstallableUnits() throws CoreException { } /** Returns installable unit identifiers and versions. */ - List getUnits() { + List getUnits() { return Collections.unmodifiableList(fIUs); } @@ -605,9 +672,17 @@ protected void associateWithTarget(ITargetDefinition target) { fSynchronizer.setIncludeConfigurePhase((fFlags & INCLUDE_CONFIGURE_PHASE) == INCLUDE_CONFIGURE_PHASE); } - private static final Comparator ID_FIRST_VERSION_SECOND = Comparator // - .comparing(IVersionedId::getId) // - .thenComparing(IVersionedId::getVersion); + private static final Comparator ID_FIRST_VERSION_SECOND = Comparator // + .comparing(UnitDescription::id) // + .thenComparing(unit -> { + Optional singleVersion = unit.getSingleVersion(); + if (singleVersion.isEmpty()) { + return singleVersion.get(); + } else { + // TODO: better solution? E.g. prefer version over ranges? + return unit.version().getMinimum(); + } + }); @Override public String serialize() { @@ -636,8 +711,8 @@ public String serialize() { // Generate a predictable order of the elements fIUs.stream().sorted(ID_FIRST_VERSION_SECOND).forEach(iu -> { Element unit = document.createElement(TargetDefinitionPersistenceHelper.INSTALLABLE_UNIT); - unit.setAttribute(TargetDefinitionPersistenceHelper.ATTR_ID, iu.getId()); - unit.setAttribute(TargetDefinitionPersistenceHelper.ATTR_VERSION, iu.getVersion().toString()); + unit.setAttribute(TargetDefinitionPersistenceHelper.ATTR_ID, iu.id()); + unit.setAttribute(TargetDefinitionPersistenceHelper.ATTR_VERSION, iu.versionString()); containerElement.appendChild(unit); }); try { @@ -659,9 +734,9 @@ IInstallableUnit[] getRootIUs(IProgressMonitor monitor) throws CoreException { IQueryable repos = P2TargetUtils.getQueryableMetadata(getRepositories(), monitor); MultiStatus status = new MultiStatus(PDECore.PLUGIN_ID, 0, Messages.IUBundleContainer_ProblemsLoadingRepositories, null); List result = new ArrayList<>(); - for (IVersionedId iu : fIUs) { + for (UnitDescription iu : fIUs) { // For versions such as 0.0.0, the IU query may return multiple IUs, so we check which is the latest version - IQuery query = QueryUtil.createLatestQuery(QueryUtil.createIUQuery(iu)); + IQuery query = QueryUtil.createLatestQuery(iu.createIUQuery()); addElement(repos, query, iu, result, status); } if (!status.isOK()) { @@ -671,6 +746,11 @@ IInstallableUnit[] getRootIUs(IProgressMonitor monitor) throws CoreException { return result.toArray(new IInstallableUnit[0]); } + // // FIXME: check discrepancy?! Is this even necessary?! We have a + // // specific version why the latest? In case the version is 0.0.0! But + // // isn't that also interesting fpr the other caller?! + // IQuery query = QueryUtil.createLatestQuery(iuQuery); + @Override public T getAdapter(Class adapter) { if (adapter.isInstance(fSynchronizer)) { diff --git a/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/IULocationFactory.java b/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/IULocationFactory.java index 11f2e7d18a..8531da7f62 100644 --- a/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/IULocationFactory.java +++ b/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/IULocationFactory.java @@ -22,6 +22,7 @@ import org.eclipse.core.runtime.CoreException; import org.eclipse.core.runtime.Status; +import org.eclipse.equinox.p2.metadata.Version; import org.eclipse.pde.core.target.ITargetLocation; import org.eclipse.pde.core.target.ITargetLocationFactory; import org.w3c.dom.Document; @@ -72,10 +73,11 @@ public ITargetLocation getTargetLocation(String type, String serializedXML) thro String id = element.getAttribute(TargetDefinitionPersistenceHelper.ATTR_ID); if (id.length() > 0) { String version = element.getAttribute(TargetDefinitionPersistenceHelper.ATTR_VERSION); - if (version.length() > 0) { - ids.add(id); - versions.add(version); + if (version.isBlank()) { + version = Version.emptyVersion.toString(); } + ids.add(id); + versions.add(version); } } else if (element.getNodeName().equalsIgnoreCase(TargetDefinitionPersistenceHelper.REPOSITORY)) { String loc = element.getAttribute(TargetDefinitionPersistenceHelper.LOCATION); @@ -101,8 +103,7 @@ public ITargetLocation getTargetLocation(String type, String serializedXML) thro flags |= Boolean.parseBoolean(includeAllPlatforms) ? IUBundleContainer.INCLUDE_ALL_ENVIRONMENTS : 0; flags |= Boolean.parseBoolean(includeSource) ? IUBundleContainer.INCLUDE_SOURCE : 0; flags |= Boolean.parseBoolean(includeConfigurePhase) ? IUBundleContainer.INCLUDE_CONFIGURE_PHASE : 0; - return TargetPlatformService.getDefault().newIULocation(iuIDs, iuVer, uris, - flags); + return TargetPlatformService.getDefault().newIULocation(iuIDs, iuVer, uris, flags); } return null; } diff --git a/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/P2TargetUtils.java b/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/P2TargetUtils.java index 7a5f01ebcf..ec4255434b 100644 --- a/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/P2TargetUtils.java +++ b/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/P2TargetUtils.java @@ -64,7 +64,6 @@ import org.eclipse.equinox.p2.metadata.IInstallableUnit; import org.eclipse.equinox.p2.metadata.IProvidedCapability; import org.eclipse.equinox.p2.metadata.IRequirement; -import org.eclipse.equinox.p2.metadata.IVersionedId; import org.eclipse.equinox.p2.metadata.MetadataFactory; import org.eclipse.equinox.p2.metadata.MetadataFactory.InstallableUnitDescription; import org.eclipse.equinox.p2.metadata.Version; @@ -87,9 +86,9 @@ import org.eclipse.pde.core.target.ITargetHandle; import org.eclipse.pde.core.target.ITargetLocation; import org.eclipse.pde.core.target.ITargetPlatformService; -import org.eclipse.pde.core.target.NameVersionDescriptor; import org.eclipse.pde.internal.core.ICoreConstants; import org.eclipse.pde.internal.core.PDECore; +import org.eclipse.pde.internal.core.target.IUBundleContainer.UnitDescription; import org.eclipse.pde.internal.core.util.CoreUtility; import org.osgi.framework.BundleContext; import org.osgi.framework.InvalidSyntaxException; @@ -518,9 +517,9 @@ private boolean checkProfile(ITargetDefinition target, final IProfile profile) { // still in the profile, we need to recreate (rather than uninstall) IUProfilePropertyQuery propertyQuery = new IUProfilePropertyQuery(PROP_INSTALLED_IU, Boolean.toString(true)); IQueryResult queryResult = profile.query(propertyQuery, null); - Set installedIUs = new HashSet<>(); + Map> installedIUs = new HashMap<>(); for (IInstallableUnit unit : queryResult) { - installedIUs.add(new NameVersionDescriptor(unit.getId(), unit.getVersion().toString())); + installedIUs.computeIfAbsent(unit.getId(), id -> new ArrayList<>(1)).add(unit); } ITargetLocation[] containers = target.getTargetLocations(); if (containers == null) { @@ -528,9 +527,16 @@ private boolean checkProfile(ITargetDefinition target, final IProfile profile) { } for (ITargetLocation container : containers) { if (container instanceof IUBundleContainer bc) { - for (IVersionedId iu : bc.getUnits()) { + for (UnitDescription iu : bc.getUnits()) { // if there is something in a container but not in the profile, recreate - if (!installedIUs.remove(new NameVersionDescriptor(iu.getId(), iu.getVersion().toString()))) { + boolean[] removed = { false }; + installedIUs.computeIfPresent(iu.id(), (id, units) -> { + // TODO: what happens if there are two ranges, one more + // specific and one wider that match different IUs? + removed[0] = units.removeIf(u -> iu.version().isIncluded(u.getVersion())); + return units.isEmpty() ? null : units; + }); + if (!removed[0]) { return false; } } diff --git a/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/TargetPlatformService.java b/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/TargetPlatformService.java index 67c1660cf9..238ef6d6f2 100644 --- a/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/TargetPlatformService.java +++ b/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/TargetPlatformService.java @@ -61,8 +61,6 @@ import org.eclipse.e4.core.services.events.IEventBroker; import org.eclipse.equinox.frameworkadmin.BundleInfo; import org.eclipse.equinox.p2.metadata.IInstallableUnit; -import org.eclipse.equinox.p2.metadata.IVersionedId; -import org.eclipse.equinox.p2.metadata.VersionedId; import org.eclipse.osgi.service.datalocation.Location; import org.eclipse.osgi.util.NLS; import org.eclipse.pde.core.plugin.IPluginModelBase; @@ -77,6 +75,7 @@ import org.eclipse.pde.internal.core.PDECore; import org.eclipse.pde.internal.core.PDEPreferencesManager; import org.eclipse.pde.internal.core.TargetDefinitionManager; +import org.eclipse.pde.internal.core.target.IUBundleContainer.UnitDescription; import org.osgi.service.prefs.BackingStoreException; /** @@ -644,7 +643,8 @@ public IStatus compareWithTargetPlatform(ITargetDefinition target) throws CoreEx @Override public ITargetLocation newIULocation(IInstallableUnit[] units, URI[] repositories, int resolutionFlags) { - Stream ius = Arrays.stream(units).map(iu -> new VersionedId(iu.getId(), iu.getVersion())); + Stream ius = Arrays.stream(units) + .map(iu -> UnitDescription.create(iu.getId(), iu.getVersion())); return new IUBundleContainer(ius.toList(), List.of(repositories), resolutionFlags); } @@ -653,8 +653,8 @@ public ITargetLocation newIULocation(String[] unitIds, String[] versions, URI[] if (unitIds.length != versions.length) { throw new IllegalArgumentException("Units and versions must have the same length"); //$NON-NLS-1$ } - Stream ius = IntStream.range(0, unitIds.length) - .mapToObj(i -> new VersionedId(unitIds[i], versions[i])); + Stream ius = IntStream.range(0, unitIds.length) + .mapToObj(i -> UnitDescription.parse(unitIds[i], versions[i])); return new IUBundleContainer(ius.toList(), List.of(repositories), resolutionFlags); }