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

[m2e] Remove references to internal packages and avoid static methods #5422

Merged
merged 1 commit into from Nov 11, 2022

Conversation

timothyjward
Copy link
Contributor

The latest m2e updates have moved methods in internal packages. This breaks m2e support in Bndtools where we were going around the official API. This is primarily an attempt to remove the usage of internal types in an effort to remain compatible between versions. Furthermore we need to coexist in scenarios where:

  • A maven project change listener whiteboard is built in to M2E 2+ and replaces the one we created
  • Services are used for M2E objects such as IMaven - access these in a consistent way
  • Public lookup methods now exist, but we must still use reflection while 1.x is supported

A good deal of cleanup will be possible once we have a base of 2.0, but for now this is a good next step.

Known issues: The bndrun "Browse Repos" section populates, but then disappears again after about a second. Resolving is possible.

Signed-off-by: Tim Ward timothyjward@apache.org

The latest m2e updates have moved methods in internal packages. This breaks m2e support in Bndtools where we were going around the official API. This is primarily an attempt to remove the usage of internal types in an effort to remain compatible between versions. Furthermore we need to coexist in scenarios where:

* A maven project change listener whiteboard is built in to M2E 2+ and replaces the one we created
* Services are used for M2E objects such as IMaven - access these in a consistent way
* Public lookup methods now exist, but we must still use reflection while 1.x is supported

A good deal of cleanup will be possible once we have a base of 2.0, but for now this is a good next step.

Known issues: The bndrun "Browse Repos" section populates, but then disappears again after about a second. Resolving is possible.

Signed-off-by: Tim Ward <timothyjward@apache.org>
@timothyjward timothyjward requested a review from bjhargrave Nov 10, 2022
@timothyjward
Copy link
Contributor Author

@laeubi - Some more changes that you might want to look at to see how we're mangling M2E!

@timothyjward timothyjward requested a review from rotty3000 Nov 10, 2022
@laeubi
Copy link
Contributor

laeubi commented Nov 10, 2022

This is primarily an attempt to remove the usage of internal types in an effort to remain compatible between versions.

Is there really a value in supporting different m2e versions? I think that really complicates things a lot, and most probably two versions of m2e in the same eclipse wont work at all.

So probably switch to m2e 2.x completely and ask people to upgrade their IDE if they want to use newer bndtools?

By the way, if you think something is missing or should be improved in m2e let us know.

@timothyjward
Copy link
Contributor Author

timothyjward commented Nov 10, 2022

This is primarily an attempt to remove the usage of internal types in an effort to remain compatible between versions.

Is there really a value in supporting different m2e versions? I think that really complicates things a lot, and most probably two versions of m2e in the same eclipse wont work at all.

The support requirement is that we run on a base of 2020-06, which includes M2E 1.x. I also want to run on 2022-06+, which is M2E 2.x. That's why we have to support both at the moment. I hope that we will drop 1.x in the future, but I don't think now is the right time.

So probably switch to m2e 2.x completely and ask people to upgrade their IDE if they want to use newer bndtools?

This argument is unlikely to get much traction with @pkriens or @bjhargrave. We've only recently moved up our base.

By the way, if you think something is missing or should be improved in m2e let us know.

The plugin components published as services and the listener whiteboard are great (and I look forward to removing the bnd whiteboard once we move to a 2.x base). One question is how to properly make use of things like IMaven from inside a org.eclipse.m2e.core.projectConfigurators extension, or whether we can provide the configurator as a service?

Copy link
Contributor

@rotty3000 rotty3000 left a comment

Choose a reason for hiding this comment

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

I don't see any reason not to merge this.

@bjhargrave
Copy link
Member

This argument is unlikely to get much traction with @pkriens or @bjhargrave. We've only recently moved up our base.

For Bnd 7 which will start shortly, we can update our dependencies. Bnd 7 will use Java 17 and I expect we will also want to update our Eclipse dependencies from 2020-06 to perhaps 2022-06. This would also include updating the Eclipse m2e dependency.

But for Bnd 6.4, we need to support Eclipse 2020-06 and the m2e version associated.

projectManager.addMavenProjectChangedListener(projectChangedListenersTracker);
Bundle bundle = FrameworkUtil.getBundle(MavenPlugin.class);
isM2E_v2 = bundle.getVersion()
.getMajor() == 2;
Copy link
Member

Choose a reason for hiding this comment

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

Should this be

Suggested change
.getMajor() == 2;
.getMajor() >= 2;

for some level of future proofing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did wonder, but the boolean would need to be renamed too.
The whole codebase needs a refactor, and fundamentally this class will be removed completely once we have 2.x as a base. I therefore leaned towards “make it work for now”.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It’s also worth noting that the ceiling for the import is capped at 3.0.0) in the bnd file, so > 2 can’t occur without changing that.

@@ -34,11 +33,12 @@

public interface MavenRunListenerHelper {
Copy link
Member

Choose a reason for hiding this comment

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

Does this type still need to be an interface since all the methods are now static? Perhaps it should be a final class with a private constructor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was previously an interface used for a sort of horrible multiple inheritance. It’s another type that would look very different after a proper refactor.
I’m not claiming that this commit is beautiful, it’s a sticking plaster.

@laeubi
Copy link
Contributor

laeubi commented Nov 10, 2022

The support requirement is that we run on a base of 2020-06

Alright, that's quite hard to support more than 2 year old software ...

One question is how to properly make use of things like IMaven from inside a org.eclipse.m2e.core.projectConfigurators extension

Actually one should hopefully not need IMaven anymore but can use the IMavenProjectFacade from the ProjectConfigurationRequest instead, e.g. when you want to llokup a component, the m2e 2.x way would be:

IMavenProjectFacade facade ...
IComponentLookup lookup = facade.getComponentLookup()
MyMavenComponent plexusComponent = lookup.loockup(MyMavenComponent.class);

If you like to even access project extensions you need to start an execution, then you also have acces to the project and session of the project.

whether we can provide the configurator as a service?

This is a bit ongoing development and currently not possible because m2e uses extension points here because the configurator is bound to the id in the life-cycle mappings. I'm not quite happy with that and plan to change it some time but currently there are some more important topics ...

@kriegfrj
Copy link
Contributor

whether we can provide the configurator as a service?

This is a bit ongoing development and currently not possible because m2e uses extension points here because the configurator is bound to the id in the life-cycle mappings. I'm not quite happy with that and plan to change it some time but currently there are some more important topics ...

I don't know if this is relevant to the above discussion, but I started trying to roll out the facade pattern which can bridge a service/component-style interface to an extension point. Some parts of the code use this already (the org.bndtools.launch bundle, for example), and there is a pending PR #5036 which shows further examples.

@laeubi
Copy link
Contributor

laeubi commented Nov 11, 2022

@kriegfrj I think the point is to get rid of extension points all together (at least that would me my final goal for m2e after converting everything to DS).
But in general you should consider to contribute your idea of dynamic service proxy to equinox directly, https://github.com/eclipse-equinox/equinox/tree/master/bundles/org.eclipse.equinox.registry/src/org/eclipse/core/internal/registry/osgi seems a good place for such enhancement.

@kriegfrj
Copy link
Contributor

@kriegfrj I think the point is to get rid of extension points all together (at least that would me my final goal for m2e after converting everything to DS).

👍 Yes, convert to DS "native" implementation is a superior option if that is available. I did the same thing in the PR with some of the custom extension points that Bndtools declared - eliminating them in favour of a direct DS implementation.

But in general you should consider to contribute your idea of dynamic service proxy to equinox directly, https://github.com/eclipse-equinox/equinox/tree/master/bundles/org.eclipse.equinox.registry/src/org/eclipse/core/internal/registry/osgi seems a good place for such enhancement.

I agree - if I had time, that would be the ideal place for it.

@laeubi
Copy link
Contributor

laeubi commented Nov 11, 2022

I agree - if I had time, that would be the ideal place for it.

Sure just wanted to note that, because anything you implement in BNDtools only will only be available there ... and always has the cost of maintaining it yourself.

Same applies to m2e, if we can implement things there it won't be needed for BNDtools to "catch up" with m2e changes so let us know if there is something that probably is generic enough and would not require bndtools to provide additional configuration.

@bjhargrave
Copy link
Member

Can we merge?

@bjhargrave bjhargrave merged commit 8598911 into bndtools:master Nov 11, 2022
11 checks passed
@timothyjward
Copy link
Contributor Author

Can we merge?

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants