Community
Participate
Working Groups
Build ID: I20090611-1540 First experienced with 3.5M7 (but I didn't try earlier milestones): Changing the "source code attachment" of one of several "Referenced Libraries" to a source code JAR will not only affect this jar but several other libraries (but not all, which is funny). Important detail here: If I select f.e. jdom-1.0.jar and goto its "Properties" page -> "Java Source Attachment" I can read there "Select the location (folder, JAR or zip) containing the source of gt-main-2.5.5.jar. (which is another library). If I change to the "Resource" section, it will, however show the correct resource, which is jdom-1.0.jar. So Eclipse obviously shuffles the libraries in the "Java Source Attachment" section. I have already removed all libs from the classpath, restarted Eclipse, re-defined the dependencies and started assigning the source files -> Same problem again. I am quite convinced this was fine in Eclipse 3.4.x Steps To Reproduce: 1. Create a new project (I choose plugin project) 2. Put several libraries (.jar files) into a subfolder of the project, e.g. "libs" 3. Open the project's manifest (PDE tools -> Open manifest) 4. In the classpath section, add the libraries They will now appear under "Referenced Libraries" as expected. So far, so good. 5. Place the source code jars for the libraries into another project subfolder, e.g. "libsrc" 6. Choose one of the libraries -> Goto "Properties" -> "Java Source Attachment" and select the corresponding source jar. 7. Confirm with OK. Now the library has its source attached, but three other libraries also get the same source attached. If I repeat step 6+7 for one of them, again the source code changes for all four, overriding the source code setting I did for the first library. Example: I set the source for jts-1.9.jar and this very source jar gets attached to the JTS, Vecmath, and three Geotools library JARs.
Cannot reproduce using R3.5 from here: http://download.eclipse.org/eclipse/downloads/drops/R-3.5-200906111540/index.php using attached demo project. If you can still reproduce the please provide more detailed / self-contained steps to reproduce.
Created attachment 140203 [details] demo project
Created attachment 140271 [details] Screenshot showing described effect in a fresh project
Reopening bug. I will try to add a test project and screenshot for you. Further information (may or maybe helpful): - This is a fresh installation of Eclipse 3.5 Classic (just after release) with only Subclipse and ResourceBundleEditor plugins added to it. - Java6u14 (latest update) is used, but this happend with Java6u10 already. - This project is just one in a workspace with over a dozend projects, some of which currently have compile time errors. Could this possibly confuse Eclipse?
Created attachment 140280 [details] Sample project (including libs) showing the library source mixup. I have done a further test: I created a blank workspace and created the project there (from scratch, again). Attached you find this brand new workspace with the brand new project including the libraries I added to it and including the described bug. A maybe related detail that caught my attention: The libraries seem to have no specific order when shown under "Referenced libraries" (see provided screenshot). It is definitely not the order at which they appear in the "Classpath" section of the manifest editor. So what else determines their order?
Thanks for the example Matthias. I see the problem now: some of the JARs are twice on the build path because we have to automatically add the JARs that are referenced from a JARs MANIFEST.MF. For example geoapi-2.2.0.jar has this entry: Class-Path: jsr-275-1.0-beta-2.jarThe The code that handles source attachment seems to be broken for such automatically added JARs. I'll have to investigate whether this is on the UI or Core side.
JDT Core returns/creates an additional classpath entry for the original raw entry and this additional entry has the same attachments set as the raw entry (which it probably shouldn't). We need support from JDT Core to set/modify the attachments for the additional entry e.g. via extra attributes. Question: what is the correct code to detect such a generated entry? For now we need to disable the UI for those.
*** Bug 283502 has been marked as a duplicate of this bug. ***
It appears that when a jar in the rawClasspath refers to another jar (which is also in the rawClasspath) via the jar's manifest, there ia a chance that PerProjectInfo.rootPathToRawEntries contains the mapping between the latter's path and the first's IClasspathEntry. For e.g. both first.jar and second.jar are added to the project and first.jar refers to second.jar in the manifest's Class-path. Then the rootPathToRawEntries might have something like this: first.jar => first.jar second.jar => first.jar Fixing this should address the bug reported.
>Fixing this should address the bug reported. This will only make sure that the additional JAR doesn't get the wrong attachment. However, we also need a way (i.e. API) to set/modify the attachments for the additional entry e.g. via extra attributes.
May be we can keep these as two separate bugs: One for not creating additional classpath entries when the chained library is also part of the raw classpath. And the second for the API to represent the chained entries and source attachment for them. Bug 252431 seems to have some details with respect to this already. Perhaps could we use that for the API part?
Bug 252431 is unrelated. It's really this bug here that needs to be addressed. The attachment of the additional is wrong because it has no own slot and API to set that slot.
Would you have some suggestions as what you expect to get?
>Would you have some suggestions as what you expect to get? As a user I'd expect to set the Javadoc location for the automatically added JARs and as a JDT UI developer I expect to get API to do that.
Dani Megert wrote: > I see the problem now: some of the JARs are > twice on the build path because we have to > automatically add the JARs that are > referenced from a JARs MANIFEST.MF. > For example geoapi-2.2.0.jar has this entry: > Class-Path: jsr-275-1.0-beta-2.jar Sorry, seems I misunderstood your explaination until today. I thought you meant that libs are automatically added to Java build path when they are added using the OSGI manifest editor. But of course, this is completely independent from OSGI. You meant that one library added to the Java classpath requires other jars to be on the classpath and obviously since Eclipse 3.5 they are automatically added as well. Funny ... obviously this is meant to be a new feature but I consider i a bug! I have this opinion not only because of the source code attachment problem but also because this "automagic" behaviour has nasty side effects. Just yesterday I wanted to remove a single library from a build path (in order to replace it by another version), but half of the other libraries - obviously its dependencies - were also removed! This is inacceptable imho. Hum, is there any possibility to switch this "feature" off altogether? Otherwise I have to switch back to v3.4.1 because I simply cannot get my work done when Eclipse patronizes me on every addition/removal of build path changes and I have no access to the source code. (Sorry for this long comment.)
>Funny ... obviously this is meant to be a new feature but I consider i a bug! >I have this opinion not only because of the source code attachment problem but <also because this "automagic" behaviour has nasty side effects Please file a separate bug with that. >Hum, is there any possibility to switch this "feature" off altogether? No. >Otherwise I have to switch back to v3.4.1 because I simply cannot get my work >done If it is really blocking you (except for the attachment) then please file the bug with steps that break you compared to 3.4.x.
Added my vote -- this has been a big headache for me since moving to 3.5. Basically, my scenario is I have an Eclipse plug-in that has a bunch of jars under "Referenced Libraries"; some of them have the source in, let's call it, project1, and others have their source in project2. The only way I can debug is, as I step from one jar to the other, to repeatedly click the "attach source" button and change to "/project1", then back to "/project2", and so on. And interestingly, the dialog prompting me for the source often is displaying the wrong jar name, indicating that it seems to be confused. I haven't read through all the notes yet; I'll do so and see if there is anything I can add. And I'll see if I can make a small repro case that works in 3.4 but not in 3.5 (it sounds like you don't have one yet?).
I see now, it sounds like you *do* have a repro case for the source attachment problem, right? If I'm reading correctly, I think the following comment by Dani about filing steps that break compared to 3.4.x was in reference to Matthias's complaints about the automatic modifications to the class path, not in reference to the source attachment problem: Dani wrote: >Matthias wrote: >>Otherwise I have to switch back to v3.4.1 because I simply cannot get my work >>done >If it is really blocking you (except for the attachment) then please file the >bug with steps that break you compared to 3.4.x.
@Mike Morearty Correctly understood. P.S. I have, in the meantime, opened a second ticket (issue 288700) that covers the mentioned, more general problem of Eclipse 3.5 compomising my attempts to create a specific build path that's different from what Eclipse thinks is right. I think you can reproduce both problems with the sample project.
Even though the referenced libraries don't have a way to have their own source attachment and other attributes, what is really causing this bug is little different. When a particular library is part of the raw classpath and also being referred by another library (which is also part of the raw classpath), there is no need to add the chained library once again to the resolved classpath. However, we end up doing this and in the end overwriting whatever attributes the original classpath entry might have had. I will keep this bug to fix this wrong behavior and use the bug #252431 for the API changes. Olivier, please let me know if this alright.
Created attachment 150497 [details] Proposed Fix Olivier, please review this patch. You may note that I am currently going through all CPE_LIB entries in the raw classpath in order to find whether or not a particular jar is already present as a raw classpath. If there is a better way, please let me know. JDT/Core tests and JDT/UI tests pass with this patch.
It makes sense that the same entry should not be added twice. The first one should win. The jars referenced inside other jars should be added at the right location in the classpath as this mimic the runtime classpath. So making sure they don't override existing entries is one part of the complete fix. We should also make sure that they can get their own attributes for javadoc and source attachments.
I am not sure however that checking the raw classpath is enough. Don't you also need to make sure that both entries don't resolve to the same entry ?
Please review my comments and let me know what you think.
(In reply to comment #22) > The jars referenced inside other jars should be added at the right > location in the classpath as this mimic the runtime classpath. > So making sure they don't override existing entries is one part of the complete > fix. The fix ensures that the main jars (not the chained ones) get added to the resolved classpath first by making sure that the chained ones don't get added to the resolved classpath at all. In the case where the same jar is referred by two main jars (without being present in the raw classpath by itself), nothing has changed. We should also make sure that they can get their own attributes for > javadoc and source attachments. This requires API changes and will be supported in the bug #252431. (In reply to comment #23) > I am not sure however that checking the raw classpath is enough. Don't you also > need to make sure that both entries don't resolve to the same entry ? I see the point. We will have to take care of such scenarios too.
*** Bug 295993 has been marked as a duplicate of this bug. ***
Targeting 3.6M5
Moving to 3.6M6 as this will be done with the API change required to support this.
A patch with changes that are prerequisite to this bug has been added here - bug 252431 comment 20. The JDT/UI code uses IPackageFragmentRoot#getRawClasspathEntry, which returns the reference to the raw entry and not the resolved (chained) entry and hence the chained library doesn't hold it's own source attachment path and other attributes.
Discussed this with Olivier, Jay, and Dani. The solution that assumes that a referenced JAR has just one parent (IClasspathEntry#getReferencingEntry()) is good for telling the user why a referenced JAR ended up on the resolved classpath at all. But in the .classpath file and on the Build Path properties page, it becomes difficult to manage if e.g. a referenced JAR gets a new parent because its old parent disappeared. The graph of referenced JARs is not necessarily a tree, it's an arbitrary graph. We should not pretend it was a tree. An easier model is to have just a flat list of entries that carry the additional information about the referenced entries: - In .classpath, that would look like this, with a new element type <referencedentry>: <classpath> <classpathentry kind="lib" path="lib/LibWithReference.jar" sourcepath="/LibWithReference_src.jar"/> <referencedentry kind="lib" path="lib/Referenced.jar" sourcepath="/Referenced_src.jar/> </classpath> - IJavaProject#getRawClasspath() should not return the new <referencedentry> elements, but IJavaProject#getResolvedClasspath(boolean) should use the information from there and set it to the resolved IClasspathEntries for the referenced JARs. - In JavaCore, we need a new API: IClasspathEntry[] getReferencedClasspathEntries(IClasspathEntry libraryEntry) => Takes a library entry and returns entries for all JARs that are referenced by the given JAR (including all transitively referenced JARs). The UI will use this to create entries for the referenced JARs, similar to how we already do it today for classpath containers via JavaCore.getClasspathContainer(..).getClasspathEntries(). - In IJavaProject, we need a new API: void setRawClasspath(IClasspathEntry[] entries, IClasspathEntry[] referencedEntries, IPath outputLocation, IProgressMonitor monitor) => Should work like setRawClasspath(IClasspathEntry[], IPath, IProgressMonitor), but it should also store the referencedEntries as <referencedentry> in the .classpath file. We don't need support for the boolean canModifyResources from other setRawClasspath(..) methods. For compatibility with old clients, the old setRawClasspath(..) APIs should not touch the <referencedentry> elements in the .classpath file. The new API could specify that passing null for referencedEntries will keep the existing entries and passing an empty array will clear them. - In IPackageFragmentRoot, we need new API: IClasspathEntry getResolvedClasspathEntry() => Should return the resolved classpath entry that corresponds to the package fragment root, see bug 252431 comment 42. On the returned IClasspathEntry, #getReferencingEntry() should return one of the parents, or null if it's not a referenced library. - IClasspathEntry#getReferencingEntry() is nice, but its Javadoc should tell that it returns just one of the possibly multiple parents, and that it returns null for a library that is itself on the raw classpath. - IClasspathEntry#getReferencedEntries() should be removed.
Sounds good, Markus. One concern, which is more of an implementation problem, is once we read the .classpath, we have no way of differentiating between a raw classpath entry and a referenced entries, since we decided to use the same entry 'kind'. Perhaps we could use a flag in the ClasspathEntry to do that. Or the methods (not public API) need to return a two dimensional array (IClasspathEntry[2][]) , where the first array will be the rawClasspathEntry while the second will be the referenced entries array. The problems with the latter is i) the method signatures change, which means any (external) code using these methods will be broken. ii) I don't quite like the way the code looks when I have to retrieve the raw classpath array and referenced entries array. But then there are issues with the first approach also. Nevertheless, I don't see any major issues with the proposed API. I will post the first patch by the end of the day.
> we have no way of differentiating between a raw >classpath entry and a referenced entries getReferencingEntry() can do that job, can't it?
(In reply to comment #30) > - In JavaCore, we need a new API: > IClasspathEntry[] getReferencedClasspathEntries(IClasspathEntry libraryEntry) > => Takes a library entry and returns entries for all JARs that are referenced > by the given JAR (including all transitively referenced JARs). > The UI will use this to create entries for the referenced JARs, similar to how > we already do it today for classpath containers via > JavaCore.getClasspathContainer(..).getClasspathEntries(). > Let's say both lib1.jar and lib2.jar reference lib3.jar and calling JavaCore#getReferencedClasspathEntries for both lib1 and lib2 as arguments, can one say it returns the same instance of lib3. For example, if lib1.jar appears first in the raw classpath, lib3.getReferencingEntry() will always return lib1. (In reply to comment #32) > getReferencingEntry() can do that job, can't it? Not really. We will have those relationships created only after the classpath has been resolved. Right after the .classpath has been read and before the classpath has been resolved, there is a lot of things that happen internally, during which we need to make sure that they don't get mixed up.
> Let's say both lib1.jar and lib2.jar reference lib3.jar and calling > JavaCore#getReferencedClasspathEntries for both lib1 and lib2 as arguments, can > one say it returns the same instance of lib3. For example, if lib1.jar appears > first in the raw classpath, lib3.getReferencingEntry() will always return lib1. I think getReferencingEntry() is not that important in this scenario. As discussed in chat with Jay, JavaCore#getReferencedClasspathEntries(..) needs a second parameter 'IJavaProject project' so that the attributes of the referenced entries can be filled in from the current classpath of the given project. But getReferencedClasspathEntries(..) should always return entries for all JARs that are referenced, not only the ones that are not yet on the raw classpath. When the UI calls this API, you don't know yet how the final classpath will look like, so getReferencingEntry() cannot always be correct (one of the referenced entries may end up on the classpath itself, and then getReferencingEntry() will return null). I'd say getReferencingEntry() is undefined for results from getReferencedClasspathEntries(..). It's also not important whether the returned entries from multiple calls are the identical object or just equals(..) to each other. What's important though, is that ClasspathEntry#equals(Object) does not consider the referencingEntry, so e.g. an entry with a non-null getReferencingEntry() is equal to an entry a client created via JavaCore#newLibraryEntry(..) with the proper arguments). getReferencingEntry() should only be well-defined for entries that come from IJavaProject#getResolvedClasspath(..) or the new IPackageFragmentRoot#getResolvedClasspathEntry().
Created attachment 160194 [details] Patch with new API Attaching the patch with the new API as per previous comments. The documentation may not be perfect and couple more tests might be added. Wanted to post anyway so that Markus can have an early look at it and confirm that we haven't overlooked anything. Markus, please let me know all your comments, including those about documentation. Also please take a look at the tests ClasspathTests#testBug252341a and testBug252341b (test names might change) and tell me if the scenarios being tested are close to what we will be doing from the UI.
APIs looks good (as requested). The Javadocs all need some language and spelling cleanup. E.g. JAR should be spelled all-caps, MANIFEST should get ".MF" at the end. I started with implementing the UI, and hit a barrier: When I want to set e.g. a source attachment to a referenced entry, the only way to do this is via IJavaProject#setRawClasspath(..). There, I need to pass all the referencedEntries. But I don't have a good way to get hold of the current referencedEntries so that I can just replace the one I need. I would have to call getResolvedClasspath(boolean), then get the referenced classpath entries, and then throw away all entries that are already on the raw classpath. Could we simplify this with another API IJavaProject#getReferencedClasspathEntries() that returns all currently stored referenced entries? Tests also look good. In the first test, you could add an assertion that verifies that a referenced IClasspathEntry is equal to an independent one that has been created via Javacore#newLibraryEntry(..) (although their getReferencingEntry() methods return different results). In the second test, you could add a source attachment to lib3 and lib5, to ensure that this info is not lost when lib3 changes its referencing entry.
Created attachment 160304 [details] Updated patch Alright, here we go! As per chat discussion with Markus, here is what kind of validations IJavaProject#setRawClasspath() does. If there are duplicates of a referenced entry or if any of the referencedEntries is already present in the raw classpath those referenced entries will be excluded and will not be persisted. More importantly, if any of the referenced entries are NOT in the resolved classpath, the referenced entries still NOT removed from the list. Despite the risk of ending with some garbage in the .classpath, we need to avoid data loss (e.g. when the user changes the classpath while another JAR on the classpath is not available) One more test added in ClasspathTests#testBug252341c()
(In reply to comment #37) > Created an attachment (id=160304) [details] [diff] Looks good, I already have the Properties pages on the referenced entries working. I still have some Javadoc nitpicking here and there, but I guess it's easier if you just release it when Olivier has made his pass, and then I make a patch with my suggested tweaks. I did not look at the implementation and only did limited testing, but the updates in the .classpath file and API behavior looked fine so far.
I wonder if the access to the Map rootPathToResolvedEntries = project.getPerProjectInfo().rootPathToResolvedEntries; inside org.eclipse.jdt.internal.core.PackageFragmentRoot#getResolvedClasspathEntry is thread-safe. rootPathToResolvedEntries is assigned inside synchronized code. So would it be better to use an access method that is also synchronized?
Created attachment 160356 [details] Updated patch Updated patch to fix missing @since tags and update copyrights for some types. The patch looks good beside my previous comment.
(In reply to comment #40) > Created an attachment (id=160356) [details] > Updated patch > > Updated patch to fix missing @since tags and update copyrights for some types. > The patch looks good beside my previous comment. Thanks Olivier. And I do agree with your comment (comment # 39). In fact the patch uses this in another place as well - JavaModelManager#getReferencedClasspathEntries(IClasspathEntry, IJavaProject) and there are already few other places where an un-synchronized access to PerProjectInfo#rootPathToResolvedEntries and PerProjectInfo#rootPathToRawEntries is made. PackageFragmentRoot#getRawClasspathEntry itself is one such example. However, I think, the PerProjectInfo#setClasspath is invoked during classpath resolution, which may be already be blocking the other UI operations (like the two I just mentioned). But, with my limited knowledge in this area, I might be wrong.
(In reply to comment #41) > However, I think, the PerProjectInfo#setClasspath is invoked during classpath > resolution, which may be already be blocking the other UI operations (like the > two I just mentioned). But, with my limited knowledge in this area, I might be > wrong. Then I think we can go ahead, but I would like that you review the synchronization of this variable. This can be done post M6. We need to make sure that we are not causing deadlocks or that we don't return invalid or stale values for the classpath entries.
(In reply to comment #42) > we can go ahead, but I would like that you review the > synchronization of this variable. This can be done post M6. We need to make > sure that we are not causing deadlocks or that we don't return invalid or stale > values for the classpath entries. I have raised a new bug 304450 to track this.
The API part of the fix has already been released as part of bug 252431. The UI changes are being tracked under bug 304083. Hence closing this bug.
Verified for 3.6M6 using build I20100305-101
Verified.
