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

List:       openjdk-2d-dev
Subject:    Re: [OpenJDK 2D-Dev] Add optional support for using the system libicu
From:       "Steven R. Loomis" <steven.loomis () oracle ! com>
Date:       2013-06-25 0:24:15
Message-ID: 51C8E32F.5090908 () oracle ! com
[Download RAW message or body]

Omair,
  I'm sorry I did not see this before. I probably would not have noticed 
it except I happened to notice this reply.

  I'm responsible for maintaining the version of ICU in JDK, and also 
(read: main day job) IBM's lead for ICU for C/C++.  ( But I speak for 
myself here, of course. )

  I think this patch is certainly a good direction (especially with 
regards HarfBuzz - I wrote that recommendation you cite), at least 
longer term, however, I have quite a number of concerns with this patch.

• The big one - is that the ICU and JDK versions of the LE are 
divergent, so much so that, to put it mildly, I am a little surprised 
that this patch builds at all.
  Now, I would like - love - for enough upstream changes to ICU to 
happen so that the APIs are compatible enough for this change to be 
possible.  But we aren't there yet.
   • … and when we are there, it won't be compatible with any shipping 
ICU, so you will need to wait until the future ICU is picked up.

• You mentioned the issues about which version of the headers to compile 
against. You MUST compile against the same version of the headers you 
are linking against.  Period. This is C++, not Java or even C.  The 
vtable expected in the ICU version versus the JDK version of the LE are 
different, have different parentage.

• As to tests, I would start with everything in 
jdk/test/java/awt/font/TextLayout

  ( NB Phil-  I had started a port of ICU's letest.xml conformance test 
to the JDK- this would be helpful in making sure we get the same results.)


I'm sorry I didn't see this earlier. I don't always monitor this address 
or even 2d-dev as closely as I could.

Regards,
Steven

( If you don't get a response from me, you can always ping me at 
srl@icu-project.org …  )

On 6/24/13 3:14 p.m., Omair Majid wrote:
> Hi Phil,
>
> Updated webrev:
> http://cr.openjdk.java.net/~omajid/webrevs/system-icu/01/
>
> It's still against jdk8/build and missing support for the old build system.
>
> On 06/05/2013 02:02 PM, Phil Race wrote:
>> Since this entirely affects a 2D component, please include 2d-dev in
>> this discussion.
>> I would have been 'surprised' to see this change if I hadn't just
>> spotted this thread.
> Mea culpa. I pushed a few similar system-library patches using this
> identical process and no one corrected me. So I assumed this was right.
> For the future, I will ensure the relevant groups are CC'd.
>
>> And I believe this change should be integrated via the 2D forest.
> Okay. Are there any guidelines for this? It's not obvious to me when a
> change is more appropriate for build vs 2D.
>
>> I am not sure what, if any, runtime problems might occur from using a
>> different
>> version. We've generally been able to swap in newer versions of ICU without
>> much trouble, but I recommend to run appropriate tests before shipping ..
> Thanks for the suggestions. Do you have any particular tests in mind?
>
> For some background, the goal with this change is two fold:
>
> 1. Gain benefits from system-installed libraries. This is one library
> where OpenJDK does not lag behind in security updates, but in some other
> cases, embedded libraries can lag behind. It also makes it easier to use
> the distributions preferred policies (hardening flags on libraries and
> so on).
>
> 2. Make it easier to switch to HarfBuzz at some later point. ICU itself
> strongly recommends switching [1]. Through ice-le-hb [2], only a
> recompile should be needed to switch (hopefully).
>
>> Note that JDK8 in fact has a very "current" ICU with some security fixes.
>> So I would not recommend using the native lib on a system with an older
>> ICU.
> Thanks for pointing it out. I see that ICU recently released a security
> update [1] too, but probably many distributions will not have this
> up-to-date version (my current distro, a little out-of-date, does not :( ).
>
> Thanks,
> Omair
>
> [1] http://site.icu-project.org/download/51
> [2] https://github.com/behdad/icu-le-hb
>

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