Community
Participate
Working Groups
When jars are referenced in the Class-Path: entry, they are all displayed using the name of the jar that referenced them. This makes it really difficult to: - know the names of the jars referenced inside the Class-Path: clause - distinguish them in the package explorer. A new API might be needed to let JDT/UI provide a good display of these jar entries.
They all relate to the same original raw entry which can be used for grouping purpose (exactly as it is done with containers).
moving to jdt/ui for further comments. I think it is a good idea
There's already a corresponding bug in JDT UI (bug 252432). This one is for JDT Core to provide JDT UI with the API. Olivier, can you give more details about the JDT Core API you had in mind?
Personnaly I am expecting to see: 1) one entry corresponding to the jar that has the Class-Path: clause 2) one child to this entry for each jar referenced on the Class-Path: clause, with the name corresponding to the referenced jar 3) subchild if required (if a referenced jar also has a Class-Path: clause) 4) an entry tagged with a red X is one referenced jar is missing I hope to be clear enough in my explanation. With such a display, it would be easy for the user: - to know what jars are pulled from the Class-Path: clauses - to know what jars are missing if any - to know the contents of each jar
For the point (4), maybe not a red X as this should refer to a marker and I don't think this should be a buildpath error. Maybe a disabled entry would be sufficient? Anything that could visually make it clear that the jar is missing.
I don't think we do this for containers anyway. I suspect that most of what is required is available from the original raw entry API.
Note that there are several problems reported here: 1. When jars are referenced in the Class-Path: entry, they are all displayed using the name of the jar that referenced them (comment 0) => This sounds like a bug since most of the time, they are displayed with a different name, but Olivier and I saw the problem. I need to investigate. I will open a new bug once I have steps. 2. Olivier would also like to have a tree of jars (points (1) (2) and (3) from comment 4). This is what would require a new api. We can keep this bug to discuss this. 3. A way to show that the jar is present or not (point (4) from comment 4). No new API is needed. This is a JDT/UI decision (whether they show IPackageFragmentRoot that exists() or not). Olivier you should open a separate bug against JDT/UI for this.
For me (4) is part of what I expect from JDT/UI for bug 252432.
Whatever we do here needs to be consistent with support for classpath containers, basically a JAR can now act as a container. In particular if we are to show ghost jars (referenced but not existing), the same should occur for containers. If we provide an API representing the JAR tree structure, it should be the same API for containers (once they support nesting).
(In reply to comment #7) > Note that there are several problems reported here: > > 1. When jars are referenced in the Class-Path: entry, they are all displayed > using > the name of the jar that referenced them (comment 0) > => This sounds like a bug since most of the time, they are displayed with a > different name, but Olivier and I saw the problem. I need to investigate. I > will open a new bug once I have steps. Entered bug 252591
Jay, please investigate.
(In reply to comment #4) > Personnaly I am expecting to see: > 1) one entry corresponding to the jar that has the Class-Path: clause > 2) one child to this entry for each jar referenced on the Class-Path: clause, > with the name corresponding to the referenced jar > 3) subchild if required (if a referenced jar also has a Class-Path: clause) Considering these and the bug 283806, we would have to persist the structure (not a requirement in itself) and the source for referenced jars somewhere. As of now the classpath file contains both the information. However, currently no entry exists here for the referenced jars and hence no sourcepath info as well. Besides, the classpath entries are written flat and hence may have to be modified to support tree like structure. However one of the issues here would be keeping this structure and the one obtained from the MANIFEST in sync. Your thoughts on this please.
Olivier, Please refer to bug #288700, which contains few other scenarios that need around the referenced libraries that may have to be fixed. And apparently there are other similar bugs too (like bug 283806, bug 281551). Do you think we need to put these all together or something?
This is the proposed change to the API (interface IClasspathEntry) /** * Returns the set of classpath entries that are referenced by this * entry. Typically this will be the referenced Jars in the manifest * Class-Path clause. Returns empty if no entries are found from the * manifest or if this entry is not a Jar. * * @return array of IClasspathEntry resolved from the manifest's * Class-Path. */ IClasspathEntry[] getReferencedEntries(); This should take care of the following: 1. Identify/resolve the referenced jars and use them in the UI, in a tree-like structure 2. To be able to attach source for the referenced jars. Of course, this will require additional changes for persisting and retrieving the new structure, which I am still working on.
(In reply to comment #13) > Please refer to bug #288700, which contains few other scenarios that need > around the referenced libraries that may have to be fixed. And apparently there > are other similar bugs too (like bug 283806, bug 281551). Do you think we need > to put these all together or something? This API needs to address all the bugs mentioned above. Once the UI can identify what entries correspond to a specific entry they might provide a way to disable those without disabling all of them. I think having this API is a first step in the right direction. We need to improve the cases reported in bug 288700, bug 283806 and bug 281551. Daniel any comment ?
Jay, Please try to use classpath attributes to persist information for: - the source attachment - the javadoc attachment - the fact that the classpath entry is a nested entry - the fact that the classpath entry is included on the classpath. We need to be able to: - attach a different source attachment than the main jar - attach a different javadoc attachment than the main jar - detect in the UI that this entry corresponds to a nested jar - know that the corresponding entry is enabled on the classpath. Using classpath attributes, the corresponding information can be persisted through sessions. Also this makes it easier to handle copy of classpath entries. We might want an extra attribute to know from what main jar a nested jar is coming from. Daniel, please comment if I missed anything.
>Daniel, please comment if I missed anything. >- know that the corresponding entry is enabled on the classpath. I think it's good enough to allow disabling the whole feature on the raw entry at once i.e. we don't need to allow this per generated/artificial entry.
(In reply to comment #16) > Using classpath attributes, the corresponding information can be persisted > through sessions. > Also this makes it easier to handle copy of classpath entries. > > We might want an extra attribute to know from what main jar a nested jar is > coming from. > We can have scenarios where more than one jar can refer to the same library, like this one here - bug 281551 comment 5. If we choose to use an attribute, we may not be able to represent such references in the UI tree. I might be wrong if it's possible to have multiple attribute values for the same key.
Not sure I understood the previous comment. We are not planning to show nested JARs if that's what you meant.
Created attachment 157728 [details] Proposed Patch Patch includes the API tests and required implementation for the same and few tests. More tests to follow. The patch also includes some JDT/UI changes - I used them to test the front end part of the feature. We can keep them if the changes are appropriate. Olivier/Daniel, Please review the patch.
Created attachment 157806 [details] Updated patch I added missing @since tags. Some new methods in JavaCore need to be be commented. The javadoc of org.eclipse.jdt.core.IClasspathEntry must be updated to reflect the new method on JavaCore. I am worried a bit about the method org.eclipse.jdt.internal.core.JavaProject.isLibraryInRawClasspath(IClasspathEntry[], IPath) that is a loop and it is called in another loop. I wonder if there is not a more efficient way to detect existing entries on the classpath. Why don't we directly check the resolved classpath when looking for this org.eclipse.jdt.internal.core.JavaProject.isLibraryInRawClasspath(IClasspathEntry[], IPath)? We compare resolved entries against raw entries. I removed a javacore dump inside the tests. I also updated the tests so that the patch can be applied. I had trouble to apply the previous patch. Jay, please commment on this before Daniel looks at it.
(In reply to comment #21) > Created an attachment (id=157806) [details] > Updated patch >... org.eclipse.jdt.internal.core.JavaProject.isLibraryInRawClasspath(IClasspathEntry[], > IPath) that is a loop and it is called in another loop. > I wonder if there is not a more efficient way to detect existing entries on the > classpath. > > Why don't we directly check the resolved classpath when looking for this > org.eclipse.jdt.internal.core.JavaProject.isLibraryInRawClasspath(IClasspathEntry[], > IPath)? We compare resolved entries against raw entries. Well, I wasn't completely convinced myself with that approach. However, we have to ensure that a particular entry is not part of the raw classpath before processing that as a chained entry and adding it to the resolved classpath. The other alternative I thought about was to go ahead and add the chained library to the resolved classpath and to the rawReverseMap. Later on when if we come across a raw entry which resolves to the same path, we will replace the earlier ones from both resolvedEntries and rawReverseMap. However, I wasn't very sure if that was any better. I will work on the other points and post an updated patch soon.
Created attachment 158142 [details] Updated patch Updated the patch incorporating the suggestions given by Olivier. Few points to mention: 1. The patch persists/supports only two attributes for the chained libraries - sourceAttachmentPath and sourceAttachmentRootPath. Please tell me if we intend to persist more. 2. I have added references to one of the JavaCore#newLibraryEntry methods in IClasspathEntry - this is the one that has the complete set of parameters. There are other overloaded methods as well. Do we need references for them as well? 3. The new code you see (JavaProject#populateChainedEntries) is required to satisfy the rules mentioned in IClasspathEntry#getChainedEntries. This patch doesn't include the nested for loops. However, it does make use of Maps and Sets to ensure the right attachment of chains and order. I might include more tests and intend to run few performance tests. Will post the results shortly.
(In reply to comment #23) > Updated the patch incorporating the suggestions given by Olivier. Few points to > mention: > 1. The patch persists/supports only two attributes for the chained libraries - > sourceAttachmentPath and sourceAttachmentRootPath. Please tell me if we intend > to persist more. I would say the same thing should be done for javadoc attachment. > 2. I have added references to one of the JavaCore#newLibraryEntry methods in > IClasspathEntry - this is the one that has the complete set of parameters. > There are other overloaded methods as well. Do we need references for them as > well? I think this is good enough. > 3. The new code you see (JavaProject#populateChainedEntries) is required to > satisfy the rules mentioned in IClasspathEntry#getChainedEntries. This patch > doesn't include the nested for loops. However, it does make use of Maps and > Sets to ensure the right attachment of chains and order. > I might include more tests and intend to run few performance tests. Will post > the results shortly. Please do so. Patch looks good so far. We need to make sure this is not too costly. I'll finish the review tomorrow. Daniel, you can start to look if this is good enough for the UI side. Thanks.
Created attachment 158444 [details] Patch with performance test This patch has the following updates: 1) Support for javadoc location (and to extra attributes in general) for the chained libraries 2) Changes to JavaProject#resolveClasspath() - the previous patch results in quite a few HashMap additions and removal in certain cases. That part of the code has been changed - somewhat similar to the approach in the first patch but the nested for loop has been replaced with maps. - The code that sets the chainedEntries has been moved away and put inside a synchronized block to take care of the multi-thread situations. 3) More tests scenarios have been added to ClasspathTests. 4) Performance test has been added Other than these, the rest of the changes still remain intact as in the previous patch. I will post the performance results with this patch and without (i.e. HEAD) in a bit. Dani, you will notice that I have removed the JDT/UI related changes - it has not been fully tested. Please let me know about the patch, particularly the API part.
Created attachment 158445 [details] Performance test Attaching the performance test results with and without the patch. Test used: FullSourceWorkspaceModelTests#testProjectResolveClasspath
Fred, can you please look at the performance results comparison and see if the results are acceptable? The 'Elapsed Process' and 'CPU Time' seem comparable. I am not very sure about the other ones, though.
(In reply to comment #27) > Fred, can you please look at the performance results comparison and see if the > results are acceptable? The 'Elapsed Process' and 'CPU Time' seem comparable. I > am not very sure about the other ones, though. That the only numbers we look at for performance results. The most important is the 'Elapsed Process Time' one and I agree it looks the same with or without the patch. Hence, I'd say the patch has no performance impact. Note that you also need to create a patch in the perf_35x branch in order to add your test into the performance baseline run and so, have reference numbers from the 3.5 version...
We should not use "chained" which introduces a new term which hasn't been used in JDT so far and which is also not mentioned in the manifest spec. There it is called "Download Extensions". I suggest we stick to that term. Q: how does JDT Core deal with that part of the spec: " Extensions may reference each other in the same way. "? The getter methods should mention null in the @return comment. I'm not yet convinced that we need the other APIs (see comment in bug 288700).
> We should not use "chained" which introduces a new term which hasn't been used > in JDT so far and which is also not mentioned in the manifest spec. The manifest spec that is linked from the Javadoc of java.util.jar.Manifest ( http://java.sun.com/javase/6/docs/technotes/guides/jar/jar.html ) just says "extensions or libraries that this application or extension needs". > There it is called "Download Extensions". I suggest we stick to that term. That's from a book that explains the mechanism in the context of applets, see http://java.sun.com/developer/Books/javaprogramming/JAR/basics/manifest.html . I discussed this with Dani and we agreed that this is also not the best term. Here, we should just use a general term, e.g. "referenced" or "required" classpath entries. I agree that "chained" may be confusing, since it's not actually a chain (like in classloader chaining), but it's a list of references with arbitrary substructure.
Considering that method that returns the referencing jar is named getReferencingEntry, perhaps 'referenced' makes more sense. Another point is that this is very specific to CPE_LIBARY type entries. So, should we reconsider the 'Entries' in the getChainedEntries too?
I think "entry"/"entries" is fine: it allows to use that later, should we have the need to extend the scope beyond JARs.
boolean excludeChainedJars(); will only be needed according to what we do for bug 288700. Right now I am not convinced there is an actual use case for this since runtime doesn't allow these referenced jars to be removed.
Discussed with Jay, that we decouple the several issues/bugs, so we can move forward one by one.
Created attachment 159398 [details] Updated patch Has the following updates to the previous patch: 1. Removed excludeChainedJar attribute 2. Moved the fix for 294360 to the corresponding bug 3. Removed the other API changes And few minor changes as per review comments.
API method names are good but the Javadoc still speaks about "chained". Please don't use that term.
Created attachment 159406 [details] Patch with updated document Removed the usage 'chained' in the documentation and few other places.
(In reply to comment #37) > Created an attachment (id=159406) [details] [diff] > Patch with updated document The API for getReferencedEntries() sounds a bit strange in some places. We'll need to see where we really use this in the UI, and if we don't use it in 3.6, I would pull that one out. Issues I saw: - "The referenced libraries themselves will not have any further referenced entries.": AFAIK, nested references are all resolved at run time, so this sounds rather restrictive, and does not reflect what happens at run time. - "when a same (referenced) library is referenced by more than one library {@link #CPE_LIBRARY}, the referenced library is attached only to the first raw classpath entry and not to the latter ones.": That sounds like an implementation detail and not something the client would want to know. In the UI, it would be strange if we showed a referenced JAR only under one parent but not under all that reference it (e.g. in Project Properties > Java Build Path > Libraries)
(In reply to comment #38) > > - "The referenced libraries themselves will not have any further referenced > entries.": AFAIK, nested references are all resolved at run time, so this > sounds rather restrictive, and does not reflect what happens at run time. Yes, that particular detail has not been mentioned there in the documentaion. If you find it ambiguous, we can perhaps add more details. Basically, all the referenced jars (including the further nested ones) are all gathered and added in a flat structure to the raw classpath entry. > - "when a same (referenced) library is referenced by more than one library > {@link #CPE_LIBRARY}, the referenced library is attached only to the first raw > classpath entry and not to the latter ones.": That sounds like an > implementation detail and not something the client would want to know. In the > UI, it would be strange if we showed a referenced JAR only under one parent but > not under all that reference it (e.g. in Project Properties > Java Build Path > > Libraries) And as per comment 19, we are not going to show the nested structure in the UI. So, we might only need to know that a particular classpath entry is a referenced one or otherwise.
We might change the wording a bit, but I think we should now check how this fits into the UI.
Markus, do you still have any concerns with the API?
> And as per comment 19, we are not going to show the nested structure in the UI. We're not going to add them as children in the Package Explorer, but I think we should identify referenced JARs, e.g. by adding "(referenced by <name-of-referencing-JAR>)" to their labels, see bug 252432 comment 1. Unfortunately, the API proposed in the last patch is not too helpful for that. We have an IPackageFragmentRoot to render, and that interface only has a getRawClasspathEntry() which returns questionable results for a referenced JAR: It says the entry "corresponds to a package fragment root if once resolved this entry's path is equal to the root's path.". But it actually returns a classpath entry for a JAR that references the given JAR, so the thing about the paths equality is not true. => I think the Javadoc for IPackageFragmentRoot#getRawClasspathEntry() should be adapted to tell what it currently does, and a new API IPackageFragmentRoot#getResolvedClasspathEntry() should be added that returns an IClasspathEntry which really stands for the given root. From that entry, we could then walk up the parent chain via IClasspathEntry.getReferencingEntry(). We could probably implement all this via IJavaProject#getResolvedClasspath(..) and then comparing paths etc., but I think that should be done in Core. Currently, we don't have a use case for getReferencedEntries(), so we shouldn't add it as API. But we may show the structure in Project Properties > Java Build Path, if that proves to be a useful UI for bug 281551 / bug 283806, so we could add it when tackling those bugs.
(In reply to comment #37) > Created an attachment (id=159406) [details] [diff] I think you should *not* release that patch right now, since it adds entries like this to the .classpath file (as soon as the classpath gets modified): <classpathentry kind="lib" path="lib/aref.jar"> <references> <reference path="lib/a.jar"/> </references> </classpathentry> Since we're not yet sure how the source attachment story will look like, we should not risk that clients use this intermediate version and corrupt their .classpath files with structures we're maybe not going to support in 3.6.
I agree with Markus that an API on IPackageFragmentRoot would better fit here and of course we don't want to change the .classpath file to achieve this. We will update the other bug regarding the source attachment story soon.
(In reply to comment #43) > Since we're not yet sure how the source attachment story will look like, we > should not risk that clients use this intermediate version and corrupt their > .classpath files with structures we're maybe not going to support in 3.6. I concur with this. We need to release patches when we agree on how to support the referenced jars completely in Core and UI.
Created attachment 160573 [details] Updated API I think it's better to keep the API part in this bug rather than 281551. Some of the API discussion has been captured in bug 281551 comments 30 through 37. This patch is same as the one that is attached to bug 281551 comment 40, with some spelling corrections and one fix.
Released in HEAD for 3.6M6.
*** Bug 283806 has been marked as a duplicate of this bug. ***
Verified for 3.6M6 using build I20100305-101
Verified.
