Community
Participate
Working Groups
"Illegal type of archive for required library" is an incorrect message, since ther e is actually no check if a library is optional or not before settings this warning. So the text needs to change to "Illegal type of archive for library" or ignore the message when the ClassPathEntry is marked as optional.
This can also be solved by fixing issue 104157
Message to update is 'classpath_illegalLibraryArchive'.
This message is no longer used. However we could re-use this entry for bug 229042. Deferring post 3.4.
See also bug 232816
Srikanth, while you are at changing messages in this area, could you please also make sure that we use the "required" word only when the library is not optional ?
1. The phrase "required library" is used either - by the compiler in messages such as "The type/field/ctor/method is not accessible due to restriction on required library". In these instances the library is really required i.e A reference has first determined to be bound to that library by the compiler and then the access restrictions are checked. - by the JavaModel message catalog in the following JavaModelStatus codes: classpath_illegalLibraryPath, classpath_illegalLibraryPathInContainer, classpath_illegalLibraryArchive, classpath_illegalExternalFolder, classpath_illegalExternalFolderInContainer, classpath_unboundLibrary, classpath_unboundSourceAttachment classpath_unboundSourceAttachmentInContainedLibrary 2. In the JavaModel's case: - all of the above usages happen in the call stack below and set the JavaModel status to be INVALID_CLASSPATH (org.eclipse.jdt.internal.core.) ClasspathEntry.validateLibraryEntry ClasspathEntry.validateClasspathEntry(IJavaProject, IClasspathEntry, IClasspathContainer, boolean, boolean) ClasspathEntry.validateClasspathEntry(IJavaProject, IClasspathEntry, boolean, boolean) only the last item of which is a public interface of the class ClasspathEntry 3. Currently, both the clients of ClasspathEntry.validateClasspathEntry namely JavaConventions.validateClasspathEntry and ClasspathValidation.validate "sanitize" the results from ClasspathEntry.validateClasspathEntry by doing something along the lines of IJavaModelStatus status = ClasspathEntry.validateClasspathEntry if (status.getCode() == IJavaModelStatusConstants.INVALID_CLASSPATH && ((ClasspathEntry) entry).isOptional()) // Ignore the INVALID_CLASSPATH status and consider all is well. 4. As a result, as things stand now, we would not use the phrase "required library" at all if the entry is optional and this bug is effectively a NOP. 5. However, an improvement would be to carry out the "sanitization" step right inside the (public) ClasspathEntry.validateClasspathEntry and remove these lines from the two clients mentioned above. 6. This would ensure that in future were this method to be called from other places, we would not erroneously flag an optional library as required and burden all users of the interface to carry out this sanitization step.
(In reply to comment #6) > 4. As a result, as things stand now, we would not use the phrase "required > library" at all if the entry is optional and this bug is effectively a NOP. That's good news. So it looks like this bug should really be closed as WORKSFORME. > 5. However, an improvement would be to carry out the "sanitization" step right > inside the (public) ClasspathEntry.validateClasspathEntry and remove these > lines from the two clients mentioned above. > > 6. This would ensure that in future were this method to be called from other > places, we would not erroneously flag an optional library as required and > burden all users of the interface to carry out this sanitization step. That's a good idea. Could you please provide the patch that does so?
Created attachment 120196 [details] Proposed patch Please see if this patch with code changes & tests look reasonable. We now ignore class path errors if the class path entry is marked optional. New test: ClasspathTests.test124117() Old tests ClasspathTests.testOptionalEntry*() created for bug 124117 also test this code change.
(In reply to comment #8) I'm not sure to understand why the isOptional() check in ClasspathEntry.validateClasspathEntry(IJavaProject, IClasspathEntry, boolean, boolean) is needed, since the same check is done in ClasspathEntry.validateClasspathEntry(IJavaProject, IClasspathEntry, IClasspathContainer, boolean, boolean) ?
> --- Comment #9 from Jerome Lanneluc <jerome_lanneluc@fr.ibm.com> > 2008-12-11 12:42:04 -0400 --- > (In reply to comment #8) > I'm not sure to understand why the isOptional() check in > ClasspathEntry.validateClasspathEntry(IJavaProject, IClasspathEntry, boolean, > boolean) is needed, since the same check is done in > ClasspathEntry.validateClasspathEntry(IJavaProject, IClasspathEntry, > IClasspathContainer, boolean, boolean) ? This double check exists because the current fix also tries to consolidate the changes made for bug 124117 (and relevant changes from rewrite of ClasspathEntry.java) into one place. As noted in comment 6 point 3, the sanitization of the validation status (i.e pretend verification was OK if CPE was optional) was happening at the two client sites of ClasspathEntry.validateClasspathEntry(IJavaProject, IClasspathEntry, boolean, boolean). This has been now "extracted into" the single location ClasspathEntry.validateClasspathEntry(IJavaProject, IClasspathEntry, boolean, boolean) itself. However this check alone is not sufficient to handle the current bug. As the new test ClasspathTests.test124117() shows, with class path containers in the picture, we can have a situation where the CPE for the container itself may not be flagged optional, but the contained entries may be flagged optional. Just the check in ClasspathEntry.validateClasspathEntry(IJavaProject, IClasspathEntry, boolean, boolean) will not handle this correctly. In summary - without the check in ClasspathEntry.validateClasspathEntry(IJavaProject, IClasspathEntry, boolean, boolean), (some of ) the older tests written for bug 124117 will fail. - without the check in ClasspathEntry.validateClasspathEntry(IJavaProject, IClasspathEntry, IClasspathContainer, boolean, boolean), the new test ClasspathTests.test124117 will fail. This assumes of course that the optional attribute makes sense when applied to libraries contained in a container - let me know if this is not the case.
(In reply to comment #10) Thanks for the explanation, this totally makes sense.
Comment on attachment 120196 [details] Proposed patch Code changes and test look good. I will commit once 3.5M4 is declared.
Fix and test released for 3.5M5
Verified for 3.5M5 using I20090126-1300
