[builder] Change to facade #5036
Conversation
kriegfrj
force-pushed
the
builder-facade-with-central
branch
4 times, most recently
from
0704c84
to
abaf55b
Compare
|
Still a little bit of work to do but I think it is mostly working and probably at a good point where it could benefit from a review. |
kriegfrj
force-pushed
the
builder-facade-with-central
branch
3 times, most recently
from
1ef3215
to
7d512cc
Compare
|
Think I've done most of the tidying-up now. Let me have it, @bjhargrave! 😄 |
kriegfrj
force-pushed
the
builder-facade-with-central
branch
from
7d512cc
to
252e13f
Compare
bndtools.api/src/org/bndtools/api/builder/BuildLoggerConstants.java
Outdated
Show resolved
Hide resolved
bndtools.builder/src/org/bndtools/builder/facade/IProjectBuilder.java
Outdated
Show resolved
Hide resolved
org.bndtools.builder/src/org/bndtools/builder/impl/CnfWatcher.java
Outdated
Show resolved
Hide resolved
Well, when you take out moved files and updates from Eclipse automated refactoring, it's probably not as big as it looks at first glance. 😄
I was hoping (and expecting) as much! 😄 |
|
Could you describe the problem leading to NPEs when using DS in more detail? From the description below it doesn’t make any sense to me that this could be happening.
Thanks
David Jencks
… On Jan 18, 2022, at 8:53 PM, Fr Jeremy Krieg ***@***.***> wrote:
@kriegfrj commented on this pull request.
In bndtools.core/src/bndtools/central/QueryWhiteboard.java <#5036 (comment)>:
> +import org.osgi.service.component.annotations.Activate;
+import org.osgi.service.component.annotations.Component;
+import org.osgi.service.component.annotations.Deactivate;
+import org.osgi.service.component.annotations.Reference;
+import org.osgi.util.tracker.ServiceTracker;
+import org.osgi.util.tracker.ServiceTrackerCustomizer;
+
+/**
+ * This component adds whiteboard facility to IQueryListeners.
+ */
***@***.***(immediate = true)
+public class QueryWhiteboard implements ServiceTrackerCustomizer<IQueryListener, IQueryListener> {
+ @reference
+ IWorkbench wb;
+
+ ServiceTracker<IQueryListener, IQueryListener> listeners;
This was one of the things that I hoped you would comment on. 😄
I originally tried bind/unbind/updated, but I found that I was getting NPEs. I realised that this was because the get called before the reference to IWorkbench was satisifed and before the component was activated. It got complicated to track them as they were removed/added before activation, and then to track them differently after activation (when the workspace is available), and as I thought about how to handle this problem I realised that this was exactly the sort of problem that ServiceTracker was designed to solve.
However, I admit to not being a DS expert. If there is an easy way to configure DS so that the dynamic reference updates are queued by DS and only delivered after activation, then that would work. (If there is no such feature in DS, maybe we could consider it a feature request?)
If you have a better suggestion, I'd love to hear it.
—
Reply to this email directly, view it on GitHub <#5036 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAELDXWLYCWLAH2EFO2ZPFLUWY7VLANCNFSM5LO3DA4Q>.
You are receiving this because you are subscribed to this thread.
|
Sorry, it wasn't a generic DS problem but a problem with my specific implementation. The original implementation simply called |
|
I looked at the code, I see what you mean, thanks!!
David Jencks
… On Jan 18, 2022, at 10:34 PM, Fr Jeremy Krieg ***@***.***> wrote:
Could you describe the problem leading to NPEs when using DS in more detail? From the description below it doesn’t make any sense to me that this could be happening. Thanks David Jencks
Sorry, it wasn't a generic DS problem but a problem with my specific implementation.
The original implementation simply called workspace.add/RemoveResourceChangeListener() as was appropriate. The problem was that the component wasn't fully activated (workspace was still null) when the first ResourceChangeListener services started getting implemented, which is why my implementation was causing an NPE. I couldn't see an easy way to fix that without buffering the resource listeners as they were registered and then registering them once the component was fully activated, which is why I employed a ServiceTracker to do that for me.
—
Reply to this email directly, view it on GitHub <#5036 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAELDXWEL2LL6ZEKDUIBDV3UWZLOJANCNFSM5LO3DA4Q>.
You are receiving this because you commented.
|
kriegfrj
force-pushed
the
builder-facade-with-central
branch
from
beb6289
to
2ea1a38
Compare
|
New push incorporates most of the changes above. Waiting for further discussion on the remainder. |
kriegfrj
force-pushed
the
builder-facade-with-central
branch
from
2ea1a38
to
ad124c0
Compare
|
Latest push incorporates the last suggestions. I think this still needs some more testing before we merge though - in particular, I think there are issues with the Java search and seems occasional startup issues where the builder is not ready in time. |
kriegfrj
force-pushed
the
builder-facade-with-central
branch
from
ad124c0
to
5fc5c2d
Compare
|
I think we should defer merging this until after 6.2.0 is released. We plan 6.2.0-RC1 for Feb 4. |
I agree. Though I was hoping at least a fix for #3175 might be able to be cherŕypicked. |
On second thoughts, probably better to defer that fix too to give the early adopters more time to test it out. |
For posterity:
|
Temporarily turn on debugging for review in case some CI failures pop up that don't happen on local box. Expect that this commit will be removed from the final PR. Signed-off-by: Fr Jeremy Krieg <fr.jkrieg@greekwelfaresa.org.au>
Was previously dependent on Bnd workspace only. Signed-off-by: Fr Jeremy Krieg <fr.jkrieg@greekwelfaresa.org.au>
Signed-off-by: Fr Jeremy Krieg <fr.jkrieg@greekwelfaresa.org.au>
Also fixes a potential race condition (albeit rare) in the dynamic reference provider where the isDefaultWorkspace() call was called outside of the read lock, so there was potential for its default status to change before the information could be retrieved. Signed-off-by: Fr Jeremy Krieg <fr.jkrieg@greekwelfaresa.org.au>
Because it is an abstract class rather than an interface the regular ExtensionFacade approach will not work. Signed-off-by: Fr Jeremy Krieg <fr.jkrieg@greekwelfaresa.org.au>
Signed-off-by: Fr Jeremy Krieg <fr.jkrieg@greekwelfaresa.org.au>
Signed-off-by: Fr Jeremy Krieg <fr.jkrieg@greekwelfaresa.org.au>
Signed-off-by: Fr Jeremy Krieg <fr.jkrieg@greekwelfaresa.org.au>
Signed-off-by: Fr Jeremy Krieg <fr.jkrieg@greekwelfaresa.org.au>
Since the upgrade to Eclipse baseline of 2022-09, it seems that Eclipse registers the workspace as an IWorkspace object. An exception stacktrace even recommends depending on this using DS or similar instead of using the static method ResourcePlugin.getWorkspace(). Adding this dependency fixes some runtime startup ordering problems. Signed-off-by: Fr Jeremy Krieg <fr.jkrieg@greekwelfaresa.org.au>
Hopefully the fixes to startup ordering in bndtools#5036 have fixed the flakiness issues. Signed-off-by: Fr Jeremy Krieg <fr.jkrieg@greekwelfaresa.org.au>
The BndContainerInitializer incorrectly depended on Central instead of ICentral. The Central component itself depended on the (Eclipse) Workspace, this has now been changed to depend on the Workbench so that Central is not created too early, which was causing a deadlock in the integration tests. Signed-off-by: Fr Jeremy Krieg <fr.jkrieg@greekwelfaresa.org.au>
Signed-off-by: Fr Jeremy Krieg <fr.jkrieg@greekwelfaresa.org.au>
Signed-off-by: Fr Jeremy Krieg <fr.jkrieg@greekwelfaresa.org.au>
Signed-off-by: Fr Jeremy Krieg <fr.jkrieg@greekwelfaresa.org.au>
Signed-off-by: Fr Jeremy Krieg <fr.jkrieg@greekwelfaresa.org.au>
Signed-off-by: Fr Jeremy Krieg <fr.jkrieg@greekwelfaresa.org.au>
Signed-off-by: Fr Jeremy Krieg <fr.jkrieg@greekwelfaresa.org.au>
Signed-off-by: Fr Jeremy Krieg <fr.jkrieg@greekwelfaresa.org.au>
Hopefully this will fix a startup issue. Signed-off-by: Fr Jeremy Krieg <fr.jkrieg@greekwelfaresa.org.au>
…tartup Signed-off-by: Fr Jeremy Krieg <fr.jkrieg@greekwelfaresa.org.au>
kriegfrj
force-pushed
the
builder-facade-with-central
branch
from
8a6dea8
to
5a6f3be
Compare
Signed-off-by: Fr Jeremy Krieg <fr.jkrieg@greekwelfaresa.org.au>
Specifying an explicit EE requires that EE to be registered in the workspace. That is too hard... Signed-off-by: Fr Jeremy Krieg <fr.jkrieg@greekwelfaresa.org.au>
Signed-off-by: Fr Jeremy Krieg <fr.jkrieg@greekwelfaresa.org.au>
kriegfrj
force-pushed
the
builder-facade-with-central
branch
from
5a6f3be
to
ddc7314
Compare
|
|
||
| void removeModelListener(ModelListener m); | ||
|
|
||
| void close(); |
There was a problem hiding this comment.
Is it really whise to have close on a service interface? How one would ever open the workspace again? what happens if one is currently workin on it and it gets closed concurrently...
There was a problem hiding this comment.
Yes, you're probably right. In fact, I just did some code searches and found that the original Central.close() is not even called anywhere from within Bndtools, so I think we can safely remove it.
|
|
||
| void changed(Project model); | ||
|
|
||
| void addModelListener(ModelListener m); |
There was a problem hiding this comment.
Maybe model listener can better use the whiteboardpattern instead?
There was a problem hiding this comment.
I agree 100%.
This was more about me trying to limit the scope of my PR which had already grown into a behemoth. This part of the code is not particularly well tested in CI (which is to say, not really tested at all!) and so I had a fear that something might break. So the goal was to make the minimal changes I needed in this PR to get the automated testing working reliably, and also so that it could get merged so that it could be subjected to some real-world testing by us on the bleeding edge. Then I could then proceed with additional refactorings (like this) with a little bit more confidence that I haven't broken anything in a subtle or fundamental way. I can foresee this interface evolving significantly as we start to leverage the power of a more component-driven approach.
There was a problem hiding this comment.
It dosen't hurt to have those though...
| @ProviderType | ||
| public interface ICentral { | ||
|
|
||
| Project getModel(IJavaProject project); |
There was a problem hiding this comment.
| Project getModel(IJavaProject project); | |
| Optional<Project> getModel(IProject project); |
There was a problem hiding this comment.
Our codebase hasn't made heavy use of the Optional pattern. But I suppose we probably should...
There was a problem hiding this comment.
Just noticed that this was also a change of the argument type from IJavaProject to IProject. I'm curious as to the reasoning? Every Bnd project is also a Java project.
There was a problem hiding this comment.
It is easy to get from IJavaProject > IProject but the other is not that easy (and better handled on a central 😛 place), also as it is maybe now the case who knows what the future brings?
So usually IProject is much more comfortable to use from api user point of view as you almost ever have one at hand.
There was a problem hiding this comment.
Our codebase hasn't made heavy use of the Optional pattern. But I suppose we probably should...
I've been using Result more and more. This has all the methods of Optional but
- it allows to carry a message when it fails,
- easy message construction with improved String.format,
- It allows lambdas that throw exceptions.
The idea is derived from Rust.
|
@laeubi , thank you so much for reviewing and your suggested changes! I have made some counter-comments above. |
|
After doing a lot of thinking and can't getting it out of my head, I've prepared a general solution for the facades that would be useful in Extension Points. See #5900 |
This is an early-access PR for those interested in how it's coming together.
The code is in obvious need of some tidy-ups. But I think I have reached a point where I can start trying to implement some regression tests. Before I ploughed ahead too much further I wanted to give the opportunity for some feedback on the direction/architecture.
Key features:
IResourceChangeListeners. You don't have to worry about registering/unregistering your listeners as DS will do it for you courtesy of the ResourceWhiteboard component. Just register a service/component that implementsIResourceChangeListenerand it will automatically receive resource change event notifications.There is still further simplification to be made by converting other pieces to DS components (eg, BndPreferences). But I think this is a good point to stop and add regression tests that will then make further such refactoring safer.