[icedtea-web] RFC: find jars based on Class-Path entry in manifest

[Omair Majid]

On 04/20/2011 03:15 PM, Deepak Bhole wrote:
> * Omair Majid<omajid at redhat.com>  [2011-04-20 15:09]:
>> On 04/20/2011 02:45 PM, Deepak Bhole wrote:
>>> * Omair Majid<omajid at redhat.com>   [2011-04-15 12:28]:
>>>> Updated patch attached.
>>>>
>>>
>>> I'll let Andrew reply to his concerns. I have only one concern (below):
>>>
>>>> -
>>>> -                            try {
>>>> -                                u = tracker.getCacheURL(remoteURL);
>>>> +                                addNewJar(desc);
>>>>                               } catch (Exception e) {
>>>>                                   throw new ClassNotFoundException(name);
>>>>                               }
>>>> -
>>>
>>> We are catching any exception and re-throwing a CNFE. However exceptions
>>> could occur due to something like a file mentioned in the jarindex not
>>> existing on the server. This may not necessarily be fatal to the app
>>> (the class may exist in the next jar specified in the index). The CNFE
>>> will however make it fatal..
>>>
>>
>> That's what I thought when I was going over this code - but that's
>> how the old code was written. You can see that the new section that
>> handles classpaths does not throw CNFE - it just logs the exception
>> and continues.
>>
>> FWIW, addNewJar() and getCacheURL() do not throw any checked
>> exceptions. I believe this is more of a safety net than anything
>> else. Do you want me to update this code to just log the exception?
>>
>
> Yes, I think that will be better. If the current code is throwing
> exceptions in such instances, it is incorrect IMO.
>

Sure, I will post an updated patch in a bit.

>> On a slightly different note, this code needs to be moved to the
>> findResource(s) methods. As it is, searching for anything other than
>> classes will not use Class-Path jars or the JarIndexes.
>>
>
> Good point. Would you be doing that? If so, do you want to do it in this
> patch or a new one?
>

I can take care of it, but I would like to do it in a separate patch. I 
dont think this is important enough to block 1.1 branching.

Cheers,
Omair