Community
Participate
Working Groups
Build Identifier: I20100608-0911 The method call "javaProject.findPackageFragmentRoots(cpe)" returns an empty list when cpe is of kind CPE_PROJECT and exists in javaProject's raw classpath. This occurs even when the project referenced by cpe has source or exported package fragment roots. Reproducible: Always Steps to Reproduce: Call javaProject.findPackageFragmentRoots(cpe) where cpe is of kind CPE_PROJECT and the referenced project has source or exported package fragment roots. See the attachments for an Eclipse plug-in with a JUnit test case to replicate this problem.
Created attachment 178116 [details] Eclipse plug-in to replicate bug Import the project org.eclipse.bugs in the archive to your workspace and run the class TestFindPackageFragmentRoots as a 'JUnit Plug-in Test.'
I will see what's happening.
Created attachment 178134 [details] Proposed patch The problem seems to arise from the assumption that a required project entry in the classpath should also be marked as 'exported'. But I am not able to see this specified anywhere in the API docs. As per my understanding, the required projects' source projects are automatically chained and only the library, container and variable entries that need explicit exporting. The attached patch fixes this in the area that is causing this bug. Tests to be added.
May be I was wrong. As per the following API org.eclipse.jdt.core.JavaCore.newProjectEntry(IPath, IAccessRule[], boolean, IClasspathAttribute[], boolean) it would mean that even project entries need to be explicitly exported in order to be available for the dependent projects.
(In reply to comment #4) > May be I was wrong. As per the following API > > org.eclipse.jdt.core.JavaCore.newProjectEntry(IPath, IAccessRule[], boolean, > IClasspathAttribute[], boolean isExported) > > it would mean that even project entries need to be explicitly exported in order > to be available for the dependent projects. The term 'dependent projects' in the documentation seem confusing. Here are some fragments from the Javadoc. "A project entry is used to denote a prerequisite project on a classpath." ... "The isExported flag indicates whether this entry is contributed to dependent projects. If not exported, dependent projects will not see any of the classes from this entry. If exported, dependent projects will concatenate the accessible files patterns of this entry with the accessible files patterns of the projects, and they will concatenate the non accessible files patterns of this entry with the non accessible files patterns of the project. " After going through this again, it seems to me that in the context of project references, it means the 'dependent project' is the project (A) that is referencing the project (B) that includes this project entry (C) in it's classpath. For example, Project A depends on Project B. Project B has Project C as a project entry it's classpath. Here, in the case Project C as a project entry, A is the 'dependent project'.
Created attachment 178248 [details] Updated patch with test This patch should be good enough. The patch also includes the test. pesckal, can you please see if this patch address your problem and if the new test is close enough to the scenario you described?
(In reply to comment #6) > Created an attachment (id=178248) [details] > Updated patch with test > > This patch should be good enough. The patch also includes the test. > > pesckal, can you please see if this patch address your problem and if the new > test is close enough to the scenario you described? Perfect! The method now works the way the API made me expect. The test also looks good. I think it could have another case to check that source package fragment roots are also returned by findPackageFragmentRoots though (just add a source PFR to referencedProject and check that the length of the returned array is two.)
Created attachment 179224 [details] Patch with updated test Patch contains updated test.
Markus, This patch fixes one of the APIs. Could you please review this and see if the changes are in line with the current API specifications?
> This patch fixes one of the APIs. Could you please review this and see if the > changes are in line with the current API specifications? Looks good.
Released in HEAD for 3.7M3.
This causes a major issues for the PDE container which now shows unexpected entries: 1. start new workspace 2. import 'org.eclipse.jface.text' as source 3. import 'org.eclipse.ui.editors' as source 4. expand the PDE container The changed method was not including the exported roots since it got added in 2.1, hence when changing such a fundamental thing you should check at least all clients inside the SDK to make sure the change does not break them. In this case also clients outside the SDK (e.g. WTP) might be broken. I suggest to revert the change and adjust the Javadoc. NOTE 1: you also need to change the Javadoc of getPackageFragmentRoots(ICPE) NOTE 2: the bug number in the CVS comment of the change seems wrong
.
Dani, I agree I should have anticipated the breakages with this change. But as reported, the API in question, includes neither the exported fragments nor the source entries. The documentation states in several places that the source entries should be automatically included while the other types will be included if 'exported'. I also agree this change might have affected more than just eclipse projects. My question, though is, can we say that 'having additional unexpected entries' is better than 'not having some of the expected entries' and make a case for this? If you don't agree, I will post the reverse patch and updated docs.
>is better than 'not having some of the expected entries' and make a case for >this? It worked like that since 2.1, so yes adjusting the doc is better than changing the behavior (and also add potential performance issues by returning more nodes).
(In reply to comment #15) > >is better than 'not having some of the expected entries' and make a case for > >this? > It worked like that since 2.1, so yes adjusting the doc is better than changing > the behavior (and also add potential performance issues by returning more > nodes). Should I just say in the two APIs' documentation that if the classpath entry is a project, we return an empty list? This way, we may not have to touch the other places where we talk about 'exported'.
> Should I just say in the two APIs' documentation > that if the classpath entry is a project, we return an empty list? This way, we > may not have to touch the other places where we talk about 'exported'. I guess we can simply remove the "Note..." part. If there is real/proven need for the currently specified behavior then we might consider adding a new method with an additional Boolean parameter and deprecate the existing method. Please discuss and decide this (incl. Javadoc change) together with Markus.
(In reply to comment #17) > If there is real/proven need for the currently specified behavior then we might > consider adding a new method with an additional Boolean parameter and deprecate > the existing method. > > Please discuss and decide this (incl. Javadoc change) together with Markus. I am interested in the new API idea. This way, we won't break the existing clients and also we don't have to change the doc to suit the behavior. Markus, will it be too late for a new API?
> I am interested in the new API idea. This way, we won't break the existing > clients and also we don't have to change the doc to suit the behavior. Well, the current method can't be changed and since it is wrong we have to change the doc. > Markus, > will it be too late for a new API? API freeze is with M6 (in March).
Fix made has been reverted. We will keep this bug for API changes or document changes, whichever is the outcome of further discussions.
> I am interested in the new API idea. This way, we won't break the existing > clients and also we don't have to change the doc to suit the behavior. You're contradicting yourself here: Either we break existing clients, or we change the Javadoc of the existing method. We will do the latter, since this bug has been in place forever. pesckal, do you have a real need for a new API IJavaProject# findPackageFragmentRoots(IClasspathEntry entry, boolean includeSourceFolders)? If yes, we can add this (with the implementation that was in 3.7M3), although I don't see any places where we would use this in the SDK.
(In reply to comment #21) > You're contradicting yourself here: Either we break existing clients, or we > change the Javadoc of the existing method. We will do the latter, since this > bug has been in place forever. I overlooked the fact that the new API will not be used by the SDK but by other clients that may want this behavior.
By the way, IJavaProject#getAllPackageFragmentRoots() does consider the exported classpath entries in the referenced project.
*** Bug 330765 has been marked as a duplicate of this bug. ***
Created attachment 184336 [details] A test on API behavior This patch has tests the behavior of several API methods relevant to exported entries. There are five tests totally and all of them currently pass. The first two APIs (IJavaProject#isOnClasspath(IResource) and IJavaProject#getAllPackageFragmentRoots()) consider the exported classpath entries from referenced projects while others don't. I think instead of removing the doc about the 'exported' itself, we would be better of just adding this to the concerned APIs alone, just like in IJavaProject#getPackageFragmentRoots, which has the following line: "The result does not include package fragment roots in other projects referenced on this project's classpath." Markus, what do you think?
Created attachment 184342 [details] Updated doc patch I have added similar documentation as noted in comment #25 to the following APIs: IJavaProject#findPackageFragmentRoots(IClasspathEntry) IJavaProject#getPackageFragmentRoots(IClasspathEntry)
(In reply to comment #26) > Created an attachment (id=184342) [details] [diff] > Updated doc patch The added paragraph looks good, but you also have to remove the wrong parts of the second sentence: * Note that a classpath entry that refers to another project may * have more than one root (if that project has more than on root * containing source), and classpath entries within the current * project identify a single root. E.g. replace with: * A classpath entry within the current project identifies a single root.
Created attachment 184524 [details] Updated documentation This patch incorporates the suggestions from Markus.
Released the documentation changes in HEAD for 3.7M4.
Verified for 3.7M4 using build I20101205-2000 looking at the Javadoc
