Community
Participate
Working Groups
I20080427-2000. Currently JDT Core allows to add JARs to the build path that aren't valid ZIPs. Only when accessing it a CoreException is thrown (see bug 229038 for details). It would be nice if a build path error would be added at the time the JAR is added to the build path.
Could be made part of classpath validation.
We could re-use org.eclipse.jdt.internal.core.util.Messages.classpath_illegalLibraryArchive. However see bug 171136 (the message would need to be corrected)
Created attachment 181326 [details] Proposed patch New warning added for invalid/unsupported archive content. New validation added in ClasspathEntry for this, which checks for the validity/support of the archive and raises the warning. The code could be improved by caching the invalid archive files, just like the JavaModelManager#nonChainingJars. I intend to do this along with the fix for bug 321170, as it's even more relevant there.
Created attachment 181536 [details] Updated patch Added a cache (just like JavaModelManager#nonChainingJars) to keep track of the invalid archive files. Once an archive has been marked invalid, further calls to JavaModelManager#getZipFile(IPath) do not make any attempt to open the zip file. I.e. until the cache is reset, which typically happens during a resource change. Another change worth mentioning is the change to the following method: DeltaProcessor.validateClasspaths(IResourceDelta, HashSet) This change is necessary to invoke validation of a particular project (and its libraries) when a library that has contributed to the project has changed. This will ensure that a resource change event (of a contributing library) is enough to validate the project's libraries. Earlier, only a project close/open or reset of project's classpath would trigger this. Several tests that were benefiting from the fact that we didn't bother about the validity of the archive content have been modified to accommodate this change. Note that we still allow any extensions for archives. Olivier, please review.
Patch looks good. My only comment is about the logging of the error. If we report a build path error, do we really need to log the error ?
(In reply to comment #5) > Patch looks good. > My only comment is about the logging of the error. > If we report a build path error, do we really need to log the error ? I agree with Olivier.
Created attachment 181618 [details] Updated patch Replaced the exception logging by a simple error logging. This was an already existing message and all I have done is just removed the exception to be passed to the logger. This gets logged only when the user tries to open the invalid archive file. This fixes bug 321170 too. Will release after rerunning the tests.
Released in HEAD for 3.7M3.
Verified in I20101025-1800: it works but the message: Illegal type of archive for required library: {0} in project {1} is not very intuitive as has already been noted in comment 2 and in bug 171136. The problem is not about the "type" but rather the "content".
(In reply to comment #9) > Verified in I20101025-1800: it works but the message: > > Illegal type of archive for required library: {0} in project {1} > > is not very intuitive as has already been noted in comment 2 and in bug 171136. > > The problem is not about the "type" but rather the "content". How about this: Unsupported archive content in library: {0} in project {1}
(In reply to comment #10) > How about this: > > Unsupported archive content in library: {0} in project {1} Perhaps this sounds better: Unsupported archive format: {0} in project {1}
(In reply to comment #11) > (In reply to comment #10) > > How about this: > > > > Unsupported archive content in library: {0} in project {1} > > Perhaps this sounds better: > Unsupported archive format: {0} in project {1} Nope: {0} is not an archive format ;-) The reported problem not only covers invalid format issues but also the case where the file cannot be read for whatever reason. ==> Archive for required library: ''{0}'' in project ''{1}'' cannot be read or is not a valid ZIP file
Created attachment 181725 [details] Patch with modified error message New error introduced as suggested in comment #12.
Created attachment 181732 [details] Updated tests Test cases have been updated too. Will release after running all tests.
As Olivier mentioned, I think I can remove the error message we printing in the log file too at this point. Dani, what do you think about this?
(In reply to comment #15) > As Olivier mentioned, I think I can remove the error message we printing in the > log file too at this point. > > Dani, what do you think about this? In comment 6 I agreed to it but given that the reported problem covers several scenarios, it might be good to log the error.
Released in HEAD for 3.7M3
Verified in I20101028-1800.
*** Bug 337500 has been marked as a duplicate of this bug. ***
