Remove irrelevant attributes of plugin element in feature.xml

[HannesWell]

For Plug-ins included in a feature the Feature Editor adds many attributes to the plugin element in a feature.xml.
For a regular Plugin in the simplest form the element looks like

   <plugin
         id="foo.bar"
         download-size="0"
         install-size="0"
         version="0.0.0"
         unpack="false"/>

For a Fragment it is

   <plugin
         id="foo.bar.fragment"
         download-size="0"
         install-size="0"
         version="0.0.0"
         fragment="true"
         unpack="false"/>

Like for the version attribute, the value of the install/download-size are currently computed at build time and set in the final build jar of the feature.
But many of those attributes are unnecessary:

• install/download-size is also encoded in the P2 metadata and P2 only reads those. As far as I know nobody is reading those values from a feature
• unpack - if a bundle is unpacked during installation by P2 is controlled inside the Plugin with a Manifest header Eclipse-BundleShape: dir. The value of this attribute is read nowhere AFAIK.
• fragment - if a bundle is a fragment or not is controlled within the Plugin with a Manifest header Fragment-Host and a corresponding Bundle-Symbolic name as value. The value of this attribute is read nowhere AFAIK.

Since all these attributes are unnecessary respectively redundant they should all be removed. This means PDE should ignore them when it reads existing feature.xml files and should not write them writing the feature.xml file (yes, this will probably significantly shorten existing feature.xml on the next change).
Consequently all code associated (to read, write, etc) with those attributes should be removed and if necessary replaced by better means (e.g. the icon displayed for Plugins/Feature should be selected based on if it is a Plugin/Feature according to the manifest).

The attributes os, ws, nl, arch are currently considered by P2 as far as I can tell. On the long run we could consider to even derive those values from the value of the Eclipse-PlatformFilter or Require-Capability header and just install what can resolve in the current environment. But that's another discussion and out of the scope of this issue.

[HannesWell]

This seems to be a good first issue and everyone that wants to become more familiar with the PDE code-base is invited to work on this.
If you decide to work on this, please just let us and everyone know that to prevent duplicated work.
If nobody steps up within the next one or two month I will take care of this by myself.

[merks]

Indeed, these attributes are mostly confusing and misleading.

[vik-chand]

This seems to be a good first issue and everyone that wants to become more familiar with the PDE code-base is invited to work on this. If you decide to work on this, please just let us and everyone know that to prevent duplicated work. If nobody steps up within the next one or two month I will take care of this by myself.

@alshamams Do you want to take this up?

[alshamams]

Hi @vik-chand @HannesWell ,
Sure, I would like to take this up.

[HeikoKlare]

• unpack - if a bundle is unpacked during installation by P2 is controlled inside the Plugin with a Manifest header Eclipse-BundleShape: dir. The value of this attribute is read nowhere AFAIK.

It seems to me like this assumption is not completely correct and the removal of the unpack=false attributes produces some "regressions" or, at least, derivation of behavior. We face some problems because of plugins being unpacked during installation, which particularly affects the org.eclipse.equinox.launcher plugin that must not be unpacked. The problem is that the PDE feature export as well as the p2 metadata generation (FeaturesAndBundlesPublisherApplication), which is also used by tycho, somehow consider the unpack attribute. During p2 repository generation, the unpack attribute defaults to true (see here):

        public boolean isUnpack() {
                return (unpack == null || unpack.booleanValue());
        }

This is, for example, used to generate some advice to use bundle shape "dir", see here:

               if (entry.isUnpack() && entry.isPlugin() && !entry.isRequires())
                       publisherInfo.addAdvice(new BundleShapeAdvice(entry.getId(), Version.parseVersion(entry.getVersion()), IBundleShapeAdvice.DIR));

This functionality results in different artifact descriptions in the p2 metadata depending on whether unpack=false is specified or unpack is not specified at all. Precisely it adds the zipped instruction to an included plugin that is not defined as unpack=false. This, in turn, results in an unpack during p2 installation. You can see this when exporting the feature in following simple demo project and then putting the plugin into a targlet of a target platform: demo.zip
This will unpack the plugin during installation, even though the bundle shape is defined as default (i.e., jar) and the unpack attribute is not set.
For the demo project, the p2 metadata contain the following with unpack=false:

      <touchpointData size='1'>
        <instructions size='1'>
          <instruction key='manifest'>
            Bundle-SymbolicName: Plugin&#xA;Bundle-Version: 1.0.0.202402281700
          </instruction>
        </instructions>
      </touchpointData>

and they contain the following with unpack=true or unpack not being set:

      <touchpointData size='1'>
        <instructions size='2'>
          <instruction key='zipped'>
            true
          </instruction>
          <instruction key='manifest'>
            Bundle-SymbolicName: Plugin&#xA;Bundle-Version: 1.0.0.202402281700
          </instruction>
        </instructions>
      </touchpointData>

I am not sure which of the behaviors here is wrong / unintended (or misunderstood by me?): is it correct that the "zipped" instruction results in an unpack during p2 installation? Is it intended that PDE and p2 publisher consider the unpack attribute for p2 metadata generation? Maybe you can enlighten me, @HannesWell?

[laeubi]

@HeikoKlare can you derive a testcase for P2 and Tycho?

@merks might know better, but when an artifact is mirrored, we must use a jar/zip because only streams can be transfered (not folders), the "zipped" touchpoint now tells that this zip actually WAS a folder and therefore must be unsizipped.

That unpack is assumed to be true seems wrong for me, I can't find a proof for it only a screenshot here and how it behaved before that unpack actually was always false by default (from PDE side!):
https://help.eclipse.org/latest/topic/org.eclipse.pde.doc.user/guide/tools/editors/feature_editor/plugins.htm?cp=4_3_2_1_2

Even worse (and that's why we actually wanted to get rid of it), what should happen if one feature says unpack=true, one feature says unpack=false and the bundle says Shape:jar, who should win here? So I think this should probably be removed from P2 altogether.

[merks]

Yes, of course to transfer an artifact, we need it to be an artifact, a single file. Nothing really cares that sometime WAS a folder, but rather the artifact (or the use of the artifact) cars what it should be in the actual installation.

I agree that it really only makes sense of the bundle to decide its shape, not for some external use of it to make that decision and @laeubi has pointed out it's strange if two different features impose different shapes. That being said, I doubt anything is broken at runtime simply because a jar is unzipped when it doesn't need to be unzipped. Secondly, a bundle itself generally knows it should be unzipped, e.g., when it contains things like native libraries that necessarily must be accessed as direct file in the file system. But even for this case, the OSGi runtime will unpack the jar somewhere else such that it can use the contents directly as files; this results in duplication though, which is the reason why it makes sense for some artifacts to be unpacked.

[laeubi]

The Equinox launcher is used a a jar file with a main class so I'm not sure if java can use it as an exploded folder:
https://wiki.eclipse.org/Equinox_Launcher

Also jar signatures might or might not work in the form of a folder (never tested) and @tjwatson mentioned that sometimes jars are faster ass less file handles / fopen calls are required and some filesystms tend to assign a full block (4k for example) even for small files that only need less space and of course in general it could be confusing if everything is unpacked.

That's why I mentioned to have test cases that show the behavior and maybe if it causes issues, as it seems only be an issue at creation time of the repo one maybe would be able to mitigate the problem at least temporary.

[laeubi]

@HeikoKlare @merks sorry I just noticed too late, is this only a problem of FeaturesAndBundlesPublisherApplication? How is it called here? If we only encounter it with Tycho please provide an integration-test to demonstrate the issue, then we can probably fix / mitigate it there.

[HeikoKlare]

Thanks for the feedback and the information, @laeubi. That confirms my expectation that the produced "zipped" touchpoint is unexpected. I also agree that only the bundle itself should decide its shape, but unfortunately that's currently not the case.

The Equinox launcher is used a a jar file with a main class so I'm not sure if java can use it as an exploded folder:

That's what does not work for use: we build a customized launcher and that does not start anymore because of the launcher being unpacked during p2 installation.

sorry I just noticed too late, is this only a problem of FeaturesAndBundlesPublisherApplication

It is a problem of the FeaturesAndBundlesPublisherApplication, which is also used by tycho-extras (publish-features-and-bundles). In my opinion, the application should be corrected to not treat an unspecifed unpack attribute as unpack=true or maybe not even have that being used to decide about the bundle shape at all, i.e., completely remove the according code.

As mentioned, the problem also manifests in PDE feature export, but I did not find a hint that PDE also uses the FeaturesAndBundlesPublisherApplication for export but rather has an own implementation for that which considers the unpack attribute in an unexpected way.

can you derive a testcase for P2 and Tycho?

I can try to provide a reproducing test for p2 and maybe also for Tycho as soon as possible. Let me know if that's the best invest of my time right now or whether I can contribute something else (also in terms of potential emergency fixes, as you mentioned in the RC2 signoff, maybe for PDE feature export?).

[laeubi]

I'm working on a Test/fix for P2 so if you can reproduce the problem in Tycho it would be good, we maybe need an intermediate fix there...

[HeikoKlare]

Tycho simply uses the FeaturesAndBundlesPublisherApplication of p2, so if the p2 application is fixed, that should also apply to Tycho. But, of course, I try to create a reproducer that we can (also) use for Tycho.

[laeubi]

Tycho has some copy of P2 code (to speed up things for example) and some extensions (to handle deviations), even worse any fixes to P2 often go in only later after a release and this case seems not to be covered right now, specifically here some usages of the attributes where removed + tests but it didn't reveal the problem:

• Fix addition of unnecessary attributes in source-features and add Tests eclipse-tycho/tycho#2925
• Only set download/install-size attributes in features if they exist eclipse-tycho/tycho#2917

and as Tycho is probably the most important source of a lot of updatesites for platform / eclipse projects it seems we should fix this there as soon as possible.

[HeikoKlare]

@laeubi I created an initial draft for a Tycho test. Even though it's not a "proper" test yet, you can reproduce the problem with it: eclipse-tycho/tycho#3538

[HeikoKlare]

I just found that PDE (feature) export also uses the p2's FeaturesAction:

eclipse.pde/build/org.eclipse.pde.build/src_ant/org/eclipse/pde/internal/build/publisher/FeaturesAndBundlesTask.java

Line 46 in d89028d

application.addAction(new FeaturesAction(f));

So the fix in eclipse-equinox/p2#477 will also apply to PDE export functionality.