[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-distro-pkg-dev
Subject:    Re: [icedtea-web][rfc] Extract native code caching from JNLPClassLoader into a small class
From:       Jiri Vanek <jvanek () redhat ! com>
Date:       2013-05-31 11:54:56
Message-ID: 51A88F90.7040004 () redhat ! com
[Download RAW message or body]

On 05/30/2013 05:10 PM, Adam Domurad wrote:
> On 05/29/2013 06:21 AM, Jiri Vanek wrote:
>> On 05/23/2013 09:19 PM, Adam Domurad wrote:
>>> On 03/12/2013 10:26 AM, Jiri Vanek wrote:
>>>> On 03/05/2013 10:20 PM, Adam Domurad wrote:
>>>>> This is an incremental part of the effort to reduce the responsibilities of JNLPClassLoader.
>>>>>
>>>>> 2013-03-05 Adam Domurad <adomurad@redhat.com>
>>>>>
>>>>> * netx/net/sourceforge/jnlp/cache/NativeLibraryStorage.java: New,
>>>>> stores and searches for native library files that are loaded from jars.
>>>>> * netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java: Move code
>>>>> that handled native jar caching to NativeLibraryStorage.
>>>>>
>>>>> Happy hacking,
>>>>> -Adam
>>>>
>>>> Is there any more reason for this refactoring then much nicer, more readable, and testable
>>>> jnlpclasslaoder?
>>>> Anyway I'm fan of this kind of refactoring. And I'm for this to be done.
>>>>
>>>> - there must be unittests for this chnage
>>>> - i would like to see even reproducer for this
>>>> - it should go to 1.3 to after some time in head.
>>>
>>> Probably a little late now, but 1.4 sure, unless you think 1.3 is still a good idea.
>>
>> I'm hesitating with 1.4 now. But probably yes.  But you owe me your head:)
>>>
>>>>
>>>> I have not check if there is something more then pure refactoring, but if there isn't and tsts
>>>> will be added, then this will be approved.
>>>
>>> It is a refactoring only.
>>>
>>>>
>>>> J.
>>>
>>> I have created some unit tests, hopefully it is enough to push with ?
>>
>> nope. Some moreover important changes needed.
>>
>> Especially ExecUtils.execAndLog have no reason to live. Please use processWrapper. It is designed
>> for this and have logging correctly adapted.
>>
>> Also I'm against such a huge usage  of ecxec. Java is not Python.
>>
>> Especiallythe touch and mkdir  is nothing but pure laziness :) Please use java calls.
>
> Agreed
>
>>
>> For jar -cf ... well.. Java have api to work with its own jars.. please follow this api. Do not
>> fork processes if possible. But ..well. this api can be trap :) So choose wisely.
>>
>> I would recommend to do the refactoring of DummyJNLPFileWithJar into separate changset which you
>> can proceed immediately, but I do not insists.
>
> I have spliced this and pushed.
>
>>
>> In all cases, thanx for check and refactoring! It is deeply appreciated.
>>
>> J.
>
> See new version, I have removed all execs from both the new test, and JNLPClassLoaderTest.
>
> New changelog:
> 2013-XX-XX  Adam Domurad  <adomurad@redhat.com>
>
>      * netx/net/sourceforge/jnlp/util/StreamUtils.java
>      (copyStream): New, copies input stream to output stream
>      * tests/netx/unit/net/sourceforge/jnlp/cache/NativeLibraryStorageTest.java:
>      New, tests lookup of native libraries from folders and jars.
>      * tests/test-extensions/net/sourceforge/jnlp/util/FileTestUtils.java:
>      New, contains utilities for testing open file descriptors, creating temporary
>      directories, and creating jars.
>      * tests/netx/unit/net/sourceforge/jnlp/runtime/JNLPClassLoaderTest.java:
>      Replace jar creation methods with ones from FileTestUtils.
>
> Happy hacking,
> -Adam
>


Still nits:

+    /* All the native library types we support, as well as one negative test */
+    static final FileExtension[] extensionsToTest = {
+        fileExt(".foobar", false), /* Dummy non-native test extension */
+        fileExt(".so", true),
+        fileExt(".dylib", true),
+        fileExt(".jnilib", true),
+        fileExt(".framework", true),
+        fileExt(".dll", true)
+    };

is also in deffined in
+        String[] librarySuffixes = { ".so", ".dylib", ".jnilib", ".framework", ".dll" };

please reuse

Otherwise ok to go both patch and tests.. well thsi take time :)


J.
[prev in list] [next in list] [prev in thread] [next in thread]