Consider other locations when resolving IUs from updatesites #792

laeubi · 2023-10-08T17:47:06Z

Currently the IULocation can only consider units from other locations if these are also of type IU location but other target types might be able to supply items in a way not know to the IULocation.

This adds the necessary infrastructure to support this usecase by using the adapter pattern.

Fix #207
Requires eclipse-equinox/p2#348

github-actions · 2023-10-08T17:51:42Z

Test Results

    273 files +      6     273 suites +6 1h 4m 43s ⏱️ + 19m 8s
  3 340 tests -       1   3 310 ✔️ ±      0 30 💤 ±  0 0 ❌ - 1
10 317 runs +2 721 10 227 ✔️ +2 698 90 💤 +24 0 ❌ - 1

Results for commit 3e98782. ± Comparison against base commit fccc098.

This pull request removes 1 test.

AllPDETests org.eclipse.pde.ui.tests.launcher.AllLauncherTests org.eclipse.pde.ui.tests.launcher.FeatureBasedLaunchTest ‑ Unknown test

♻️ This comment has been updated with latest results.

ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/P2TargetUtils.java

laeubi · 2023-10-09T08:24:10Z

This is now ready as the first minimal increment. It is likely that there are some changes one like to adjust sometime but at least this allows to resolve an IU location when using a Directory location that supply a missing bundle as in this example:

<target name="target_require_other">
   <locations>
      <location includeAllPlatforms="false" includeConfigurePhase="false" includeMode="planner" includeSource="true" type="InstallableUnit">
         <repository location="https://download.eclipse.org/linuxtools/updates-docker-nightly/"/>
         <unit id="com.google.guava" version="0.0.0"/>
      </location>
      <location path="${project_loc:/test.pde}/lib" type="Directory"/>
      </locations>
</target>

laeubi · 2023-10-10T07:14:23Z

Testfailure seems unrelated, build is otherwhise green...

laeubi · 2023-10-11T06:43:39Z

Using a Generic Type is simply bad and fixing this later on up by adding filter only complicates the whole story for absolutely no gain and will limit our ability to later, also this discussion about "API" or "not API" does not bring any value this is not API this is a decoupled way of using optional features and if we remove it and later replace it by some other means things have to be adjusted or one can use another Adapter that bridges old -> new behavior as done already in the past when migrating to the more feature rich target locations..

With that squashed into your commit I would be more than happy to have this great new feature. :)

Because of mentioned flaws I discard that change. Let me know if we should close this instead as I don't want to invest more time in a crippled solution that sooner or later will break or only works by some inherit assumptions about Types we never can reliable enforce..

Currently the IULocation can only consider units from other locations if these are also of type IU location but other target types might be able to supply items in a way not know to the IULocation. This adds the necessary infrastructure to support this usecase by using the adapter pattern. Fix eclipse-pde#207

laeubi · 2023-10-22T17:24:39Z

@HannesWell I now reworked the whole part so it automatically queries the locations and converts them I hope this now addresses your concerns.

HannesWell

@HannesWell I now reworked the whole part so it automatically queries the locations and converts them I hope this now addresses your concerns.

Yes, this looks perfectly fine, thanks. 👍🏽
I'm glad we came to a better solution that has less requirements and also works without adjustments in other target-locations. :)

Just two small style-remarks below, but nothing critical.

HannesWell · 2023-10-22T21:44:00Z

ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/InstallableUnitGenerator.java

+	public static Stream<IInstallableUnit> generateInstallableUnits(TargetBundle[] bundles) {
+		if (bundles == null || bundles.length == 0) {
+			return Stream.empty();
+		}
+		return Arrays.stream(bundles).flatMap(InstallableUnitGenerator::generateInstallableUnits);
+	}
+
+	public static Stream<IInstallableUnit> generateInstallableUnits(TargetBundle targetBundle) {
+		BundleInfo bundleInfo = targetBundle.getBundleInfo();
+		if (bundleInfo != null) {
+			String manifest = bundleInfo.getManifest();
+			if (manifest != null) {
+				try {
+					Manifest parsed = new Manifest(new ByteArrayInputStream(manifest.getBytes(StandardCharsets.UTF_8)));
+					Attributes mainAttributes = parsed.getMainAttributes();
+					CaseInsensitiveDictionaryMap<String, String> headers = new CaseInsensitiveDictionaryMap<>(
+							mainAttributes.size());
+					Set<Entry<Object, Object>> entrySet = mainAttributes.entrySet();
+					for (Entry<Object, Object> entry : entrySet) {
+						headers.put(entry.getKey().toString(), entry.getValue().toString());
+					}
+					PublisherInfo publisherInfo = new PublisherInfo();
+					publisherInfo.setArtifactOptions(IPublisherInfo.A_INDEX);
+					BundleDescription bundleDescription = BundlesAction.createBundleDescription(headers, null);
+					IInstallableUnit iu = BundlesAction.createBundleIU(bundleDescription,
+							BundlesAction.createBundleArtifactKey(bundleDescription.getSymbolicName(),
+									bundleDescription.getVersion().toString()),
+							publisherInfo);
+					return Stream.of(iu);
+				} catch (IOException e) {
+					// can't use it then...
+				}
+			}
+		}
+		return Stream.empty();
+	}


Using Stream.mapMulti() would save the creation of Streams with one element.

Suggested change

public static Stream<IInstallableUnit> generateInstallableUnits(TargetBundle[] bundles) {

if (bundles == null || bundles.length == 0) {

return Stream.empty();

}

return Arrays.stream(bundles).flatMap(InstallableUnitGenerator::generateInstallableUnits);

}

public static Stream<IInstallableUnit> generateInstallableUnits(TargetBundle targetBundle) {

BundleInfo bundleInfo = targetBundle.getBundleInfo();

if (bundleInfo != null) {

String manifest = bundleInfo.getManifest();

if (manifest != null) {

try {

Manifest parsed = new Manifest(new ByteArrayInputStream(manifest.getBytes(StandardCharsets.UTF_8)));

Attributes mainAttributes = parsed.getMainAttributes();

CaseInsensitiveDictionaryMap<String, String> headers = new CaseInsensitiveDictionaryMap<>(

mainAttributes.size());

Set<Entry<Object, Object>> entrySet = mainAttributes.entrySet();

for (Entry<Object, Object> entry : entrySet) {

headers.put(entry.getKey().toString(), entry.getValue().toString());

}

PublisherInfo publisherInfo = new PublisherInfo();

publisherInfo.setArtifactOptions(IPublisherInfo.A_INDEX);

BundleDescription bundleDescription = BundlesAction.createBundleDescription(headers, null);

IInstallableUnit iu = BundlesAction.createBundleIU(bundleDescription,

BundlesAction.createBundleArtifactKey(bundleDescription.getSymbolicName(),

bundleDescription.getVersion().toString()),

publisherInfo);

return Stream.of(iu);

} catch (IOException e) {

// can't use it then...

}

}

}

return Stream.empty();

}

private static Stream<IInstallableUnit> generateInstallableUnits(TargetBundle[] bundles) {

if (bundles == null || bundles.length == 0) {

return Stream.empty();

}

return Arrays.stream(bundles).mapMulti(InstallableUnitGenerator::generateInstallableUnits);

}

private static void generateInstallableUnits(TargetBundle targetBundle, Consumer<IInstallableUnit> downStream) {

BundleInfo bundleInfo = targetBundle.getBundleInfo();

if (bundleInfo != null) {

String manifest = bundleInfo.getManifest();

if (manifest != null) {

try {

Manifest parsed = new Manifest(new ByteArrayInputStream(manifest.getBytes(StandardCharsets.UTF_8)));

Attributes mainAttributes = parsed.getMainAttributes();

CaseInsensitiveDictionaryMap<String, String> headers = new CaseInsensitiveDictionaryMap<>(

mainAttributes.size());

Set<Entry<Object, Object>> entrySet = mainAttributes.entrySet();

for (Entry<Object, Object> entry : entrySet) {

headers.put(entry.getKey().toString(), entry.getValue().toString());

}

PublisherInfo publisherInfo = new PublisherInfo();

publisherInfo.setArtifactOptions(IPublisherInfo.A_INDEX);

BundleDescription bundleDescription = BundlesAction.createBundleDescription(headers, null);

IInstallableUnit iu = BundlesAction.createBundleIU(bundleDescription,

BundlesAction.createBundleArtifactKey(bundleDescription.getSymbolicName(),

bundleDescription.getVersion().toString()),

publisherInfo);

downStream.accept(iu);

} catch (IOException e) {

// can't use it then...

}

}

}

}

HannesWell · 2023-10-22T21:44:17Z

ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/InstallableUnitGenerator.java

+		return Stream.empty();
+	}
+
+	public static Stream<IInstallableUnit> generateInstallableUnits(TargetFeature[] features) {


Suggested change

public static Stream<IInstallableUnit> generateInstallableUnits(TargetFeature[] features) {

private static Stream<IInstallableUnit> generateInstallableUnits(TargetFeature[] features) {

HannesWell · 2023-10-22T21:44:24Z

ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/InstallableUnitGenerator.java

+		return Arrays.stream(features).flatMap(InstallableUnitGenerator::generateInstallableUnits);
+	}
+
+	public static Stream<IInstallableUnit> generateInstallableUnits(TargetFeature targetFeature) {


Suggested change

public static Stream<IInstallableUnit> generateInstallableUnits(TargetFeature targetFeature) {

private static Stream<IInstallableUnit> generateInstallableUnits(TargetFeature targetFeature) {

…e#792 Given that units from non-IU locations are considered when resolving IUs, it may happen that units are contained in the resolution path, which are not contained by any repository. This may break previously functional target definitions. To resolve this problem, an in-memory artifact and metadata repository is created, based on the metadata of the non-IU locations. Those repositories are then added to the provisioning context for further processing. Amends dd321b5

HannesWell reviewed Oct 8, 2023

View reviewed changes

ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/P2TargetUtils.java Outdated Show resolved Hide resolved

laeubi force-pushed the allow_other_locations_to_supply_units branch from bcedeb8 to 66a5c24 Compare October 9, 2023 08:16

laeubi marked this pull request as ready for review October 9, 2023 08:18

HannesWell force-pushed the allow_other_locations_to_supply_units branch from 66a5c24 to 04fc433 Compare October 10, 2023 22:04

laeubi force-pushed the allow_other_locations_to_supply_units branch from f31c542 to 66a5c24 Compare October 11, 2023 06:44

laeubi force-pushed the allow_other_locations_to_supply_units branch 2 times, most recently from 968dac7 to e825d30 Compare October 22, 2023 09:17

laeubi mentioned this pull request Oct 22, 2023

Rework TargetDefinition#resolve #812

Merged

laeubi force-pushed the allow_other_locations_to_supply_units branch from e825d30 to 3e98782 Compare October 22, 2023 17:23

HannesWell approved these changes Oct 22, 2023

View reviewed changes

laeubi merged commit dd321b5 into eclipse-pde:master Oct 23, 2023
16 checks passed

HannesWell mentioned this pull request Oct 24, 2023

Revert "Add IInstallableUnitQueryable interface to allow easier adaption" eclipse-equinox/p2#360

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consider other locations when resolving IUs from updatesites #792

Consider other locations when resolving IUs from updatesites #792

laeubi commented Oct 8, 2023

github-actions bot commented Oct 8, 2023 •

edited

Loading

laeubi commented Oct 9, 2023

laeubi commented Oct 10, 2023

laeubi commented Oct 11, 2023 •

edited

Loading

laeubi commented Oct 22, 2023

HannesWell left a comment

HannesWell Oct 22, 2023

HannesWell Oct 22, 2023

HannesWell Oct 22, 2023

	public static Stream<IInstallableUnit> generateInstallableUnits(TargetFeature[] features) {
	private static Stream<IInstallableUnit> generateInstallableUnits(TargetFeature[] features) {

Consider other locations when resolving IUs from updatesites #792

Consider other locations when resolving IUs from updatesites #792

Conversation

laeubi commented Oct 8, 2023

github-actions bot commented Oct 8, 2023 • edited Loading

Test Results

laeubi commented Oct 9, 2023

laeubi commented Oct 10, 2023

laeubi commented Oct 11, 2023 • edited Loading

laeubi commented Oct 22, 2023

HannesWell left a comment

Choose a reason for hiding this comment

HannesWell Oct 22, 2023

Choose a reason for hiding this comment

HannesWell Oct 22, 2023

Choose a reason for hiding this comment

HannesWell Oct 22, 2023

Choose a reason for hiding this comment

github-actions bot commented Oct 8, 2023 •

edited

Loading

laeubi commented Oct 11, 2023 •

edited

Loading