Bug 305037 - missing story for attributes of referenced JARs in classpath containers
Summary: missing story for attributes of referenced JARs in classpath containers
Status: VERIFIED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Core (show other bugs)
Version: 3.6   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: 3.6 M7   Edit
Assignee: Jay Arthanareeswaran CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-03-08 13:57 EST by Markus Keller CLA
Modified: 2010-05-21 14:49 EDT (History)
7 users (show)

See Also:


Attachments
Proposed patch (11.91 KB, patch)
2010-04-15 02:05 EDT, Jay Arthanareeswaran CLA
srikanth_sankaran: review+
Details | Diff
Patch with renamed tests (11.91 KB, patch)
2010-04-20 06:59 EDT, Jay Arthanareeswaran CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Markus Keller CLA 2010-03-08 13:57:57 EST
HEAD

Create a user library and add a JAR with a "Class-Path:" reference. On the resolved build path, the referenced JAR appears, but IClasspathContainer#getClasspathEntries() does not include the referenced JAR, so clients cannot set attributes (source & Javadoc location) for such JARs.

I see 3 possible solutions:

a) create an IClasspathContainerExtension with a method like on IJavaProject:
IClasspathEntry[] getReferencedClasspathEntries().

Subclasses of ClasspathContainerInitializer would then need to update their 
requestClasspathContainerUpdate(..), etc. so that attributes can be stored by the container.

b) treat the referenced JARs like entries referenced from "lib" entries, and store and retrieve their attributes in the .classpath file as <referencedentry>.

c) don't resolve referenced JARs in classpath containers (i.e. let the container implementer deal with this)


(a) would need even more API and changes in client code for a feature that's probably rarely used and that can already be implemented by container initializers if they really want it.
(b) would somehow defeat the whole purpose of classpath containers as containers that keep state and can e.g. use that information in multiple instances of the same container (since the referenced JARs would still have to be configured in every project).
(c) would kill two birds with one stone

My current favorite is (c). Other ideas or opinions?
Comment 1 Olivier Thomann CLA 2010-03-08 14:06:42 EST
(In reply to comment #0)
> My current favorite is (c). Other ideas or opinions?
I thought this is what we decided when we talked over the phone. Classpath containers must update if they want to take care of the referenced jars.

So I also vote for (c).
Comment 2 Dani Megert CLA 2010-03-18 05:07:10 EDT
C: +1 ;-)
Comment 3 Jay Arthanareeswaran CLA 2010-03-18 05:27:26 EDT
I am fine with (c) too. 
Just one concern - wouldn't this break the existing projects and cause compilation errors?
Comment 4 Markus Keller CLA 2010-03-19 07:49:40 EDT
(In reply to comment #3)
> I am fine with (c) too. 
> Just one concern - wouldn't this break the existing projects and cause
> compilation errors?

This could be a problem with user libraries, but the user can easily add the referenced JAR and everything's fine. Other classpath containers typically use more modern ways to manage dependencies, so I don't expect big problems here. If it's really a problem for a classpath container, it can easily add the referenced libraries itself.

I would accept some problems in corner cases, given that this fix make our story more consistent and that there's not good other fix to make the attributes story work for these cases.
Comment 5 Jay Arthanareeswaran CLA 2010-04-06 02:56:24 EDT
I just noticed that this will impact the requirement mentioned here - bug 304081 comment 2. We don't want to make an exception for CPE_CONTAINER case, do we?
Comment 6 Markus Keller CLA 2010-04-06 08:39:36 EDT
(In reply to comment #5)
Yes, we don't want any exceptions. Referenced libraries should only be considered for JARs from library or variable entries.
Comment 7 Jay Arthanareeswaran CLA 2010-04-08 10:40:09 EDT
(In reply to comment #6)
> (In reply to comment #5)
> Yes, we don't want any exceptions. Referenced libraries should only be
> considered for JARs from library or variable entries.

I still see a problem with this - even if the container takes care of the referenced entries, ultimately we (or the container provider) would want the referenced library entries to be part of the resolved classpath and that's the only way we could use the classes inside that library entry. Correct me if I am wrong here.
Comment 8 Markus Keller CLA 2010-04-09 14:48:10 EDT
The ClasspathContainerInitializer decides what's in the container, so if it wants to add referenced entries, it can just add them to its classpath. It can easily find referenced entries with JavaCore#getReferencedClasspathEntries(IClasspathEntry, IJavaProject).

Comment 0 outlines the problems that show up if we add the referenced entries silently, and we've decided to avoid these problems by not supporting referenced entries in containers.
Comment 9 Jay Arthanareeswaran CLA 2010-04-15 02:05:18 EDT
Created attachment 164932 [details]
Proposed patch

Patch contains two changes:
1. Removed the chained(referenced) libraries resolution for container. This means containers (including user libraries) will need to take care of the chained libraries themselves. For user libraries, I will raise a new defect.
2. Fixed a minor bug came across when I added a new test for persisting referenced entries for variable classpath entries with referenced jar.

Srikanth, could you review this patch please?
Comment 10 Srikanth Sankaran CLA 2010-04-19 06:28:03 EDT
(In reply to comment #9)

> Srikanth, could you review this patch please?

Jay, your walk through of the code sounds reasonable to me. 
I would heartily recommend separate bugs for unrelated issues
in future though - Things get quickly tangled up and
it is hard to keep separate concerns separate !
Comment 11 Jay Arthanareeswaran CLA 2010-04-20 06:59:03 EDT
Created attachment 165408 [details]
Patch with renamed tests

Renamed and moved the new test around.
Comment 12 Jay Arthanareeswaran CLA 2010-04-20 07:06:26 EDT
Released in HEAD for 3.6M7.
Comment 13 Satyam Kandula CLA 2010-04-27 08:15:44 EDT
Verified for 3.6M7 using build I20100424-2000
Comment 14 Jay Arthanareeswaran CLA 2010-04-27 08:56:56 EDT
Verified.
Comment 15 Konstantin Komissarchik CLA 2010-05-20 15:25:28 EDT
You guys do understand that you broke your API with this change? We found this bug when we were trying to figure out why code that we shipped on Galileo stopped working on Helios. We have a container that expects manifest resolution to work. 

Even for user libraries, this will cause user applications that build fine in Galileo to stop building. The user will need to know to go update their user library definitions. Who will know to do this when the app built fine in Galileo? They will just assume that Helios has a bug and head to the forums.

It seems to me that the problem that this bug was trying to solve is relatively minor compared to the havoc that this will cause for third parties building on JDT and end users.

Are you sure you want to keep this change in Helios?
Comment 16 Dani Megert CLA 2010-05-21 03:00:09 EDT
Konstantin, please file a bug report where you describe the setup and how/why it is broken and which API we broke. We can then take a look and decide what to do about it.
Comment 17 Jay Arthanareeswaran CLA 2010-05-21 06:55:47 EDT
Raised a new defect (bug 313890) to get this in to the migration guide.
Comment 18 Konstantin Komissarchik CLA 2010-05-21 14:49:44 EDT
> Konstantin, please file a bug report where you describe the setup and how/why
> it is broken and which API we broke. We can then take a look and decide what to
> do about it.

Sure. Bug 313965.