Hi Kurt,
On 11/10/2011 11:09 AM, Kurt Miller wrote:
> On 11/09/11 03:01, Jonathan Lu wrote:
>> On 11/04/2011 01:26 PM, David Holmes wrote:
>>> On 4/11/2011 2:53 PM, David Holmes wrote:
>>>> On 4/11/2011 2:13 PM, Masayoshi Okutsu wrote:
>>>>> Probably the difference isn't documented. I tried Solaris 10 and Ubuntu
>>>>> 10.03. The difference still exists.
>>>>>
>>>>> Solaris 10:
>>>>> $ unset TZ
>>>>> $ date
>>>>> Fri Nov 4 13:04:45 JST 2011
>>>>> $ TZ="" date
>>>>> Fri Nov 4 13:04:53 JST 2011
>>>>>
>>>>> Ubuntu 10.04:
>>>>> $ unset TZ
>>>>> $ date
>>>>> Fri Nov 4 13:05:50 JST 2011
>>>>> $ TZ="" date
>>>>> Fri Nov 4 04:05:55 UTC 2011
>>>>>
>>>>> When the TZ value is an empty string, Ubuntu uses UTC while Solaris
>>>>> still looks up the system default.
>>>> I have to take back my comment regarding this not seeming to be platform
>>>> specific code - it is highly platform specific! It seems that on Linux
>>>> we are happy to report a TZ of "" but not so on Solaris. I presume this
>>>> is an attempt to keep Java's use of TZ consistent with how other apps
>>>> handle it on that platform. (environ(5) gives a little insight on
>>>> Solaris as to how TZ is used.)
>>>>
>>>> So the key thing here is to not disturb the existing behaviour on either
>>>> linux or Solaris - which suggests the original patch. That said I'm not
>>>> convinced - given this is so platform specific - that simply treating
>>>> non-linux the same as Solaris is a reasonable thing to do. I think it
>>>> would be useful to see what the BSD/OSX port(s) had to do with this code
>>>> - if anything.
>>> To answer my own queries BSD/OSX does
>>>
>>> 511 #if defined(__linux__) || defined(_ALLBSD_SOURCE)
>>> 512 if (tz == NULL) {
>>> 513 #else
>>> 514 #ifdef __solaris__
>>> 515 if (tz == NULL || *tz == '\0') {
>>> 516 #endif
>>> 517 #endif
>>>
>>> so the suggested patch would at least not interfere.
>>>
>>> Anyway this needs input from other core-libs folk. I didn't intend to
>>> get quite so heavily involved. ;-)
>>>
>>> David
>>> -----
>>>
>>>
>>>
>>>> David
>>>> -----
>>>>
>>>>
>>>>> Thanks,
>>>>> Masayoshi
>>>>>
>>>>> On 11/3/2011 4:16 PM, Jonathan Lu wrote:
>>>>>> Hi Masayoshi,
>>>>>>
>>>>>> I did find some references about date-time related functions / TZ
>>>>>> variables on Linux but got only a few about Solaris, so could not see
>>>>>> any differences between those two platforms about the changes
>>>>>> described in my patch. Have you got any links or references about
>>>>>> these differences? I'm interested in it and may update the patch again
>>>>>> after reading them.
>>>>>>
>>>>>> Thanks a lot!
>>>>>> - Jonathan
>>>>>>
>>>>>> On 11/02/2011 10:27 PM, Masayoshi Okutsu wrote:
>>>>>>> Hi Jonathan,
>>>>>>>
>>>>>>> IIRC, the difference came from some behavioral difference between the
>>>>>>> Linux and Solaris libc date-time functions and/or the date command,
>>>>>>> and TimeZone_md.c tries to follow the difference. But the code was
>>>>>>> written looooong ago. The difference may no longer exist.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Masayoshi
>>>>>>>
>>>>>>> On 11/2/2011 8:39 PM, Jonathan Lu wrote:
>>>>>>>> On 11/02/2011 07:00 PM, David Holmes wrote:
>>>>>>>>> On 2/11/2011 7:01 PM, Jonathan Lu wrote:
>>>>>>>>>> On 11/02/2011 04:56 PM, Jonathan Lu wrote:
>>>>>>>>>>> Hi core-libs-dev,
>>>>>>>>>>>
>>>>>>>>>>> In jdk/src/solaris/native/java/util/TimeZone_md.c, starting from
>>>>>>>>>>> line
>>>>>>>>>>> 626, I found that the scope of "#ifdef __solaris__" might be too
>>>>>>>>>>> narrow, since it also works for some kind of OS which I'm
>>>>>>>>>>> currently
>>>>>>>>>>> working on, such as AIX.
>>>>>>>>>>> So I suggest to just remove the '#ifdef __solaris__' and leave
>>>>>>>>>>> the
>>>>>>>>>>> "#else" to accommodate more conditions, see attachment
>>>>>>>>>>> 'patch.diff'. I
>>>>>>>>>>> think this may enhance the cross-platform ability, any ideas
>>>>>>>>>>> about
>>>>>>>>>>> this modification?
>>>>>>>>>>>
>>>>>>>>>>> Regards
>>>>>>>>>>> - Jonathan Lu
>>>>>>>>>> I'm not sure why the attachment got filtered, here paste it to the
>>>>>>>>>> mail
>>>>>>>>>> content directly.
>>>>>>>>>>
>>>>>>>>>> diff -r 4788745572ef src/solaris/native/java/util/TimeZone_md.c
>>>>>>>>>> --- a/src/solaris/native/java/util/TimeZone_md.c Mon Oct 17
>>>>>>>>>> 19:06:53
>>>>>>>>>> 2011 -0700
>>>>>>>>>> +++ b/src/solaris/native/java/util/TimeZone_md.c Thu Oct 20
>>>>>>>>>> 13:43:47
>>>>>>>>>> 2011 +0800
>>>>>>>>>> @@ -626,10 +626,8 @@
>>>>>>>>>> #ifdef __linux__
>>>>>>>>>> if (tz == NULL) {
>>>>>>>>>> #else
>>>>>>>>>> -#ifdef __solaris__
>>>>>>>>>> if (tz == NULL || *tz == '\0') {
>>>>>>>>>> #endif
>>>>>>>>>> -#endif
>>>>>>>>>> tz = getPlatformTimeZoneID();
>>>>>>>>>> freetz = tz;
>>>>>>>>>> }
>>>>>>>>> I'm unclear why any of that code needs to be platform specific - is
>>>>>>>>> an empty TZ string somehow valid on linux? I would have thought the
>>>>>>>>> following would be platform neutral:
>>>>>>>>>
>>>>>>>>> if (tz == NULL || *tz == '\0') {
>>>>>>>>> tz = getPlatformTimeZoneID();
>>>>>>>>> freetz = tz;
>>>>>>>>> }
>>>>>>>>>
>>>>>>>> Hi David,
>>>>>>>>
>>>>>>>> getenv("TZ") returns NULL when TZ environment variable is not set at
>>>>>>>> all and returns '\0' when TZ was exported as empty string. After
>>>>>>>> more checking for both cases, I agree with you, nothing useful can
>>>>>>>> be retrieved from that environment variable.
>>>>>>>> So I changed the patch to this,
>>>>>>>>
>>>>>>>> diff -r 7ab0d613cd1a src/solaris/native/java/util/TimeZone_md.c
>>>>>>>> --- a/src/solaris/native/java/util/TimeZone_md.c Thu Oct 20 10:32:47
>>>>>>>> 2011 -0700
>>>>>>>> +++ b/src/solaris/native/java/util/TimeZone_md.c Wed Nov 02 19:34:51
>>>>>>>> 2011 +0800
>>>>>>>> @@ -623,13 +623,7 @@
>>>>>>>>
>>>>>>>> tz = getenv("TZ");
>>>>>>>>
>>>>>>>> -#ifdef __linux__
>>>>>>>> - if (tz == NULL) {
>>>>>>>> -#else
>>>>>>>> -#ifdef __solaris__
>>>>>>>> if (tz == NULL || *tz == '\0') {
>>>>>>>> -#endif
>>>>>>>> -#endif
>>>>>>>> tz = getPlatformTimeZoneID();
>>>>>>>> freetz = tz;
>>>>>>>> }
>>>>>>>>
>>>>>>>>> David
>>>>>>>>> -----
>>>>>>>>>
>>>>>>>>>> Regards
>>>>>>>>>> - Jonathan Lu
>>>>>>>> Regards
>>>>>>>>
>>>>>>>> - Jonathan
>>>>>>>>
>> Copy to bsd-port-dev and macosx-port-dev lists to see if anybody here
>> has some ideas about this issue.
> Hi Jonathan,
>
> The above email is a bit hard to follow due to the mixture of top and
> bottom replies.
>
> I can confirm that OpenBSD and Mac OS X 10.5.8 follow the Linux behavior
> which confirms the need for platform ifdef's in this code.
>
> Seems like you need to make the following change:
>
> -#ifdef __solaris__
> +#if defined(__solaris__) || defined(__AIX__)
>
> or something similar to maintain compatibility.
>
> In general the approach taken for adding BSD support was to never
> assume you can change other supported code paths. If your architecture
> follows an existing code path behavior add it like I did above.
> Otherwise just create a #ifdef myarch section for it.
>
But for this case, I think it is a good idea to leave a default code
path here.
Since in src/solaris/native/java/util/TimeZone_md.c starting from line
624, the return value of getenv("TZ"); has to be tested afterward on any
platforms.
So to improve portability for OpenJDK, how about leaving Solaris style
checking as the default approach?
> Unifying or changing another architecture's code path requires access
> to the arch, research and confirmation that the change is ok. Typically
> this may be done by writing independent test programs and running them
> on each arch.
>
> Regards,
> -Kurt
>
>
>
