Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support specifying the baseline artifact version range #4317

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

HannesWell
Copy link
Member

This can be used to select the desired baseline artifact if multiple versions of an artifact are available and to compare.

Instead of specifing the version range explicitly we could also define an enum similar to ReportBehavior to specify 'named version rules' for the baseline artifact, like same-major and latest.

This can be used to select the desired baseline artifact if multiple
versions of an artifact are available and to compare.
Copy link

Test Results

  603 files    603 suites   3h 59m 20s ⏱️
  430 tests   423 ✅  7 💤 0 ❌
1 290 runs  1 268 ✅ 22 💤 0 ❌

Results for commit 94dc487.

@HannesWell HannesWell added the backport-to-tycho-4.0.x Can be added to a PR to trigger an automatic backport of the change label Sep 29, 2024
Copy link
Member

@laeubi laeubi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thing an explicit range is not really suitable here.

Also it is a bit unclear how to use it, maybe provide an integration-test to demonstrate the use-case would be helpful here also to prevent regressions in the future.

* </p>
*/
@Parameter(defaultValue = "0.0.0", alias = "tycho.p2.baseline.versionRange")
private String baselineVersionRange = "0.0.0";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems hardly usable, as one needs to explicitly configure the version per project (also 0.0.0 is not really a version range).

In PDE features we have the concept of a "match rule" that might be more sufficient here as we then can compute a version range based on the current project version.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that's what I meant in my initial comment with:

Instead of specifying the version range explicitly we could also define an enum similar to ReportBehavior to specify 'named version rules' for the baseline artifact, like same-major and latest.

So I think the main question is what rules to support?
We could start simple and just have something like same-major and latest. Using the exact set of rules from PDE doesn't look suitable to me, because Perfect and Greater or Equal doesn't make sense in this context, because this mojo also checks for backward steps.
So besides same-major and latest actually only same-minor might be of interest.
But the names of the rules could be more speaking.

For the implementation we maybe could re-use PDE's new:
https://github.com/eclipse-pde/eclipse.pde/blob/3f973e87fd7ede513388e8ed870d1b65de438947/ui/org.eclipse.pde.core/src/org/eclipse/pde/core/plugin/VersionMatchRule.java

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I think the main question is what rules to support?

We can just support what is useful today and add more if required, I think thats how its already done on some other places in Tycho

We could start simple and just have something like same-major and latest.

I would prefer to just use the same naming as for features as this is a P2 concept and we should not mix to much naming.

Using the exact set of rules from PDE doesn't look suitable to me, because Perfect and Greater or Equal doesn't make sense in this context,

Greater or equal is actually what is the default today, the only one not so useful might be "Perfect" that actually would never match anything.

For the implementation we maybe could re-use PDE's new:

Tycho has already org.eclipse.tycho.model.Feature.getVersionRange(ImportRef) we can simply generalize that method, so I don't think relying on PDE-UI in Tycho just to reuse an enum is very useful, if we ever do

it would of course be good to reuse as much as possible.

@HannesWell
Copy link
Member Author

Also it is a bit unclear how to use it, maybe provide an integration-test to demonstrate the use-case would be helpful here also to prevent regressions in the future.

The idea was to use it for eclipse-jdt/eclipse.jdt.core#3027, but the old version might vanish anyways soon.

@laeubi
Copy link
Member

laeubi commented Nov 4, 2024

The idea was to use it for eclipse-jdt/eclipse.jdt.core#3027, but the old version might vanish anyways soon.

So do we still need this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-to-tycho-4.0.x Can be added to a PR to trigger an automatic backport of the change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants