Import aQute.bnd packages and remove bnd.lib from pde-feature

[HannesWell]

We should use Import-Package to reference the bnd.lib packages and not require the bundle. It was suggested explicitly in #445 but it is OSGi best-practice in general.

Furthermore the bnd.lib bundles should not be included into the m2e.pde-feature to simplify updates that do not require to touch that feature. P2 had the same problem with sat4j and PDE with ASM. And it is P2's job to collect dependencies not ours.
We only have to make sure the bnd.lib dependencies are contained in the m2e repo because nobody else contributes it to SimRel otherwise.
But this is better done in the m2e.repository/category.xml, than in the feature (which I assume was the reason to include it). The category.xml is not bound to a specific version (in contrast to the feature), so this shift is a simplification.

[HannesWell]

@laeubi , @mickaelistria any objections?

[HannesWell]

This requires some version increments.
With #653 a new major version bump is likely so I will wait until that is completed.

[laeubi]

We should use Import-Package to reference the bnd.lib packages and not require the bundle. It was suggested explicitly in #445 but it is OSGi best-practice in general.

That's fine, beside the rationale, we should not try to "align" dependencies with other unrelated project, OSGi can perfectly handle this case and its not our obligation to handle such cases. Beside that import-package is always better.

Furthermore the bnd.lib bundles should not be included into the m2e.pde-feature to simplify updates that do not require to touch that feature.

Instead of removing that we could thing about including it as a dependency with a compatible rule.

[laeubi]

Actually I think we should bump all bundles to 2.0.0 if we assume a major change it dosen't make sense to keep old versions mixed with new versions. With #644 I'll anyway have some more version bumps to 2.0 ...

[mickaelistria]

Actually I think we should bump all bundles to 2.0.0 if we assume a major change it dosen't make sense to keep old versions mixed with new versions.

That's not how semantic versioning is supposed to work. How version does bump depends on the changes that participate in the new version and only a new API introduced (or re-exported) by a bundle is supposed to require a major bump. This allows to optimize re-usability and modularity.
That said, I don't care much; I just wanted to claim that it's not best to bump all bundles to 2.0.0; it's more some "quick and dirty" path; but I don't think it's a major issue either, so if it's really what you both think is simpler, go for it.
The only think I really want we keep doing is to avoid bumping version when there is no change; as there is no point in asking millions of users to re-download a whole bundle without any payload change.

[laeubi]

How version does bump depends on the changes that participate in the new version and only a new API introduced (or re-exported) by a bundle is supposed to require a major bump.

That's not completely true. e.g a bundle that uses API from m2e.core 2.x (is there any not depending on that?) needs add a new version import as we claim API version is incompatible to 1.x. As we currently not versioning packages this requires the consumer to become incompatible as well as it obviously can't work with an older version.

[mickaelistria]

That's not completely true. e.g a bundle that uses API from m2e.core 2.x (is there any not depending on that?) needs add a new version import as we claim API version is incompatible to 1.x.

Yes, but as long as the core API doesn't "leak" in the public API of the consumer API, it doesn't have to go to new major, it's actually an internal change that qualifies for +0.0.1. However, if the core API is used in signatures of the consumer API, then yes, it's propagating; and -I believe- the requirement on Core API should ideally be re-exported by the consumer bundle to clarify that.

[mickaelistria]

This should be a dedicated category like "Transitive dependencies" or, even better but harder to setup, no category at all (but removing a category create a dumb "Uncategorized" category....)

[laeubi]

as mentined above I don't see any value in adding this in the category as no one want to install this independent from the pde feature... it should simply be a Dependecy their with proper version range...

[mickaelistria]

Making them a dependency in the feature doesn't include them in the p2 repository. Things that are included in p2 repo are only what's explicitly part of the category.xml or .product files, and their included dependencies. Turning an inclusion into a requirement usually removes the content from the p2 repo, so we need to explicit somewhere else to add the content to the p2 repo.

[laeubi]

I don't think that this is true, is there an integration test showing that? At the p2 level there is no difference between an included versus depended they both end up as a (direct) requirement that is then included in the update-site (even if not visible in the install dialog).

[mickaelistria]

I don't think that this is true, is there an integration test showing that?

Please try and see by yourself.

[HannesWell]

This should be a dedicated category like "Transitive dependencies" or, even better but harder to setup, no category at all (but removing a category create a dumb "Uncategorized" category....)

Good point! I will see what I can achieve.

[laeubi]

I have created eclipse-tycho/tycho#898
Until this is resolved we should stick to include then, that's the version we have build, tested and veted our build and I don't see a value to take additional effort to move that around as it is a requirement of the pde feature and not of m2e in general.

[mickaelistria]

The benefit is that it doesn't require a rebuild of the feature to update the version.

[laeubi]

as the feature and the bundle are part of the same build I don't see an issue here and otherwise I see the risk that p2 won't update the version as nothing is changed (neither feature nor bundle).

[HannesWell]

This should be a dedicated category like "Transitive dependencies" or, even better but harder to setup, no category at all (but removing a category create a dumb "Uncategorized" category....)

I removed the category elements so that they are uncategorized and in my local test build they were included.

But I cannot judge how sever Christophs suspected risk is. But I can test locally if an update of an uncategorized bundle in the repo that is not included in any feature works or not. Of course that would only be for one case. Maybe there are other cases.

[HannesWell]

Actually I think we should bump all bundles to 2.0.0 if we assume a major change it dosen't make sense to keep old versions mixed with new versions. With #644 I'll anyway have some more version bumps to 2.0 ...

I agree with Mickael that we should not just bump all versions for the sake of having the same base.
However we should also not hesitate to bump them when it is necessary/appropriated.

[laeubi]

is this solved now or still an issue?

[laeubi]

@HannesWell do we still need this or should we close the PR?

[HannesWell]

@HannesWell do we still need this or should we close the PR?

Thanks for the reminder.
It depends on, if we are in general willing to follow this approach?
If yes I can test if a user can use the update site created with this change to only update the bnd-bundles (starting with a feature that contains them and later does not and starting with a feature that not contains them before too).

The main motivation to remove the deps from the feature is to avoid the forced-qualifier update of the feature if the dependency changes.

[laeubi]

Can't we just keep the bundles in the feature for now but still upgrade to latest version + package imports? I'm working on a feature in Tycho that will allow us to use Requirements with a version range, that should then make the version bumps unnecessary.

[HannesWell]

Can't we just keep the bundles in the feature for now but still upgrade to latest version + package imports?

OK lets to that then.
From my experience Features also work well in uninstalling previously included plug-ins when they are removed, probably because (if I understood it correctly) P2 reverts the installation process of the old feature version in an 'update' process.
So we are quite flexible any ways in the future.

I'm working on a feature in Tycho that will allow us to use Requirements with a version range, that should then make the version bumps unnecessary.

Sounds interesting. For requirements of a feature or included Plug-ins/Features? Isn't the cause of the problem that the baseline replacement of the p2e-metadata plugin replaces the feature from the reactor (with the new version of the included dependency) with the feature from the baseline (with the old version of the dep) because no qualifier update happend?

[github-actions]

Unit Test Results

50 tests   49 ✔️  1m 56s ⏱️
12 suites    1 💤
12 files      0 ❌

Results for commit 2a02916.

[laeubi]

Looks good, do we need to update the target for the latest version of BND?

Changes on outdated content

[HannesWell]

Looks good, do we need to update the target for the latest version of BND?

Thanks. Nope that was done already with #772 :)

[laeubi]

Sounds interesting. For requirements of a feature or included Plug-ins/Features?

Requirements, see eclipse-tycho/tycho#898

Isn't the cause of the problem that the baseline replacement of the p2e-metadata plugin replaces the feature from the reactor (with the new version of the included dependency) with the feature from the baseline (with the old version of the dep) because no qualifier update happend?

Actually the problem is more that the feature has a fixed version, but Requirements can have version ranges.