Summary: | Breaking change in classpath container API | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | [Eclipse Project] JDT | Reporter: | Konstantin Komissarchik <konstantin> | ||||||||
Component: | Core | Assignee: | Jay Arthanareeswaran <jarthana> | ||||||||
Status: | VERIFIED FIXED | QA Contact: | |||||||||
Severity: | critical | ||||||||||
Priority: | P3 | CC: | amj87.iitr, daniel_megert, jarthana, markus.kell.r, nadeem.aboobaker, Olivier_Thomann, srikanth_sankaran | ||||||||
Version: | 3.6 | Flags: | Olivier_Thomann:
review+
markus.kell.r: review+ srikanth_sankaran: review+ frederic_fusier: review+ |
||||||||
Target Milestone: | 3.6 RC3 | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Whiteboard: | |||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 313890 | ||||||||||
Attachments: |
|
Description
Konstantin Komissarchik
2010-05-21 14:48:51 EDT
Would it be feasible for you to update to your product with an updated ClasspathContainerInitializer? > Would it be feasible for you to update to your product with an updated
> ClasspathContainerInitializer?
Yes, we can address this on our side. We have a release planned soon after Helios launch. Other products may not be so fortunate.
Jay, Classpath containers got the referenced entries for free in 3.5. So users that are relying on this behavior might be broken when moving to 3.6 with missing type errors. We discuss that situation at the JDT call and we came up with this plan: - add a system property to be able to revert back to the old (3.5) behavior for the classpath container. - add a readme and a migration guide entry so that we can warn users of potential problems and let them know that there is a workaround using the system property. - we don't want to add support for attributes on these referenced entries inside classpath container. Let me know if you have any question. You can discuss the case with Srikanth. Please prepare a patch and set the review flag to Markus, Srikanth and myself. Thanks. Created attachment 170061 [details] Proposed patch Patch contains changes mentioned in comment 3 with tests. Tests have been added in ClasspathTests#testBug313965 and ClasspathTests#testBug313965a. Srikanth/Markus/Olivier, Please review. Bug 313890 captures the documentation changes such as Javadoc, migration guide and readme. I would remove the if statement to write it as: this.resolveReferencedLibrariesForContainers = "true".equalsIgnoreCase(includeContainerReferencedLib); //$NON-NLS-1$ Created attachment 170081 [details]
Proposed fix + regression tests
New patch based on my last comment.
+1 Created attachment 170086 [details]
Proposed fix + regression tests
New patch that reuses the constant "true".
(In reply to comment #8) > Created an attachment (id=170086) [details] > Proposed fix + regression tests > > New patch that reuses the constant "true". Shouldn't addToResult(...) be called with the parameter addAsChainedEntry set to true instead of false in the added if block in JavaProject? Similar blocks of code already present in JavaProject.resolveClasspath(...) method call this method with this parameter equals to true... But maybe I missed something here!? (In reply to comment #9) > (In reply to comment #8) > > Created an attachment (id=170086) [details] [details] > > Proposed fix + regression tests > > > > New patch that reuses the constant "true". > > Shouldn't addToResult(...) be called with the parameter addAsChainedEntry set > to true instead of false in the added if block in JavaProject? > > Similar blocks of code already present in JavaProject.resolveClasspath(...) > method call this method with this parameter equals to true... But maybe I > missed something here!? Since we don't want support for attributes to be persisted for container libraries as Olivier mentioned in comment 3, a false needs to be passed there. (In reply to comment #10) > Since we don't want support for attributes to be persisted for container > libraries as Olivier mentioned in comment 3, a false needs to be passed there. To make it clearer, the two cases where we are passing true (as value for parameter addAsChainedEntry) to the addToResult() method are in the case of CPE_LIBRARY and CPE_VARIABLE, where we do want to support additional attributes for referenced libraries. In all other cases, we pass 'false'. Patch looks good. Note that the header comment in both junits need to be fixed as they are referencing a wrong system property name: "includeContainerReferencedLib" instead of the final choice "resolveReferencedLibrariesForContainers" (In reply to comment #10) OK, thanks for the clarification. Hence, patch looks good to me +1 for RC3. Released in HEAD for 3.6RC3 with one minor change - as per comment 12. verified for 3.6RC3 using build I20100527-1700. |