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

Add support for switching to recommended perspective #1529

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

briandealwis
Copy link
Member

@briandealwis briandealwis commented Mar 6, 2017

Makes the J2EE perspective the recommended perspective (the final perspective) for our New Project wizards, but marks the J2EE and Web perspectives as being acceptable (the preferred perspectives).

No explicit test for this, but I had to change the integration tests to switch to the J2EE perspective as the would-you-like-to-switch-perspective dialog interfered.

Fixes #1455

Copy link
Contributor

@chanseokoh chanseokoh left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@@ -35,6 +35,8 @@
class="com.google.cloud.tools.eclipse.util.service.ServiceContextFactory:com.google.cloud.tools.eclipse.appengine.newproject.flex.AppEngineFlexProjectWizard"
icon="platform:/plugin/com.google.cloud.tools.eclipse.appengine.ui/icons/gae-16x16.png"
project="true"
finalPerspective="org.eclipse.jst.j2ee.J2EEPerspective"
preferredPerspectives="org.eclipse.jst.j2ee.J2EEPerspective,org.eclipse.wst.web.ui.webDevPerspective"
Copy link
Contributor

Choose a reason for hiding this comment

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

Will adding the Java perspective at the end help users installing our plugin on non-J2EE Eclipse versions?

Copy link
Member Author

Choose a reason for hiding this comment

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

We pull in WTP and the J2EE support.

I did wonder about including plain Java perspectives, especially since my preferred perspective is the Java Browsing perspective. But I figured most people would say "No" to the switch-perspective in that case.


@Override
public void setInitializationData(IConfigurationElement config, String propertyName, Object data)
throws CoreException {
if (data == null || !(data instanceof String)) {
throw new CoreException(StatusUtil.error(getClass(), "Data must be a class name"));
}
configElement = config;
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm... indentation of all the lines in this method seems off.


public class MavenArchetypeProjectWizard extends Wizard implements INewWizard {
public class MavenArchetypeProjectWizard extends Wizard
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: if we change plugin.xml to use com.google.cloud.tools.eclipse.util.service.ServiceContextFactory like in the non-Maven plugin.xml, configElement could be injected, we won't need setInitializationData(), and this class will not implement IExecutableExtension?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep. Will do.

// open most important file created by wizard in editor
IFile file = runnable.getMostImportant();
BasicNewProjectResourceWizard.selectAndReveal(file, workbench.getActiveWorkbenchWindow());
Copy link
Contributor

Choose a reason for hiding this comment

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

Could use org.eclipse.ui.wizards.newresource.BasicNewResourceWizard.selectAndReveal(IResource) instead.
Or at least change the class to BasicNewResourceWizard.

Copy link
Member Author

Choose a reason for hiding this comment

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

BasicNewResourceWizard.selectAndReveal(IResource) is an instance method. Do you see any real benefit of inheriting from BasicNewResourceWizard?

chanseokoh
chanseokoh previously approved these changes Mar 7, 2017
Copy link
Contributor

@chanseokoh chanseokoh left a comment

Choose a reason for hiding this comment

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

It's working.

* Expected to be created via the {@link ServiceContextFactory}.
*/
public class MavenArchetypeProjectWizard extends Wizard
implements INewWizard {
Copy link
Contributor

Choose a reason for hiding this comment

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

Put this in a single line?

@briandealwis
Copy link
Member Author

With these test failures, likely because of perspective-switch dialogs, I'm wondering if this change is really a good idea :-S

@briandealwis
Copy link
Member Author

devappserver returning 503?

testDebugLaunch(com.google.cloud.tools.eclipse.integration.appengine.DebugNativeAppEngineStandardProjectTest)  Time elapsed: 31.312 sec  <<< ERROR!
java.io.IOException: Server returned HTTP response code: 503 for URL: http://localhost:8080/hello
	at sun.net.www.protocol.http.HttpURLConnection.getInputStream0(HttpURLConnection.java:1839)
	at sun.net.www.protocol.http.HttpURLConnection.getInputStream(HttpURLConnection.java:1440)
	at com.google.cloud.tools.eclipse.integration.appengine.DebugNativeAppEngineStandardProjectTest.getUrlContents(DebugNativeAppEngineStandardProjectTest.java:161)
	at com.google.cloud.tools.eclipse.integration.appengine.DebugNativeAppEngineStandardProjectTest.testDebugLaunch(DebugNativeAppEngineStandardProjectTest.java:120)

@briandealwis
Copy link
Member Author

briandealwis commented Mar 10, 2017

I can only think that the devappserver isn't quite ready. The test waits until it sees:

Starting module \"default\" running at: http://localhost:8080

I'll change it to look for

INFO: Dev App Server is now running

I don't understand what this PR changes to cause this.

@briandealwis
Copy link
Member Author

Now dumps the contents of the devappserver console. Uh what's this ClassNotFoundException!?

INFO: submitting command: /home/travis/build/GoogleCloudPlatform/google-cloud-eclipse/google-cloud-sdk/bin/dev_appserver.py /home/travis/build/GoogleCloudPlatform/google-cloud-eclipse/plugins/com.google.cloud.tools.eclipse.integration.appengine/target/work/data/.metadata/.plugins/org.eclipse.wst.server.core/tmp0/testapp --host=localhost --port=8080 --admin_host=localhost --admin_port=8000 --max_module_instances=1 --jvm_flag=-Xdebug --jvm_flag=-Xrunjdwp:transport=dt_socket,server=n,suspend=y,quiet=y,address=33445 --automatic_restart=false
---- Dev App Server ----
INFO     2017-03-10 05:21:21,659 devappserver2.py:764] Skipping SDK update check.
WARNING  2017-03-10 05:21:21,705 simple_search_stub.py:1146] Could not read search indexes from /tmp/appengine.None.travis/search_indexes
INFO     2017-03-10 05:21:21,707 api_server.py:268] Starting API server at: http://localhost:39939
INFO     2017-03-10 05:21:21,711 dispatcher.py:199] Starting module "default" running at: http://localhost:8080
INFO     2017-03-10 05:21:21,711 admin_server.py:116] Starting admin server at: http://localhost:8000
Mar 10, 2017 5:21:23 AM com.google.appengine.tools.development.AbstractContainerService configure
WARNING: Null value for containerConfigProperties.get(devappserver.portMappingProvider)
Mar 10, 2017 5:21:23 AM com.google.apphosting.utils.jetty.JettyLogger info
INFO: Logging to JettyLogger(null) via com.google.apphosting.utils.jetty.JettyLogger
Mar 10, 2017 5:21:23 AM com.google.apphosting.utils.jetty.JettyLogger info
INFO: jetty-6.1.x
Mar 10, 2017 5:21:23 AM com.google.apphosting.utils.jetty.JettyLogger warn
WARNING: EXCEPTION 
java.lang.ClassNotFoundException: app.engine.test.HelloAppEngine
	at java.net.URLClassLoader$1.run(URLClassLoader.java:372)
	at java.net.URLClassLoader$1.run(URLClassLoader.java:361)
	at java.security.AccessController.doPrivileged(Native Method)

@codecov-io
Copy link

codecov-io commented Mar 14, 2017

Codecov Report

Merging #1529 into master will increase coverage by 1.56%.
The diff coverage is 80%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1529      +/-   ##
============================================
+ Coverage      69.5%   71.06%   +1.56%     
- Complexity     2029     2460     +431     
============================================
  Files           324      329       +5     
  Lines         12768    14579    +1811     
  Branches       1240     1669     +429     
============================================
+ Hits           8874    10361    +1487     
- Misses         3406     3661     +255     
- Partials        488      557      +69
Impacted Files Coverage Δ Complexity Δ
...e/appengine/newproject/AppEngineProjectWizard.java 70.17% <100%> (+1.08%) 7 <0> (ø) ⬇️
...ls/eclipse/util/service/ServiceContextFactory.java 81.81% <77.77%> (+4.04%) 6 <4> (ø) ⬇️
.../core/project/DataflowProjectValidationStatus.java 64.28% <0%> (-30.46%) 8% <0%> (ø)
.../dataflow/core/project/DataflowProjectCreator.java 4.29% <0%> (-12.87%) 3% <0%> (ø)
...e/dataflow/ui/wizard/NewDataflowProjectWizard.java 30.55% <0%> (-10.19%) 3% <0%> (ø)
...ols/eclipse/appengine/libraries/model/Library.java 92.66% <0%> (-7.34%) 54% <0%> (+32%)
...le/cloud/tools/eclipse/util/status/StatusUtil.java 90.16% <0%> (-4.84%) 29% <0%> (+9%)
...NewDataflowProjectWizardDefaultRunOptionsPage.java 27.27% <0%> (-3.98%) 2% <0%> (+1%)
...pse/dataflow/ui/launcher/PipelineArgumentsTab.java 56.54% <0%> (-2.95%) 29% <0%> (+5%)
...ataflow/core/project/FetchStagingLocationsJob.java 91.3% <0%> (-0.37%) 6% <0%> (+3%)
... and 43 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 408f64d...484df42. Read the comment docs.

@briandealwis
Copy link
Member Author

I output a list of files in the project directory and the deployed server module location. There are no class files… and I suspect that our project is out-of-sync with the file system:

files in /home/travis/build/GoogleCloudPlatform/google-cloud-eclipse/plugins/com.google.cloud.tools.eclipse.integration.appengine/target/work/data/testapp
>> .project
>> src/main/java/app/engine/test/HelloAppEngine.java
>> src/main/webapp/WEB-INF/appengine-web.xml
>> src/main/webapp/WEB-INF/web.xml
>> src/main/webapp/index.html
>> src/main/webapp/favicon.ico
>> src/main/webapp/META-INF/MANIFEST.MF
>> src/test/java/app/engine/test/HelloAppEngineTest.java
>> src/test/java/app/engine/test/MockHttpServletResponse.java
>> .classpath
>> .settings/org.eclipse.jdt.core.prefs
>> .settings/org.eclipse.wst.common.project.facet.core.xml
>> .settings/org.eclipse.wst.common.component
>> .settings/.jsdtscope
>> .settings/org.eclipse.wst.jsdt.ui.superType.name
>> .settings/org.eclipse.wst.jsdt.ui.superType.container

files in /home/travis/build/GoogleCloudPlatform/google-cloud-eclipse/plugins/com.google.cloud.tools.eclipse.integration.appengine/target/work/data/.metadata/.plugins/org.eclipse.wst.server.core/tmp0/testapp
>> META-INF/MANIFEST.MF
>> WEB-INF/appengine-web.xml
>> WEB-INF/web.xml
>> favicon.ico
>> index.html

@briandealwis
Copy link
Member Author

So it doesn't seem like the project is being built before launch, which is bizarre. So I enabled some builder logging, which produces repeated output like:

Fri Mar 17 05:23:58 UTC 2017 - [Worker-6] org.eclipse.core.internal.events.AutoBuildJob(Building workspace): Auto-Build requested, needsBuild: false state: 4 delay: 100
Fri Mar 17 05:23:58 UTC 2017 - [Worker-6] org.eclipse.core.internal.events.AutoBuildJob(Building workspace): Scheduling rebuild due to interruption
Fri Mar 17 05:23:58 UTC 2017 - [Worker-6] org.eclipse.core.internal.events.AutoBuildJob(Building workspace): Top-level build-start of: .org.eclipse.jdt.core.external.folders; [], appWithDefault; [], INCREMENTAL_BUILD
Fri Mar 17 05:23:58 UTC 2017 - [Worker-6] org.eclipse.core.internal.events.AutoBuildJob(Building workspace): java.lang.RuntimeException: Autobuild was interrupted
	at org.eclipse.core.internal.events.AutoBuildJob.setInterrupted(AutoBuildJob.java:253)
	at org.eclipse.core.internal.events.AutoBuildJob.isInterrupted(AutoBuildJob.java:204)
	at org.eclipse.core.internal.events.BuildManager.checkCanceled(BuildManager.java:424)
	at org.eclipse.core.internal.events.BuildManager.basicBuild(BuildManager.java:241)
	at org.eclipse.core.internal.events.BuildManager$1.run(BuildManager.java:301)
	at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:42)
	at org.eclipse.core.internal.events.BuildManager.basicBuild(BuildManager.java:304)
	at org.eclipse.core.internal.events.BuildManager.basicBuildLoop(BuildManager.java:360)
	at org.eclipse.core.internal.events.BuildManager.build(BuildManager.java:383)
	at org.eclipse.core.internal.events.AutoBuildJob.doBuild(AutoBuildJob.java:144)
	at org.eclipse.core.internal.events.AutoBuildJob.run(AutoBuildJob.java:235)
	at org.eclipse.core.internal.jobs.Worker.run(Worker.java:55
Fri Mar 17 05:23:58 UTC 2017 - [Worker-6] org.eclipse.core.internal.events.AutoBuildJob(Building workspace): Build canceled
Fri Mar 17 05:23:58 UTC 2017 - [Worker-6] org.eclipse.core.internal.events.AutoBuildJob(Building workspace): Top-level build-end time: 1

Looking at org.eclipse.core.internal.events.AutoBuildJob.isInterrupted(AutoBuildJob.java:204), I see that the AutoBuildJob interrupts itself if it detects that it is blocking some other higher-priority job.

So I suspect we have a bit of a race condition: if the auto-build job is running or scheduled as we enter ProjectUtils.waitForProjects(), then we join() the job causing it to be seen as needing to be interrupted. That causes the job to reschedule itself. Whether we continue in the join() or whether we loop around and join() anew, we get into this interrupt-reschedule loop and the build never happens.

And I recall now occasionally seeing the DebugNativeAppEngineStandardProjectTest tests seem to hang; looking at the Progress view, I'll see a single sleeping job Building workspace (which turns out to be the AutoBuildJob). Cancelling that job then lets the launch actually happen.

Maybe the solution is to never join(), or at least never join() on the AUTOBUILD family.

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

@briandealwis
Copy link
Member Author

Building directly in waitForProjects() can results in a seeming deadlock on that dratted DependencyGraph.

| "WorkbenchTestable" [71] WAITING on java.lang.Object@a3ad501
|     at java.lang.Object.wait(Native Method)
|     - waiting on java.lang.Object@a3ad501
|     at java.lang.Object.wait(Object.java:502)
|     at org.eclipse.core.internal.jobs.JobManager.waitForUnblocked(JobManager.java:1561)
|     at org.eclipse.core.internal.jobs.JobManager.yieldRule(JobManager.java:1485)
|     at org.eclipse.core.internal.jobs.InternalJob.yieldRule(InternalJob.java:527)
|     at org.eclipse.core.runtime.jobs.Job.yieldRule(Job.java:942)
|     at org.eclipse.wst.common.componentcore.internal.builder.DependencyGraphImpl.waitForAllUpdates(DependencyGraphImpl.java:691)
|     at org.eclipse.wst.common.componentcore.internal.builder.DependencyGraphImpl.getReferencingComponents(DependencyGraphImpl.java:126)
|     at org.eclipse.wst.common.componentcore.internal.builder.DependencyGraphImpl.getReferencingComponents(DependencyGraphImpl.java:116)
|     at org.eclipse.wst.common.componentcore.internal.resources.VirtualComponent.getReferencingComponents(VirtualComponent.java:579)
|     at org.eclipse.jst.j2ee.project.facet.EarFacetValidator.validate(EarFacetValidator.java:77)
|     at org.eclipse.wst.common.project.facet.core.internal.FacetedProjectValidationBuilder.build(FacetedProjectValidationBuilder.java:132)
|     at org.eclipse.core.internal.events.BuildManager$2.run(BuildManager.java:735)
|     at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:42)
|     at org.eclipse.core.internal.events.BuildManager.basicBuild(BuildManager.java:206)
|     at org.eclipse.core.internal.events.BuildManager.basicBuild(BuildManager.java:246)
|     at org.eclipse.core.internal.events.BuildManager$1.run(BuildManager.java:301)
|     at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:42)
|     at org.eclipse.core.internal.events.BuildManager.basicBuild(BuildManager.java:304)
|     at org.eclipse.core.internal.events.BuildManager.basicBuild(BuildManager.java:263)
|     at org.eclipse.core.internal.events.BuildManager.build(BuildManager.java:403)
|     at org.eclipse.core.internal.resources.Project$1.run(Project.java:551)
|     at org.eclipse.core.internal.resources.Workspace.run(Workspace.java:2240)
|     at org.eclipse.core.internal.resources.Project.internalBuild(Project.java:529)
|     at org.eclipse.core.internal.resources.Project.build(Project.java:112)
|     at com.google.cloud.tools.eclipse.test.util.project.ProjectUtils.waitForProjects(ProjectUtils.java:276)
|     at com.google.cloud.tools.eclipse.swtbot.SwtBotWorkbenchActions.waitForProjects(SwtBotWorkbenchActions.java:80)
|     at com.google.cloud.tools.eclipse.integration.appengine.SwtBotAppEngineActions.createWebAppProject(SwtBotAppEngineActions.java:106)
|     at com.google.cloud.tools.eclipse.integration.appengine.SwtBotAppEngineActions.createMavenWebAppProject(SwtBotAppEngineActions.java:55)
|     at com.google.cloud.tools.eclipse.integration.appengine.NewMavenBasedAppEngineProjectWizardTest.createAndCheck(NewMavenBasedAppEngineProjectWizardTest.java:86)
|     at com.google.cloud.tools.eclipse.integration.appengine.NewMavenBasedAppEngineProjectWizardTest.testHelloWorld(NewMavenBasedAppEngineProjectWizardTest.java:57)

Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

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

Is there anyway to rebase against current head? It's hard to see what's changed right now.

@briandealwis
Copy link
Member Author

Will do — I curious to see if we were still having the same build errors.

@googlebot
Copy link

CLAs look good, thanks!

@briandealwis
Copy link
Member Author

Looking at org.eclipse.core.internal.events.AutoBuildJob.isInterrupted(AutoBuildJob.java:204), I see that the AutoBuildJob interrupts itself if it detects that it is blocking some other higher-priority job.

I added some instrumentation to dump the current jobs, along with their state, rules, and I suspect the culprit is the DelayedSnapshotJob (Periodic workspace save).

ProjectUtils#dumpJobsState: 8 jobs:
 + SLEEPING{pri=30,rule=R/,system} Periodic workspace save.(6) (org.eclipse.core.internal.resources.DelayedSnapshotJob)
 + SLEEPING{pri=30,system} Animation start(18) (org.eclipse.ui.internal.progress.AnimationManager$1)
 + SLEEPING{pri=30,system} Change cursor(31) (org.eclipse.ui.internal.progress.WorkbenchSiteProgressService$SiteUpdateJob)
 + SLEEPING{pri=30,system} Change cursor(42) (org.eclipse.ui.internal.progress.WorkbenchSiteProgressService$SiteUpdateJob)
 + SLEEPING{pri=30,system} Workbench Auto-Save Job(15) (org.eclipse.ui.internal.Workbench$59)
 + SLEEPING{pri=40,rule=R/} Building workspace(4) (org.eclipse.core.internal.events.AutoBuildJob)
 + SLEEPING{pri=50,system} Update Progress(19) (org.eclipse.ui.internal.progress.ProgressViewUpdater$1)
 + SLEEPING{pri=50,system} Compacting resource model(12) (org.eclipse.core.internal.utils.StringPoolJob)

@briandealwis
Copy link
Member Author

Seems to happen more consistently on Neon and Oxygen.

@briandealwis
Copy link
Member Author

Sounds like bug 486083.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants