Bugzilla – Full Text Bug Listing
| Summary: | exclude dependency action issues | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | z_Archived | Reporter: | Milos Kleint <mkleint> | ||||||
| Component: | m2e | Assignee: | Matthew Piggott <matthew> | ||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||
| Severity: | normal | ||||||||
| Priority: | P3 | CC: | fbricon, igor, pascal | ||||||
| Version: | unspecified | Flags: | pascal:
review?
(mkleint) |
||||||
| Target Milestone: | --- | ||||||||
| Hardware: | PC | ||||||||
| OS: | Mac OS X - Carbon (unsup.) | ||||||||
| Whiteboard: | |||||||||
| Attachments: |
|
||||||||
|
Description
Milos Kleint
I tried to exclude several dependencies at once from the Maven Library in the Project explorer view and found more issues :
- each exclusion displays an unexpected version tag (causes a pom error)
- the exclusions tags in the pom are not formatted (displayed on a single line)
- at some point, an error popup is displayed. Adding the following dependency :
<dependency>
<groupId>org.springframework</groupId>
<artifactId>spring-orm</artifactId>
<version>3.0.5.RELEASE</version>
</dependency>
If I try to exclude both spring-core and commons-logging (both transitive deps), there's a popup displaying : "Failed to locate commons-logging:commons-logging:1.1.1"
Have fun ;-)
Created attachment 189731 [details]
Fixes
I believe I've fixed the problems brought up by Fred.
- removed version tag
- formatted exclusions
- in this case commons-logging was brought in by spring-core and the refactoring would stop traversing that branch of the tree when finding spring-core. I've changed it to continue traversing the tree and display an informational message when the operation doesn't result in an explicit exclusion. (There is also now an informational message when it can't be excluded in the workspace.)
Milos could you please review and commit. git am <patch says Patch format detection failed. the first two of fred's issues seem to be fixed by the patch. for the last one: I would prefer a solution where we either show the possible warnings up front or shut up about them altogether. There is no point in showing a popup message after the fact. The exclude is effectively doing what was intended by the user, but more effectively (by just excluding the higher level transitive dependency +1 for shutting up. User will be amazed to see how clever m2e is and will praise the Lord (or whatever deities he believes in). Matt and I discussed this for a while and we decided to go with the message because if the user looks at the diff page, then he may wonder why it did not exclude one library whereas it did exclude the others. Another point, the current behaviour (before Matt's changes) will look into changing the parent if this is where the dependency is found. However this is not necessarily correct since a change in a child will cause changes in other children. Milos and I discussed this and we are thinking that a less surprising behaviour would be to prompt the user for the scope of the exclusion at the beginning of the wizard. By scope I mean, show the user the parent hierarchy and let him select where he could actually make changes. By default the scope would only be local, meaning that we would only change the current pom. One interesting aspect is that to exclude a transitive dependency obtained from a dependency declared in the parent section, it may be necessary to add a new dependency section in the current pom. Finally exclusions can also go in dependency management section. I've released a new patch from Matt. A few things that need fixing: - Review enablement of the wizard button. When I want to pick the parent, and don't select anything the next should be disabled - Warn the user when we are actually removing the dependency in the parent. - When I select a parent, then go back to select the "current" pom option, the selection of the parent is still used to perform the operation - Show name of the pom in the first dialog. - In B trying to remove a dependency defined in A (where the relationship is A <- B <- C), then B is touched even though nothing changed in it. - Review the undo behaviour, it does not seem to work when the editor is not opened - Review behaviour of opened editor: is it refreshed when the exclusion is changed? is it refreshed when the editor is dirty Another thing I ran into is: - When the dependency being excluded is managed and I exclude it, the snippet added is invalid since it contains the version tags but not he version itself <dependency> <artifactId>spring-core</artifactId> <groupId>org.springframework</groupId> <version></version> <exclusions> <exclusion> <artifactId>commons-logging</artifactId> <groupId>commons-logging</groupId> </exclusion> </exclusions> </dependency> (In reply to comment #8) > - When I select a parent, then go back to select the "current" pom option, the > selection of the parent is still used to perform the operation encountered this as well while testing - http://git.eclipse.org/c/m2e/m2e-core.git/commit/?id=cf3055482504d8e2c694dbc99a12633e84194cf8 a few concerns of my own: - the pom selection panel needs more labels, it's not entirely clear what is to be decided here. - i've sort of expected that the pom selection page only shows when there are any used dependencies defined in those parent (and these parents are editable). I've seen code that shows related error messages after-the-fact (after selecting parent pom that cannot be modified). - *IMPORTANT* when I select the parent pom, all the dependencies from the child are placed in the parent (and excluded). That's not what I expected to happen. My expectation was that the selection of the parent pom allows excludes to be placed in the parent but only those that are actually already present there. The dependencies from child poms shall not be moved up in this refactoring (and indeed is a candidate for a refactoring of it's own). As a matter of fact pulling a dependency to parent and excluding there has *not* the desired effect of actually excluding that dependency, as the child's definition keeps it. - active profiles are not taken into account when looking for the dependency in the pom file. (as far as I could tell) - when I have dependencies in activated profiles, I get an error saying "No pom file found for operation" which felt misleading (just a Czech IMHO) (In reply to comment #8) > - Review the undo behaviour, it does not seem to work when the editor is not > opened I've tried to debug the undo behaviour. here's what I've found. the problem is primarily in the link between the editor's undomanager and the refactoring one. 1. when I perform undo on POM A and it changes A, the undo operation in the open editor is not named according to the refactoring, but appears as single text change. 2. when I perform undo on POM A and it changes B, the undo action in the open editor is disabled. when I close the affected POM A file and select something in the package explorer, the undo action carries the name of the refactoring and performs undo on the affected files. number 1. in our pom editors differs from the behaviour of the java files. When the java file is refactored, the undo action in the open editor carries the name of the refactoring action. But even the java files only include the undo operation to the undo stack of the main java file. If I rename a class from A to A1, the undo action/operation is only present on A1, not in any file that used A and was modified. Created attachment 190664 [details]
Fixes for Exclude Refactoring
(In reply to comment #12) > Created attachment 190664 [details] > Fixes for Exclude Refactoring "MavenProject does not exist, try cleaning workspace & rebuilding" when not being able to resolve the pom sounds misleading to me. In most cases the pom will have unparsable content, or unreachable parent or something along these lines. Not sure how rebuilding helps here. Also I'm not in favour of exposing "MavenProject" as a word, it's just a class name in the maven codebase. I suggest we replace it with "Maven POM cannot be resolved" or similar.. In PomHelper you've internationalized the log message, that's not required. changes to preference files (org.eclipse.m2e.core.ui/.settings/org.eclipse.jdt.core.prefs, org.eclipse.m2e.refactoring/.settings/org.eclipse.jdt.core.prefs) shall not be part of the commit, not related to the change at hand. this snippet in PomHelper.java is wrong. 74 model = StructuredModelManager.getModelManager().getExistingModelForEdit(file); 75 if(model == null) { 76 existing = false; 77 model = StructuredModelManager.getModelManager().getModelForRead(file); 78 } you are mixing read and edit models and only the read model gets ever released. this way you are creating a memory leak (edit model gets never released) and can even cause a data loss because other edits will check the counter and not save the file if it's still being held. fix for the PomHelper issue - http://git.eclipse.org/c/m2e/m2e-core.git/commit/?id=650b685dfe2ae4e6dc27d7bc0a4666501112503b I looked at the Undo behaviour for the PomEditor compared to what occurs in Java. It appears that the reason our Undo operation does not work across multiple files is due to the StructuredTextEditor using the StructuredTextViewerUndoManager for the undo/redo stack. This skips using the Change operation created by the refactoring (which I believe would provide cross-document support). When the dependency is added by an active profile, should it always be excluded? If not, what determines when the dependency from the profile should be excluded? (In reply to comment #16) > When the dependency is added by an active profile, should it always be > excluded? If not, what determines when the dependency from the profile should > be excluded? benjamin could comment as well, but AFAIK if the profile is active and the dependency is found there (the direct/non-transitive one) then it's where the exclude should happen. The stuff from profiles overrides what is in the base section. so I think if we should place the include on the element which is used (the profile in this case) and ignore the elements not used (not activated profiles or the base section if the profile overrides) The case with 2 profiles overriding a base version and both being active is well, well.. unsolvable.. I might bet on changing both profiles. another unrelated possible "smartism" is to check if the dependency is managed and the managed element contains excludes and if so, add the excludes there. (In reply to comment #17) > > another unrelated possible "smartism" is to check if the dependency is managed > and the managed element contains excludes and if so, add the excludes there. But that's an issue for an enhancement request I suppose.. this issue is already overloaded with details.. Are you planning any further work on this? If not, please close this bug. Any further refinements to exclude dependency action will be tracked via new separate bugzilla tickets |