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

Moving PublishedGradleVersionsWrapper to a declarative OSGi service #1152

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions org.eclipse.buildship.core/.project
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,11 @@
<arguments>
</arguments>
</buildCommand>
<buildCommand>
<name>org.eclipse.pde.ds.core.builder</name>
<arguments>
</arguments>
</buildCommand>
</buildSpec>
<natures>
<nature>org.eclipse.jdt.core.javanature</nature>
Expand Down
4 changes: 3 additions & 1 deletion org.eclipse.buildship.core/META-INF/MANIFEST.MF
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@ Require-Bundle: org.eclipse.core.expressions,
com.google.gson;bundle-version="[2.7.0,3.0.0)",
org.eclipse.buildship.compat;visibility:=reexport
Bundle-ActivationPolicy: lazy
Import-Package: org.slf4j;version="1.7.2"
Import-Package: org.osgi.service.component.annotations;version="1.3.0";resolution:=optional,
org.slf4j;version="1.7.2"
Export-Package: org.eclipse.buildship.core,
org.eclipse.buildship.core.internal;x-friends:="org.eclipse.buildship.ui,org.eclipse.buildship.kotlin",
org.eclipse.buildship.core.internal.configuration;x-friends:="org.eclipse.buildship.ui,org.eclipse.buildship.kotlin",
Expand All @@ -46,3 +47,4 @@ Export-Package: org.eclipse.buildship.core,
org.eclipse.buildship.core.internal.util.variable;x-friends:="org.eclipse.buildship.ui,org.eclipse.buildship.kotlin",
org.eclipse.buildship.core.internal.workspace;x-friends:="org.eclipse.buildship.ui,org.eclipse.buildship.kotlin",
org.eclipse.buildship.core.invocation
Service-Component: OSGI-INF/org.eclipse.buildship.core.internal.util.gradle.PublishedGradleVersionsWrapper.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<?xml version="1.0" encoding="UTF-8"?>
<scr:component xmlns:scr="http://www.osgi.org/xmlns/scr/v1.1.0" name="org.eclipse.buildship.core.internal.util.gradle.PublishedGradleVersionsWrapper">
<property name="service.ranking" type="Integer" value="1"/>
<service>
<provide interface="org.eclipse.buildship.core.internal.util.gradle.PublishedGradleVersionsWrapper"/>
</service>
<implementation class="org.eclipse.buildship.core.internal.util.gradle.PublishedGradleVersionsWrapper"/>
</scr:component>
3 changes: 2 additions & 1 deletion org.eclipse.buildship.core/build.properties
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@ bin.includes = .,\
META-INF/,\
about.html,\
plugin.xml,\
.options
.options,\
OSGI-INF/org.eclipse.buildship.core.internal.util.gradle.PublishedGradleVersionsWrapper.xml
src.includes = plugin.xml,\
schema/,\
build.properties,\
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import java.util.Dictionary;
import java.util.Hashtable;
import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;
import java.util.stream.Collectors;

import org.osgi.framework.Bundle;
Expand Down Expand Up @@ -95,7 +96,6 @@ public final class CorePlugin extends Plugin {
// service tracker for each service to allow to register other service implementations of the
// same type but with higher prioritization, useful for testing
private ServiceTracker loggerServiceTracker;
private ServiceTracker publishedGradleVersionsServiceTracker;
private ServiceTracker workspaceOperationsServiceTracker;
private ServiceTracker internalGradleWorkspaceServiceTracker;
private ServiceTracker processStreamsProviderServiceTracker;
Expand All @@ -110,6 +110,9 @@ public final class CorePlugin extends Plugin {
private DefaultExternalLaunchConfigurationManager externalLaunchConfigurationManager;
private ToolingApiOperationManager operationManager;
private ExtensionManager extensionManager;

private static Map<Class<?>, ServiceTracker<?, ?>> trackers = new ConcurrentHashMap<>();


@Override
public void start(BundleContext bundleContext) throws Exception {
Expand Down Expand Up @@ -141,7 +144,6 @@ private void registerServices(BundleContext context) {

// initialize service trackers before the services are created
this.loggerServiceTracker = createServiceTracker(context, Logger.class);
this.publishedGradleVersionsServiceTracker = createServiceTracker(context, PublishedGradleVersionsWrapper.class);
this.workspaceOperationsServiceTracker = createServiceTracker(context, WorkspaceOperations.class);
this.internalGradleWorkspaceServiceTracker = createServiceTracker(context, InternalGradleWorkspace.class);
this.processStreamsProviderServiceTracker = createServiceTracker(context, ProcessStreamsProvider.class);
Expand Down Expand Up @@ -250,7 +252,6 @@ private void unregisterServices() {
this.processStreamsProviderServiceTracker.close();
this.internalGradleWorkspaceServiceTracker.close();
this.workspaceOperationsServiceTracker.close();
this.publishedGradleVersionsServiceTracker.close();
this.loggerServiceTracker.close();
}

Expand All @@ -263,7 +264,7 @@ public static Logger logger() {
}

public static PublishedGradleVersionsWrapper publishedGradleVersions() {
return (PublishedGradleVersionsWrapper) getInstance().publishedGradleVersionsServiceTracker.getService();
return getService(PublishedGradleVersionsWrapper.class);
}

public static WorkspaceOperations workspaceOperations() {
Expand Down Expand Up @@ -309,4 +310,20 @@ public static ToolingApiOperationManager operationManager() {
public static ExtensionManager extensionManager() {
return getInstance().extensionManager;
}

private static <T> T getService(Class<T> service) {
BundleContext context = plugin.getBundle().getBundleContext();
Copy link
Member

Choose a reason for hiding this comment

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

you might want to check if the plugin !=null

if(context == null) {
return null;
}
return service.cast(trackers.computeIfAbsent(service, key -> {
ServiceTracker<?, ?> tracker = new ServiceTracker<>(context, key, null);
tracker.open();
return tracker;
}).getService());
}


}


Original file line number Diff line number Diff line change
Expand Up @@ -18,58 +18,60 @@
import org.eclipse.core.runtime.IStatus;
import org.eclipse.core.runtime.Status;
import org.eclipse.core.runtime.jobs.Job;

import org.osgi.service.component.annotations.Component;
import org.eclipse.buildship.core.internal.CorePlugin;
import org.eclipse.buildship.core.internal.util.gradle.PublishedGradleVersions.LookupStrategy;

/**
* Wraps the {@link PublishedGradleVersions} functionality in a background job that handles all
* exceptions gracefully. If an exception occurs while calling the underlying
* {@link PublishedGradleVersions} instance, default version information is provided. This handles,
* for example, those scenarios where the versions cannot be retrieved because the user is behind a
* proxy or offline.
* Wraps the {@link PublishedGradleVersions} functionality in a background job
* that handles all exceptions gracefully. If an exception occurs while calling
* the underlying {@link PublishedGradleVersions} instance, default version
* information is provided. This handles, for example, those scenarios where the
* versions cannot be retrieved because the user is behind a proxy or offline.
*/
@Component(property = { "service.ranking:Integer=1" }, service = PublishedGradleVersionsWrapper.class)
Copy link
Member

Choose a reason for hiding this comment

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

Do you really need a non default service rank here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you really need a non default service rank here?

If you look at the activator it also sets this ranking so I just replicated it.

Copy link
Member

Choose a reason for hiding this comment

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

This is a final class, it is very unlikley that there are more than one service and even if they seem to be totaly eqivialent so this is most probably obsolete.

Service ranking is only important if there are multiple service of the same type and you need to have some kind of ordering of them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a final class, it is very unlikley that there are more than one service and even if they seem to be totaly eqivialent so this is most probably obsolete.

Service ranking is only important if there are multiple service of the same type and you need to have some kind of ordering of them.

Agree, but changing logic AND implementation is IMHO a bad way. So leaving the priority in should allow @donat to review this easier. And next step could be to remove with a follow-up commit.

public final class PublishedGradleVersionsWrapper {

private final AtomicReference<PublishedGradleVersions> publishedGradleVersions;
private final AtomicReference<PublishedGradleVersions> publishedGradleVersions;

public PublishedGradleVersionsWrapper() {
this.publishedGradleVersions = new AtomicReference<PublishedGradleVersions>();
new LoadVersionsJob().schedule();
}
public PublishedGradleVersionsWrapper() {
this.publishedGradleVersions = new AtomicReference<PublishedGradleVersions>();
new LoadVersionsJob().schedule();
}

public List<GradleVersion> getVersions() {
PublishedGradleVersions versions = this.publishedGradleVersions.get();
return versions != null ? versions.getVersions() : ImmutableList.of(GradleVersion.current());
}
public List<GradleVersion> getVersions() {
PublishedGradleVersions versions = this.publishedGradleVersions.get();
return versions != null ? versions.getVersions() : ImmutableList.of(GradleVersion.current());
}

/**
* Loads the published Gradle versions in the background.
* @author Stefan Oehme
*/
private final class LoadVersionsJob extends Job {
/**
* Loads the published Gradle versions in the background.
*
* @author Stefan Oehme
*/
private final class LoadVersionsJob extends Job {

public LoadVersionsJob() {
super("Loading available Gradle versions");
setSystem(true);
}
public LoadVersionsJob() {
super("Loading available Gradle versions");
setSystem(true);
}

@Override
protected IStatus run(IProgressMonitor monitor) {
try {
PublishedGradleVersions versions = PublishedGradleVersions.create(LookupStrategy.REMOTE_IF_NOT_CACHED);
PublishedGradleVersionsWrapper.this.publishedGradleVersions.set(versions);
} catch (RuntimeException e) {
CorePlugin.logger().warn("Could not load Gradle version information", e);
}
return Status.OK_STATUS;
}
@Override
protected IStatus run(IProgressMonitor monitor) {
try {
PublishedGradleVersions versions = PublishedGradleVersions.create(LookupStrategy.REMOTE_IF_NOT_CACHED);
PublishedGradleVersionsWrapper.this.publishedGradleVersions.set(versions);
} catch (RuntimeException e) {
CorePlugin.logger().warn("Could not load Gradle version information", e);
}
return Status.OK_STATUS;
}

@Override
protected void canceling() {
Thread.currentThread().interrupt();
}
@Override
protected void canceling() {
Thread.currentThread().interrupt();
}

}
}

}