Remove unused attributes in auto-generated feature.xml

[alshamams]

This commit removes some of the unused attributes of plugin element, which are redundant and are never read from a feature.xml file: install-size, download size and unpack attributes.

Fixes: 730

[github-actions]

Test Results

     270 files  ±0       270 suites  ±0   1h 14m 13s ⏱️ + 6m 46s
  3 327 tests ±0    3 297 ✔️ ±0  30 💤 ±0  0 ❌ ±0 
10 278 runs  ±0  10 188 ✔️ ±0  90 💤 ±0  0 ❌ ±0 

Results for commit 67a7a83. ± Comparison against base commit 6465a65.

♻️ This comment has been updated with latest results.

[alshamams]

I did not remove the fragment attribute, as it causes warnings in feature.xml compiler. I will investigate where the compiler is processing that, and create a separate PR to take that out

[alshamams]

Investigating the test failures.

[HannesWell]

@alshamams thanks for working on this.
It looks like this change can go much further and remove all modelling of all attributes mentioned in #730 (comment) as well as their reading and writing and the UI support for them.
Eventually they should not be written, read or displayed in the UI. For existing Features (e.g. from a target-platform) those attributes should just be ignored. New or modified features should have them removed (which I assume happens automatically if they are not read and written on changes, so you don't have to add some logic to actively remove them).
To make it short, anything that belongs to those attributes should be removed.

[HannesWell]

Thanks @alshamams, after a quick look this looks much better. 👍🏽
I'm away over the weekend until mid of next week, but will review it in detail when back.
Of course everybody else review is also welcome.

[HannesWell]

I reviewed this in detail and everthing that was removed is fine.
There was just a little bit more to remove.
In FeaturePlugin the fFragment field can be removed and instead isFragment() returns if the PluginBase model is a fragment. The returned value is only considered to select the icon of an included Plugin in the Feature editor.
This also removes the fragment attribute, which should not be read or written anymore.

The method FeatureExportOperation.setAdditionalAttributes() can be removed because all overrides are now removed and the texts for all removed elements in the PluginDetailsSection in the Feature Edeitor can be removed too.

For all of that I have pushed another commit at the tip of your branch.

I also verified that with your change and all the unnecessary attributes are not written and displayed anymore. 👍🏽

Please address the only remaining comment and squash all your commits and mine into one commit with a reasonable overall message.
Then I think this should be ready for submission.

@laeubi can you tell if Tycho already handles features with all those features removed properly?

[HannesWell]

Instead of implementing a logic without isFragment I think it is sufficient to remove that parameter from validatePluginExists and remove the associated logic in that method and then just call:

Suggested change

	String id = attr.getValue();
	int severity = CompilerFlags.getFlag(fProject, CompilerFlags.F_UNRESOLVED_PLUGINS);
	if (severity != CompilerFlags.IGNORE) {
	IPluginModelBase model = PluginRegistry.findModel(id);
	if (model == null || !model.isEnabled()) {
	VirtualMarker marker = report(NLS.bind(PDECoreMessages.Builders_Feature_reference, id),
	getLine(plugin, attr.getName()), severity, PDEMarkerFactory.CAT_OTHER);
	addMarkerAttribute(marker, PDEMarkerFactory.compilerKey,
	CompilerFlags.F_UNRESOLVED_PLUGINS);
	}
	}
	validatePluginExists(plugin, attr);

[szarnekow]

@HannesWell Do I read this part as you intended?

In FeaturePlugin the fFragment field can be removed

Is this a breaking change? Would existing feature.xml files no longer be understood by the newest revision of the parser?

Why I am asking this:
We build a product based on Eclipse, compatible with many Eclipse versions. We use multiple target platforms in our build as a sanity check to ensure that we can compile against old and new versions before we run our tests against different TPs, too. Our workspace doesn't have any warnings.
Old versions require the fragment property to be present in the feature.xml if the feature includes a fragment. If the new version doesn't allow this anymore, it will not be source-compatible and require a major version increment. Would it be worth it?

[HannesWell]

In FeaturePlugin the fFragment field can be removed

Is this a breaking change? Would existing feature.xml files no longer be understood by the newest revision of the parser?

It shouldn't be a breaking change. For existing feature.xml the now unsupported attributes are simply ignored. And the other way round if e.g. the download-size attribute is absent a current Eclipse simply shows a 0 in the Editor.
You can verify that by simplify removing the attributes to be discontinued in a current workspace.

Our workspace doesn't have any warnings.
Old versions require the fragment property to be present in the feature.xml if the feature includes a fragment.

Yes a missing fragment attribute leads to a warning.
If that isn't possible it also isn't a problem to keep the fragment attribute (or the others) as they are then simply ignored. Those attributes are not even reported as Illegal attributes since the validation is (for now) kept intact:

eclipse.pde/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/builders/FeatureErrorReporter.java

Lines 124 to 126 in 6b0f0ed

	} else if (!name.equals("os") && !name.equals("ws") && !name.equals("nl") //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$
	&& !name.equals("arch") && !name.equals("download-size") //$NON-NLS-1$ //$NON-NLS-2$
	&& !name.equals("install-size") && !name.equals("filter")) { //$NON-NLS-1$ //$NON-NLS-2$

Some time later we can consider to remove even that so that a Illegal attribute will be issued for them if they are present.
The only inconvenience is that each time you modify a feature.xml through the editor all unknown attributes get removed.

But what makes me wonder, why can't you just use the latest Eclipse release?
Once a Feature is build and is in the TP it should not be validated anymore by the FeatureErrorReporter, so downstream consumers should not be affected.

[HannesWell]

I also checked Tycho. A feature.xml without install-size, etc. can be build even with a current Tycho. But at the moment Tycho then still adds the install/download-size into the build feature.xml.
That should be adjusted.

[alshamams]

Hi @HannesWell ,
I have addressed your latest review comment and squashed the commits. Kindly have a look and let me know.

[vogella]

@HannesWell have your concerns be adressed?

[HannesWell]

Hi @HannesWell , I have addressed your latest review comment and squashed the commits. Kindly have a look and let me know.

Thanks for the update @alshamams! Your change looks good now. 👍🏽
I also checked the code-base of P2 for references to the attributes about to be removed and found references to unpack

But while checking the P2 code-base I found some references to the attributes about to be removed, i.e. the unpack attribute, where I'm not yet entirely sure if it is absolutely not used. I already checked with Tycho that it has no effect, but I have to figure out if the same applies for all use cases in P2. I'm working on it but would like to have confidence about it before submitting this.
Thus I would like to keep this open probably for a few more days, but you can probably consider this work as done.

[HannesWell]

A quick update on this one.
The next Tycho release will handle feature.xml without the attributes to be removed properly (i.e. does not add the inferred sizes).
For Tycho 5 there is no full aggreement yet how to handle those attributes, but since its release is further in the future that's not a concern for this one.

What's currently blocking the submission of this one is that CoreUtility.guessUnpack() is currently used in P2 and we need to remove its usage before we can remove it here. Unfortunately the usage is quite convoluted and I didn't went 100% through it yet.
I'll be at EclipseCon from tomorrow on and will therefore not be able to continue the work on the P2 site until the end of the coming week. If any body of you attends as well, I'll be glad to have a chat about this. :)

[vogella]

Is there an issue to follow the work of CoreUtility.guessUnpack()?

Maybe remove the UI elements as a first step?

[HannesWell]

Is there an issue to follow the work of CoreUtility.guessUnpack()?

I just created eclipse-equinox/p2#368 to some related work on P2. But it looks like PDE-build again uses some parts of that again.
However the good news is that I confused CoreUtility.guessUnpack() with another Utility class in P2 and it is in fact not used and this change is ready for submission now.

Thank you @alshamams for your work on this issue.

[alshamams]

Is there an issue to follow the work of CoreUtility.guessUnpack()?

I just created eclipse-equinox/p2#368 to some related work on P2. But it looks like PDE-build again uses some parts of that again. However the good news is that I confused CoreUtility.guessUnpack() with another Utility class in P2 and it is in fact not used and this change is ready for submission now.

Thank you @alshamams for your work on this issue.

Thanks @HannesWell !

[vogella]

Thanks @alshamams and @HannesWell , always great if we get rid of outdated stuff.