Maven 2 & 3

m2 eclipse plugin improvements (source download and attachment, customization of natures/builders/conclasspath, flexible project dupport) and refactoring

Details

  • Type: Improvement Improvement
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Fixed
  • Affects Version/s: 2.0-beta-1
  • Fix Version/s: 2.0-beta-1
  • Component/s: None
  • Labels:
    None
  • Testcase included:
    yes
  • Number of attachments :
    4

Description

This patch adds the following to the M2 eclipse plugin:

  • downloading of source attachments and configuration in .classpath
  • customization of project builders and natures in .project (like in the m1 plugin)
  • additional conclasspath entries in .classpath (like in the m1 plugin)
  • fix: don't add duplicate directories if main/resources directories overlap (like in the m1 plugin)
  • support for flexible projects (.wtpmodules file generation for utility modules, wars, ejbs)

Along with these new features the plugin has been refactored, splitting the single big EclipseWriter class to several specific classes and all the messages have been externalized in a property file.
There are still some todos in the code, which probably some M2 guru could look at, but anyway all the existing functionalities continue to work and some other tests have been added.

Due to the refactoring the patch looks more like a complete rewrite: sorry for that, but adding new features without splitting the existing file was ugly

Attachments

  2. File
    MNG-760.diff
    21/Aug/05 9:13 AM
    102 kB
    fabrizio giustina
  3. File
    MNG-760-1.diff
    22/Aug/05 4:13 PM
    97 kB
    fabrizio giustina
  4. File
    MNG-760-2.diff
    26/Aug/05 2:46 PM
    108 kB
    fabrizio giustina

Issue Links

This issue is duplicated by:
MNG-769 m2 eclipse:eclipse - duplicate ressources Minor Closed
MNG-770 m2 eclipse:eclipse should look at the JDK versions Minor Closed

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Trygve Laugstol added a comment -

A couple quick comments on a very good patch:

  • Please use the i18n component for internationalization stuff. Not sure if there are any other Mojos that's using it but the CLI in maven-core use it.
  • Please find a more descriptive name than "conclasspath". This parameter should also be a List, not a comma separated list.
Show
Trygve Laugstol added a comment - A couple quick comments on a very good patch:
  • Please use the i18n component for internationalization stuff. Not sure if there are any other Mojos that's using it but the CLI in maven-core use it.
  • Please find a more descriptive name than "conclasspath". This parameter should also be a List, not a comma separated list.
Hide
Permalink
fabrizio giustina added a comment -

> * Please use the i18n component for internationalization stuff. Not sure if there are any other Mojos that's using it but the CLI in maven-core use it.

I tried looking at maven-core and I found the properties files, but I could not understand how/where these are used. Messages in org.apache.maven.cli.* seems not to be internationalized and I couldn't see anything related to i18n in maven components... Am I blind? Could you give me some other pointer?

> * Please find a more descriptive name than "conclasspath".

"conclasspath" was the name from the M1 plugin... "con" stands for classpath container, so the new name could be "classpathContainers", I'll update it.

> This parameter should also be a List, not a comma separated list.
Can you direct me to an example of xml configuration and default-value tag for lists? I stuck with the comma separated string because I couldn't find how to describe a list in the javadoc default-value tag...

(while I'm fixing these things: is there a better way to read plugin configurations such as the war webapp directory? I used the same method used for the compiler plugin settings, but it sounds pretty bad...)

thanks for looking at the patch, Trygve

Show
fabrizio giustina added a comment - > * Please use the i18n component for internationalization stuff. Not sure if there are any other Mojos that's using it but the CLI in maven-core use it. I tried looking at maven-core and I found the properties files, but I could not understand how/where these are used. Messages in org.apache.maven.cli.* seems not to be internationalized and I couldn't see anything related to i18n in maven components... Am I blind? Could you give me some other pointer? > * Please find a more descriptive name than "conclasspath". "conclasspath" was the name from the M1 plugin... "con" stands for classpath container, so the new name could be "classpathContainers", I'll update it. > This parameter should also be a List, not a comma separated list. Can you direct me to an example of xml configuration and default-value tag for lists? I stuck with the comma separated string because I couldn't find how to describe a list in the javadoc default-value tag... (while I'm fixing these things: is there a better way to read plugin configurations such as the war webapp directory? I used the same method used for the compiler plugin settings, but it sounds pretty bad...) thanks for looking at the patch, Trygve
Hide
Permalink
Trygve Laugstol added a comment -

I think this is solved by MNG-760. Fabrizio: can you verify this?

Show
Trygve Laugstol added a comment - I think this is solved by MNG-760. Fabrizio: can you verify this?
Hide
Permalink
fabrizio giustina added a comment -

done some homework:

  • "conclasspath" has been renamed to "classpathContainers"
  • parameters are now Lists and not comma separated values, as suggested by Trygve, and example configuration has been added to javadocs
  • incorporated patch for MNG-772 and cleaned up a bit that part of code

BUT

  • after converting parameters to List I couldn't find any way to set default values using the default-value tag. Maybe should I renounce to the magic plugin.xml generation and write it by hand? actually default values are set manually and there is a todo in the code
  • still no luck with the i18n component: there is nothing i18nized in maven or mojos actually. I figured out the component Trygve talked about was the org.codehaus.plexus.i18n.I18N, and tried adding it to dependencies (1.0-beta-5?) and lookup it as usual (expression=${component...}) but someway it doesn't get instantiated (and, just like for default values, I still don't know how to set the resource bundle to use). Actually the code for messages has been left like in the previous patch.

Maybe somebody would like to start committing this before going on?

Show
fabrizio giustina added a comment - done some homework:
  • "conclasspath" has been renamed to "classpathContainers"
  • parameters are now Lists and not comma separated values, as suggested by Trygve, and example configuration has been added to javadocs
  • incorporated patch for MNG-772 and cleaned up a bit that part of code
BUT
  • after converting parameters to List I couldn't find any way to set default values using the default-value tag. Maybe should I renounce to the magic plugin.xml generation and write it by hand? actually default values are set manually and there is a todo in the code
  • still no luck with the i18n component: there is nothing i18nized in maven or mojos actually. I figured out the component Trygve talked about was the org.codehaus.plexus.i18n.I18N, and tried adding it to dependencies (1.0-beta-5?) and lookup it as usual (expression=${component...}) but someway it doesn't get instantiated (and, just like for default values, I still don't know how to set the resource bundle to use). Actually the code for messages has been left like in the previous patch.
Maybe somebody would like to start committing this before going on?
Hide
Permalink
Kenney Westerhof added a comment -

I'm sorry but I can't get this patch to apply, even when I downgraded to the revision specified in that dir.

Could you please svn up, fix merge conflicts if possible, and send me a new patch relative to the
maven-2 directory? The D:/... confuses diff; after stripping that it still didn't apply (problems with the svn properties,
removed those too but then kept getting errors).

Sorry I didn't see this issue before I started working on the plugin. I know what it feels like to keep merging
your local diff against an ever changing m2..

Show
Kenney Westerhof added a comment - I'm sorry but I can't get this patch to apply, even when I downgraded to the revision specified in that dir. Could you please svn up, fix merge conflicts if possible, and send me a new patch relative to the maven-2 directory? The D:/... confuses diff; after stripping that it still didn't apply (problems with the svn properties, removed those too but then kept getting errors). Sorry I didn't see this issue before I started working on the plugin. I know what it feels like to keep merging your local diff against an ever changing m2..
Hide
Permalink
Trygve Laugstol added a comment -

Please read http://maven.apache.org/maven2/developers/development-guide.html on how to create and submit patches.

Show
Trygve Laugstol added a comment - Please read http://maven.apache.org/maven2/developers/development-guide.html on how to create and submit patches.
Hide
Permalink
Trygve Laugstol added a comment -

> - after converting parameters to List I couldn't find any way to set default values using the default-value tag. Maybe should I renounce to the magic plugin.xml generation and write it by hand? actually default values are set manually and there is a todo in the code

For now it's fine to set a default value in the code and just document it outside.

> - still no luck with the i18n component: there is nothing i18nized in maven or mojos actually. I figured out the component Trygve talked about was the org.codehaus.plexus.i18n.I18N, and tried adding it to dependencies (1.0-beta-5?) and lookup it as usual (expression=${component...}) but someway it doesn't get instantiated (and, just like for default values, I still don't know how to set the resource bundle to use). Actually the code for messages has been left like in the previous patch.

I will dig up an example for you tomorrow.

Show
Trygve Laugstol added a comment - > - after converting parameters to List I couldn't find any way to set default values using the default-value tag. Maybe should I renounce to the magic plugin.xml generation and write it by hand? actually default values are set manually and there is a todo in the code For now it's fine to set a default value in the code and just document it outside. > - still no luck with the i18n component: there is nothing i18nized in maven or mojos actually. I figured out the component Trygve talked about was the org.codehaus.plexus.i18n.I18N, and tried adding it to dependencies (1.0-beta-5?) and lookup it as usual (expression=${component...}) but someway it doesn't get instantiated (and, just like for default values, I still don't know how to set the resource bundle to use). Actually the code for messages has been left like in the previous patch. I will dig up an example for you tomorrow.
Hide
Permalink
fabrizio giustina added a comment -

try 3

updated the patch to the latest svn revision, incorporating all the changes by Kenney. All the tests continue to work, and other tests have been added. Note that the Map stuff to avoid duplicate resources is now replaced by this patch (order should anyway be fixed).

> Sorry I didn't see this issue before I started working on the plugin. I know what it feels like to keep merging your local diff against an ever changing m2..

this time was terrible, due to the heavy refactoring and all the changes committed, I don't know if I will do that again...

Show
fabrizio giustina added a comment - try 3 updated the patch to the latest svn revision, incorporating all the changes by Kenney. All the tests continue to work, and other tests have been added. Note that the Map stuff to avoid duplicate resources is now replaced by this patch (order should anyway be fixed). > Sorry I didn't see this issue before I started working on the plugin. I know what it feels like to keep merging your local diff against an ever changing m2.. this time was terrible, due to the heavy refactoring and all the changes committed, I don't know if I will do that again...
Hide
Permalink
Kenney Westerhof added a comment -

Hi Fabrizio,

Thanks for all your hard work.

Usually we recommend to only modify the lines affected by your change. That makes the diff easier
to read. Also, adding new features and doing refactoring at the same time is discouraged, but I know
that these usually go hand in hand.

Anyway, I've tried to apply the patch but no go. Not sure if it's my version of patch (2.5.9), or your svn client.
There where ^M all over the file, and you should have file endings set to native in svn. Also, the new
files didn't get created; i had to touch them, and then i got errors like these:

$ patch -p3 < ~/MNG-760-2.diff
patching file src/main/java/org/apache/maven/plugin/eclipse/EclipseClasspathWriter.java
patch: **** malformed patch at line 264:

So, i was wondering if you could just attach a zipped/tar.gz version of your maven-eclipse-plugin
dir this time..?

Show
Kenney Westerhof added a comment - Hi Fabrizio, Thanks for all your hard work. Usually we recommend to only modify the lines affected by your change. That makes the diff easier to read. Also, adding new features and doing refactoring at the same time is discouraged, but I know that these usually go hand in hand. Anyway, I've tried to apply the patch but no go. Not sure if it's my version of patch (2.5.9), or your svn client. There where ^M all over the file, and you should have file endings set to native in svn. Also, the new files didn't get created; i had to touch them, and then i got errors like these: $ patch -p3 < ~/MNG-760-2.diff patching file src/main/java/org/apache/maven/plugin/eclipse/EclipseClasspathWriter.java patch: **** malformed patch at line 264: So, i was wondering if you could just attach a zipped/tar.gz version of your maven-eclipse-plugin dir this time..?
Hide
Permalink
fabrizio giustina added a comment -

Zipped version, containg the full plugin directory (svn control files have been removed, you will have to set excludes for generated files)

> There where ^M all over the file, and you should have file endings set to native in svn. Also, the new
> files didn't get created; i had to touch them, and then i got errors like these:
> patch: **** malformed patch at line 264:

well, I had svn:eol-style=native set, but those ^M were probably generated when saving the diff in Windows...
It seems that your patch has problems with svn properties in this diff? I tried using both tortoisesvn/svn 1.2.3 and the eclipse client (JavaSVN) but the generated patch doesn't change... do you know which is the format expected by patch for property changes?

Show
fabrizio giustina added a comment - Zipped version, containg the full plugin directory (svn control files have been removed, you will have to set excludes for generated files) > There where ^M all over the file, and you should have file endings set to native in svn. Also, the new > files didn't get created; i had to touch them, and then i got errors like these: > patch: **** malformed patch at line 264: well, I had svn:eol-style=native set, but those ^M were probably generated when saving the diff in Windows... It seems that your patch has problems with svn properties in this diff? I tried using both tortoisesvn/svn 1.2.3 and the eclipse client (JavaSVN) but the generated patch doesn't change... do you know which is the format expected by patch for property changes?
Hide
Permalink
Brett Porter added a comment -

this was in no way fun to apply. Unfortunately you need to remove the property sections manually from diffs generated by SVN.
I don't know what client you were using but its patch format was broken: I needed to change +1 +1,XXX to -0,0 +1,XXX (and sometimes subtract 1
Try ugrading your tortoise client - the version I have works just fine.
I also had to reapply a change Kenney made to EclipseWriter in the mean time.

Also, patch-3 was missing two files I got from the zip. The zip on its own wasn't any good without the .svn files - its actually better to leave the .svn files in there (but remove target), as that should allow it to be committed directly (after updating and resolving conflicts).

Anyway, done now. Someone please test.

Show
Brett Porter added a comment - this was in no way fun to apply. Unfortunately you need to remove the property sections manually from diffs generated by SVN. I don't know what client you were using but its patch format was broken: I needed to change +1 +1,XXX to -0,0 +1,XXX (and sometimes subtract 1 Try ugrading your tortoise client - the version I have works just fine. I also had to reapply a change Kenney made to EclipseWriter in the mean time. Also, patch-3 was missing two files I got from the zip. The zip on its own wasn't any good without the .svn files - its actually better to leave the .svn files in there (but remove target), as that should allow it to be committed directly (after updating and resolving conflicts). Anyway, done now. Someone please test.

People

Vote (0)
Watch (0)

Dates

  • Created:
    Updated:
    Resolved:
Atlassian JIRA (v4.4.3#663-r165197) | Report a problem
Powered by a free Atlassian JIRA open source license for Codehaus. Try JIRA - bug tracking software for your team.