Add adapter for IProject > (bnd) Project

[laeubi]

If one wants to write generic UI components then there is often the question to answer to what (bnd) project an Eclipse IProject belongs (if any).

This adds an Adapter that can adapt from IProject > (bnd) Project if the IProject is a PDE Bnd backed project.

See also

• https://bnd.discourse.group/t/announcement-bnd-and-pde-cooperation/372
• Corresponding PDE change Add adapter for IProject > (bnd) Project eclipse-pde/eclipse.pde#918

[pkriens]

I think this is a great approach, we do lack a lot of Eclipse knowledge. It would imho be a very good idea to create a bundle for this? We already have the PDE project for this. I would be ok to always install this with bndtools.

Idea to move this code to the PDE project?

@kriegfrj @juergen-albert wdyt?

[laeubi]

It would imho be a very good idea to create a bundle for this?

I don't think an extra bundle is required here, it can happily live with the bndtools-core part as it does not introduce any special dependency (if you are already an eclipse-plugin) and this is not PDE, not even Platform UI it is basic Equinox API.

We already have the PDE project for this

I hope we can remove this soon see:

• Remove migrated pdetools.pde project #5887

[laeubi]

@pkriens I added one example that shows how one can now easily make a component previously bound to Central/Bndtools only a component that can be used everywhere with any tooling that is capable to provide a (bnd) Project for an IProject.

[maho7791]

For me it is an appropriate approach.

[pkriens]

Well, I see this now replaces the static reference from Central to Adapters?

Are you sure you're not introducing ordering/startup issues here? This pattern to register an component in a static class is notorious for startup issues. You lose any dependency handling?

It is exactly the aggregate service problem.

[laeubi]

Are you sure you're not introducing ordering/startup issues here?

Yes.

This pattern to register an component in a static class is notorious for startup issues.

I don't see a component registered in a static class.

You lose any dependency handling?

What dependencies are you referring to?

It is exactly the aggregate service problem.

I can only tell that this works well for Eclipse IDE and for my custom applications and is a standard pattern of Eclipse IDE to decouple model from view:

https://www.eclipse.org/articles/Article-Adapters/index.html

[laeubi]

I did some simple test and they seem work fine but would desire if some long bndtools users can verify that patch as well!

[pkriens]

This is the nastiest problem with OSGi, the ordering problems you get and I think we're running head on in them. This will definitely become problematic with @kriegfrj's work making it all more dynamic. It is a problem where I've tried umpteen times to find a solution. The by far best solution was https://github.com/aQute-os/biz.aQute.osgi.util/tree/master/biz.aQute.aggregate.provider but I notice I do not use it often. However, letting the service registry pass through a static class is making me very uncomfortable. @kriegfrj wdyt?

The ICentral proposal is I think less fragile since users can make it a @Reference and it has a larger granularity.

[laeubi]

Can you explain what all of this has to do with my proposal? I really don't understand, this all is dynamic and using standard OSGi technology + Equinox Adapter pattern what also is dynamic and is used in large scale (Eclipse IDE) so if we don't trust this what can we trust then?

However, letting the service registry pass through a static class is making me very uncomfortable. @kriegfrj wdyt?

Where the heck do you see a "service registry pass through a static class" this is simply not the case...

[laeubi]

by the way once @kriegfrj proposal with ICentral is merged you would simply replace

Central.getProject(...) with a call to central.getProject(...) injected with plain @Reference at contructor...

[chrisrueger]

I did some simple test and they seem work fine but would desire if some long bndtools users can verify that patch as well!

I have checked out your branch. Could you briefly describe a test I could do to verify this too?
So far my existing bnd workspace builds and the app starts up too. But I guess I need to test something else to trigger your changes.

[kriegfrj]

@pkriens wrote:

Well, I see this now replaces the static reference from Central to Adapters?

Are you sure you're not introducing ordering/startup issues here? This pattern to register an component in a static class is notorious for startup issues. You lose any dependency handling?

I don't think that this particular change will suffer from that issue.

The problem that you can get by (eg) referencing classes from other bundles statically is that this then triggers that bundle to activate, which can have a trickle-down effect on activating other bundles. Eclipse is particularly finnicky in that some bundles can't be activated too early (eg, before the Eclipse IWorkspace is activated, or before the Workbench is running) or else their activation fails, or even causes a deadlock.

This is unlikely to be the case with a static reference to the Adapters class, because the Adapters class is part of the Equinox framework and is guaranteed to be fully loaded (along with its dependencies) by the time the BuildpathQuickFixProcessor is activated. The worst that could happen in this implementation is that the BndAdapter is not loaded at the time that getCorrections() is called - in which case, the way @laeubi has written it, it simply won't return any suggestions unless/until the appropriate adapter is started.

That said, on reflection, while I was initial enthusiastic about this, I'm not certain that changing from Central.getProject() to Adapters.adapt() gains us a lot. This is a general, philosophical comment on the Adapter pattern, which I've always struggled to see the value of. I am reminded of the Fundamental theorem's exception - the only problem that cannot be solved by a layer of indireciton is too many levels of indirection. 😄 Behind the scenes the adapter is using Central.getProject() anyway, so we haven't fully decoupled from Central - we've simply changed our coupling from compile-time to runtime. The price we pay for this extra level of indirection is extra complexity - both in source/maintenance, and at runtime. This extra level of complexity introduces the possibility of extra bugs (eg, the one that I found in the review). We also have sacrificed a degree of compile-time type checking which we now have to do at runtime (witness the extra runtime type checks and casting that takes place in BndAdapter). I would be happy to make this sacrifice if there were a significant gain to be made by it, but it is not immediately clear to me at this stage what that gain might be.

So while this suggestion is not a definitely "no" from me, I would like to hear some more advocacy from @laeubi as to what the potential gains are.

I guess there is minimal harm in registering BndAdapter anyway. It doesn't mean that components have to use it. But we should probably include some regression tests for it.

[kriegfrj]

So while this suggestion is not a definitely "no" from me, I would like to hear some more advocacy from @laeubi as to what the potential gains are.

I have just been thinking a bit more. What this change does gain us is that it is likely to improve the startup ordering situation.

The current implementation requires that Central be loaded (and its bundle activated) when the quickfix processor is loaded, due to the static reference. This can cause the ripple-through startup effects. Typically the quickfix processor is started late so it's not an issue in practice. However, a static reference to Adapters (which is much lower-level than Central) is safer in this regard, as it is not going to cause any of these startup issues. So that is indeed an improvement. An injected ICentral would be a further improvement though, as it fixes the startup problem without losing the compile-time type safety.

[laeubi]

I did some simple test and they seem work fine but would desire if some long bndtools users can verify that patch as well!

I have checked out your branch. Could you briefly describe a test I could do to verify this too? So far my existing bnd workspace builds and the app starts up too. But I guess I need to test something else to trigger your changes.

@chrisrueger thanks for looking into it, basically you need to have a project and remove one of the required bundles/projects from the buildpath what should give you a compile error.
Then you should open the Quickfix and see bnd suggest you to add the one removed again (or an alternative of course)

[laeubi]

Eclipse is particularly finnicky in that some bundles can't be activated too early (eg, before the Eclipse IWorkspace is activated, or before the Workbench is running) or else their activation fails

I have fixed this a few releases ago so it is now save to reference IWorkspace (and resource bundle itself) without having such negative effects. Beside that the call to Adapter is only done when the bundle already is active or you won't see any bnd project and won't get a Quickfix anyways ;-)

The worst that could happen in this implementation is that the BndAdapter is not loaded at the time that getCorrections() is called - in which case, the way @laeubi has written it, it simply won't return any suggestions unless/until the appropriate adapter is started.

This can't really happen except you assume that DS is not started at all what has some other bad side-effects. As soon as bndtools bundle is starting (and it will as it is lazy-activate) DS will register a proxy service and then any adaption of one of its declared types will trigger the loading of the real service.

That said, on reflection, while I was initial enthusiastic about this, I'm not certain that changing from Central.getProject() to Adapters.adapt() gains us a lot.

It does gain a lot as you instantly won't need Central.getProject() anymore (so no direct coupling to a static singleton of an explicit implementation bundle) and you can use such components in any context (e.g. in PDE itself) and you have solved the problem of "maybe there are multiple workspace what should I choose" for those components (in fact with bndtools+pde installed there could be at least two of them, the bndtools Central and the PDE autogenerated manifest one depending on the project type). I even would claim that with that change you won't need to reference the central class anywhere in the UI ...

I guess there is minimal harm in registering BndAdapter anyway. It doesn't mean that components have to use it.

Exactly, this is an option and only an advice towards @pkriens

we do lack a lot of Eclipse knowledge

So I can open you a door into this world and even if I won't claim to know everything about Eclipse, I defiantly know about the Adapters and the OSGi Service integration of it (Disclaimer: I have implemented that) as I'm heavily involved in those parts.

Adapters is to the Eclipse Ui what services are to OSGi, and was a main reason I added first-class support for OSGi services so one can easily connect those worlds.

[laeubi]

@kriegfrj thanks for the review and testing I should now have addressed your mentioned comments.

[pkriens]

I am going to amplify @kriegfrj's sentiment. Like Father Krieg, I also was enthusiastic since we clearly lack knowledge of common Eclipse patterns and Adapters seemed to provide an unknown and useful pattern. However, after analysis my decision is:

• No use of Adapters in Bndtools for internal code, use ICentral,
• We need an ICentral and an API jar,
• No objection to register usefule IAdapterFactory's for PDE,
• Strongly recommend PDE to use a proper service like ICentral if possible

If there is disagreement with this, setup a zoom meeting. I will not argue this further in this PR, this is costing me way too much time.

Analysis

OSGi is a dynamic environment which means we should never use statics. (Anselm Baird, the designer of OSGi's predecessor Java Embedded Server said "statics are evil!") The Central class is an unfortunate accident by people who had no clue what they were doing. There also was no DS at the time and statics is the common pattern in Eclipse. However, we have DS nowadays, we should therefore follow proper service design patterns.

A primary goal to services for bndtools is to be able to use the dynamic loading in development, @kriegfrj is doing lots of work for that. Although Eclipse is profoundly static, in Bndtools we must no rely on it.

By going through Adapters, the BuildpathQuickFixProcessor assumes there is someone out there that registered sometime, or will register sometime, an appropriate IAdapterFactory (BndAdapter). This is probably the worst anti-pattern in OSGi there is. Since Eclipse is still 99% static, it will work for them but it will cause problems when we move to make Bndtools development more dynamic. Having proper services is paramount so we can use SCR to properly manage the existing dependencies.

If BuildpathQuickFixProcessor would @Reference BndAdapter (or better an ICentral service), this dependency would be explicit and could be managed by the tooling.

A key aspect of OSGi is that it has multiple class loaders. This implies the same class can come from different bundles. Sometimes a different version, sometimes because multiple bundles carry the same package. Where the OSGi Service Registry manages class space consistency (the uses constraints), Adapters ignores this aspect. Classes are looked up by name but returned by a class of that name loaded from the factory bundle. The returned object is therefore not guaranteed to be the proper type and can cause a confusing Class Cast Exception: "Class A cannot be cast to class A".

BuildpathQuickFixProcessor has a mandatory dependency on BndAdapter but since it hides behind Adapters, the tooling cannot help in any way. bnd will not encode an import, the resolver will miss it during resolving. Changes in the API will be missed and not verified by the compiler. Some of these are the price to pay when you need an abstraction but in this case the abstraction is an emperor without clothes as far as I can see.

Not unimportant, readability. This is subjective, and I understand the argument that Adapters looks more familiar to Eclipse developers than BndAdapter and we all hate to import more Jars to compile against. However, the dependency on the Eclipse-bndlib bridge must really be there. It is good to see in the code that BndAdapter (or better ICentral) is a dependency. It allows easy navigation to give you API documentation.

Generally patterns like Adapters (or in general the OSGi Service Registry) are good because they allow us to decouple. However, using Adapters in BuildpathQuickFixProcessor it is just hide and seek. We do not remove any dependencies, we do not reduce the code, we increase complexity, we just pretend it does not exist and open ourselves to a lot of problems.

Many of the analyzed problems might be theoretical when the code runs in today's Eclipse. I know it is very unlikely to have two bridges, or two bndlib bundles, that could cause class loading issues. The static initialization pattern tends to work for Eclipse. However, our goal to develop using the OSGi dynamics will show, let me rephrase, has shown, how incredibly static and fragil Eclipse is. From our side, we should at least give the example (and reap the benefits) of how you properly use OSGi. And yes, I know we started wrong with Central.

Therefore:

• Dependencies must be explicitly managed,
• Class space consistency must be ensured via OSGi,
• Using ICentral, would make it simpler to add related functions,
• Compiler should be able to detect API mismatches/evolution,
• bnd must create proper imports,
• Resolver must automatically include the needed bridge code,
• Code should be easy to understand, showing the actual dependencies,
• We should not use anti-patterns.

[laeubi]

Well its your decision in the end, I'll proceed in bringing good things to PDE, bndtools might adapt or not I can't influence of course...

So I'd like to ask the bnd devlopers to drive the part at Bnd tools as I'm about to give up on this now, issues for discussions are not desired, PRs should be used, PRs are concerned as long as one has digress to a point that has nothing to do anymore with the initial intend (that is/was to have a minimal, non intrusive way to decouple from static central now and not in an unforeseeable future) and then getting blamed for "costing too much time"... sadly my time is also limited to not repeat the same discussions and theoretical concerns over and over again that do not relate anything to the proposed change especially if what is "good" is decided by personal opinions instead of technical facts.