Absence of an unpack attribute must not result in BundleShapeAdvice

Currently the FeaturesAction assumes that the unpack attribute is always
set, if that is not the case it leads to unexpected results because then
unpack=true is assumed always. This is especially dangerous as PDE since
2023-12 release removes the attributes and Tycho assume they are false
by default the same as PDE has done in the past when adding new items.

To mitigate this this adds an additional check to only evaluate the
unpack if it is set, together with a testcase that covers all possible
combinations.

bundles/org.eclipse.equinox.p2.publisher.eclipse/src/org/eclipse/equinox/p2/publisher/eclipse/FeaturesAction.java

@@ -186,7 +186,7 @@ private void createAdviceFileAdvice(Feature feature, IPublisherInfo publisherInf
 	private void createBundleShapeAdvice(Feature feature, IPublisherInfo publisherInfo) {
 		FeatureEntry entries[] = feature.getEntries();
 		for (FeatureEntry entry : entries) {
-			if (entry.isUnpack() && entry.isPlugin() && !entry.isRequires())
+			if (entry.unpackSet() && entry.isUnpack() && entry.isPlugin() && !entry.isRequires())
 				publisherInfo.addAdvice(new BundleShapeAdvice(entry.getId(), Version.parseVersion(entry.getVersion()), IBundleShapeAdvice.DIR));
 		}
 	}

bundles/org.eclipse.equinox.p2.tests/src/org/eclipse/equinox/p2/tests/AbstractProvisioningTest.java

@@ -739,7 +739,7 @@ private static void indent(OutputStream output, int indent) {
 			}
 	}
 
-	public static void writeBuffer(File outputFile, StringBuilder buffer) throws IOException {
+	public static void writeBuffer(File outputFile, CharSequence buffer) throws IOException {
 		outputFile.getParentFile().mkdirs();
 		try (FileOutputStream stream = new FileOutputStream(outputFile)) {
 			stream.write(buffer.toString().getBytes());

bundles/org.eclipse.equinox.p2.tests/src/org/eclipse/equinox/p2/tests/publisher/actions/FeaturesActionTest.java

@@ -48,13 +48,15 @@
 import org.eclipse.equinox.p2.publisher.IPublisherInfo;
 import org.eclipse.equinox.p2.publisher.IPublisherResult;
 import org.eclipse.equinox.p2.publisher.PublisherInfo;
+import org.eclipse.equinox.p2.publisher.PublisherResult;
 import org.eclipse.equinox.p2.publisher.actions.IAdditionalInstallableUnitAdvice;
 import org.eclipse.equinox.p2.publisher.actions.ICapabilityAdvice;
 import org.eclipse.equinox.p2.publisher.actions.IFeatureRootAdvice;
 import org.eclipse.equinox.p2.publisher.actions.IPropertyAdvice;
 import org.eclipse.equinox.p2.publisher.actions.ITouchpointAdvice;
 import org.eclipse.equinox.p2.publisher.actions.IUpdateDescriptorAdvice;
 import org.eclipse.equinox.p2.publisher.eclipse.FeaturesAction;
+import org.eclipse.equinox.p2.publisher.eclipse.IBundleShapeAdvice;
 import org.eclipse.equinox.p2.repository.artifact.IArtifactDescriptor;
 import org.eclipse.equinox.p2.tests.TestActivator;
 import org.eclipse.equinox.p2.tests.TestData;
@@ -191,6 +193,50 @@ public void testMatchGreaterOrEqual() throws Exception {
 		}
 	}
 
+	public void testUnpack() throws Exception {
+		File testFolder = getTestFolder("FeaturesAction.testUnpack");
+		File featureXML = new File(testFolder, "feature.xml");
+		writeBuffer(featureXML, """
+				<feature id="test.feature" version="1.0.0" >
+				   <plugin id="org.plugin1" version ="1.0.0" />
+				   <plugin id="org.plugin2" version ="1.0.0" unpack="true" />
+				   <plugin id="org.plugin3" version ="1.0.0" unpack="false" />
+				   <requires>
+				      <import plugin="org.import1"/>
+				      <import plugin="org.import2" unpack="true"/>
+				   	  <import plugin="org.import3" unpack="false"/>
+				   </requires>
+				</feature>
+								""");
+		PublisherInfo info = new PublisherInfo();
+		FeaturesAction action = new FeaturesAction(new File[] { testFolder });
+		PublisherResult results = new PublisherResult();
+		action.perform(info, results, new NullProgressMonitor());
+		// no unpack attribute --> jar
+		assertShapeAdvice(info, "org.plugin1", IBundleShapeAdvice.JAR);
+		// unpack=true --> dir
+		assertShapeAdvice(info, "org.plugin2", IBundleShapeAdvice.DIR);
+		// unpack=false --> JAR
+		assertShapeAdvice(info, "org.plugin3", IBundleShapeAdvice.JAR);
+		// imports should never unpack as attribute is invalid for them!
+		assertShapeAdvice(info, "org.import1", IBundleShapeAdvice.JAR);
+		assertShapeAdvice(info, "org.import2", IBundleShapeAdvice.JAR);
+		assertShapeAdvice(info, "org.import3", IBundleShapeAdvice.JAR);
+	}
+
+	protected void assertShapeAdvice(PublisherInfo info, String bundle, String shape) {
+		for (IBundleShapeAdvice shapeAdvice : info.getAdvice(null, true, bundle, Version.create("1.0.0"),
+				IBundleShapeAdvice.class)) {
+			assertEquals(bundle + " has wrong advice", shape, shapeAdvice.getShape());
+			return;
+		}
+		if (IBundleShapeAdvice.JAR.equals(shape)) {
+			return;
+		}
+		fail(shape + " was expected for " + bundle + " but no advice was found!");
+
+	}
+
 	public void testFilters() throws Exception {
 		File testFolder = getTestFolder("FeaturesAction.testFilters");
 		StringBuilder buffer = new StringBuilder();