Modifying classpath takes previous attributes into account

[WonderCsabo]

This little PR is loosely related to #182. It is not a good practice to create new entries, this change takes previous attributes into account. Also i refactored the code so i think it is a little bit cleaner now. I did not add any tests, since there is nothing to test in this PR.

[WonderCsabo]

As i previously said, no tests, since there is nothing to test. From my investigation it seems any change to the classpath by the user is not reflected in m2e. For example if i change the src/main/java folder to ignore optional problems, and than run a "Maven->Update project", that classpath attribute is dropped by m2e itself. The same applies to classpath containers.
But it is still a bad practice to drop everyting we get from the upstream m2e plugin, and re-add all our modified entries with empty attributes, since we could never no when m2e will provide additional attributes to those.
Actually we could test this by mocking out m2e to not drop the attributes, and then test that we do not drop, too. But it sounds very hard or more likely impossible.

[rgladwell]

@WonderCsabo That use-case seems easy to test: just modify a POM and simulate a "Maven -> Update project" command via the m2e API. Is it more complicated than this?

What leads you to believe this is a problem with the upstream m2e project?

[WonderCsabo]

Unfortunately m2e has indeed some problems with classpath attributes. Non-pom-derived attributes are just dropped when updating a maven project.

But, your idea about testing is good. Now i just have to find a modification in the POM, which adds an additional pom derived attribute to a classpath entry. But i am not what POM config is reflect in the Eclipse classpath in this way, if there is a config at all.

[rgladwell]

@WonderCsabo If this is an issue with the upstream project, I would suggest we close this ticket in favour of the one currently open against m2e.

[WonderCsabo]

Nope, because m2e still can add attributes from the pom we would drop. The idea of this change: inherit the classpath from m2e and only modify the necessary things.

[WonderCsabo]

For example: i have to set the output foldet to bin.
Old implementation sends the old entry to trash, creates a new entry with modified output, other attributes are lost. Then adds the new entry.
The new implementation simply sets the output folder on the existing entry.

[WonderCsabo]

I added a test which fails with the old implementation and passes with the new. m2e reads include patterns from the POM and adds them to the classpath entry. When we modify the output folder of the entry, we should not touch these includes, but with the old implementation, we totally drop them.

[WonderCsabo]

Does it test classpath attributes are retained when a classpath container is marked exported/un-exported and when the output location is changed?

Also you do not test all the classpath attributes are maintained as per your comment here.

I think m2e does not add any properties to the classpath container, at least what i found when i investigated m2e codebase. So there is no property we could test against, and my change about classpath containers is a "just in case" thing.

I think i fully covered the case with the source entry. Also please note that my comment from February is not true, and i corrected myself a week ago. Also ignore_optional_problems is dropped by m2e itself (it is a known m2e bug), so we cannot test that either.

[rgladwell]

I think m2e does not add any properties to the classpath container, at least what i found when i investigated m2e codebase. So there is no property we could test against, and my change about classpath containers is a "just in case" thing.

Refactorings un-related to the main change like this should be kept in separate commits and clearly marked as refactorings to avoid confusion.

I think i fully covered the case with the source entry. Also please note that my comment from February is not true, and i corrected myself a week ago. Also ignore_optional_problems is dropped by m2e itself (it is a known m2e bug), so we cannot test that either.

This is confusing: so we do not drop the maven.pomderived or optional .classpath attributes when we modify the classpath? Does m2e drop these? We only lose the inclusion pattern attribute?

[WonderCsabo]

Refactorings un-related to the main change like this should be kept in separate commits and clearly marked as refactorings to avoid confusion.

I'll split the commits, then.

This is confusing: so we do not drop the maven.pomderived or optional .classpath attributes when we modify the classpath? Does m2e drop these? We only lose the inclusion pattern attribute?

These classpath attributes are added by m2e. m2e reads inclusions, encodings, etc from the POM, and adds these properties to the classpath. And with the previous implementation, we dropped almost all properties, because we created a completely new object with default values. My implementation just reuses the entry which was created by m2e, so nothing is dropped by us what was added by m2e.

maven.pomderived was not dropped in the previous implementation. Optional was also retained, because you called it with a true parameter:

classpath.addSourceEntry(entry.getPath(), project.getFullPath().append(path), true);

If you think, i can add another test which tests optional, but i think these tests are a little bit dumb, since as i said, we just take the object what was created by m2e.

[rgladwell]

I'm still really confused: you say:

And with the previous implementation, we dropped almost all properties, because we created a completely new object with default values.

But then you say:

... maven.pomderived was not dropped in the previous implementation. Optional was also retained ...

These are all the attributes you listed as being dropped? What other attributes are being dropped? These statements appear to contradict each other.

If you think, i can add another test which tests optional, but i think these tests are a little bit dumb, since as i said, we just take the object what was created by m2e.

I do not think these additional tests would be "dumb": if previous implementations break behaviour by dropping specific attributes I need to see proof that this is no longer happening. This also prevents future changes to the code introducing regressions.

[WonderCsabo]

These are not contradictions. Yeah, optional and maven.pomderived were indeed dropped by us, but we immediately re-added them.

optional: we passed true the the generated parameter, so the entry was marked as optional by the addSourceEntry() method:

if(generated) {
   descriptor.setClasspathAttribute(IClasspathAttribute.OPTIONAL, "true"); //$NON-NLS-1$
}

maven.pomderived: When you call addSourceEntry(), it will call addEntryDescriptor, which executes descriptor.setPomDerived(true);. So actually all source entries added to the ClasspathDescriptor are set as pomderived.

[rgladwell]

So we are retaining the optional attribute and indirectly retaining the maven.pomderived attributes in our code? In which, case we should have tests to verify this behaviour.

[WonderCsabo]

Updated.

[rgladwell]

Thanks, I'll try and re-review soon.

Also, the CI build is still reporting a failure.

[WonderCsabo]

The failures in the CI build are totally unrelated. Unforgettably our Travis build became unstable some time ago.

I found out that the Maven Dependency Container also has maven.pomderived attribute, i am squashing the commits and writing a test to make sure that is not lost.

[rgladwell]

These need to go green before a PR can be merged. Please let me know when they do.

[WonderCsabo]

The CI build is done. You have to restart the jobs to make it green, or merge it without passing.

[WonderCsabo]

The Travis build is ridiculously unstable. I just started it my fork again for the 4th time, and Indigo and Juno are still failing. I think we should not wait for passing.

[rgladwell]

I appreciate its annoying nut I think, rather than ignore these results, we should focus on improving the stability of the build. If we start ignoring build failures we may break something for older builds.

[kingargyle]

Actually, I think the issue is not with the tests or how we are building, but with the Travis CI system in general. It keeps getting error code 137s which are system errors. None of those issues show up locally in my Jenkins CI build of the various target platforms.

[WonderCsabo]

I also suspect that something changed about Travis, and this is not our fault. (I already wrote this in another issue). These issues started to appear some time ago, and they were never present before.

@kingargyle is there any way to get the real error behind code 137? I added all the debug flags in a test Travis build and unfortunately it did not reveal more info.

[kingargyle]

It is basically the OS killing the process. Some information here: http://stackoverflow.com/questions/18524574/java-program-terminate-with-java-result-137

[WonderCsabo]

Then it can be the OOM problem which was logged in one case.

[kingargyle]

If we are getting OOM issues, those we can handle by increasing the heapsize or permgen that is passed into the tycho-surefire-tests as part of the arguments parameter.

[WonderCsabo]

The current argline is:

<vmargs>-XX:MaxPermSize=1024m -Xmx1024m -XstartOnFirstThread</vmargs>

How should we modify this? It is a shame, but i am quiet noob at these parameters...
Also @manandbytes already had a comment about this, but i am not sure what "strange" means.

PS: maybe we should continue the discussion in the referenced thread.

[kingargyle]

Not sure we need to have 1024m of PermSize. Usually 256m is more than enough. Also, for the Xmx 512m should be enough. I would start -Xms at 256m as well. Part of the problem is that we may be requesting more memory than the machine that the job runs on has available.

[WonderCsabo]

I learned that the machine has 3GB of memory, but i do not know how much free memory we have at build. I added a free -m command to get an idea. Also it seems these were on much lower values, but @rgladwell increased them in 13eb5db. Test job is here.

[WonderCsabo]

It seems these settings did not help. I really hate this. BTW, free -m output this:

             total       used       free     shared    buffers     cached
Mem:          3072       2280        791          0          0          0
-/+ buffers/cache:       2280        791
Swap:         1536        165       1370

[rgladwell]

CI fixed (#274) so you might want to rebase this so it goes green.

[GitCop]

There were the following issues with your Pull Request

• Commit: 86cb043
  • Your subject line is longer than 50 characters
• Commit: 1823285
  • Your commit message does not contain a body
  • Your subject line is longer than 50 characters

Guidelines are available at https://github.com/rgladwell/m2e-android

---

This message was auto-generated by https://gitcop.com

[GitCop]

There were the following issues with your Pull Request

• Commit: 932460d
  • Your commit message does not contain a body

Guidelines are available at https://github.com/rgladwell/m2e-android/blob/master/CONTRIBUTING.md

---

This message was auto-generated by https://gitcop.com

[rgladwell]

Great PR, thanks for this.

[WonderCsabo]

Thanks for the review and feedback!