Fix order of source folders in classpath. #364

rgladwell · 2015-07-09T18:47:37Z

During classpath configuration the order of source code folders changes
causing problems with source code lookup, for example during debugging:

https://github.com/rgladwell/m2e-android/issues/issue/245

The order should always be:

src/main/java
src/main/resources
src/test/java
src/test/resources
Android Framework
Android Private Libraries
Android Dependencies
Maven Dependencies
Non-runtime Maven Dependencies

This change reorders the classpath as above. Includes tests.

WonderCsabo · 2015-07-09T18:51:35Z

@rgladwell this PR lacks lots of classpath entries, which should be ordered properly. Did you intentionally skip those?

rgladwell · 2015-07-13T08:59:08Z

I find these code reviews go better if we try to politely discuss both the positives and negatives.

Depends what you mean by "lacks lots of": after your commit that refactored the classpath API it seems most of the ordering issues were resolved except for the /gensource folder, and the business code changes are mostly for this.

The tests only verify that the src/main, 'gen', 'src/test' and classpath containers (as a group) are in the correct order. It doesn't verify the resources folders. This is sufficient to test the change I've made, but perhaps not sufficient to prevent classpath order regressions in the future. I can add more conditions if you think this would be necessary.

WonderCsabo · 2015-07-13T09:23:38Z

Ricardo, I meant to be polite. Did you find my tone too hard? I do not, and if it is, that is due to translation, I guess, sorry.

I think it would be more safe to explicitly set the order here and do not rely on the other code which may order the entries by luck.

Also please note the last change adds the Android entries in a work space configurer, so ordering should be after that.

rgladwell · 2015-07-13T10:21:10Z

No worries, I appreciate English isn't your first language.

When you say "set the order" do you mean in the tests?

I'm not sure how much detail to set the order: down to the individual order of each classpath container, or just the order of the src/* folders including src/main/java?

Is there any reason to believe a WorkspaceConfigurer is called after a RawClasspathConfigurer?

WonderCsabo · 2015-07-13T11:22:59Z

When you say "set the order" do you mean in the tests?

Now, i would order all the entries in the configurer just like @kingargyle's old implementation did.

Is there any reason to believe a WorkspaceConfigurer is called after a RawClasspathConfigurer?

I am not really sure, however that was the only way when i created 90b5c7c. When i did it in the 90b5c7c, the Android containers were not available, yet.

rgladwell · 2015-07-13T13:07:06Z

Now, i would order all the entries in the configurer just like @kingargyle's old implementation did.

I think additional code to order the classpath would be redundant after your refactor of the classpath API. It appears only the /gen folder is unordered.

I can certainly strengthen the tests to prevent regressions though.

Does this also mean that the order of the classpath containers themselves is important? Or only that they appear after the source code folders?

I am not really sure, however that was the only way when i created 90b5c7c.

I am uncertain myself although the tests seem to be passing.

WonderCsabo · 2015-07-13T13:19:32Z

I'm with writing tests for now only to verify the order. Good point!

WonderCsabo · 2015-07-13T13:20:53Z

The order of the containers is not important for compilation, but it is better to see a fixed, meaningful order in the gui.

rgladwell · 2015-07-13T13:52:27Z

better to see a fixed, meaningful order in the gui.

I'm not sure how @kingargyle derived the order of the classpath containers.

rgladwell · 2015-07-14T09:59:36Z

Fixed PR, I decided not to test the order of the classpath containers as I'm unsure as to what is the correct order at this time.

Also added some minor refactorings.

@WonderCsabo please let me know what you think.

WonderCsabo · 2015-07-14T10:11:31Z

...java/me/gladwell/eclipse/m2e/android/configuration/classpath/ReorderClasspathConfigurer.java

+            IJavaProject javaproject = JavaCore.create(eclipseProject.getProject());
+            IClasspathEntry[] tosort = javaproject.readRawClasspath();
+            IClasspathEntry[] sorted = sortClasspath(mavenProject, tosort);
+            javaproject.setRawClasspath(sorted, new NullProgressMonitor());


I am not sure this is the good approach. You could just modify the IClasspathDescriptor here. Then m2e will persist its contents.

I agree. However, I'm trying to experiment with alternatives, but the m2e IClasspathDescriptor class doesn't allow you to update all its entries, and is basically not easy to order. Instead you have to remove all entries and then add them again in the right order which is also ugly and seems to drop the /gen folder for some reason.

Unfortunately, the Java sort algorithms also only seem to work on arrays, not sure if there is some other way we could order it.

Any ideas?

Unfortunately, the Java sort algorithms also only seem to work on arrays, not sure if there is some other way we could order it.

You can sort any List w/ Collections.sort(). Just use it on descriptor.getEntryDescriptors().

Good point: I did try this and the code is much simpler. However, I think we're going to have to call javaproject.readRawClasspath because the IClasspathDescriptor that is passed to this method is stale and lacks a gen/ entry.

When the gen/ is added it breaks the order of the classpath.

Yeah, that is the problem i faced when i wrote the m2e 1.6 support, which i mentioned to you earlier. :(

Cool, at least we've verified it twice. Maybe we should raise with upstream if we haven't already.

I'll revert my changes but tidy the code.

This is not an upstream problem. This only can we fixed if m2e would run lot of configuration cycles, to make sure other pulgins already contributed to the classpath. I think m2e should not modify the ClasspathDescriptor in while configuration cycle.

I'm confused: did you mean "m2e should not modify the ClasspathDescriptor" or "m2e-android should not modify the ClasspathDescriptor"?

I was talking about the upstream, m2e. So basically if it would update the descriptor concurrently after the plugins change the classpath, everything would break for sure. The current behavior us better.

kingargyle · 2015-07-14T12:49:15Z

Actually in some cases Order is necessary for compilation and also for some tooling to beable to look up the source code for a project. Especially if an Eclipse Plugin is only checking for the first source folder when looking up the code, then it is critical that the src/main/java always shows up first.

Also, my experience when I was writing the original sorter, is that the particular configurator could run about 3 or 4 times and if you have other maven plugins also configuring and adding their entries, you could get stuff out of order again. Especially if you are adding generated-sources and test-generated-sources to the list of entries.

As for how I derrived the order of the claspath containers, it was based on a clean implementation that the old ADT plugin wizards would create a clean project. I then added any Maven stuff to the end of that list.

rgladwell · 2015-07-14T13:23:37Z

@kingargyle do we know if changing the order of the classpath containers would cause problems or is it OK leave this?

WonderCsabo · 2015-07-14T13:46:33Z

@kingargyle

Especially if an Eclipse Plugin is only checking for the first source folder when looking up the code, then it is critical that the src/main/java always shows up first.

Indeed! That is already handled in this PR.

Especially if you are adding generated-sources and test-generated-sources to the list of entries.

Good point, i also experienced that. It is more safe to order all the entries, not just the source folders.

During classpath configuration the order of source code folders changes causing problems with source code lookup, for example during debugging: https://github.com/rgladwell/m2e-android/issues/issue/245 The order should always be: src/main/java src/main/resources src/test/java src/test/resources Android Framework Android Private Libraries Android Dependencies Maven Dependencies Non-runtime Maven Dependencies This change reorders the classpath as above. Includes tests.

rgladwell · 2015-07-14T14:43:56Z

PR updated.

kingargyle · 2015-07-14T17:12:39Z

The only order I think has to occur @rgladwell is that the Android SDK should be the first classpath container after all the sources so that ADT/Andmore finds that in case it is order dependent (haven't really looked). Some of the other is preference, but I'd like to have Maven Runtime, and then Non Maven Runtime in that order but not sure it really matters unless you have different libraries for Tests.

WonderCsabo · 2015-07-15T09:52:35Z

@rgladwell i am afraid this PR is not correct. The problem is you read the raw classpath, then persist it in your configurer. However, after all raw classpath configurers ran, m2e-core will persist the raw classpath according to the ClasspathDescriptor. I think it will overwrite your changes. :(

rgladwell · 2015-07-15T09:53:53Z

@WonderCsabo are you seeing problems on your local workspace?

WonderCsabo · 2015-07-15T11:11:15Z

This is only a theory. I will try to verify this at home.

rgladwell · 2015-07-15T14:23:04Z

Seems to be working locally for me.

Perhaps a more future-proof iteration of this would be to do the following:

Change the AddGenFolderClasspathConfigurer to be a RawClasspathConfigurer so it runs before the ReorderClasspathConfigurer.
Update ReorderClasspathConfigurer to directly order the IClasspathDescriptor as you suggest above.

This way we might be able to avoid directly writing the raw classpath and working with m2e.

I'll give it a try

WonderCsabo · 2015-07-15T14:24:47Z

Try to add a new entry as the first entry in the last raw classpath
configurer to the descriptor.
On Jul 15, 2015 16:23, "Ricardo Gladwell" notifications@github.com wrote:

Seems to be working locally for me.

Perhaps a more future-proof iteration of this would be to do the following:

Change the AddGenFolderClasspathConfigurer to be a
RawClasspathConfigurer so it runs before the ReorderClasspathConfigurer
.

Update ReorderClasspathConfigurer to directly order the
IClasspathDescriptor as you suggest above.

This way we might be able to avoid directly writing the raw classpath and
working with m2e.

I'll give it a try

—
Reply to this email directly or view it on GitHub
#364 (comment)
.

rgladwell · 2015-07-15T14:39:24Z

Sorry, I didn't understand that?

rgladwell · 2015-07-15T14:45:25Z

I attempted my suggestion above but it didn't work: lots of test failures.

WonderCsabo · 2015-07-15T15:28:48Z

Sorry, I quickly wrote down that in email. I will test it shortly after I arrive home.

WonderCsabo · 2015-07-15T17:05:25Z

@rgladwell sorry for my "theory", i think it is not true. This configurer is called when the classpath is resolved, so that is totally cool.

I also figured out why the IClasspathDescriptor cannot be used: IJavaProjectConfigurator.configureClasspath is used to configure the Maven classpath actually, so the descriptor only contains the Maven classpath entries.

WonderCsabo · 2015-07-18T08:45:57Z

:( This worked for my real project, but now target/generated-sources/annotations is added as the first entry. (I am using m2e-apt for that.)

rgladwell added the waffle:in progress label Jul 9, 2015

rgladwell mentioned this pull request Jul 9, 2015

Ensure source folders appear first in the classpath list. #248

Closed

rgladwell force-pushed the reorder-classpath branch 2 times, most recently from f57680e to 095e429 Compare July 14, 2015 09:58

WonderCsabo reviewed Jul 14, 2015
View reviewed changes

rgladwell force-pushed the reorder-classpath branch from 095e429 to 72dbd71 Compare July 14, 2015 14:44

rgladwell merged commit 72dbd71 into master Jul 14, 2015

rgladwell removed the waffle:in progress label Jul 14, 2015

rgladwell deleted the reorder-classpath branch July 19, 2015 11:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix order of source folders in classpath. #364

Fix order of source folders in classpath. #364

rgladwell commented Jul 9, 2015

WonderCsabo commented Jul 9, 2015

rgladwell commented Jul 13, 2015

WonderCsabo commented Jul 13, 2015

rgladwell commented Jul 13, 2015

WonderCsabo commented Jul 13, 2015

rgladwell commented Jul 13, 2015

WonderCsabo commented Jul 13, 2015 via email

WonderCsabo commented Jul 13, 2015 via email

rgladwell commented Jul 13, 2015

rgladwell commented Jul 14, 2015

WonderCsabo Jul 14, 2015

rgladwell Jul 14, 2015

WonderCsabo Jul 14, 2015

rgladwell Jul 14, 2015

WonderCsabo Jul 14, 2015

rgladwell Jul 14, 2015

WonderCsabo Jul 14, 2015 via email

rgladwell Jul 14, 2015

WonderCsabo Jul 14, 2015 via email

kingargyle commented Jul 14, 2015

rgladwell commented Jul 14, 2015

WonderCsabo commented Jul 14, 2015

rgladwell commented Jul 14, 2015

kingargyle commented Jul 14, 2015

WonderCsabo commented Jul 15, 2015

rgladwell commented Jul 15, 2015

WonderCsabo commented Jul 15, 2015 via email

rgladwell commented Jul 15, 2015

WonderCsabo commented Jul 15, 2015

rgladwell commented Jul 15, 2015

rgladwell commented Jul 15, 2015

WonderCsabo commented Jul 15, 2015 via email

WonderCsabo commented Jul 15, 2015

WonderCsabo commented Jul 18, 2015

Fix order of source folders in classpath. #364

Fix order of source folders in classpath. #364

Conversation

rgladwell commented Jul 9, 2015

WonderCsabo commented Jul 9, 2015

rgladwell commented Jul 13, 2015

WonderCsabo commented Jul 13, 2015

rgladwell commented Jul 13, 2015

WonderCsabo commented Jul 13, 2015

rgladwell commented Jul 13, 2015

WonderCsabo commented Jul 13, 2015 via email

WonderCsabo commented Jul 13, 2015 via email

rgladwell commented Jul 13, 2015

rgladwell commented Jul 14, 2015

WonderCsabo Jul 14, 2015

Choose a reason for hiding this comment

rgladwell Jul 14, 2015

Choose a reason for hiding this comment

WonderCsabo Jul 14, 2015

Choose a reason for hiding this comment

rgladwell Jul 14, 2015

Choose a reason for hiding this comment

WonderCsabo Jul 14, 2015

Choose a reason for hiding this comment

rgladwell Jul 14, 2015

Choose a reason for hiding this comment

WonderCsabo Jul 14, 2015 via email

Choose a reason for hiding this comment

rgladwell Jul 14, 2015

Choose a reason for hiding this comment

WonderCsabo Jul 14, 2015 via email

Choose a reason for hiding this comment

kingargyle commented Jul 14, 2015

rgladwell commented Jul 14, 2015

WonderCsabo commented Jul 14, 2015

rgladwell commented Jul 14, 2015

kingargyle commented Jul 14, 2015

WonderCsabo commented Jul 15, 2015

rgladwell commented Jul 15, 2015

WonderCsabo commented Jul 15, 2015 via email

rgladwell commented Jul 15, 2015

WonderCsabo commented Jul 15, 2015

rgladwell commented Jul 15, 2015

rgladwell commented Jul 15, 2015

WonderCsabo commented Jul 15, 2015 via email

WonderCsabo commented Jul 15, 2015

WonderCsabo commented Jul 18, 2015