Improvement: Extend m2e connector to allow more flexible Eclipse Web Application configuration

[GoogleCodeExporter]

This is less of a defect and more of an improvement to Maven integration via 
the m2e extension provided with GPE.  Specifically, its adds flexibility to 
configuring the Google Web Application settings based on gwt-maven-plugin 
configuration. 

Currently, the configuration values for the WAR directory and whether Eclipse 
will launch a web server pointing at that directory are hardcoded here:
https://code.google.com/p/google-plugin-for-eclipse/source/browse/trunk/plugins/
com.google.gdt.eclipse.maven.e37/src/com/google/gdt/eclipse/maven/e37/configurat
ors/GoogleProjectConfigurator.java#135

What steps will reproduce the problem?
1.  Import any Maven GWT project
2.  Open Properties of imported project, navigate to the Google -> Web 
Application section
3.  The values for WAR directory is hardcoded to 'src/main/webapp' and the 
Launch and deploy from this directory checkbox is hardcoded as unchecked.
4.  Setting these properties to anything else is overwritten by the hardcoded 
values upon invoking Maven -> Update Project...

I have a simple solution that I have tested locally in Eclipse.  See below for 
version information.  Using the patch, both of those settings can be controlled 
by the m2e connector by picking up elements in the gwt-maven-plugin 
configuration block.  Specifically:

- 'WAR directory' is taken from hostedWebapp 
(http://mojo.codehaus.org/gwt-maven-plugin/run-mojo.html#hostedWebapp)
- 'Launch and deploy from this directory' is taken from a new boolean element: 
'eclipseLaunchFromWarDir'

The fix applies only to a single plugin of the whole feature:  
com.google.gdt.eclipse.maven.e37

What version of the product are you using? On what operating system?
I'm running Eclipse Juno (4.2) on Mac OSX 10.7.5 using Java 1.7.0_25.  
The hardcoded values affect the current trunk of GPE.  I was working against 
version 3.2.3.v201304260926-rel-r42.

Original issue reported on code.google.com by dkichler on 8 Oct 2013 at 8:53

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

Attached is a patch implementing the solution described in the issue

I have already (digitally) signed the Google Individual Contributor License 
Agreement, v1.1 and so am hoping this patch can be accepted without too much 
delay after it has been reviewed.

Original comment by dkichler on 8 Oct 2013 at 9:11

Attachments:

• GPE-242.diff

[GoogleCodeExporter]

Original comment by rdayal@google.com on 14 Oct 2013 at 3:56

• Changed state: Accepted
• Added labels: Release-3.4.3

[GoogleCodeExporter]

Original comment by rdayal@google.com on 14 Oct 2013 at 3:59

[GoogleCodeExporter]

Hi - just a question about this - what behavior does this enable? Doesn't 
launching work without this patch?

Also, you're just testing with GWT, right? Or is App Engine in the mix, too?

Original comment by rdayal@google.com on 1 Dec 2013 at 10:02

[GoogleCodeExporter]

It mostly just provides flexibility in Eclipse project configuration as 
determined by the m2e connector.  Specifically, it adds support for two more 
configuration elements to be pulled from the Maven pom plugin configuration 
instead of hardcoded/not supported.  

Yes, launching currently works without the patch, but only for default Maven 
configuration.  For configurations not using src/main/webapp as the exploded 
WAR directory, for example, launching is broken unless manual project 
configuration is done each time following a Maven project refresh in Eclipse 
(ie, the m2e connector reconfigures based on pom). 

I have only tested this with GWT based projects.

Original comment by dkichler on 2 Dec 2013 at 5:37

[GoogleCodeExporter]

Ok, thanks for the explanation.

A couple more questions:

1) Is "eclipseLaunchFromWarDir" a documented property in the gwt-maven-plugin?

2) I think what you want to have is to set the 

In the patch, you have:

WebAppProjectProperties.setWarSrcDir(project, new Path(getWarSrcDir(config)));

A couple things about this:

-in the case where <hostedWebApp> is set, it needs to be set to a relative 
path. If it is absolute, it won't work (I don't think). This is fine, it's just 
something that people need to be aware of. 

-you're setting the WAR source directory to a folder under target/, which means 
that all of the validation that GPE does on web.xml will be performed relative 
to the one in target/. I think what you want to do is change line 163 of 
GoogleProjectConfigurator to take into account the value set by <hostedWebApp> 
(that deals with the war output location, not the source location), and leave   
WebAppProjectProperties.setWarSrcDirIsOutput(project, false) as-is.

However, if you do want to override the source WAR location independent of the 
output location, perhaps we need to introduce another property?

Original comment by rdayal@google.com on 2 Dec 2013 at 7:08

[GoogleCodeExporter]

1.  The "eclipseLaunchFromWarDir" is in fact a new element and will need to be 
documented if this patch is accepted.  I am certainly willing to follow up with 
gwt-maven-plugin contributors in this respect.  

2.  The <hostedWebApp> value is being used to override the default value of the 
WAR directory, falling back on the default of 'src/main/webapp', which used to 
be hardcoded, if that element is null or not present.  In the existing plugin, 
there is no way to control the value set to setWarSrcDirIsOutput (shows up as 
'Launch and deploy from this directory' in Eclipse project setup).  The new 
<eclipseLaunchFromWarDir> element is used to control that setting, falling back 
on the default of false, which was previously hard coded.  So in both cases, 
unless these tags are used, default behavior remains consistent with the 
existing plugin.  

Regarding your comments surrounding line 163, I was not able to determine 
exactly how the plugin was using the LastUsedWarOutLocation setting, but it 
again seemed to rely on a hardcoded path based on the default configuration of 
Maven's WAR plugin output directory (target/artifact-version).  This value is 
then cached in the GPE Eclipse plugin config and if it is actually used in 
setting up Eclipse project configuration, I was not able to find where.  For 
these reasons, I left that snippet of code completely alone.  

Are you suggesting that the setting of LastUsedWarOutLocation become dynamic to 
consider what has been set in WarSrcDir?   I can reason about why this may be 
necessary:  the existing code relied on using the default values that were 
hardcoded for WarSrcDir, WarSrcDirIsOutput and LastUsedWarOutLocation.  Now 
that WarSrcDir and WarSrcDirIsOutput have the option of being dynamic based on 
pom settings, the LastUsedWarOutLocation must also account for this.  However, 
without a better understanding of what LastUsedWarOutLocation is and where is 
it used, I would have trouble coding the dynamic solution based on the other 
parameters.  

Hopefully this all makes sense, please follow up on anything that doesn't.

Original comment by dkichler on 2 Dec 2013 at 10:15

[GoogleCodeExporter]

>>The <hostedWebApp> value is being used to override the default value of the 
WAR directory, falling back on the default of 'src/main/webapp', which used to 
be hardcoded, if that element is null or not present.  In the existing plugin, 
there is no way to control the value set to setWarSrcDirIsOutput (shows up as 
'Launch >and deploy from this directory' in Eclipse project setup).  The new 
<eclipseLaunchFromWarDir> element is used to control that setting, falling back 
on the default >of false, which was previously hard coded.  So in both cases, 
unless these tags are used, default behavior remains consistent with the 
existing plugin." 

Ok, I see what you mean - but the issue that I'm pointing out is that 
<hostedWebApp> points to a directory that lives under the target/ folder, does 
it not? Or am I wrong about that? If it is the case that <hostedWebApp> points 
to a directory that lives under target/, then it's a bit weird to set it as the 
source WAR directory. 

>>Regarding your comments surrounding line 163, I was not able to determine 
exactly how the plugin was using the LastUsedWarOutLocation setting, but it 
again >seemed to rely on a hardcoded path based on the default configuration of 
Maven's WAR plugin output directory (target/artifact-version).  This value is 
then >cached in the GPE Eclipse plugin config and if it is actually used in 
setting up Eclipse project configuration, I was not able to find where.  For 
these reasons, I left >that snippet of code completely alone.  

Basically, if "Launch and Deploy From this Directory" is not checked, then the 
user is prompted (at launch time) for the location of their WAR output 
directory (which would be target/.. in the Maven case). By setting the property 
directly when "Launch and Deploy From This Directory" is unchecked, the user 
will not be prompted to select their output WAR directory; the property value 
will be used.

>>Are you suggesting that the setting of LastUsedWarOutLocation become dynamic 
to consider what has been set in WarSrcDir?   I can reason about why this may 
>be necessary:  the existing code relied on using the default values that were 
hardcoded for WarSrcDir, WarSrcDirIsOutput and LastUsedWarOutLocation.  Now 
that >WarSrcDir and WarSrcDirIsOutput have the option of being dynamic based on 
pom settings, the LastUsedWarOutLocation must also account for this.  However, 
>without a better understanding of what LastUsedWarOutLocation is and where is 
it used, I would have trouble coding the dynamic solution based on the other 
>parameters.

I'm suggesting that if <hostedWebApp> is supposed to point to the target/.. 
directory (again, not sure what value this is supposed to point to)) , then the 
code at line 163 should change to consider the value set for <hostedWebApp>. 

Original comment by rdayal@google.com on 2 Dec 2013 at 11:16

[GoogleCodeExporter]

Right, I understand, and after reviewing the documentation for the GWT Maven 
plugin, you are correct that <hostedWebapp> 
(http://mojo.codehaus.org/gwt-maven-plugin/run-mojo.html#hostedWebapp) is meant 
to point to the WAR build output directory.  The Eclipse setting I'm using it 
to control is meant to point at the WAR source directory, or more specifically, 
the directory holding only static resources, which is denoted by 
<warSourceDirectory>(http://mojo.codehaus.org/gwt-maven-plugin/compile-mojo.html
#warSourceDirectory), which does default to 'src/main/webapp'.  

I've updated the patch to draw from the <warSourceDirectory> element now 
instead.  Default behavior should remain the same if either elements are 
omitted. 

I think this should avoid needing to refactor the snippet dealing with 
LastUserWarOutLocation (line 163)

Also, must extend some thanks to you for prompting me to give a full review of 
all plugin (GWT, WAR, Jetty) config as I learned a couple more subtle things 
and corrected a minor config error in our project. :)

Original comment by dkichler on 3 Dec 2013 at 10:40

Attachments:

• GPE-242-v2.patch

[GoogleCodeExporter]

>>Right, I understand, and after reviewing the documentation for the GWT Maven 
plugin, you are correct that <hostedWebapp> 
(http://mojo.codehaus.org/gwt-maven-plugin/run-mojo.html#hostedWebapp) is meant 
to point to the WAR build output directory.  The Eclipse setting I'm using it 
to control is meant to point at the WAR source directory, or more specifically, 
the directory holding only static resources, which is denoted by 
<warSourceDirectory>(http://mojo.codehaus.org/gwt-maven-plugin/compile-mojo.html
#warSourceDirectory), which does default to 'src/main/webapp'.  

>>I've updated the patch to draw from the <warSourceDirectory> element now 
instead.  Default behavior should remain the same if either elements are 
omitted. 

Ok, great!

>>I think this should avoid needing to refactor the snippet dealing with 
LastUserWarOutLocation (line 163)

We could still have an improvement here, which is that LastUsedOutLocation 
could be drawn from the <hostedWebApp> property. However, we can proceed with 
the patch as-is, and add that tweak in there depending on whether or not you 
think that's a good idea. 

>>Also, must extend some thanks to you for prompting me to give a full review 
of all plugin (GWT, WAR, Jetty) config as I learned a couple more subtle things 
and corrected a minor config error in our project. :)

Ha, no problem! This is some tricky stuff :).

By the way, we have improvements along the way for Maven support. What we're 
going to do is give you the ability import your Maven project and then have the 
project automatically configured as a WTP-based project. In order to make this 
work, we're going to have to add a GWT Facet. Then, the current way of working 
with projects will be deprecated.

Original comment by rdayal@google.com on 4 Dec 2013 at 8:39

[GoogleCodeExporter]

>>We could still have an improvement here, which is that LastUsedOutLocation 
could be drawn from the <hostedWebApp> property. However, we can proceed with 
the patch as-is, and add that tweak in there depending on whether or not you 
think that's a good idea. 

I do think it's a good idea, but there is another issue related to the 
LastUsedWarOutLocation: 
https://code.google.com/p/google-plugin-for-eclipse/issues/detail?id=123

We can leave the patch for this ticket as is and I will put together another 
for the other ticket (123) to deal more specifically with lastUseWarOutLocation.

Original comment by dkichler on 5 Dec 2013 at 2:23

[GoogleCodeExporter]

Ok, sounds good. Patch is in! Putting the release together now..

Original comment by rdayal@google.com on 5 Dec 2013 at 7:13

• Changed state: Fixed

[GoogleCodeExporter]

Original comment by rdayal@google.com on 20 Dec 2013 at 9:04

• Added labels: Release-3.5.0
• Removed labels: Release-3.4.3