Remove support for unused attributes in feature.xml

[HannesWell]

This removes support for the following attributes of plugin elements in feature.xml:

• install/download-size
• unpack
• fragment

The written values are not relevant anymore and therefore can be removed.

In the context of eclipse-pde/eclipse.pde#730

For Tycho 4.x I'm about to prepare a PR where just the interference of the actual download-size and install-size is omitted if the attributes are absent (at the moment those attributes are always written with the actual value). But for Tycho 5, a new major version, I think it is ok to discontinue that feature entirely.

[github-actions]

Test Results

  594 files  +3    594 suites  +3   4h 19m 29s ⏱️ + 2m 3s
  414 tests +1    407 ✅ +2   7 💤 ±0  0 ❌  - 1 
1 242 runs  +3  1 220 ✅ +4  22 💤 ±0  0 ❌  - 1 

Results for commit 6c65244. ± Comparison against base commit b35d8e0.

♻️ This comment has been updated with latest results.

[laeubi]

@HannesWell I'm not sure I see where it removes the attributes (because otherwise they will still be persistent in the feature.xml), so can we have an integration test that ensures a feature does not contain any of these attributes after it is packed?

[HannesWell]

@HannesWell I'm not sure I see where it removes the attributes (because otherwise they will still be persistent in the feature.xml), so can we have an integration test that ensures a feature does not contain any of these attributes after it is packed?

I can add a test-case to verify this change.

This does not remove the Attributes, that's IMHO the duty of PDE, this removes the support for those attributes, especially the interference of the sizes.
With this change the Attributes are left unchanged in case they are still present.
Then e.g. the size Attributes will just have a zero value, which is not much of a concern, since those values are irrelevant.

[laeubi]

Then e.g. the size Attributes will just have a zero value, which is not much of a concern, since those values are irrelevant.

And that's because the goal is to remove them from the feature so we have some form of normalization (and hopefully less version bumps / confusions), I therefore thing we should retain support for reading those but remove them at the time where we currently fix the version.

[HannesWell]

Then e.g. the size Attributes will just have a zero value, which is not much of a concern, since those values are irrelevant.

And that's because the goal is to remove them from the feature so we have some form of normalization (and hopefully less version bumps / confusions), I therefore thing we should retain support for reading those but remove them at the time where we currently fix the version.

Just removing those attributes without having a change in the source files of the feature will also cause binary differences and thus likely require a version bump.

The transition should be made possible seamlessly in Tycho 4 and already is with #2917 and #2925. And with eclipse-pde/eclipse.pde#770 all of those attributes are removed by PDE from a feature.xml on the next change in the workspace.

For Tycho 5 the goal should IMHO be to forget everything about the handling of those attributes. A new major version seems a good opportunity for that. I assume there will be some time until Tycho 5 is released and therefore it should be enough time for those that then upgrade to Tycho 5 then to touch their feature.xml in the meantime to have those attributes removed.
If you think it is necessary I can add an entry for the migration guide mentioning this.

I've added a test to ensure it behaves as expected, which require #2924 first.

[laeubi]

Just removing those attributes without having a change in the source files of the feature will also cause binary differences and thus likely require a version bump.

Yes, like when JDT changes bytecode generation or any other "disruptive" change, nerveless it is the constant, leaving them with invalid data seems not desirable.

For Tycho 5 the goal should IMHO be to forget everything about the handling of those attributes.

Then we need a warning (best at Tycho 4 already) when we do so but this I doubt users would really otherwise be aware of this unless they complain about tycho generating "invalid" data, so either we should war or remove them or both ...

[HannesWell]

For Tycho 5 the goal should IMHO be to forget everything about the handling of those attributes.

Then we need a warning (best at Tycho 4 already) when we do so but this I doubt users would really otherwise be aware of this unless they complain about tycho generating "invalid" data, so either we should war or remove them or both ...

My suggestion for this is to submit this as it is for Tycho-5 once the build is green and update Tycho 4 to warn when these obsolete attributes are found.
With that Tycho 4 warns but can still process the features as they were but can also handle the reduced features and Tycho 5 is clean of all of those attributes.

[laeubi]

@HannesWell I just checked with one of my project that PDE removes these attributes, but open and save all features can be cumbersome, so maybe having Tycho to remove then when the feature is packed (and versions are updated) seemms still a good choice.

[akurtakov]

What is the status of this of this one?

[laeubi]

@HannesWell still something you like to work on or should we close it for now and maybe pick up later as it already has conflicts?

[HannesWell]

Yes I would like to complete this and can resolve the conflicts.

I just checked with one of my project that PDE removes these attributes, but open and save all features can be cumbersome, so maybe having Tycho to remove then when the feature is packed (and versions are updated) seemms still a good choice.

We can add some explicit clean-up to PDE. If you want to remove the attributes in Tycho I suggest doing that in Tycho 4.
IMHO Tycho 5 should just not know about it anymore.

[laeubi]

We can add some explicit clean-up to PDE. If you want to remove the attributes in Tycho I suggest doing that in Tycho 4.
IMHO Tycho 5 should just not know about it anymore.

I'm not sure I see the befit of this, Tycho is processing the feature that is taking the original, expanding versions and then produces a "build feature.xml" that is then packaged as the final artifact. I would expect that it cleanup the feature in any version and not just stops doing so and leave things to the developer. That was also the initial idea of the feature that we automatically "normalize" the features to allow easier baseline compares.

Given that in P2 / PDE last time has even had some hickups related to that I would even argue to leave things behind and maybe notice somewhere later in the chain that something goes wrong compared to just delete them consistently in the package phase is less effort than "maintain" a few string constants / methods / ... that literally never change.

[HannesWell]

We can add some explicit clean-up to PDE. If you want to remove the attributes in Tycho I suggest doing that in Tycho 4.
IMHO Tycho 5 should just not know about it anymore.

I'm not sure I see the befit of this, Tycho is processing the feature that is taking the original, expanding versions and then produces a "build feature.xml" that is then packaged as the final artifact. I would expect that it cleanup the feature in any version and not just stops doing so and leave things to the developer. That was also the initial idea of the feature that we automatically "normalize" the features to allow easier baseline compares.

If Tycho-5 starts to remove these attributes in case they are still present in a feature.xml it is a change compared to a baseline built with Tycho-4 since Tycho-4 replaced the placeholder 0.0.0-values with the real ones.
If Tycho-5 just stops replacing the placeholder 0.0.0-values and leaves them as they are (if they are still present), it is also a change compared to a baseline built with Tycho-4.
In either way there is a change in a baseline built with Tycho-4.
Therefore I would be in favor of the simpler way and just remove support for those attributes in Tycho and let PDE remove them from features.xml over time as soon as somebody does a change to a feature.xml in the PDE editor.

In PDE we could mark them with a warning. At the moment there is no warning since for PDE those attributes are still known.
After it has been a warning for some time, we can even phase them out in PDE and remove those attributes from the list of known-attributes which automatically makes their occurrence an error (if unknown attributes have that severity).

[HannesWell]

I have now resolved the conflicts.

If you think it would help, I can also add a warning to Tycho-4 if these unused attributes are encountered in a feature.

[laeubi]

If Tycho-5 starts to remove these attributes in case they are still present in a feature.xml it is a change compared to a baseline built with Tycho-4 since Tycho-4 replaced the placeholder 0.0.0-values with the real ones.
If Tycho-5 just stops replacing the placeholder 0.0.0-values and leaves them as they are (if they are still present), it is also a change compared to a baseline built with Tycho-4.

The versions are hopefully still replaced with Tycho 5... but if Tycho 4 replace the download-size but Tycho 5 not one ends up with garbage in the feature compared to the previous state. And if one then later recognize it and removes them from the feature one gets a baseline problem and need to bump or touch the feature.

So if one updates from 4>5 there might be such update needed once, but it should not appear if one later removes the obsolete items so the normalization should be that it is never present in Tycho 5 (what is fine as its a major version).

[HannesWell]

The versions are hopefully still replaced with Tycho 5...

Yes, I meant the 0 value from download-size etc.

but if Tycho 4 replace the download-size but Tycho 5 not one ends up with garbage in the feature compared to the previous state. And if one then later recognize it and removes them from the feature one gets a baseline problem and need to bump or touch the feature.

So if one updates from 4>5 there might be such update needed once, but it should not appear if one later removes the obsolete items so the normalization should be that it is never present in Tycho 5 (what is fine as its a major version).

As you said a version update is needed in any case and as long as you do it through PDE's feature editor these unused attributes are then automatically removed too. And since a feature version has to be updated as soon as any included plug-in changes, if one does baselining, features get touched frequently and the chance is high that these attributes are already removed.
Therefore I think the case you described is rare and Tycho should simply leave these attributes behind with the new major version five.
As said before, it would IMHO be more suitable to warn about these attributes in Tycho 4.

[laeubi]

As said before, it would IMHO be more suitable to warn about these attributes in Tycho 4.

Defiantly a no-go for me, If one absolutely want such warning it can happen in the IDE/PDE. but e should not add warnings to Tycho if we can fix the problem automatically, whether or not the attribute is there must not matter or produce warnings or leave unprocessed data.

So either we retain the Tycho 4 one (process them if they are there and otherwise ignore) or remove them automatically in the packaging phase what not need necessarily happen with the Transformer or whatever method you like to get rid of.

[HannesWell]

If one absolutely want such warning it can happen in the IDE/PDE.

Also fine for me.

must not [...] leave unprocessed data.

Why? Who or what mandates that there must not be unprocessed data?
I can add arbitrary attributes with arbitrary values to a plugin element and nothing (bad) happens. Well PDE complains but Tycho just keeps it as it is in the final feature.xml.
So I don't see an inherent problem in case the unused attributes end up there unprocessed. They have been there uselessly for some time and stay there until they are removed. And since they are useless their value does not matter.

And if one then later recognize it and removes them from the feature one gets a baseline problem and need to bump or touch the feature.

As already said I don't expect this specific scenario to happen often but even if that happens, it is clear what's happening and that the content of a feature can not be altered compared to a baseline without a version change. It is like if one for example wants to fix a type in the feature description (which is by the way also unprocessed). If strict base-lining is done it does not work unless there has been a version bump since the last release (for that change or a previous one). And if one does not want to touch want to bump the version for that feature, then one simply has to wait until another change makes it necessary, just like if one wants to fix a typo in the description.

But to be frank, I don't understand why such a specific use-case is so important for you?
And in case you wonder why it is important for me to remove it:
Since I have often enough searched for the reason why there is some legacy code and if it can be removed I want to avoid the same annoying work for whoever will maintain that code in the future and wonders why its there and why it why it was so important to remove those attributes that special handling was introduced.

[laeubi]

As said, the inital trigger for this was this "specific use-case" and it was agreed that Tycho should remove the attributes instead of filling them. Because otherwise people will complain that "suddenly" their feature.xml is no longer processed and there is SomeTollWeNeverHearedbout now badly fails. This has been the case already with some reported issues that "Tycho ignores the unpack in features" in the past.

Also as mentioned there is no need to retain any "legacy" test / code, we can happily have some new method named "removeObsoleteAttributes" that just blindly deletes them because these are not arbitrary attributes but ones defined by a pde feature and not doing some arbitrary xml processing...

[HannesWell]

Also as mentioned there is no need to retain any "legacy" test / code, we can happily have some new method named "removeObsoleteAttributes" that just blindly deletes them because these are not arbitrary attributes but ones defined by a pde feature and not doing some arbitrary xml processing...

Ok, I have no added such code to FeatureXmlTransformer.expandReferences(), but I had to the public method PluginRef.removeAttribute(). I hope that's fine.

[laeubi]

Ok, I have no added such code to FeatureXmlTransformer.expandReferences(), but I had to the public method PluginRef.removeAttribute(). I hope that's fine.

Thanks, we can make public whatever is needed :-)

[laeubi]

Looks good but it seems we are currently facing infra issues.

[HannesWell]

Great. :)

Looks good but it seems we are currently facing infra issues.

There is always something that makes it difficult😅