Bug 427034 - Enforce non-usage of *StandaloneSetup "when it's not appropriate"
Summary: Enforce non-usage of *StandaloneSetup "when it's not appropriate"
Status: NEW
Alias: None
Product: TMF
Classification: Modeling
Component: Xtext (show other bugs)
Version: 2.4.3   Edit
Hardware: PC Linux
: P3 normal with 1 vote (vote)
Target Milestone: ---   Edit
Assignee: Project Inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-01-30 14:02 EST by Michael Vorburger CLA
Modified: 2014-07-11 14:12 EDT (History)
3 users (show)

See Also:


Attachments
StandaloneSetupChecker.java (rough) (2.44 KB, text/plain)
2014-01-30 14:03 EST, Michael Vorburger CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Vorburger CLA 2014-01-30 14:02:43 EST
Loads of folks seem to abuse *StandaloneSetup (sorry for the broad statement - I'm saying this based on issues such as https://bugs.eclipse.org/bugs/show_bug.cgi?id=326024, and I think it's been raised in the Forum now and then?).

Would it not be feasible to programmatically enforce non-usage of *StandaloneSetup "when it's not appropriate" ? -- I'm writing it in quotes, because the exact condition to use may warrant at least some discussion:

It's probably slightly more interesting than just something à la if EMFPlugin.IS_ECLIPSE_RUNNING, as that tests would miss the valid scenario of a headless Eclipse product (I'm not trying to be difficult, but we actually use this, in production... instead of the usual Maven plug-ins for Xtext language, we have a headless "Generator Edition", and a Maven plug-in firing that up... a bit heavy weight, but historic reasons), which IS_ECLIPSE_RUNNING of course, but doesn't have UI, so no XtextBuilder, so we do use *StandaloneSetup in there (but not in the full IDE, of course).

So you could do something like http://aniszczyk.org/2007/07/24/am-i-headless/, but that's a probably bit of a pain to do in *StandaloneSetupGenerated, because that would interfer with the usage of such JARs in e.g. Maven plug-ins (or simply some main() thing around an Xtext lang) - or would org.eclipse.core.runtime JAR always have to on the classpath of any such Xtext-based tool, so cool?

You could also do something like try { Class.forName("org.eclipse.swt.SWTError"); but that misses the point, because even when running in-IDE, because the non.ui bundle doesn't actually depend on SWT, with OSGi classloading SWT classes won't be seen.

In discussion with a colleague at work, we concluded that what you'd really want to ensure is "just" that IFF you've used *StandaloneSetup, then mostly you wouldn't ever want the *UiModule.  Likewise, if *UiModule has been around town (i.e. active in the same JVM), it's most likely a bug if *StandaloneSetup is also used, right?  How to check this?

I've cobbled something together based on checking Resource.Factory.Registry.INSTANCE or IResourceServiceProvider.Registry.INSTANCE for the extension of the language as first thing in the StandaloneSetup's createInjector(), and screaming if something is already there.  That seems to work for us (unless I'm misinterpreting what's causing what, which is possible), but I'm not convinced that's the smartest way to achieve what I'm after here. I'm attaching the helper for this - we're for now using this "manually" by just adding 	StandaloneSetupChecker.abortIfAlreadyRegistered("..."); into each of our *StandaloneSetup.

I'm sure you have smarter & better ideas how this could be done?
Comment 1 Michael Vorburger CLA 2014-01-30 14:03:50 EST
Created attachment 239486 [details]
StandaloneSetupChecker.java (rough)
Comment 2 Sven Efftinge CLA 2014-01-31 02:57:51 EST
I would want to use e.g. the workspace in headless as well. So that neither seem to work. OTOH I can imagine to use languages internally in some plug-in and totally not rely on any Eclipse IDE stuff, but just want to use a parser and say an interpreter in memory. So Eclipse would run and all that, but I still would you StandaloneSetup...
Comment 3 Sven Efftinge CLA 2014-01-31 03:03:04 EST
Just for clarification. I think we should do something about it and throwing an exception whenever standalonesetup is executed inside a running equinox, is IMHO a valid assumption. It just needs to be easy to say "I know what I'm doing".
Comment 4 Jan Koehnlein CLA 2014-01-31 03:52:56 EST
Sven, I don't understand completely. In the usecase you describe, the StandaloneSetup still messes up the singleton EMF registries, which is a no go. 

Couldn't we add a StandaloneSetup.doSetup(ResourceSet) for those cases that doesn't use the singleton registries but the resource set local ones? Having that we could throw an exception in the doSetup() when Eclipse is detected running.
Comment 5 Sebastian Zarnekow CLA 2014-01-31 04:35:34 EST
(In reply to Jan Koehnlein from comment #4)
> [..] we could throw an exception in the doSetup() when Eclipse is detected
> running.

It's rather unfortunate, but I'm afraid many of our unit tests would break if we'd throw exceptions as soon as a standalone setup is run with Equinox enabled.
Comment 6 Sven Efftinge CLA 2014-01-31 04:51:43 EST
(In reply to Jan Koehnlein from comment #4)
> Sven, I don't understand completely. In the usecase you describe, the
> StandaloneSetup still messes up the singleton EMF registries, which is a no
> go. 

Why do you think that's messed up? It uses it, yes but what problem do you expect?
Of course you won't be able to have the ui stuff of that language installed in the same Eclipse instance but otherwise it should work fine.
 
> Couldn't we add a StandaloneSetup.doSetup(ResourceSet) for those cases that
> doesn't use the singleton registries but the resource set local ones? 

Yes, we should think of how we could avoid using the global registry. We could have singleton instances in the injector, but that wouldn't allow for using multiple different languages.

> Having
> that we could throw an exception in the doSetup() when Eclipse is detected
> running.

I think we should just do that, but put the checking and throwing code into some template method, that can be overridden, such that this check can be explicitly deactivated.
Comment 7 Sven Efftinge CLA 2014-05-05 09:27:47 EDT
(In reply to Sebastian Zarnekow from comment #5)
> (In reply to Jan Koehnlein from comment #4)
> > [..] we could throw an exception in the doSetup() when Eclipse is detected
> > running.
> 
> It's rather unfortunate, but I'm afraid many of our unit tests would break
> if we'd throw exceptions as soon as a standalone setup is run with Equinox
> enabled.

Anyone running plain junit test as PDE test would see that exception. So I doubt we have found a good improvement so far.
Comment 8 Michael Vorburger CLA 2014-07-11 14:12:30 EDT
Maybe the entire "check if Eclipse (UI | headless) is running" approach got us started on the wrong foot here... what about something based on other idea I initially raised - checking the global static registries?

> IFF you've used *StandaloneSetup, then mostly you wouldn't ever want the *UiModule.  Likewise, if *UiModule has been around town (i.e. active in the same JVM), it's most likely a bug if *StandaloneSetup is also used, right?  How to check this?

That's easy to implement per se, but has two big problems that I've hit when trying this out the naive way: a) Many existing tests aren't perfect, in our in-house code base at least, and don't restore registry.  That could be addressed by cleaning up such bad code - if this (new, optional) feature gave clear error messages of what last installed the entry in the global static registries.. I'm thinking some try { throw E.. } catch (E e) trick, and keep e around, like e.g. some JDBC Connection Pools do for detecting who didn't close re-used Connection last (have you seen that approach, back in the day) ?  That would help to quickly clean up.  

b) If you run both standalone @RunWith(XtextRunner.class) as well as full Eclipse UI tests, not using @InjectWith(MainLangUIInjectorProvider.class) but just.. you know, old-style Eclipse UI integration tests, which e.g. a Tycho build typically would in the same VM (AFAIU?), it would get all mixed up and this idea could never work? 

Nobody got any clever idea how to do something about this?  hm.