Summary: | missing story for attributes of referenced JARs in classpath containers | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | [Eclipse Project] JDT | Reporter: | Markus Keller <markus.kell.r> | ||||||
Component: | Core | Assignee: | Jay Arthanareeswaran <jarthana> | ||||||
Status: | VERIFIED FIXED | QA Contact: | |||||||
Severity: | normal | ||||||||
Priority: | P3 | CC: | daniel_megert, jarthana, konstantin, nadeem.aboobaker, Olivier_Thomann, satyam.kandula, srikanth_sankaran | ||||||
Version: | 3.6 | ||||||||
Target Milestone: | 3.6 M7 | ||||||||
Hardware: | PC | ||||||||
OS: | Windows XP | ||||||||
Whiteboard: | |||||||||
Attachments: |
|
Description
Markus Keller
2010-03-08 13:57:57 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). C: +1 ;-) I am fine with (c) too. Just one concern - wouldn't this break the existing projects and cause compilation errors? (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. 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? (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. (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. 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. 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?
(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 ! Created attachment 165408 [details]
Patch with renamed tests
Renamed and moved the new test around.
Released in HEAD for 3.6M7. Verified for 3.6M7 using build I20100424-2000 Verified. 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? 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. Raised a new defect (bug 313890) to get this in to the migration guide. > 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. |