Community
Participate
Working Groups
Follow up on bug 514297 comment 5. A simple call to AbstractUIPlugin.getPreferenceStore() or simply to ScopedPreferenceStore() can lead to the workspace location silently set to the default one, ignoring location selected by the user in the workspace prompt and probably leading to other startup problems. This is not acceptable behavior. We can't find this during the tests (because tests are always using -data argument) and we can't find it via code inspection (because calling the API in question is considered to be safe in general). We should make sure that we can't *silently* initialize workspace to the default before the user confirms the workspace location prompt. Related bugs also: bug 330320, bug 419940, bug 428977, bug 491850 and others I couldn't quickly find right now. I have no solution right now...
Should we return null or throw some specific kind of exception in InstancePreferences.getBaseLocation() in case the workspace location is not known yet? Whoever tries to access workspace preferences before we have the workspace instance should simply fail there, and NOT silently create and return some *default* workspace location.
(In reply to Andrey Loskutov from comment #1) > Should we return null or throw some specific kind of exception in > InstancePreferences.getBaseLocation() in case the workspace location is not > known yet? > > Whoever tries to access workspace preferences before we have the workspace > instance should simply fail there, and NOT silently create and return some > *default* workspace location. I'd prefer an exception.
+1 to Dani's opinion (exception)
This is a tricky issue. DataArea.assertLocationInitialized() logic does not check if a location is set or not, and because EquinoxLocations by default initializes the data location with a *default* value, DataArea.assertLocationInitialized() creates always a location from the default URL, and org.eclipse.core.internal.preferences.InstancePreferences.getBaseLocation() always able to return a path. OK, one could think that we should simply put "-data @nodefault" value by default to the IDE eclipse.ini, but this will do even more harm! With "-data @nodefault" option EquinoxLocations creates a BasicLocation for instance data without the default value (good), but this leads to the problem that the InstancePreferences will simply work with an "empty" workspace location, even after user selects the workspace path (bad), and that plugins asking for the data *before* workspace is set will not see actual workspace settings for the rest of the session. What we actually want is: if any client tries to access workspace preferences *before* the workspace location is set, it should receive some kind of exception and can decide what to do. In our case (bug 514297 for example) this will lead to the Eclipse throwing an exception with a very clear stack trace pointing to the place where the preferences were requested. IMHO this is much better then silently starting with a default workspace. I think I have a patch proposal for Equinox, will submit in a moment. This will throw IllegalStateException on accessing the preference store before workspace location is set. To test it, the IDEWorkbenchPlugin should be reverted to commit fcf5b6385147db2f0266993950402082e8519d8b. Unfortunately this patch conflicts with the promise of the AbstractUIPlugin.getPreferenceStore(): "If an error occurs reading the preference store, an empty preference store is quietly created, initialized with defaults, and returned." So I'm not sure how to proceed now. Current state is bad, but breaking API contract is not good too.
New Gerrit change created: https://git.eclipse.org/r/94032
What happens for cases where there is an RCP application that has no workspace chooser dialog? Is it not reasonable that they would set the value osgi.instance.area.default in the config area and never explicitly set the Location URL?
(In reply to Andrey Loskutov from comment #4) > This is a tricky issue. ;-)
(In reply to Thomas Watson from comment #6) > What happens for cases where there is an RCP application that has no > workspace chooser dialog? Is it not reasonable that they would set the > value osgi.instance.area.default in the config area and never explicitly set > the Location URL? Yep, this is probably another issue with the patch (except that it breaks AbstractUIPlugin.getPreferenceStore() contract). OK, let assume we will "undefine" osgi.instance.area.default value from our IDE config ini. Currently someone defines it like @user.home/workspace. But then EquinoxLocations constructor by default sets this to defaultLocation = buildURL(new File(System.getProperty(PROP_USER_DIR), "workspace").getAbsolutePath(), true); WHY? If we comment the lines out (I will push another patch for that), then we will have a nice (expected) exception on startup.
New Gerrit change created: https://git.eclipse.org/r/94078
(In reply to Eclipse Genie from comment #9) > New Gerrit change created: https://git.eclipse.org/r/94078 This patch should be used together with the *second* patch set from https://git.eclipse.org/r/#/c/94032/2.
(In reply to Andrey Loskutov from comment #8) > (In reply to Thomas Watson from comment #6) > > What happens for cases where there is an RCP application that has no > > workspace chooser dialog? Is it not reasonable that they would set the > > value osgi.instance.area.default in the config area and never explicitly set > > the Location URL? > > Yep, this is probably another issue with the patch (except that it breaks > AbstractUIPlugin.getPreferenceStore() contract). > > OK, let assume we will "undefine" osgi.instance.area.default value from our > IDE config ini. Currently someone defines it like @user.home/workspace. > > But then EquinoxLocations constructor by default sets this to > defaultLocation = buildURL(new File(System.getProperty(PROP_USER_DIR), > "workspace").getAbsolutePath(), true); > > WHY? If we comment the lines out (I will push another patch for that), then > we will have a nice (expected) exception on startup. Coincidentally I just published a change from Mickael for bug 502220 that changes that "workspace" to be ${launcher name}-workspace. Perhaps Mickael has a motivating use case for why the instance location should get a default even when one is not specified in the configuration.
> WHY? If we comment the lines out (I will push another patch for that), then > we will have a nice (expected) exception on startup. Looking back at the history of this it seems that it was done this way because the default workspace was always set for free way back. See bug 62007.
(In reply to Thomas Watson from comment #12) > > WHY? If we comment the lines out (I will push another patch for that), then > > we will have a nice (expected) exception on startup. > > Looking back at the history of this it seems that it was done this way > because the default workspace was always set for free way back. See bug > 62007. After reading through the bug 62004 I didn't get this, sorry, may be you have some concrete commit in mind? Thebug 62007 describes the special default variables to be introduced but lines I've commented out are basically setting hard-coded default value for the instance locaton.
(In reply to Andrey Loskutov from comment #13) > (In reply to Thomas Watson from comment #12) > > > WHY? If we comment the lines out (I will push another patch for that), then > > > we will have a nice (expected) exception on startup. > > > > Looking back at the history of this it seems that it was done this way > > because the default workspace was always set for free way back. See bug > > 62007. > > After reading through the bug 62004 I didn't get this, sorry, may be you > have some concrete commit in mind? Thebug 62007 describes the special > default variables to be introduced but lines I've commented out are > basically setting hard-coded default value for the instance locaton. Unfortunately some history got lost with renaming of Equinox internals. You have to go back to the R3_9_maintenance branch to see the history of the LocationManager class where the EquinoxLocations class got much of its code and behavior: http://git.eclipse.org/c/equinox/rt.equinox.framework.git/diff/?id=573bd5235d26f67af530bb1f5ae09aec95b463f5 eGit seems to show this commit in the history of the EquinoxLocations class, but command line git log and the web UI on the repo doesn't provide much help.
(In reply to Thomas Watson from comment #12) > > WHY? If we comment the lines out (I will push another patch for that), then > > we will have a nice (expected) exception on startup. > > Looking back at the history of this it seems that it was done this way > because the default workspace was always set for free way back. See bug > 62007. Thomas, thanks to the pointer. In that sense, what do you think about new preference for osgi, which would say "explicit init only" for locations? osgi.instance.area.init=explicit Default value will be of course "implicit" to match our hisorical behavior. But if this is set in the config to "explicit", we will not use hard coded defaults in EquinoxLocations and so we will hard fail on any code trying to init the location which is not set explicitly. If we add this to the SDK no clients schould be affected. There is still the question of the AbstractUIPlugin.getPreferenceStore() contract but I think we can clarify its javadoc in the sense that *after* workspace location is set, the method will always return a properly initialized store, but may throw an IllegalStateException if the workspace location is not known yet and osgi.instance.area.init=explicit is set. I think this would make everyone happy and we will stay compatible. WDYT?
(In reply to Andrey Loskutov from comment #15) > WDYT? Seems like a reasonable approach to me.
I've now tried different approaches how to pinpoint the problem. I think I've finally found one, which requires a system property set by the product and a tiny change in DataArea. Patches will follow in few moments, old patches are obsoleted now.
(In reply to Andrey Loskutov from comment #17) > I've now tried different approaches how to pinpoint the problem. I think > I've finally found one, which requires a system property set by the product > and a tiny change in DataArea. Patches will follow in few moments, old > patches are obsoleted now. When you say system property I hope you mean configuration property set in the config.ini. That is not entirely the same as a system property.
New Gerrit change created: https://git.eclipse.org/r/94671
(In reply to Thomas Watson from comment #18) > (In reply to Andrey Loskutov from comment #17) > > I've now tried different approaches how to pinpoint the problem. I think > > I've finally found one, which requires a system property set by the product > > and a tiny change in DataArea. Patches will follow in few moments, old > > patches are obsoleted now. > > When you say system property I hope you mean configuration property set in > the config.ini. That is not entirely the same as a system property. I meant a system property, see https://git.eclipse.org/r/94671. Why is this important? Do you mean someone could set the system property after startup but before the init of the DataArea? I'm not sure if and how I can access config property in org.eclipse.equinox.preferences and org.eclipse.equinox.common plugins, if you can point me to the API, I can change that.
Created attachment 267699 [details] example stack if using preferences service I've attached the example log produced by the patch and adding the code below to the begin of org.eclipse.ui.internal.ide.IDEWorkbenchPlugin.createProblemsViews() IScopeContext[] contexts = new IScopeContext[] { InstanceScope.INSTANCE, DefaultScope.INSTANCE, BundleDefaultsScope.INSTANCE, ConfigurationScope.INSTANCE }; boolean b = Platform.getPreferencesService().getBoolean("org.eclipse.ui.ide", //$NON-NLS-1$ IDEInternalPreferences.SHOW_PROBLEMS_VIEW_DECORATIONS_ON_STARTUP, false, contexts); if(!b){ return; }
Created attachment 267700 [details] example stack if using preference store Similar example for the log produced by the patch and adding the code below to the begin of org.eclipse.ui.internal.ide.IDEWorkbenchPlugin.createProblemsViews() if (!getDefault().getPreferenceStore() .getBoolean(IDEInternalPreferences.SHOW_PROBLEMS_VIEW_DECORATIONS_ON_STARTUP)) { return; }
New Gerrit change created: https://git.eclipse.org/r/94677
(In reply to Eclipse Genie from comment #23) > New Gerrit change created: https://git.eclipse.org/r/94677 This is the patch for the SDK product to set the system property into the config.ini.
@Thomas, do you have time for a review?
To me this looks like too late and big for 4.7. I suggest to retarget to Oxygen.1. Tom, please change the target if you disagree.
(In reply to Dani Megert from comment #26) > To me this looks like too late and big for 4.7. I suggest to retarget to > Oxygen.1. > > Tom, please change the target if you disagree. I agree.
(In reply to Thomas Watson from comment #27) > (In reply to Dani Megert from comment #26) > > To me this looks like too late and big for 4.7. I suggest to retarget to > > Oxygen.1. > > > > Tom, please change the target if you disagree. > > I agree. Thomas, can we please proceed with the review? https://git.eclipse.org/r/#/c/94671/
I don't think we should be doing this for Oxygen at this point. Lets first get it into Photon.
(In reply to Thomas Watson from comment #29) > I don't think we should be doing this for Oxygen at this point. Lets first > get it into Photon. Thomas, can we please proceed with the patch review?
Gerrit change https://git.eclipse.org/r/94671 was merged to [master]. Commit: http://git.eclipse.org/c/equinox/rt.equinox.bundles.git/commit/?id=7386a58f7b3c7a192186fa1145213c5515ba2a8c
(In reply to Andrey Loskutov from comment #30) > (In reply to Thomas Watson from comment #29) > > I don't think we should be doing this for Oxygen at this point. Lets first > > get it into Photon. > > Thomas, can we please proceed with the patch review? I published the equinox changes. Leaving open until you update the SDK bit.
(In reply to Thomas Watson from comment #32) > (In reply to Andrey Loskutov from comment #30) > > (In reply to Thomas Watson from comment #29) > > > I don't think we should be doing this for Oxygen at this point. Lets first > > > get it into Photon. > > > > Thomas, can we please proceed with the patch review? > > I published the equinox changes. Leaving open until you update the SDK bit. Thanks Thomas! @Sravan, Alexander: I've rebased the patch https://git.eclipse.org/r/94677/ for the sdk.product file in org.eclipse.platform.releng.tychoeclipsebuilder. Can you please review this patch?
Gerrit change https://git.eclipse.org/r/94677 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.releng.aggregator.git/commit/?id=f50f26c40744176ae6f2ace2bd45410130ea75de
This caused p2 tests to fail with "The instance data location has not been specified yet." See http://download.eclipse.org/eclipse/downloads/drops4/I20170920-2000/testresults/html/org.eclipse.equinox.p2.tests_ep48I-unit-cen64-gtk2_linux.gtk.x86_64_8.0.html
New Gerrit change created: https://git.eclipse.org/r/105555
(In reply to Eclipse Genie from comment #36) > New Gerrit change created: https://git.eclipse.org/r/105555 This is my dirty workaround - pass the vmargh with false to enforce old state of affairs. If one thinks code should be changed to instantiate config area please do so.
(In reply to Alexander Kurtakov from comment #37) > (In reply to Eclipse Genie from comment #36) > > New Gerrit change created: https://git.eclipse.org/r/105555 > > This is my dirty workaround - pass the vmargh with false to enforce old > state of affairs. If one thinks code should be changed to instantiate config > area please do so. Alex, could you point to the preferences code in p2 where this setting makes things work? Andrey could you have a look to see if we are doing things wrong in p2?
Ping! Tests still fail.
(In reply to Dani Megert from comment #39) > Ping! Tests still fail. I'm looking into it right now.
Created attachment 270321 [details] org.eclipse.equinox.p2.tests.AutomatedTests.txt Attached is one of the logs with failing tests. The common error stack is below. The test starts Eclipse without -data and with "-Dosgi.dataAreaRequiresExplicitInit=true" option which enables strict checks coming from platform runtime eclipse.ini file (this was the part of the changes we did via https://git.eclipse.org/r/#/c/94677/). VerifierApplication does NOT need a workspace, but it tries to use MigrationWizardTestHelper which extends MigrationSupport from org.eclipse.equinox.p2.ui.sdk.scheduler bundle, loading this class triggers load of AutomaticUpdatePlugin and unattended initialization of the DataArea via PreferenceInitializer => kaboom, we detect this as a problem and we have a proof the fix for this bug works! Because this seem to be irrelevant for the p2 tests, we can suppress the strict checks via adding "-Dosgi.dataAreaRequiresExplicitInit=false" startup argument to the Eclipse instance started from inside AbstractSharedInstallTest. So far what I could see from org.eclipse.equinox.p2.tests tests they seem never use workspace (why should they), so I think it is OK to allow them to relax the check. java.lang.IllegalStateException: The instance data location has not been specified yet. at org.eclipse.core.internal.runtime.DataArea.assertLocationInitialized(DataArea.java:56) at org.eclipse.core.internal.runtime.DataArea.getStateLocation(DataArea.java:138) at org.eclipse.core.internal.preferences.InstancePreferences.getBaseLocation(InstancePreferences.java:44) at org.eclipse.core.internal.preferences.InstancePreferences.initializeChildren(InstancePreferences.java:209) at org.eclipse.core.internal.preferences.InstancePreferences.<init>(InstancePreferences.java:59) at org.eclipse.core.internal.preferences.InstancePreferences.internalCreate(InstancePreferences.java:220) at org.eclipse.core.internal.preferences.EclipsePreferences.create(EclipsePreferences.java:352) at org.eclipse.core.internal.preferences.EclipsePreferences.create(EclipsePreferences.java:340) at org.eclipse.core.internal.preferences.PreferencesService.createNode(PreferencesService.java:393) at org.eclipse.core.internal.preferences.RootPreferences.getChild(RootPreferences.java:60) at org.eclipse.core.internal.preferences.RootPreferences.getNode(RootPreferences.java:95) at org.eclipse.core.internal.preferences.RootPreferences.node(RootPreferences.java:84) at org.eclipse.core.internal.preferences.AbstractScope.getNode(AbstractScope.java:38) at org.eclipse.core.runtime.preferences.InstanceScope.getNode(InstanceScope.java:77) at org.eclipse.equinox.internal.p2.ui.sdk.scheduler.PreferenceInitializer.migratePreferences(PreferenceInitializer.java:45) at org.eclipse.equinox.internal.p2.ui.sdk.scheduler.AutomaticUpdatePlugin.start(AutomaticUpdatePlugin.java:91) at org.eclipse.osgi.internal.framework.BundleContextImpl$3.run(BundleContextImpl.java:779) 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:772) at org.eclipse.osgi.internal.framework.BundleContextImpl.start(BundleContextImpl.java:729) at org.eclipse.osgi.internal.framework.EquinoxBundle.startWorker0(EquinoxBundle.java:956) at org.eclipse.osgi.internal.framework.EquinoxBundle$EquinoxModule.startWorker(EquinoxBundle.java:308) at org.eclipse.osgi.container.Module.doStart(Module.java:581) at org.eclipse.osgi.container.Module.start(Module.java:449) at org.eclipse.osgi.framework.util.SecureAction.start(SecureAction.java:468) at org.eclipse.osgi.internal.hooks.EclipseLazyStarter.postFindLocalClass(EclipseLazyStarter.java:103) at org.eclipse.osgi.internal.loader.classpath.ClasspathManager.findLocalClass(ClasspathManager.java:529) at org.eclipse.osgi.internal.loader.ModuleClassLoader.findLocalClass(ModuleClassLoader.java:328) at org.eclipse.osgi.internal.loader.BundleLoader.findLocalClass(BundleLoader.java:368) at org.eclipse.osgi.internal.loader.BundleLoader.findClassInternal(BundleLoader.java:446) at org.eclipse.osgi.internal.loader.BundleLoader.findClass(BundleLoader.java:395) at org.eclipse.osgi.internal.loader.BundleLoader.findClass(BundleLoader.java:387) at org.eclipse.osgi.internal.loader.ModuleClassLoader.loadClass(ModuleClassLoader.java:150) at java.lang.ClassLoader.loadClass(ClassLoader.java:357) at org.eclipse.equinox.internal.p2.tests.verifier.VerifierApplication.checkMigrationWizard(VerifierApplication.java:471) at org.eclipse.equinox.internal.p2.tests.verifier.VerifierApplication.verify(VerifierApplication.java:336) at org.eclipse.equinox.internal.p2.tests.verifier.VerifierApplication.start(VerifierApplication.java:69) at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:196) at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:134) at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:104) at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:388) at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:243) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke(Method.java:498) at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:653) at org.eclipse.equinox.launcher.Main.basicRun(Main.java:590) at org.eclipse.equinox.launcher.Main.run(Main.java:1499) at org.eclipse.equinox.launcher.Main.main(Main.java:1472)
(In reply to Alexander Kurtakov from comment #37) > (In reply to Eclipse Genie from comment #36) > > New Gerrit change created: https://git.eclipse.org/r/105555 +1 from me. As commented on gerrit, I have no rights to rebase & merge, so please someone with p2 committer rights should do this. Strange enough the test started from Eclipse with this patch fail, but I see them not failing in Gerrit, so I miss something to reproduce it from IDE.
(In reply to Andrey Loskutov from comment #42) > Strange enough the test started from Eclipse with this patch fail, but I see > them not failing in Gerrit, so I miss something to reproduce it from IDE. See 522466 comment c17.
(In reply to Dani Megert from comment #43) > (In reply to Andrey Loskutov from comment #42) > > Strange enough the test started from Eclipse with this patch fail, but I see > > them not failing in Gerrit, so I miss something to reproduce it from IDE. > > See 522466 comment c17. I don't think that bug 522466 comment c17 is it. It says about differences "between maven and standalone tests executions". I run tests from IDE with patch https://git.eclipse.org/r/105555 which should unset the variable, so there should be no fails like in Gerrit. And I see that during the test it starts Eclipse multiple times, but only one time with the "expected" variable set to false, but later process starts (who is doing that?) fail with the error like in SDK build. I see that this works fine in Gerrit (it IS maven, or?), but this does not work in IDE. I'm totally confused.
Gerrit change https://git.eclipse.org/r/105555 was merged to [master]. Commit: http://git.eclipse.org/c/equinox/rt.equinox.p2.git/commit/?id=625a656a844451738afa2062c16432e390ed63b1
(In reply to Andrey Loskutov from comment #44) > (In reply to Dani Megert from comment #43) > > (In reply to Andrey Loskutov from comment #42) > > > Strange enough the test started from Eclipse with this patch fail, but I see > > > them not failing in Gerrit, so I miss something to reproduce it from IDE. > > > > See 522466 comment c17. > > I don't think that bug 522466 comment c17 is it. It says about differences > "between maven and standalone tests executions". > > I run tests from IDE with patch https://git.eclipse.org/r/105555 which > should unset the variable, so there should be no fails like in Gerrit. And I > see that during the test it starts Eclipse multiple times, but only one time > with the "expected" variable set to false, but later process starts (who is > doing that?) fail with the error like in SDK build. > > I see that this works fine in Gerrit (it IS maven, or?), but this does not > work in IDE. I'm totally confused. So I was investigating one issue (org.eclipse.equinox.p2.tests.sharedinstall.MultipleChanges) to find the root cause (launching it with -Dorg.eclipse.equinox.p2.reconciler.tests.platform.archive=/path/to/eclipse-SDK-I20170920-2000-linux-gtk-x86_64.tar.gz) which pointed me to the workaround I proposed. It most likely requires disabling the env variable in other places too but the patch was mostly a proof of concept and smth people can play with and say whether this is the path forward. Let's see today's build and what else is left, I'll fix them next week unless someone beats me to it.
(In reply to Alexander Kurtakov from comment #46) > Let's see today's > build and what else is left, I'll fix them next week unless someone beats me > to it. Thanks Alexander! For me the status looks good now, no p2 fails anymore: http://download.eclipse.org/eclipse/downloads/drops4/I20170924-2000/testResults.php I'm closing this one now, feel free to reopen if I've missed something.
