[rfc][icedtea-web] fixing CacheReproducerTest and improving VersionedJarTest

[Jiri Vanek]

On 03/23/2015 03:47 PM, Jie Kang wrote:
>
>
> ----- Original Message -----
>>>>>>>>>>> 1) user changes cachedir to custom location using itweb-settings :
>>>>>>>>>>> this
>>>>>>>>>>> is broken with your patch (bug in PathsAndFiles?)
>>>>>>>>>>
>>>>>>>>>> Sure!
>>>>>>>>>
>>>>>>>>> Hmm. Looking to it now, it stopepd to gave sense to me.
>>>>>>>>>
>>>>>>>>> The cahe si already selectable via XDG variables. Why to make it more
>>>>>>>>> complicated by completly custom value?
>>>>>>>>>
>>>>>>>>> Same is valid for logging.
>>>>>>>>>
>>>>>>>>> Is the chengable target of those two really de		sirable?
>>>>>>>>
>>>>>>>>
>>>>>>>> ping??
>>>>>>>>
>>>>>>>> I really am for dropping support of dual-setup-able cache and config.
>>>>>>>> However
>>>>>>>> it is definitely not something minor.
>>>>>>>
>>>>>>> Hello.
>>>>>>>
>>>>>>> I'm not sure what to say to continue this discussion :\ I think that
>>>>>>> the
>>>>>>> ability for users to set a custom location for their cache is useful
>>>>>>> and
>>>>>>> good to have.
>>>>>>
>>>>>> for what? You can set it ia xdg variables. Its standartized behaviour.
>>>>>
>>>>> It's been said before, when you set xdg variables you are changing the
>>>>> cache for not only icedtea-web but every other application that uses
>>>>> xdg's
>>>>> standard file locations.
>>>>
>>>> nope - yo can set variabnle per application... But thats metter of taste I
>>>> see.
>>>
>>> Can you provide an example? I can't find any documentation for this.
>>
>> VAR=val app...
>
>
> Uh... What kind of example is this?
>
> Do you mean exporting the variable before running the application? I don't expect the average user to know to do this :\

And will he dig through settings and change the dirs?
>
> So in 1.5, 1.4, users can set the cache using itweb-settings. They update to 1.6 and they can no longer set the cache. Why? Are there any benefits to this?

The question is different - is it really useful to have it? For me it seems quite confusing to have 
two mechanisms to change *nearly* same thing. The nearly is doing it even more confusing.

To prevent misunderstanding - I agree with your points. But I'm Trying to find some another point to 
keep it in (except backward compatibility)
>
>>>
>>>>>
>>>>> If a user wants to change their cache in 1.6, they must change their
>>>>> cache
>>>>> for every other program? But in 1.5 you could change just icedtea-web
>>>>> cache to any location. How is this a good thing to do?
>>>>>
>>>>>>
>>>>>>     > Prior to this patch we had this feature, but now it's not working
>>>>>>     > so I
>>>>>>     > think a fix would be nice.
>>>>>> What's wrong with allowing users to choose custom locations? We had this
>>>>>> feature before already, I
>>>>>> am just asking for a fix that makes it work again.
>>>>>>> Otherwise, if you want to to remove this feature, a patch that removes
>>>>>>> it
>>>>>>> all would be nice. Just leaving it broken is the main issue for me.
>>>>>>
>>>>>> Sure. I will fix it or remove it. this+localizations is main blocker for
>>>>>> release. Also Lukas's fix
>>>>>> for attributes is awaited.
>>>>>>>
>>>>>>>>
>>>>>>>> The resurrection of setupable paths may be tricky, but should be
>>>>>>>> doable.
>>>>>>>> Stil
>>>>>>>> it do not gave sense for me....
>>>>>>>
>>>>>>> CacheUtil tests will test the caching system and will deal with the
>>>>>>> cache
>>>>>>> on the filesystem. I think it makes sense to use a temporary cache
>>>>>>> location to do this.
>>>>>>> ResourceTracker/ResourceDownloader/PluginBridge/other tests that touch
>>>>>>> resources in the cache will deal with the cache on the filesystem. I
>>>>>>> think
>>>>>>> it also makes sense to use a temporary cache location to do this.
>>>>>>>
>>>>>>> The main reason for setupable paths is for these tests.
>>>>>>
>>>>>>
>>>>>> yes. But those test can now use fake instance of the singleton, or not?
>>>>>
>>>>> At the moment, no.
>>>>>
>>>>> An example is CacheUtil.clearCache:.
>>>>>
>>>>>        public static boolean clearCache() {
>>>>>            [...]
>>>>>            CacheLRUWrapper lruHandler = CacheLRUWrapper.getInstance();
>>>>
>>>> CacheLRUWrapper lruHandler = new CacheLRUWrapper.(paths1,path2)
>>>
>>> Uh... what? The clearCache code contains the call to
>>> CacheLRUWrapper.getInstance()... you can't just replace the code with a
>>> different call.
>>
>> No. Bud you can substitue fake object.
>
> I'm sorry but that does not seem possible to me. Show me some code. I don't think you can do this with the current implementation.

Then my impl was even more terrible then I thought. Still it made big bug visible.
>
>>>
>>>>
>>>>>            [...]
>>>>>
>>>>> How can you test this function with a temporary cache? Otherwise, as soon
>>>>> as you test it by calling CacheUtil.clearCache(), you delete the user
>>>>> cache. The instance is acquired inside, and getInstance() defaults to the
>>>>> one using IT-W's cache.
>>>>>
>>>> The issue with clearing sounds like an bug. That logic should be probably
>>>> encapsulated somewhere else.
>>>
>>> This is just one example. CacheUtil provides utility functions for the
>>> cache.
>>   > Nearly every function in this class has the same issue as clearCache when
>>   > it
>>   > comes to writing a test that modifies a temporary cache instead of the
>>   > user's current cache.
>>>
>>    Then it is even more buggy then I expected and have to be fixed
>
> It was fixed by my patch [rfc][icedtea-web] Allow cache directory to be changed during runtime.

It was fixing the disease.  Not the reasons of this sickness.  I know the patch. And it was good 
patch. But was patching rotten roots.
>
>>
>>>>> A lot of changes will need to be made to inject fake instances here, but
>>>>> you also have to consider things like
>>>>> ResourceDownloader.downloadResource,
>>>>> and making sure they can use temporary caches too, instead of downloading
>>>>> into user cache.
>>>>
>>>> In thsi case I would say
>>>>>
>>>> @beforeclass
>>>> original = CacheLRUWrapper.getInsatnce()
>>>> CacheLRUWrapper.setInsatnce(new CacheLRUWrapper(your desired patsh again))
>>>> ?
>>>>
>>>> @afterclass
>>>> CacheLRUWrapper.setInsatnce(original)
>>>
>>> CacheLRUWrapper.setInstance(...)? So, now you have a setter/getter for the
>>> entire instance,
>>> how is this any better than using a setter/getter for the cache directory
>>> itself?
>>
>> I do not care how it will be done, but what must be kept:
>
> So why did you reject the previous attempt at this?

see above.
>
>>
>> changed variable (in your case cache/config path) on one place - everything
>> will get this value (path)
>>     .. This afaik was not possible with previous implementation. Nor is done
>
> This was definitely already done in the patch I posted: [rfc][icedtea-web] Allow cache directory to be changed during runtime

It was not.  The only way how it was posisble before was by changing the property.  Still - half of 
the insides had already stored something like dir=System.getProperty(cahce/config) so even if you 
had changed the property, then the varianles were not adjusted.

No help also by changing the fields - the value was already copied on many places.
>
>>     ..
>>
>> previous impl was full of hardcoded or copied values. Current impl is - yes -
>> not much better.
>> But at least the issue is visible.
>>
>> So step one - make everything using the only (and accurate) source of truth
>> Step two , make it modifiable
>
> Once again, both step one and two were achieved in [rfc][icedtea-web] Allow cache directory to be changed during runtime.

explained above.
>
> Please consider re-reviewing the changes there. What you say you need here was already achieved there, without breaking functionality.

no - the change to this, must go via PathsAndFiles, and everything in itw had to read this in runtime.
>
> Please, please in the future don't rush patches like this and push them without full review. Now the situation is even more complicated than before.

Maybe:( But at least things had moved bit...

Again, thanx for thought!

J.