Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 350414

Summary: [patch] Store ignored m2e connectors outside project pom.xml
Product: z_Archived Reporter: Christian Moser <hanzos>
Component: m2eAssignee: Igor Fedorenko <igor>
Status: CLOSED FIXED QA Contact:
Severity: enhancement    
Priority: P3 CC: adam.j.gray, adrianshum, akdart, alx, andrew.eisenberg, basheer, bob_walker99, BorisNaguet, christian, christoph.hohmann, cnuessgens, ctran, david.matejcek, dmgloss, d_a_carver, eb, eclipse.dserodio, eclipser, fbricon, federico.grilli, galak75, greg.jos, gregory.amerson, herve.boutemy, ian, igor, iloncar.ml, jessi.abrahams, jochen.wiedmann, joelittlejohn, kamenlitchev, karsten.thoms, konrad_w, krejci.l, lizbethc, lutz.huehnken, mail, manderse, marc.heinz, marcello.teodori, mark.carroll, matti.lindell, mchenryc, mikko.taivainen, mkleint, mkouba, rcernich, rene.zanner, ricardo, sebastien.pennec, srstclair, stanio, sune, thoraageeldby, vbhavdal
Version: unspecified   
Target Milestone: ---   
Hardware: PC   
OS: Windows 7   
Whiteboard:
Attachments:
Description Flags
Patch from Milos Kleint's repository (apply first)
none
Patch from Andrew Eisenberg's repository (apply second) none

Description Christian Moser CLA 2011-06-27 05:10:06 EDT
Build Identifier: 20110615-0604

.. In consideration of various discussions on the m2e-users mailing list.

In my view, is the attempt to store M2e settings within a platform and !IDE! independent element, such as pom.xml utterly wrong located.

I’m using maven because it is an independent build tool, which does not distinguish what is used for developing or building maven projects. Even if further maven versions or other IDE’s or maven embedding tools ignore those excludes (at the moment), I don’t wanna be forced to edit all my pom’s or at least the parents (If you got any parents..) for developing with Eclipse. Consider a dev team or contributors to an open source project, which use different tools (IDE) to develope, you will end sharing information in pom.xml's which are not needed for the build and aren't even useful for every contributor.

suggestions:
1 store the ignored connectors into an xml which is located next to the project pom. For those who want to share this config, simply check the file into scm. For those who don't want, just ignore the file in scm. For those without scm, well...
2 store the ignored connectors in the user's m2e workspace configuration.
3 make it an option so the user can decide if he want's his poms modified or not and if declined ignore to check for lifecycle mappings.
4 If no m2e connectors were found, just warn the user instead of an error

If there were further suggetions, please add them in the comments.

I'd highly support suggestion one, because it's the most transparent solution and is also easy to share with others.


Reproducible: Always

Steps to Reproduce:
1. import a maven project which includes plugins in the pom which are unsupported by m2e.
Comment 1 Igor Fedorenko CLA 2011-06-27 05:19:46 EDT
Maven plugin ignore configuration stored outside of pom.xml will not be inherited from parent to child modules. For typical multimodule project this means the same config has to be replicated for every module.
Comment 2 Jochen Wiedmann CLA 2011-06-27 05:44:52 EDT
Igor, the hint on parent POM's is understood and justified. Nevertheless, most of us would prefer to have this in a local (ideally versionable) file, which is independent from the POM. If a hierarchy of lifecycle mapping definitions is already supported, perhaps we could add yet another one to the end of the hierarchy?
Comment 3 Igor Fedorenko CLA 2011-06-27 05:50:21 EDT
Yes, sure, this was just an FYI remark.
Comment 4 Max Rydahl Andersen CLA 2011-06-27 07:23:36 EDT
I think it is completely valid to allow for pom configured lifecycle IDE settings as m2eclipse currently does but it should not be the only way.

Providing a way to have ignored plugins should at least be possible on workspace level - not sure it makes sense per project level, i.e. if it will be too confusing for little value?
Comment 5 David Carver CLA 2011-06-28 10:51:13 EDT
I'd suggest storing these lifecyclemappings in an Eclipse Preference page, in which you can import and export the lifecyclemappings.
Comment 6 Matthew Piggott CLA 2011-07-06 10:03:01 EDT
*** Bug 351332 has been marked as a duplicate of this bug. ***
Comment 7 David Kendall CLA 2011-07-14 13:02:13 EDT
With appreciation for the efforts of the m2e developers....

I'm also adding my voice in support to moving this configuration out of the pom.xml.  The pom.xml is not an appropriate place to store tool specific configuration. In many development cultures there is resistance to incorporating tool specific configurations in the pom.xml and similar build artifacts.

The options I would prefer (in descending order of preference)...

1 Store this configuration as a part of the eclipse workspace. Please make it exportable/importable so that the configuration may be shared.
2 Make it so that the m2e configuration can be configured as a part of the users settings.xml, rather than a part of the project pom.xml
3 Provide an option to downgrade the errors as warnings.

The volume of emails posted to the m2e-users on this subject should attest to the shortcomings of the current design. Please don't ignore the needs and wishes of your users on this subject.
Comment 8 Milos Kleint CLA 2011-07-18 13:11:13 EDT
https://github.com/mkleint/m2e/commit/b397f90681737c77a9944ab34f2af95af055e0d2
experimental code for exportable/importable workspace preferences for ignoring mappings.

just a starting point for design discussion/experimenting..

very raw code, no manual editing, just a quick fix for error markers. can quick fix *all* markers at once though.

1. not sure how to update project config after import.
2. missing ui for removing the ignores. Currently has to manually edit the pref file.
3. pom content has preference over workspace, workspace over the rest. not sure if that's good or bad. 
In relation to 2. it's bad for cases when you want to install plugins that actually fix the problem rather than just ignoring it..

4. in terms of ui, the quickfixes got a bit confusing.  not only labeling (which can be improved) but the mere fact that there is more choice now is bad.

5. apparently not everyone will be happy with workspace preference anyway..
Comment 9 Joe Littlejohn CLA 2011-08-04 05:26:18 EDT
Irrespective of what gets implemented here (I also strongly believe something needs to be done ASAP and that adding this configuration to the POM is a fundamentally flawed idea), I'd like to see item 4 from comment 0 implemented as well:

> 4 If no m2e connectors were found, just warn the user instead of an error

... (and set the lifecycle action implicitly to 'ignore' for unsupported plugins)
Comment 10 Lutz Hühnken CLA 2011-08-05 04:46:54 EDT
Since there is no way to upvote this bug, I  would like to express by way of this comment that this change be implement with highest priority.
The current approach is unusable.
Comment 11 greg.jos CLA 2011-08-05 04:50:38 EDT
Comment 12 federico.grilli CLA 2011-08-05 04:51:53 EDT
(In reply to comment #10)
> Since there is no way to upvote this bug, I  would like to express by way of
> this comment that this change be implement with highest priority.
> The current approach is unusable.

+1
Comment 13 Stanimir Stamenkov CLA 2011-08-16 08:17:50 EDT
+1 :-)
Comment 14 Kamen CLA 2011-08-16 08:20:41 EDT
+1
Comment 15 Bob Walker CLA 2011-08-17 09:05:47 EDT
+1 - I am on a mixed development team with opinions & passions on IDE/tool choices often running high. Even as a supporter of M2E and Eclipse, I think modifying the pom is fundamentally the wrong approach, and will only give opponents fuel for the fire.
Comment 16 Rob Cernich CLA 2011-08-24 15:31:33 EDT
+1

I would add that these should be configurable on a project by project basis.  For example, code style/find bugs may not be enforced for example/demo projects.  It would be nice to disable the M2E plugins for just those projects.

I would liken this to the other M2E settings (e.g. "Resolve Workspace Dependencies").  As a matter of fact, I think the .settings/org.eclipse.m2e.core.prefs is where this stuff should go.  It would also be nice if there were global settings (i.e. a preferences page).  This is similar to the way most other builders settings are managed.


As far as sharing the configuration, I think that's a much lower priority (some users may choose to simply not use the plugins at all).
Comment 17 Jochen Wiedmann CLA 2011-08-25 04:36:07 EDT
A technical implementation note: COuldn't this be implemented as a kind of generic connector. The connector is itself an Eclipse plugin, so should be able to have a UI for configuration an read/write the configuration as preferences?
Comment 18 Vegard Havdal CLA 2011-09-01 06:48:30 EDT
+1

A workspace and project specific Eclipse config like this:

Maven plugin execution:

[x] Require connector
[ ] Ignore
[ ] Execute
Comment 19 Rob Cernich CLA 2011-09-04 11:48:42 EDT
A simple option may be to store this data in a lifecycle-mapping-metadata.xml file either in the project folder or its .settings folder.  This would be similar to the way m2e plugins contribute mappings today.

The mappings could be resolved in the following order:
For a given project:
1. pom.xml pluginManagement
2. project lifecycle-mapping-metadata.xml
3. if a parent project exists, its parent project lifecycle mapping (goto 1).
4. workspace lifecycle-mapping-metadata.xml
5. plugin contributed lifecycle-mapping-metadata.xml
6. system default lifeclycle-mapping-metadata.xml
7. prompt user

Steps 5 and 6 could be incorporated into the the workspace lifecycle mappings, in a fashion similar to other preferences where plugin contributions and user settings overlap (e.g. Ant task contributions).

Best regards,
Rob
Comment 20 Elizabeth Chatman CLA 2011-09-07 17:29:43 EDT
+1
Comment 21 Igor Fedorenko CLA 2011-10-04 07:43:21 EDT
*** Bug 359789 has been marked as a duplicate of this bug. ***
Comment 22 Adrian Shum CLA 2011-10-14 06:31:21 EDT
(In reply to comment #10)
> Since there is no way to upvote this bug, I  would like to express by way of
> this comment that this change be implement with highest priority.
> The current approach is unusable.

I am the reporter of the recently duplicated bug.  I cannot agree more about your statement of "current approach is unusable".  It is not only unusable but also irrational.

Definitely +1 for this.

I wish to see some positive response from m2e developers.  I think current design have hindered upgrade for many users.
Comment 23 Igor Fedorenko CLA 2011-10-14 07:28:01 EDT
(In reply to comment #22)
> I wish to see some positive response from m2e developers.  I think current
> design have hindered upgrade for many users.

Provide a quality patch and one of m2e developers will review and apply it. Aside from logic to read lifecycle mapping information from a file under .settigns/, the patch should provide GUI to show and modify said lifecycle mapping information, probably from a project preference page.
Comment 24 Rob Cernich CLA 2011-10-14 12:03:14 EDT
(In reply to comment #23)
> Provide a quality patch and one of m2e developers will review and apply it.
> Aside from logic to read lifecycle mapping information from a file under
> .settigns/, the patch should provide GUI to show and modify said lifecycle
> mapping information, probably from a project preference page.

Does this mean the m2e team is not planning to address this issue in any forthcoming release, major or minor?
Comment 25 Igor Fedorenko CLA 2011-10-14 12:32:31 EDT
(In reply to comment #24)
> (In reply to comment #23)
> > Provide a quality patch and one of m2e developers will review and apply it.
> > Aside from logic to read lifecycle mapping information from a file under
> > .settigns/, the patch should provide GUI to show and modify said lifecycle
> > mapping information, probably from a project preference page.
> 
> Does this mean the m2e team is not planning to address this issue in any
> forthcoming release, major or minor?

Correct. Unless a quality patch is provided, there are no immediate plans to implement this enhancement request.
Comment 26 Mark Carroll CLA 2011-10-18 08:36:02 EDT
+1

Any fix that lets me use poms in Eclipse that command-line maven has no trouble with would work for me. I won't be able to get approval to commit weird pom cruft to our shared code repository at work if maven doesn't even need it. As it is, I'll downgrade the plugin for now until this craziness is addressed properly.
Comment 27 Adrian Shum CLA 2011-11-16 02:48:47 EST
(In reply to comment #25)
> (In reply to comment #24)
> > (In reply to comment #23)
> > > Provide a quality patch and one of m2e developers will review and apply it.
> > > Aside from logic to read lifecycle mapping information from a file under
> > > .settigns/, the patch should provide GUI to show and modify said lifecycle
> > > mapping information, probably from a project preference page.
> > 
> > Does this mean the m2e team is not planning to address this issue in any
> > forthcoming release, major or minor?
> 
> Correct. Unless a quality patch is provided, there are no immediate plans to
> implement this enhancement request.

Just wanna check if there is still no plans for this enhancement? Coz I think it should be put in high priority as the current solution is really irrational and it is such a critical function...  

Although I have no experience in Eclipse plugin development (not even SWT to be honest), I may try to implement that if there is really no one in the original development team that is willing to take up this job.  Is there any information or tutorial that I can take a look first so that I can work on it?  It will be great if someone can give a short list of studies that I should do.

I may try to see if I can implement part of function, like at least giving a workspace-level setting screen (or even project level), which at least make everyone facing this problem possible to proceed.
Comment 28 Milos Kleint CLA 2011-11-16 02:53:53 EST
as I have written in comment 8 https://bugs.eclipse.org/bugs/show_bug.cgi?id=350414#c8, i've created some prototype code but so far got no feedback whatsoever..
Comment 29 Adrian Shum CLA 2011-11-16 03:05:25 EST
(In reply to comment #28)
> as I have written in comment 8
> https://bugs.eclipse.org/bugs/show_bug.cgi?id=350414#c8, i've created some
> prototype code but so far got no feedback whatsoever..

oops, it is really something I overlooked.  However I think for most eclipse user, it is hard for them to test if there is no place that they can install the plugin directly.

I may try to get your branch to compile and see how it looks once I have time :)  Thanks Milos! :)
Comment 30 Vadim Dmitriev CLA 2011-11-24 09:46:58 EST
(In reply to comment #28)
> as I have written in comment 8
> https://bugs.eclipse.org/bugs/show_bug.cgi?id=350414#c8, i've created some
> prototype code but so far got no feedback whatsoever..

Just tried it out, works like a charm. I'll continue using it and will report if something weird pops up. Thank you very much for the effort.
Comment 31 Igor Fedorenko CLA 2011-12-01 13:36:12 EST
*** Bug 365342 has been marked as a duplicate of this bug. ***
Comment 32 Alex McCarrier CLA 2011-12-01 15:49:08 EST
Lots of good suggestions here.

At the very least please make it so m2e doesn't mark the POM as being in error.  The POM is not in error.  I have several projects showing the POM being in error and they compile and run just fine.  The error marker makes it annoying when I want to see real errors.
Comment 33 Marc H. Marc H. CLA 2012-01-10 08:18:47 EST
+1

Our architects ruled that inserting IDE specific settings in our POMs (which tend to be already overly complex) is out of question. The global consensus in our team being that the IDE should support the build infrastructure and not the other way around.

This decision could also be influenced by the fact that we are in a disparate environment with a fair number of IntelliJ users, who do not want to see such things in our common code base (and I could hardly disagree on that).

As a side note, I'm considering migrating to another IDE too. While eclipse may be replaceable, maven today is not. And the recent issues (error markers madness, extremely frequent rebuild, infinite build loops, impossibility to edit in a second time part of the generated content, update configuration voodoo magic, etc. etc.) make m2e particularly unattractive. Part of these problems may be related to under-optimized configuration of build phases, or maybe to our own misunderstanding on how the plugin is supposed to work, but the fact is that our maven configuration had worked for years and play always very well with all others IDE, so nobody has the energy nor the will to refactor it (and certainly not writing m2e connectors).
Comment 34 René Zanner CLA 2012-01-13 04:57:16 EST
+1
Comment 35 Christian Kaltepoth CLA 2012-01-19 01:29:30 EST
+1 for addressing this issue with high priority.

I absolutely admire the work of the m2e developers. And IMHO m2e provides the best Maven integration available for any IDE.

BUT I cannot understand why you think it is a good idea to store IDE configuration in the pom.xml. I cannot think for any good reason for this. What about the other IDEs? Should IDEA, Netbeans and all others also store their configuration in the pom.xml. This doesn't make sense.
Comment 36 Shane StClair CLA 2012-02-03 02:12:23 EST
+1 for an external config solution, even if it's not built into the GUI on the first pass. I tried for a few hours to get the m2e sources working with Eclipse to try Milos' patch, but was unsuccessful. With much appreciation, I agree that this is causing a lot of people grief and that a solution is badly needed. For now I have to continually remove the m2e pom sections patches, revert the pom before committing, etc.
Comment 37 Igor Fedorenko CLA 2012-04-12 11:00:06 EDT
*** Bug 376613 has been marked as a duplicate of this bug. ***
Comment 38 Andrew Eisenberg CLA 2012-05-15 14:52:03 EDT
I should have some time over the next week to look at this and hopefully provide a proper patch.

Vadim (comment 8), I'll start with your branch and see where that leads me.  Rob (comment 19), your proposal on where to look seems the most appropriate.  I'll try to implement a scheme like that.

This does not seem to be a small amount of work, so my first go at a patch may not be a complete implementation, but hopefully it will be usable.  I'll update my progress as I get further along.
Comment 39 Rob Cernich CLA 2012-05-18 12:44:12 EDT
(In reply to comment #38)
> I should have some time over the next week to look at this and hopefully
> provide a proper patch.

That would be awesome!

> This does not seem to be a small amount of work, so my first go at a patch may
> not be a complete implementation, but hopefully it will be usable.  I'll update
> my progress as I get further along.

Take a look at LifecycleMappingFactory.getProjectMetadataSources().  I believe this is where the lifecycle metadata is loaded for the project.  You might want to step through getBundleMetadataSources() in the same class to see how the metadata is loaded for mappings contributed by plugins.  I'm sure between the two of those, it should be pretty easy to figure out how to scan for and load data from a lifecycle-mapping-metadata.xml file in the project root folder.

Good luck!
Comment 40 Andrew Eisenberg CLA 2012-05-18 12:58:42 EDT
Hi all,

I have a first go at a patch and I am looking for feedback.  I started with Milos's commit, but decided to go in a slightly different direction, so I don't think much of his code is remaining.  But, it was very helpful to get me going.

Here's what I did:

1. there is now a lifecycle-mapping-metadata.xml being stored in the .metadata/.plugins folder, which stores all of the mappings info for the workspace.
2. instead of adding a new quickfix proposal, there is now a checkbox in the lifecycle mappings dialog.  If checked, then the mappings are added to the workspace file, not the pom.
3. The workspace file can be opened and edited through the preferences -> Maven preferneces page.  It is opened in a regular editor and can be saved and edited as expected even though this file lives outside the workspace.

Here is what is NOT implemented:

1. nothing for individual projects.  I did not touch adding a lifecycle-mapping-metadata.xml file for projects, but based on what I have done so far, it won't be too difficult to add.
2. no tests.

Here is a link to my github repo.  I'm hoping that someone on the m2e team can look at it.

https://github.com/aeisenberg/m2e/tree/bug350414
Comment 41 Fred Bricon CLA 2012-05-19 10:48:05 EDT
Andrew I took a first look at your branch (just skimmed the surface). My first remarks so far, mostly from a user perspective :

- model = loadWorkspaceMappingsModel() is commented in LifecycleMappingProposal.java, that causes a NPE during the quickfix
- labels should be externalized
- I would put the preferences in a separate page (Lifecycle Mappings) under the Maven node
- When appending the  workspace lifecycle-mapping-metadata.xml with new ignored mappings, the file loses existing formatting.
-I'm not super fond of the "put under workspace" checkbox in 
LifecycleMappingDialog. I'd rather see it as a Workspace node, as the root of the hierarchy (if it's feasible). Selecting the workspace node would display your current description beneath the tree.
- you should add some integration tests ;-)

I was also thinking the location of the lifecycle-mapping-metadata.xml should be customizable from the preference page (but that could be part of a future enhancement).

I also noticed you lost Milos authorship in your first git commit. Better ask Milos or Igor if that's an issue. 

Anyway, that looks very promising so far, I'll try to take a deeper look at the code as soon as I can.
Comment 42 Andrew Eisenberg CLA 2012-05-20 16:43:02 EDT
Hi Fred,

Thanks for the feedback.  See below...

> - model = loadWorkspaceMappingsModel() is commented in
> LifecycleMappingProposal.java, that causes a NPE during the quickfix
Hmm...I'll have a look.

> - labels should be externalized
Sure.

> - I would put the preferences in a separate page (Lifecycle Mappings) under the
> Maven node
I don't like creating lots of preferences pages that are mostly empty.  Creating a Lifecycle Mappings page at this point would have only a single button on it, which I think is a waste.

However, if you think this goes against the designs of the project, I'm willing to reconsider.

> - When appending the  workspace lifecycle-mapping-metadata.xml with new ignored
> mappings, the file loses existing formatting.
OK.  I'll have a look.

> -I'm not super fond of the "put under workspace" checkbox in 
> LifecycleMappingDialog. I'd rather see it as a Workspace node, as the root of
> the hierarchy (if it's feasible). Selecting the workspace node would display
> your current description beneath the tree.
Hmmm...I thought about something like this, but my understanding is that the dialog shows the pom hierarchy.  Placing the mappings in the workspace metadata is outside of the pom hierarchy and so it seems reasonable to leave it out of the list.

I'm guessing that your concern is that the checkbox clutters the dialog, but we should also think about how we should incorporate putting the mappings metadata in the project, but outside of the pom.  This isn't something I was planning to do immediately, but when this happens, it seems we will need a checkbox for that (to toggle between inside and outside of the pom).  

If we do go in this direction, then putting the workspace settings in the list, wouldn't fit with toggling between inside and outside of the pom.

I'm not against trying to do this, but I think the UI is a bit clearer with the workspace checkbox separated out.

> - you should add some integration tests ;-)
This is planned, but didn't want to go through this until there was tentative feedback that this was a good direction to go.  :)


> I was also thinking the location of the lifecycle-mapping-metadata.xml should
> be customizable from the preference page (but that could be part of a future
> enhancement).
Makes sense, perhaps raising a new issue for this is a good idea.

> I also noticed you lost Milos authorship in your first git commit. Better ask
> Milos or Igor if that's an issue. 
I had to do a bit of munging and rebasing and I guess the author tag got messed up.  I'll try to fix it.

> Anyway, that looks very promising so far, I'll try to take a deeper look at the
> code as soon as I can.
Great.
Comment 43 Andrew Eisenberg CLA 2012-05-22 01:07:53 EDT
I updated my branch and pushed to my github repo.  I'll explain the changes below:


> - model = loadWorkspaceMappingsModel() is commented in
> LifecycleMappingProposal.java, that causes a NPE during the quickfix
fixed

> - labels should be externalized
fixed

> - I would put the preferences in a separate page (Lifecycle Mappings) under the
> Maven node
Done.  I also added a way to change the location of the metadata file from this preference page.

I am not much of an SWT person and the preference page does not look very nice.  Any help on formatting would be appreciated.  I imagine that fixing this page would take 10 minutes for someone with good SWT experience.

> - When appending the  workspace lifecycle-mapping-metadata.xml with new ignored
> mappings, the file loses existing formatting.
Fixed

> -I'm not super fond of the "put under workspace" checkbox in 
> LifecycleMappingDialog. I'd rather see it as a Workspace node, as the root of
> the hierarchy (if it's feasible). Selecting the workspace node would display
> your current description beneath the tree.
As mentioned previously, I am holding off on this for now.

> - you should add some integration tests ;-)
Done.  I sent a pull request to the m2e-core-tests repo on github:
https://github.com/sonatype/m2e-core-tests/pull/4

I added some tests to LifecycleMappingMetadataPrioritiesTest and also added a WorkspaceMappingsFileTest to test some simple manipulation of the workspace metadata file.  I'm not sure how to add this file to the list of junit tests to get run, so I'm hoping someone else can do this (or is this just some Tycho magic that makes it work?).

> I was also thinking the location of the lifecycle-mapping-metadata.xml should
> be customizable from the preference page (but that could be part of a future
> enhancement).
Done, as mentioned above.

> I also noticed you lost Milos authorship in your first git commit. Better ask
> Milos or Igor if that's an issue. 
His commits are still around, you just have to go a bit further back in time to find it.  Here's the link:
https://github.com/aeisenberg/m2e/commit/b397f90681737c77a9944ab34f2af95af055e0d2

My plan is to squash all of my commits into a single one once I am ready for the contribution.  This commit will have to stay as is.  It's a large commit, but not much of it is being used any more (nothing, except for LifecycleMappingFactory.java, I think), so not sure of the best way to proceed on this.
Comment 44 Andrew Eisenberg CLA 2012-05-30 12:15:42 EDT
Any motion on this?  I'm hoping that we can raise a CQ for this when you are happy with the contribution.
Comment 45 Igor Fedorenko CLA 2012-05-30 12:19:08 EDT
I won't have time to do proper review and the paperwork until after I prepare 1.1 review docoware (and I am obviously extremely excited about the docoware ;-) )
Comment 46 Andrew Eisenberg CLA 2012-07-02 11:51:47 EDT
Slideware is done, Juno is out...take a look at this patch?  Due to IPZilla, this will take a long time to be integrated, so let's get started and hopefully, we can have this for SR1.
Comment 47 Igor Fedorenko CLA 2012-07-02 11:55:49 EDT
I have not forgot ;-) this is on my todo list for next couple of weeks.

I've also opened bug 382798 as I believe that CQ process is completely unreasonable in this case.
Comment 48 Andrew Eisenberg CLA 2012-07-02 17:16:48 EDT
Thanks.  Bug 382798 seems reasonable to me, but of course IANAL, neither am I on the PMC.  Hopefully something good will come of it.
Comment 49 Igor Fedorenko CLA 2012-07-07 14:47:18 EDT
I think proposed patch is generally a step in the right direction, but it requires more work. There are at least two things I think need to be implemented before we can consider this bugzilla "implemented"

1. the user has to have a way to add new workspace-scope mapping. I would probably implement a quick-fix, but I am open to other ideas. I do not believe asking the user to manually edit workspace lifecycle file is acceptable. This one is must have, because otherwise m2e developers will almost certainly have to significant time explaining how to edit mapping document.

2. the user has to have a way to see how workspace lifecycle mapping affects their projects. I think project preferences Maven->Lifecycle_Mapping is where I'd make this appear, but again, I am open to other ideas. Regardless of actual UI, I think lack of user-friendly ui this feature will result in extra support overhead, something I would really like to avoid.

3. I did not see any unit or integration test that cover workspace-scoped lifecycle mapping. Did I miss it?

@Andrew, do you think you'll be able to address these three issues in next couple of weeks or do you need help?
Comment 50 Rob Cernich CLA 2012-07-09 08:51:43 EDT
(In reply to comment #49)
> 1. the user has to have a way to add new workspace-scope mapping. I would
> probably implement a quick-fix, but I am open to other ideas. I do not believe
> asking the user to manually edit workspace lifecycle file is acceptable. This
> one is must have, because otherwise m2e developers will almost certainly have
> to significant time explaining how to edit mapping document.

I think quick-fix integration would be great, but I believe the ability to manually edit the file should be a requirement, even if that is simply opening up the workspace lifecycle file in an editor (similar to how user settings may be opened).  I would prefer the workspace lifecycle file be located in the workspace directory, as opposed to being buried in .metadata.

Just my two cents.

Best,
Rob
Comment 51 Andrew Eisenberg CLA 2012-07-09 11:24:39 EDT
(In reply to comment #49)
> 1. the user has to have a way to add new workspace-scope mapping. I would
> probably implement a quick-fix, but I am open to other ideas. I do not believe
> asking the user to manually edit workspace lifecycle file is acceptable. This
> one is must have, because otherwise m2e developers will almost certainly have
> to significant time explaining how to edit mapping document.

I'm not exactly sure what kind of UI/quickfix I should do.  Is the integration with the LifecycleMappingDialog not sufficient?  To add lifecycle mappings, users would not need to edit the global mappings file.  They just need to use the new checkbox in the LifecycleMappingDialog.   But to remove or change them they would (this is the same behavior for mappings that are added directly to the pom).


> 2. the user has to have a way to see how workspace lifecycle mapping affects
> their projects. I think project preferences Maven->Lifecycle_Mapping is where
> I'd make this appear, but again, I am open to other ideas. Regardless of actual
> UI, I think lack of user-friendly ui this feature will result in extra support
> overhead, something I would really like to avoid.

This would be a useful thing.  I'm guessing that you're talking about some kind of treeviewer over the lifecycle mappings metadata file?


> 3. I did not see any unit or integration test that cover workspace-scoped
> lifecycle mapping. Did I miss it?

Here are a few tests that I submitted when I originally worked on this issue:
https://github.com/sonatype/m2e-core-tests/pull/4
Comment 52 Igor Fedorenko CLA 2012-07-09 16:29:05 EDT
(In reply to comment #51)
> (In reply to comment #49)
> > 1. the user has to have a way to add new workspace-scope mapping. I would
> > probably implement a quick-fix, but I am open to other ideas. I do not believe
> > asking the user to manually edit workspace lifecycle file is acceptable. This
> > one is must have, because otherwise m2e developers will almost certainly have
> > to significant time explaining how to edit mapping document.
> 
> I'm not exactly sure what kind of UI/quickfix I should do.  Is the integration
> with the LifecycleMappingDialog not sufficient?  To add lifecycle mappings,
> users would not need to edit the global mappings file.  They just need to use
> the new checkbox in the LifecycleMappingDialog.   But to remove or change them
> they would (this is the same behavior for mappings that are added directly to
> the pom).
> 

Quick-fix label still says "permanently mark goal ... in pom.xml", so at very least the wording has to change to indicate to include both pom.xml and workspace configuration file. I also think "mark in workspace" is fundamentally different from "permanently mark" as it only works in this workspace, so I think it should be handle by a separate quick-fix. 

> 
> > 2. the user has to have a way to see how workspace lifecycle mapping affects
> > their projects. I think project preferences Maven->Lifecycle_Mapping is where
> > I'd make this appear, but again, I am open to other ideas. Regardless of actual
> > UI, I think lack of user-friendly ui this feature will result in extra support
> > overhead, something I would really like to avoid.
> 
> This would be a useful thing.  I'm guessing that you're talking about some kind
> of treeviewer over the lifecycle mappings metadata file?
> 
> 

There is such tree viewer already (project preferences, Maven->Lifecycle_Mapping), it shows effective mapping, i.e. ignore, execute, configurator, but does not show the source of the mapping, i.e. pom.xml, configurator or workspace.
Comment 53 Andrew Eisenberg CLA 2012-07-09 23:22:58 EDT
(In reply to comment #52)
> Quick-fix label still says "permanently mark goal ... in pom.xml", so at very
> least the wording has to change to indicate to include both pom.xml and
> workspace configuration file. I also think "mark in workspace" is fundamentally
> different from "permanently mark" as it only works in this workspace, so I
> think it should be handle by a separate quick-fix. 

I see what you mean.  I'm of two minds here.  On the one hand, I do like having all the functionality available in a single dialog so you have full control over where the mapping goes in a single place.  And this way, the quickfix menu is not cluttered with similar quickfixes.

On the other hand, I do see how having this extra checkbox clutters the dialog.

So, the question I think, is where should the user be making the choice on where the mapping should go?  And so I am slightly partial to keeping it in one dialog (but changing the name of the quickfix as you suggest) since the entire range of options is available in one location (rather than split across the two quickfixes and the dialog).

> There is such tree viewer already (project preferences,
> Maven->Lifecycle_Mapping), it shows effective mapping, i.e. ignore, execute,
> configurator, but does not show the source of the mapping, i.e. pom.xml,
> configurator or workspace.

It seems to me that this is a very useful feature, but different from the feature covered in this issue.  Since Maven->Lifecycle_Mapping doesn't yet show the sources of any mapping, I don't see why not having this functionality would be a blocker for releasing this feature.  Or are you suggesting something different?
Comment 54 Andrew Eisenberg CLA 2012-07-17 01:44:27 EDT
So, I did do some more work on this.  See my latest few commits here: https://github.com/aeisenberg/m2e/tree/bug350414

I changed the name of the quickfix.  I also refactored the LifecycleMappingsPropertiesPage and separated out a viewer that can be used by the properties page or the preferences page.

It is ALMOST working and now I am stuck.  On line 421 of org.eclipse.m2e.core.ui.internal.preferences.LifeCycleMappingsViewer, I need to extract the workspace level mojo execution mappings, but I can't see an easy way of doing this without a massive refactoring.

At this point, I am hoping that someone with a deeper proficiency in the code can figure out a way to do this.
Comment 55 Igor Fedorenko CLA 2012-07-19 06:43:59 EDT
@Andrew although I am not too happy with single quick-fix approach, I don't want to block your efforts any longer either. I suggest we split it in two steps. First, lets commit what you have implemented so far. Then decide how to better deal with the remaining UI changes. To get this going, I need you to attach to this bug single git-format-patch with your changes rebased off current measter HEAD. I will also need the answer to the following three questions included in git commit comment

(1) you wrote 100% of the code;  
(2) you have the right to contribute the code to Eclipse; 
(3) the file header contains the appropriate License header.

(standard eclipse legal staff http://www.eclipse.org/legal/EclipseLegalProcessPoster.pdf)


Also, I think workspace lifecycle mapping should be considered first, before looking at pom.xml and other sources. So if the pom has one configuration I don't like for whatever reason, I still can override this through local configuration. This is trivial to change, so don't worry about for now.
Comment 56 Andrew Eisenberg CLA 2012-07-19 12:48:53 EDT
Hi Igor,

I'd be glad to do that, but there is a complication.  As I mentioned above, I originally started this work based off of Milos Kleint's github branch.  Not much of his original code exists, but there are a few pieces left.

I'll rebase, but I'll leave Milos's commit intact.

Also, as I mentioned in my previous comment, I'm stuck on the UI.  I'll keep the mostly finished preferences page refactoring in the final commit, and I'll leave a TODO on the place where I got stuck.
Comment 57 Andrew Eisenberg CLA 2012-07-19 13:55:00 EDT
Created attachment 218945 [details]
Patch from Milos Kleint's repository (apply first)

Here is the patch from Milos's commit for SHA 170c7936da947d28e7824bc1d2375903dbda9b17.  He will need to approve this patch.  It should be applied first, before mine.
Comment 58 Andrew Eisenberg CLA 2012-07-19 13:58:42 EDT
Created attachment 218946 [details]
Patch from Andrew Eisenberg's repository (apply second)

This is my patch.

(1) I wrote 100% of the code;  
(2) I have the right to contribute the code to Eclipse; 
(3) the file header contains the appropriate License header.

(Also included in the commit itself.)
Comment 59 Andrew Eisenberg CLA 2012-07-19 14:00:54 EDT
And don't forget about the pull request for the tests.
https://github.com/sonatype/m2e-core-tests/pull/4
Comment 60 Igor Fedorenko CLA 2012-07-19 14:18:47 EDT
(In reply to comment #56)
> 
> Also, as I mentioned in my previous comment, I'm stuck on the UI.  I'll keep
> the mostly finished preferences page refactoring in the final commit, and I'll
> leave a TODO on the place where I got stuck.

Yes, I realise this. Lets merge what you have first and finish the ui later.
Comment 61 Igor Fedorenko CLA 2012-07-29 12:43:54 EDT
opened CQ 6697 for Andrew's changes and lets assume Milos meant to commit his patch to be committed to eclipse git repository (Milos, please speak up if this was not your indent).
Comment 62 Igor Fedorenko CLA 2012-08-13 08:09:11 EDT
FYI, I pushed these two patches and the pull request to m2e git repositories.

@Andrew, 

I fixed LifecycleMappingsViewer.java file name case and reformatting commit comment to be more git-ish. Hope you don't mind. 

Also, how strongly will you object if I move workspace mapping to separate "Ignore in workspace settings (experimental)" quick fix? I believe this will better emphasize the difference between "permanent" configuration via pom.xml and "workspace" preferences. It will simplify mapping dialog behaviour as well, which I believe became rather confusing. What do you think?
Comment 63 Andrew Eisenberg CLA 2012-08-13 11:25:13 EDT
> FYI, I pushed these two patches and the pull request to m2e git repositories.
Great.

> I fixed LifecycleMappingsViewer.java file name case and reformatting commit
> comment to be more git-ish. Hope you don't mind. 
Not a problem.

> Also, how strongly will you object if I move workspace mapping to separate
> "Ignore in workspace settings (experimental)" quick fix? I believe this will
> better emphasize the difference between "permanent" configuration via
> pom.xml and "workspace" preferences. It will simplify mapping dialog
> behaviour as well, which I believe became rather confusing. What do you
> think?
I'm fine with this.  Fundamentally, this is not my decision to make, since it's your project.  :)  I just want to see this patch integrated.
Comment 64 Igor Fedorenko CLA 2012-08-13 11:43:58 EDT
(In reply to comment #63)
> I'm fine with this.  Fundamentally, this is not my decision to make, since
> it's your project.  :)  I just want to see this patch integrated.

But this is your feature with all credit and all responsibility that comes with it :-)
Comment 65 Igor Fedorenko CLA 2012-08-16 09:07:14 EDT
Moved workspace mapping logic to a separate quick-fix. Did some cleanup and simplifications of the code. Updated Maven/LifecycleMapping project properties page to show mapping source. Update wiki to mention workspace mapping. 

I consider this feature request implemented. Any additional requests should go to separate bugzillas.
Comment 66 Andrew Eisenberg CLA 2012-08-16 12:10:33 EDT
Great.  I'd also like to see lifecycle mapping metadata storable inside of the current project (maybe in a .settings/lifecyclemappingmetadata.xml file).  Is there already a bug for this?
Comment 67 Christian Moser CLA 2012-09-14 05:42:11 EDT
Really nice, thanks a lot for the efforts!
Comment 69 Denis Roy CLA 2021-04-19 13:24:53 EDT
Moved to https://github.com/eclipse-m2e/m2e-core/issues/