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

kingargyle · 2014-08-08T16:05:37Z

After running the other Classpath Configuration classes the source folders
are out of sequence, causing all sorts of problems looking up source. This
is an attempt to re-order the classpaths. It hasn't yet been tied into the
code, but here for code review and comment.

WonderCsabo · 2014-08-08T22:00:45Z

I think you should use methods to get the source paths instead of using hardcoded strings.

WonderCsabo · 2014-08-25T11:51:44Z

@kingargyle if you update your patch, can you include target/generated-sources/annotations (it is used by annotation processors to generate code)? Currenty it moves before src/main/java which is really annoying. So if this entry is present, i think the classpath should look like this:

source folders
Maven Dependencies
Non-Runtime Maven Dependencies
target/generated-sources/annotations

kingargyle · 2014-09-05T19:59:37Z

Question. Is there any objection if I add Mockito as a dependency? I'd also like to add org.junit 4.8 (preferablly 4.11) as a dependency instead of org.junit 3.8.2 that is currently in the build.

WonderCsabo · 2014-09-05T20:02:31Z

Why do you need Mockito? We only have integration tests.

rgladwell · 2014-09-05T22:01:21Z

I'm not sure if junit 3 is required by the Eclipse PDE Junit runner.

Are you planning to add unit tests?

kingargyle · 2014-09-08T12:42:39Z

I'm planning on adding Unit tests for the code that I'm working on, so would like to add Mockito. You can use Junit 4 with PDE.

rgladwell · 2014-09-08T13:14:20Z

If we can switch the PDE integration tests to use JUnit 4 that would be amazing. Bear in mind, we may also be constrained because I think the m2e test code relies on the JUnit 3 framework. Did you check this?

I'm happy to add unit tests to the code, although I'm also a big fan of keeping slow-running, integration tests separate from unit tests. Is it possible to keep unit tests apart from integration test code? Perhaps we can put them, Maven-style, in a src/test folder in the me.gladwell.eclipse.m2e.android module?

kingargyle · 2014-09-08T14:37:30Z

Yes, but we could always copy the necessary m2e-test framework code. I took a look at it and it isn't using anything specific from Junit3. Wouldn't be that difficult to migrate it over to use Junit 4 instead of 3.

There are ways to combine the tests and src, but with Eclipse Plugin Tests, they unfortunately need to be in their own module. So if we want to keep unit tests and integration tests separate then I suggest a new plugin. Typically with eclipse there is a corresponding Unit Test bundle for each bundle. You then only startup the particular pieces you need instead of relying on a whole IDE instance.

I'll do a separate pull request for the suggested migration to use Junit 4.

rgladwell · 2014-09-08T16:30:40Z

👍 on the separate PR.

Could we put the unit tests in a separate src/test folder in the me.gladwell.eclipse.m2e.android.test module so we can at least run them separately there? I'd prefer to avoid another module if possible.

kingargyle · 2014-09-08T17:30:38Z

yes, should be able to put them in a test source folder. We'll just need to configure the tycho-surefire-plugin to run the tests and integration tests separately.

rgladwell · 2014-09-08T17:37:02Z

sweet

WonderCsabo · 2014-09-08T17:38:40Z

And what about running the unit tests from Eclipse? Is it possible? Also i am curious, how would you like to add Mockito to the project? Currently we just use the bundles installed to the developer's Eclipse, and it does not have Mockito for sure. I am also not sure whether Mockito is in the standard p2 repositories.

rgladwell · 2014-09-08T17:39:55Z

@WonderCsabo on the subject of selectively running tests in Eclipse, its tricky if not impossible. Maybe we should have a separate module for unit tests.

WonderCsabo · 2014-09-08T17:46:15Z

Actually we can tell the junit runner to run the tests from a folder only. Or other issues are present regarding PDE?

kingargyle · 2014-09-08T17:47:57Z

Junit 4.11 and Mockito are both in Eclipse Orbit p2 repos.

http://download.eclipse.org/tools/orbit/downloads/drops/R20130517111416/

There are a couple of ways to run tests with eclipse. You can create a couple of Test Suites if you want, or with in eclipse itself you can run individual plugin tests as you would regular junit tests (just run them as junit plugin tests). So basically everything that you can do with the standard junit runner you can do withe eclispe plugin test runner within the ide.

As for Mockito, I want to be able to mock out some of the interfaces without necessarily having to call down into specific classes. Without mocking, even "unit tests" start to become integration tests. Eclipse tests in general without mocking tend to degrade down into the realm of integration tests, with mocking you have a bit more control over things and can just test that the logic is working as expected.

rgladwell · 2014-09-08T17:49:38Z

One proviso with the unit tests: its still vital we have at least one or two integration tests to verify the new behaviour so please do not just write unit tests to check we're correctly re-ordering the classpath.

kingargyle · 2014-09-08T17:52:44Z

Yep. Agreed that it will be in addition to the reording integration tests. My testing of it this weekend, seems to indicate that it may be an issue with when things are being called, but I need to do some more work on it and step through the configurators to see when the order gets out of place and when the RawClasspath configurators are being called.

kingargyle · 2014-09-10T18:26:04Z

I still need to add an integration test this, but the basics work. I haven't looked at adding any of the target/generated* folders to the list.

WonderCsabo · 2014-09-10T18:40:46Z

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

+
+public class ReorderClasspathConfigurer implements RawClasspathConfigurer {
+
+	private static final String SRC_TEST_RESOURCES = "src/test/resources";


We cannot hardcode these. You have to obtain the location from the maven model. Also there can be multiple folders of each kind. You have to obtain them dynamically (for example with test resources you have to call MavenProject#getTestResources(). We already abstract these calls, see MavenAndroidProject#getSourcePaths().

How do we know which order things should be added then. The current implementation goes by maven naming standards. I can add code to add all the others ahead of the non-source folder items.

If main/java isn't first and you are debugging from another project, the source lookup fails. Also, the android-maven-plugin handles generation of the R files and it puts them in a particular directory, so we can look in there and grab the root folder for each of the generated files.

For most projects, there will be one source folder for each kind, and yes, with the conventional names. So if you do this order:

all source folders, in the order defined by the maven model

all resource folders,in the order defined by the maven model

all test source folders, in the order defined by the maven model

all test resource folders, in the order defined by the maven model

You will get the current order, and everybody will be happy.

BTW m2e also does the same thing. Look at AbstractJavaProjectConfigurator#addProjectSourceFolders.

In order to get the necessary information from the the MavenProject interface, I'll need to extend the MavenAndroidProject interface and add some additional methods there. I may play around with it, and see what I can get. If Maven can provide me all configured source folders and and resource folders up to that point, it should be a bit easier to support multiple src directories that don't follow the maven standard names. Particularly if they use the build-helper-plugin to define additional src directories and test directories as well as resources.

Yeah, this is what i meant exactly. Do not worry, all the source folders are provided in the maven model. If one is missing from model for some reason, it will not appear in the classpath either, so it is uninteresting for us.

WonderCsabo · 2014-09-10T18:50:02Z

About the generated source folder: it needs lot of code to obtain the location of it, since it can be created by two different plugins (maven-compiler-plugin and maven-processor-plugin), also multiple parameters can define the path of the folder. See the source of the m2e-apt plugin to get an idea how much work is needed. I think we should not reimplement this in m2e-android, but wait for m2e-apt to get a public API, so we can just retreive the path with one method call.
Until then, the generated source can wait (or we can hardcode it with the most used target/generated-sources* paths, but i am not sure).

WonderCsabo · 2014-09-10T22:44:14Z

I just realized this is a fix for #170, too (thanks @kingargyle for pointing that out). Great!

WonderCsabo · 2014-09-10T22:56:37Z

By the way, i think we should also define the location of the containers:

source folders
Android framework
Android private libraries
Android dependencies
Maven dependencies
Non-runtime Maven dependencies
the rest

Currently the order is messed up.

kingargyle · 2014-09-11T00:45:55Z

This version uses the Maven Model to reorder the source and resource directories. I haven't looked at the other directories. I also added some integration tests to make sure the order of the source/resource folders is as expected.

I'll look at adding the other requested ordering tomorrow.

WonderCsabo · 2014-09-11T09:36:31Z

...android/src/main/java/me/gladwell/eclipse/m2e/android/project/JaywayMavenAndroidProject.java

@@ -172,5 +172,9 @@ private File getConfiguredFile(String path) {
    public List<String> getSourcePaths() {
        return mavenProject.getCompileSourceRoots();
    }
+
+    public MavenProject getMavenProject() {
+    	return mavenProject;


I do not think it is nice to expose the MavenProject here, actually MavenAndroidProject is used to hide it. i think it would be cleaner to create getTestSourcePaths() etc methods in MavenAndroidProject.

I can wrap it. For the first cut I had just gone with the simpliest thing that could possibly work.

WonderCsabo · 2014-09-11T13:05:58Z

I'll look at adding the other requested ordering tomorrow.

Thanks. A little help:
Path for the Maven deps is IClasspathManager.CONTAINER_ID, Non-Runtime deps is AndroidMavenPlugin.CONTAINER_NONRUNTIME_DEPENDENCIES. Path for the Android containers are in the AdtConstants class.

kingargyle · 2014-09-11T15:19:23Z

Implemented the above suggested helper methods, and additional classpath reordering.

kingargyle · 2014-09-11T17:02:28Z

Looks like everything except for the juno build completed and passed. Let me know if I need to squash this commit or not, otherwise I think it is ready for further testing.

For those entries that are comparing path use toOSString to make sure the entry is being compared to the OS Specific string.

kingargyle · 2015-01-19T01:33:32Z

Alright this now passes and was rebased against master.

kingargyle · 2015-02-20T16:48:37Z

Bump.

rgladwell · 2015-03-07T00:31:28Z

Apologies for the delay, I've taken a break for m2e-android for a month or so to work on other things. I have some time to spend now but I still need to complete the work refactoring PR #212 before I get to this.

I did take a (very) quick look through the changesets. Excellent re-use of existing interfaces and classes (RawClasspathConfigurer and extending JaywayMavenAndroidProject) and its always good to see a PR with tests.

However, the ReorderClasspathConfigurer class seems a bit long and I'm not sure the tests are sufficient as they stand. Do we need to completely re-generate the classpath? Could we not just re-order the existing classpath? Could the Guava ordering API make the tests and configurer simpler?

kingargyle · 2015-03-11T02:14:29Z

Will take a look at ReorderClasspathConfigurer. I think I re-wrote the classpath due to the fact that I found it easier to work with, but open to other ideas.

rgladwell · 2015-03-11T11:58:24Z

Working with classpaths isn't made easy in Eclipse/m2e. What problems did you get?

kingargyle · 2015-03-11T16:06:09Z

The ReorderClasspathConfigurer ends up getting called multiple times during the m2e configuration cycle. I think it gets called about 3 times. Anyways, instead of searching for particular items, I wanted to make sure that everything was ordered a particular way and in a particular order. So I just specified it in the way that I could guarantee the less hassle to find and then move the order. Unfortunately with the andmore stuff right now I'm not sure when I'm getting back to this, probably in the next couple of weeks. working on getting things ready for M1 public release.

WonderCsabo · 2015-06-26T17:38:39Z

@kingargyle @rgladwell m2e 1.6 is killing this approach, because of the problem described in #360. You have to change the implementation to use a classpath change listener.

kingargyle · 2015-06-26T19:43:10Z

Probably will be a while before I get back to this unfortunately. Swamped with other things.

WonderCsabo · 2015-06-26T19:44:57Z

@kingargyle i guessed that, i just added that note for the future. BTW #354 partially fixes this.

rgladwell · 2015-06-28T14:14:41Z

@WonderCsabo are there any outstanding issues that #354 doesn't fix?

WonderCsabo · 2015-06-28T21:26:18Z

Yeah, this is really needed, that patch creates only a somewhat good order, and if the order is messed up (by the user or later ADT), it will not update it on Maven project update.

rgladwell · 2015-07-06T17:00:43Z

@kingargyle if this isn't progressing I'll take this over and attempt to re-factor.

kingargyle · 2015-07-07T17:33:06Z

@rgladwell please do take it over, I'm not going to have time for a while to get back to it.

rgladwell · 2015-07-09T18:48:45Z

Closed in favour of #364

rgladwell added the waffle:in progress label Aug 10, 2014

kingargyle force-pushed the reorder-classpath branch from aa697d9 to 8ed6684 Compare September 10, 2014 18:21

kingargyle changed the title ~~First cut at Reorder Classpath class to re-arrange the source folders.~~ Ensure source folders appear first in the classpath list. Sep 10, 2014

WonderCsabo reviewed Sep 10, 2014
View reviewed changes

WonderCsabo reviewed Sep 11, 2014
View reviewed changes

Make sure that paths compare the OS path string

ec6a6a2

For those entries that are comparing path use toOSString to make sure the entry is being compared to the OS Specific string.

kingargyle force-pushed the reorder-classpath branch from 44e9c52 to ec6a6a2 Compare January 19, 2015 00:51

rgladwell added waffle:in progress and removed waffle:ready labels Mar 22, 2015

rgladwell self-assigned this Mar 22, 2015

rgladwell added the Blocked label Mar 22, 2015

rgladwell force-pushed the master branch 3 times, most recently from a65fcd1 to 411c569 Compare April 6, 2015 00:29

rgladwell assigned rgladwell and unassigned rgladwell Apr 14, 2015

rgladwell mentioned this pull request Jun 30, 2015

Use mutable classpath api #354

Merged

rgladwell removed the Blocked label Jul 7, 2015

rgladwell force-pushed the master branch from 709c8c3 to 84eb15a Compare July 8, 2015 14:20

rgladwell closed this Jul 9, 2015

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


		public class ReorderClasspathConfigurer implements RawClasspathConfigurer {

		private static final String SRC_TEST_RESOURCES = "src/test/resources";

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

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

Conversation

kingargyle commented Aug 8, 2014

WonderCsabo commented Aug 8, 2014

WonderCsabo commented Aug 25, 2014

kingargyle commented Sep 5, 2014

WonderCsabo commented Sep 5, 2014

rgladwell commented Sep 5, 2014

kingargyle commented Sep 8, 2014

rgladwell commented Sep 8, 2014

kingargyle commented Sep 8, 2014

rgladwell commented Sep 8, 2014

kingargyle commented Sep 8, 2014

rgladwell commented Sep 8, 2014

WonderCsabo commented Sep 8, 2014

rgladwell commented Sep 8, 2014

WonderCsabo commented Sep 8, 2014

kingargyle commented Sep 8, 2014

rgladwell commented Sep 8, 2014

kingargyle commented Sep 8, 2014

kingargyle commented Sep 10, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WonderCsabo commented Sep 10, 2014

WonderCsabo commented Sep 10, 2014

WonderCsabo commented Sep 10, 2014

kingargyle commented Sep 11, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WonderCsabo commented Sep 11, 2014

kingargyle commented Sep 11, 2014

kingargyle commented Sep 11, 2014

kingargyle commented Jan 19, 2015

kingargyle commented Feb 20, 2015

rgladwell commented Mar 7, 2015

kingargyle commented Mar 11, 2015

rgladwell commented Mar 11, 2015

kingargyle commented Mar 11, 2015

WonderCsabo commented Jun 26, 2015

kingargyle commented Jun 26, 2015

WonderCsabo commented Jun 26, 2015

rgladwell commented Jun 28, 2015

WonderCsabo commented Jun 28, 2015

rgladwell commented Jul 6, 2015

kingargyle commented Jul 7, 2015

rgladwell commented Jul 9, 2015