Community
Participate
Working Groups
I'm using m2e 1.3 and m2e-wtp 0.17.0 on Juno Java EE package. Steps to recreate the problem I'm seeing: 1. Create a dynamic web project and target it to Tomcat 2. Convert the web project to Maven using the war packaging 3. Go to the Java build path property page of the Maven project 4. In the order and export tab, you will see the Tomcat classpath container, the JRE container and the Maven dependencies container. Make sure the Tomcat classpath container is at the bottom (use the Up and down button to move it) 5. Select Maven -> Update project in the Java maven project. The result is that the order of the entries is modified, always putting the Maven classpath container at the bottom and the JRE container above it. This causes problems if the server classpath container has old versions of jars we want to "override" using the Maven dependencies classpath container. If the Maven container is moved to be above the server runtime so we can compile against the jars in the Maven container, and then we refresh the project, then order is changes and we see compilation problems. I did some investigation and found that the method rg.eclipse.m2e.jdt.internal.AbstractJavaProjectConfigurator.configure(ProjectConfigurationRequest request, IProgressMonitor monitor) is the one changing the classpath order. Is this behavior working as designed? Is there a reason to always put the Maven and JRE classpath containers at the bottom?
I'll assume this is m2e/wtp fault unless proven otherwise.
Created attachment 225608 [details] Pure Java project with custom classpath order Nice try Igor, but this one goes to m2e-jdt :-) Attached is a test java project, already mavenized (same thing happens during conversion). The classpath order is : Maven, JRE, src/main/resources/, src/test/java, Junit4. Once imported, or when you do Maven > Update Project, the classpath order changes to : src/main/resources/, src/test/java, Junit4, JRE, Maven.
Created attachment 225911 [details] Proposed patch The proposed patch removes the lines that remove the Maven and JRE classpath containers before adding them again. This is because the implementation of IClasspathDescriptor used in AbstractJavaProjectConfigurator, which is ClasspathDescriptor, implements the method addEntry(IClasspathEntry cpe) in a way that if the path of the cpe used as input already exist, it is replaced to avoid duplicates.
Roberto, your patch doesn't properly handle the conversion scenario you described in your first comment. You end up with 2 JRE entries : http://screencast.com/t/cGopPz3VHcDq For that kind of sensitive change, you should also provide some ITs, make sure the existing test suite doesn't exhibit regressions. There will most likely be failing tests if the behaviour is changed, but you need to make sure the failures are expected (and change the expectation) or not (that'd be bad)
(In reply to comment #4) > Roberto, your patch doesn't properly handle the conversion scenario you > described in your first comment. > You end up with 2 JRE entries : http://screencast.com/t/cGopPz3VHcDq > > For that kind of sensitive change, you should also provide some ITs, make > sure the existing test suite doesn't exhibit regressions. > There will most likely be failing tests if the behaviour is changed, but you > need to make sure the failures are expected (and change the expectation) or > not (that'd be bad) Yes, you are correct, I found the same problem last night while doing some testing. In fact, in the scenario I have, only the Maven dependencies classpath container is the one causing problems. I mean, I can live with the JRE container being moved to the bottom, so probably I could modify the patch to change the behavior of the Maven dependencies container only, what do you think? BTW, what do you mean by IT? Integration testing?
(In reply to comment #5) > In fact, in the scenario I have, only the Maven dependencies classpath > container is the one causing problems. I mean, I can live with the JRE > container being moved to the bottom, so probably I could modify the patch to > change the behavior of the Maven dependencies container only, what do you > think? Having different classpath entries behaving seemingly randomly doesn't seem a very good idea IMHO, but I'm not too familiar with the JDT integration. Igor's opinion would probably be more insightful here. > > BTW, what do you mean by IT? Integration testing? yes
Created attachment 226348 [details] Proposed patch (v2)
Created attachment 226349 [details] JUnits
I dont' believe checking if the Maven Classpath Library path makes sense. That path is controlled by m2e AND used to identify the library
I dont' believe checking if the Maven Classpath Library path *changed* makes sense. That path is controlled by m2e AND used to identify the library -- gotta love BZ editing abilities
At this moment, I agree. I was thinking in the future, in case something else is added to the path of this container. But if you believe there are few chances for this to happen, then I believe I can remove the code that removes the Maven classpath container, and always add it, like in the first patch.
I'm not aware of any plans for changing that path in any forseeable future. Igor?
I will not have time to review this for m5 but plan to provide feedback before m6.
Created attachment 226425 [details] Proposed patch (v3) Patch that does not check for modified Maven classpath container.
(In reply to comment #10) > I dont' believe checking if the Maven Classpath Library path *changed* makes > sense. That path is controlled by m2e AND used to identify the library > > -- gotta love BZ editing abilities Based on your comment, I just updated a new patch, which does not check for changed Maven classpath container
Created attachment 226426 [details] Proposed patch (v3-corrected) Same patch as v3, but removing a local variable that was not used.
Pinging.... I believe M5 is over and M6 started this week (at least for other projects, I'm not sure if is the same for m2e).
Like I said, I plan to provide feedback *before* m6, i.e. by http://wiki.eclipse.org/Kepler/Simultaneous_Release_Plan http://wiki.eclipse.org/Kepler/Simultaneous_Release_Plan . Also note use of "plan" in the previous sentence and plans do change.
Applied the patch, thank you. https://git.eclipse.org/c/m2e/m2e-core.git/commit/?id=26bd4382b76f7b51dbac2dd4a5934f9e5824e48e https://github.com/sonatype/m2e-core-tests/commit/c5af471b8c5c4f2ac6e0bd1793880034a9d49222
Moved to https://github.com/eclipse-m2e/m2e-core/issues/
