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.

Sign up for GitHub

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

Jump to bottom

Add pub support #107

Merged
merged 19 commits into from
Jul 31, 2019
Merged

Add pub support #107

merged 19 commits into from
Jul 31, 2019

Conversation

jonas-jonas
Copy link
Member

@jonas-jonas jonas-jonas commented Jul 21, 2019 •
edited
Loading

  • Automatic synchronization of pub depencies (using pub get) for any project.
    -Can be disabled via the Preference page
  • A command in the project context menus for Dart projects to sync the dependencies (also via pub get) manually
  • Preference to use the --offline flag for all pub get operations (to use locally cached packages for package resolution)

WIP Adds pub support
542a349 
Signed-off-by: Jonas Hungershausen <jonas.hungershausen@vogella.com>
@jonas-jonas
Copy link
Member Author

Hi @lak-proddev,

I tinkered around with a workspace listener to listen on changes to "pubspec.yaml" files. It is quite tedious and I'm not sure if this is the correct way to do this. Could you review and test? Thanks.

@lak-proddev
Copy link
Contributor

Hi @lak-proddev,

I tinkered around with a workspace listener to listen on changes to "pubspec.yaml" files. It is quite tedious and I'm not sure if this is the correct way to do this. Could you review and test? Thanks.

As per @vogella comment, We need to use startup OSGi service to register the resource listener.
I think this is what he meant for
https://wiki.eclipse.org/Eclipse4/RCP/Lifecycle

    @PostContextCreate
    public void startup(IEclipseContext context) {
       // do initialization 
    }

I also looking same solution for registring the logger

@lak-proddev
- using e4 model
50ff97c 
Signed-off-by: Lakshminarayana Nekkanti <narayana.nekkanti@gmail.com>
@lak-proddev
Copy link
Contributor

@jonas-jonas I pushed mistakenly. Please, check whether it's working for you as expected.

@lak-proddev
Revert "- using e4 model"
34a9bfa 
This reverts commit 50ff97c.
@jonas-jonas
Copy link
Member Author

@jonas-jonas I pushed mistakenly. Please, check whether it's working for you as expected.

No problem at all. For me it seems to work with your commit before the revert. Why did you revert?

Also do feel free to push onto branches of mine, if you feel it's needed to improve the code. No need to communicate code changes solely through comments. :)

@vogella
Copy link
Contributor

vogella commented Jul 22, 2019 via email

@lak-proddev
Copy link
Contributor

lak-proddev commented Jul 22, 2019 •
edited
Loading

Jonas, Yes it will work. But, need to improve a bit. With my commit, the listener will add for every event (multiple times).
So, need to find the correct event.

@jonas-jonas
Copy link
Member Author

IIRC the annotation provides a way to enable it eagerly. Jonas, please have
a look and add this to the tutorial.
You can use the OSGi console to see if it is running

@vogella, @lak-proddev didn't use an annotation for the service. It is registered via the org.eclipse.dartboard.workbench.model.WorkbenchModelProcessor extension point.
In there a subscription to the APP_STARTUP_COMPLETE event is done. And in there the Listener is added to the workspace. As this should only be fired once (on App startup), I don't see a problem there @lak-proddev. But I'm probably missing something here.

Use OSGI service instead of StartupListener
94be338 
Signed-off-by: Jonas Hungershausen <jonas.hungershausen@vogella.com>
jonas-jonas
jonas-jonas commented Jul 22, 2019
View reviewed changes
Copy link
Member Author

@jonas-jonas jonas-jonas left a comment

Choose a reason for hiding this comment

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

I used an OSGI service here instead of the StartupListener. I'm not sure if I've done this correctly. Could someone verify?

org.eclipse.dartboard/META-INF/MANIFEST.MF Outdated Show resolved Hide resolved
org.eclipse.dartboard/OSGI-INF/org.eclipse.dartboard.ListenerService.xml Outdated Show resolved Hide resolved
@vogella
Copy link
Contributor

vogella commented Jul 22, 2019 via email

Jonas Hungershausen added 5 commits July 22, 2019 22:23
Merge branch master into pub
bc9e605 
Signed-off-by: Jonas Hungershausen <jonas.hungershausen@vogella.com>
Merge remote-tracking branch 'origin/master' into pub
7b31109
Add support for --offline and use NLS for messages
be7bfb6 
Signed-off-by: Jonas Hungershausen <jonas.hungershausen@vogella.com>
Merge branch master
efc7fc3 
Signed-off-by: Jonas Hungershausen <jonas.hungershausen@vogella.com>
Add "get dependencies" handler and command
370dccd 
Signed-off-by: Jonas Hungershausen <jonas.hungershausen@vogella.com>
@jonas-jonas
Copy link
Member Author

This PR adds multiple things related to the Pub package manager.
Pub is distributed with any Dart SDK (in the bin/ directory).

Features added by the PR:

  • Automatic synchronization of pub depencies (using pub get) for any project.
    • Can be disabled via the Preference page
  • A command in the project context menus for Dart projects to sync the dependencies (also via pub get) manually
  • Preference to use the --offline flag for all pub get operations (to use locally cached packages for package resolution)

I tested it on my windows VM and it worked for me. Please do test on your machine.

Also I'd like to provide PubServer as an @Injectable service but it doesn't seem to work with AbstractHandlers with the org.eclipse.ui.handlers extension point. Is this even possible?

Jonas Hungershausen added 2 commits July 25, 2019 17:47
Add explanatory comment to PUBSPEC constant
9702545 
Signed-off-by: Jonas Hungershausen <jonas.hungershausen@vogella.com>
Refine check for visibility of Pub popup menu entry
f2793c8 
Signed-off-by: Jonas Hungershausen <jonas.hungershausen@vogella.com>
@jonas-jonas jonas-jonas removed the request for review from lak-proddev July 29, 2019 14:42
Jonas Hungershausen added 2 commits July 29, 2019 21:53
Add PUB_ENVIROMENT variable to the pub get call
6d58a26 
Signed-off-by: Jonas Hungershausen <jonas.hungershausen@vogella.com>
Add PubUtil
0274a9c 
Signed-off-by: Jonas Hungershausen <jonas.hungershausen@vogella.com>
lak-proddev
lak-proddev reviewed Jul 30, 2019
View reviewed changes
org.eclipse.dartboard/src/org/eclipse/dartboard/ListenerService.java Outdated Show resolved Hide resolved
org.eclipse.dartboard/src/org/eclipse/dartboard/pub/PubGetHandler.java Outdated Show resolved Hide resolved
org.eclipse.dartboard/src/org/eclipse/dartboard/pub/PubGetHandler.java Show resolved Hide resolved
org.eclipse.dartboard/src/org/eclipse/dartboard/pub/PubGetHandler.java Outdated Show resolved Hide resolved
org.eclipse.dartboard/src/org/eclipse/dartboard/pub/PubGetHandler.java Show resolved Hide resolved
org.eclipse.dartboard/src/org/eclipse/dartboard/util/DartUtil.java Show resolved Hide resolved
org.eclipse.dartboard/src/org/eclipse/dartboard/util/DartUtil.java Show resolved Hide resolved
org.eclipse.dartboard/src/org/eclipse/dartboard/ListenerService.java Outdated
@Activate
public void registerListeners() {
IWorkspace workspace = ResourcesPlugin.getWorkspace();
workspace.addResourceChangeListener(new PubspecChangeListener());
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we need to remove the listener?. I am not sure.

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, ideally we would remove the listener when the user unchecks the checkbox on the preference page. But as the ListenerService is not available in the class in the current design, the listener stays registered as is and the necessary checks are done whenever it fires.

And it should stay registered until the workspace is closed.

org.eclipse.dartboard/src/org/eclipse/dartboard/propertytester/IsDartProjectPropertyTester.java Outdated Show resolved Hide resolved
org.eclipse.dartboard/src/org/eclipse/dartboard/pub/PubService.java
* A {@link Map} holding an {@link IProject} and a {@link Job} as ideally there
* should only be one {@code pub get} job running per project.
*/
private Map<IProject, Job> pubGetJobs = new HashMap<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure about the flow of this class. Can you check do we need to clear or making null.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure about the flow of this class. Can you check do we need to clear or making null.

I'm not sure what you mean by this. The Map should always be the same object for the lifetime of the object.

Jonas Hungershausen added 2 commits July 30, 2019 16:43
Fix comments
9dc6df3 
Signed-off-by: Jonas Hungershausen <jonas.hungershausen@vogella.com>
Remove obsolete object reference
2aeb5f1 
Signed-off-by: Jonas Hungershausen <jonas.hungershausen@vogella.com>
@jonas-jonas
Copy link
Member Author

Alright, I think I tested the functionality of this patch thoroughly on Windows and Linux.

As mentioned in the comments above, most of the service architecture depends on a working structure of OSGi services, which is out of scope of this patch. I will ask @vogella once he is back in office to see what we can do about it.

I'd like to merge and see what happens. Any objections?

@lak-proddev
Copy link
Contributor

No objections from my side.

@jonas-jonas
Copy link
Member Author

And I would merge if there wasn't an exception on every startup related to the OSGi service:

If the plugin is installed from an update site, after a second restart after the install the IDE fails to start with the following exception in the log. It does not happen if I remove the ListenerService.

Anyone know a solution to this? If not, the startup extension would be the other solution, we could use.

Root exception:
java.lang.IllegalStateException: The instance data location has not been specified yet.
	at org.eclipse.core.internal.runtime.DataArea.assertLocationInitialized(DataArea.java:59)
	at org.eclipse.core.internal.runtime.DataArea.getStateLocation(DataArea.java:136)
	at org.eclipse.core.internal.runtime.InternalPlatform.getStateLocation(InternalPlatform.java:545)
	at org.eclipse.core.runtime.Plugin.getStateLocation(Plugin.java:259)
	at org.eclipse.core.internal.resources.LocalMetaArea.<init>(LocalMetaArea.java:63)
	at org.eclipse.core.resources.ResourcesPlugin.start(ResourcesPlugin.java:481)
	at org.eclipse.osgi.internal.framework.BundleContextImpl$3.run(BundleContextImpl.java:842)
	at org.eclipse.osgi.internal.framework.BundleContextImpl$3.run(BundleContextImpl.java:1)
	at java.security.AccessController.doPrivileged(Native Method)
	at org.eclipse.osgi.internal.framework.BundleContextImpl.startActivator(BundleContextImpl.java:834)
	at org.eclipse.osgi.internal.framework.BundleContextImpl.start(BundleContextImpl.java:791)
	at org.eclipse.osgi.internal.framework.EquinoxBundle.startWorker0(EquinoxBundle.java:1013)
	at org.eclipse.osgi.internal.framework.EquinoxBundle$EquinoxModule.startWorker(EquinoxBundle.java:365)
	at org.eclipse.osgi.container.Module.doStart(Module.java:598)
	at org.eclipse.osgi.container.Module.start(Module.java:462)
	at org.eclipse.osgi.framework.util.SecureAction.start(SecureAction.java:493)
	at org.eclipse.osgi.internal.hooks.EclipseLazyStarter.postFindLocalClass(EclipseLazyStarter.java:117)
	at org.eclipse.osgi.internal.loader.classpath.ClasspathManager.findLocalClass(ClasspathManager.java:570)
	at org.eclipse.osgi.internal.loader.ModuleClassLoader.findLocalClass(ModuleClassLoader.java:330)
	at org.eclipse.osgi.internal.loader.BundleLoader.findLocalClass(BundleLoader.java:396)
	at org.eclipse.osgi.internal.loader.sources.SingleSourcePackage.loadClass(SingleSourcePackage.java:41)
	at org.eclipse.osgi.internal.loader.BundleLoader.findClassInternal(BundleLoader.java:470)
	at org.eclipse.osgi.internal.loader.BundleLoader.findClass(BundleLoader.java:423)
	at org.eclipse.osgi.internal.loader.BundleLoader.findClass(BundleLoader.java:415)
	at org.eclipse.osgi.internal.loader.ModuleClassLoader.loadClass(ModuleClassLoader.java:155)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:357)
	at java.lang.Class.getDeclaredConstructors0(Native Method)
	at java.lang.Class.privateGetDeclaredConstructors(Class.java:2671)
	at java.lang.Class.getConstructors(Class.java:1651)
	at org.apache.felix.scr.impl.inject.ComponentConstructor.<init>(ComponentConstructor.java:94)
	at org.apache.felix.scr.impl.inject.ComponentMethodsImpl.initComponentMethods(ComponentMethodsImpl.java:106)
	at org.apache.felix.scr.impl.manager.AbstractComponentManager.initDependencyManagers(AbstractComponentManager.java:1008)
	at org.apache.felix.scr.impl.manager.AbstractComponentManager.collectDependencies(AbstractComponentManager.java:1026)
	at org.apache.felix.scr.impl.manager.SingleComponentManager.getServiceInternal(SingleComponentManager.java:936)
	at org.apache.felix.scr.impl.manager.AbstractComponentManager.activateInternal(AbstractComponentManager.java:765)
	at org.apache.felix.scr.impl.manager.AbstractComponentManager.enableInternal(AbstractComponentManager.java:666)
	at org.apache.felix.scr.impl.manager.AbstractComponentManager.enable(AbstractComponentManager.java:432)
	at org.apache.felix.scr.impl.manager.ConfigurableComponentHolder.enableComponents(ConfigurableComponentHolder.java:665)
	at org.apache.felix.scr.impl.BundleComponentActivator.initialEnable(BundleComponentActivator.java:339)
	at org.apache.felix.scr.impl.Activator.loadComponents(Activator.java:381)
	at org.apache.felix.scr.impl.Activator.access$200(Activator.java:49)
	at org.apache.felix.scr.impl.Activator$ScrExtension.start(Activator.java:263)
	at org.apache.felix.scr.impl.AbstractExtender.createExtension(AbstractExtender.java:196)
	at org.apache.felix.scr.impl.AbstractExtender.modifiedBundle(AbstractExtender.java:169)
	at org.apache.felix.scr.impl.AbstractExtender.addingBundle(AbstractExtender.java:139)
	at org.apache.felix.scr.impl.AbstractExtender.addingBundle(AbstractExtender.java:49)
	at org.osgi.util.tracker.BundleTracker$Tracked.customizerAdding(BundleTracker.java:475)
	at org.osgi.util.tracker.BundleTracker$Tracked.customizerAdding(BundleTracker.java:1)
	at org.osgi.util.tracker.AbstractTracked.trackAdding(AbstractTracked.java:256)
	at org.osgi.util.tracker.AbstractTracked.track(AbstractTracked.java:229)
	at org.osgi.util.tracker.BundleTracker$Tracked.bundleChanged(BundleTracker.java:450)
	at org.eclipse.osgi.internal.framework.BundleContextImpl.dispatchEvent(BundleContextImpl.java:973)
	at org.eclipse.osgi.framework.eventmgr.EventManager.dispatchEvent(EventManager.java:234)
	at org.eclipse.osgi.framework.eventmgr.ListenerQueue.dispatchEventSynchronous(ListenerQueue.java:151)
	at org.eclipse.osgi.internal.framework.EquinoxEventPublisher.publishBundleEventPrivileged(EquinoxEventPublisher.java:234)
	at org.eclipse.osgi.internal.framework.EquinoxEventPublisher.publishBundleEvent(EquinoxEventPublisher.java:140)
	at org.eclipse.osgi.internal.framework.EquinoxEventPublisher.publishBundleEvent(EquinoxEventPublisher.java:132)
	at org.eclipse.osgi.internal.framework.EquinoxContainerAdaptor.publishModuleEvent(EquinoxContainerAdaptor.java:231)
	at org.eclipse.osgi.container.Module.publishEvent(Module.java:493)
	at org.eclipse.osgi.container.Module.start(Module.java:480)
	at org.eclipse.osgi.container.ModuleContainer$ContainerStartLevel$1.run(ModuleContainer.java:1820)
	at org.eclipse.osgi.internal.framework.EquinoxContainerAdaptor$2$1.execute(EquinoxContainerAdaptor.java:150)
	at org.eclipse.osgi.container.ModuleContainer$ContainerStartLevel.incStartLevel(ModuleContainer.java:1813)
	at org.eclipse.osgi.container.ModuleContainer$ContainerStartLevel.incStartLevel(ModuleContainer.java:1769)
	at org.eclipse.osgi.container.ModuleContainer$ContainerStartLevel.doContainerStartLevel(ModuleContainer.java:1735)
	at org.eclipse.osgi.container.ModuleContainer$ContainerStartLevel.dispatchEvent(ModuleContainer.java:1661)
	at org.eclipse.osgi.container.ModuleContainer$ContainerStartLevel.dispatchEvent(ModuleContainer.java:1)
	at org.eclipse.osgi.framework.eventmgr.EventManager.dispatchEvent(EventManager.java:234)
	at org.eclipse.osgi.framework.eventmgr.EventManager$EventThread.run(EventManager.java:345)

@vogella
Copy link
Contributor

vogella commented Jul 30, 2019 via email

Remove osgi services
cf0f06a 
Signed-off-by: Jonas Hungershausen <jonas.hungershausen@vogella.com>
@jonas-jonas
Copy link
Member Author

After trying virtually every approach I could find, with non of them working without any issues, I reverted back to the startup extension that works without issues out of the box.

As mentioned in previous comments, I'd like to use OSGi services later on for all sorts of services, but couldn't figure out how to use them everywhere. @vogella when you're back we should try to sort this out.

For now, I'm going to merge this patch because I'd like to move forward with featuers. We can refactor it later to conform to OSGi services.

Jonas Hungershausen added 3 commits July 31, 2019 11:10
Further removals of OSGi related stuff
b53d514 
Signed-off-by: Jonas Hungershausen <jonas.hungershausen@vogella.com>
autorefactor
9119b10 
Signed-off-by: Jonas Hungershausen <jonas.hungershausen@vogella.com>
Add missing line to end of ListenerService file
5cabde8 
Signed-off-by: Jonas Hungershausen <jonas.hungershausen@vogella.com>
@jonas-jonas jonas-jonas changed the title WIP Add pub support Add pub support Jul 31, 2019
@jonas-jonas jonas-jonas merged commit f8696b7 into master Jul 31, 2019
This was referenced Jul 31, 2019
Closed
Closed
@jonas-jonas jonas-jonas deleted the pub branch August 1, 2019 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lak-proddev lak-proddev lak-proddev left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jonas-jonas @lak-proddev @vogella