Le 17/07/2009 16:59, Mamoru Tasaka a écrit :
> Christopher Stone wrote, at 07/17/2009 02:22 PM +9:00:
>> On Thu, Jul 16, 2009 at 9:26 PM, Mamoru
>> Tasaka<mtasaka(a)ioa.s.u-tokyo.ac.jp> wrote:
>>> Christopher Stone wrote, at 07/17/2009 11:59 AM +9:00:
>>>> On Thu, Jul 16, 2009 at 1:35 AM, Mamoru
>>>> Tasaka<mtasaka(a)ioa.s.u-tokyo.ac.jp> wrote:
>>>>> You must explain why you want to force to change understore to hyphen,
>>>>> even if installed files actually uses understore in its name.
>>>> So trying to think back to when we originally created the PHP
>>>> guidelines, specifically for php-pear, I think we had already
>>>> discussed this issue. You may want to search the packaging list
>>>> archives, I forget when we formulated the guidelines. Anyway, I think
>>>> the logic behind it was that the perl packages converted the :: to -
>>>> in their packages. So in order to be consistent with the perl packages
>>>> we used the same convention in the php-pear packages. There might be
>>>> some other technical reason why fedora converts upstream's :: to - for
>>>> perl packages which I'm not aware of however. But IIRC, that was the
>>>> logic behind the package naming.
>>> perl modules have some underscore naming like
>>> perl-Convert-NLS_DATE_FORMAT. We have no reason we must apply perl
>>> naming rule here and again there is no reason we should convert
>>> to hyphen
>>> which would just cause confusion.
>>> Please think in the way that we need less guidelines as much as
>>> If you have some reason *specific to php-pear*, please explain, and such
>>> reason must be written somewhere properly.
>> Then I guess we should search the packaging list archives and see if
>> we can find the reasoning when the guidelines were being formulated.
>> It must have been discussed or else the entire packaging committee
>> misunderstood the guidelines at the time. The majority of the
>> packages are owned by Remi and myself. I don't know how much work it
>> will be to rename all the packages.
First, I think this discussion should go to fedora-php-devel (a public
place), as I'd like to heard from others (tim please ?)
> So at least for now we cannot justify why underscore should be converted
> to hyphen, right?
I'm still not convinced.. but probably because I hate underscore...
> For me it is strange that you or Remi don't remember
> that (, however I guess there was actually no _real_ reason anyway).
> If we cannot find out what was discussed, we must withdraw the proposal
> about converting underscore to hyphen and at least new packages must
> use underscore if upstream's naming uses it.
> (However I think remaing php-pear related packages on Fedora is preferable
> and it is better that such renaming should be begun as early as possible)
:( really a big work
Hi Sean, sorry I didn't see your comments earlier.
On Wed, Jul 15, 2009 at 01:14:46AM +0200, sean finney wrote:
> in your comments you say:
> /* Parse an ISO-6709 date as used in zone.tab. Returns end of the
> s/date/coordinate/ ? :)
Hah! I've fixed this in patch v7.
> but more seriously:
> * in create_zone_index() it looks like
> find_zone_info(system_zone_info...) is called but system_zone_info
> isn't initialized until after create_zone_index has returned.
This was fixed in v6.
> * i'm not sure but it seems like there might be a few corresponding free()'s
> missing from the mallocs/strdups.
Yeah, everything here is malloc'ed but never free'd. We could set it up
to be free'd in the date extension's global destructor, but, it would
require modifying php_date.c which I am reluctant to do. I'm not sure
it makes much difference; the memory will remain allocated for the
lifetime of the process in any case, right?
> * it looks like load_zone_table() is called unconditionally, even if
> it's not needed (i.e. a scall to localtime() will trigger it)
I think this is necessary, though I didn't realize that before v6, and
didn't explain it at all in the v6 comments. I've added some better
commentary now in v7 - the problem is that we need the fake tzdb->data
array to be created once the tzdb is created, since
timezone_identifiers_list() peeks directly into it.
> * is parsing the comments really necessary? i don't see any php functions
> that use them.
They are exposed by the getLocation() interface -
$ php -r '$m = new DateTimeZone("America/New_York"); \
[country_code] => US
[latitude] => 40.71416
[longitude] => -73.99362
[comments] => Eastern Time
but yes they do seem to be kind of useless, in general.
> also, before i came to the conclusion that some kind of intermediate
> hashtable was needed to do the mapping (at which point i gave up and
> figured i'd wait to hear back from you :), i had done a slightly
> simpler implementation of the parsing using a clean mmap'd buffer
> and sscanf with "%ms" type formats which avoids a lot of the hard-coded
> lengths etc. if you're interested i can hack that code into the
> load_zone_info (and parse_iso6709) to make a leaner/cleaner/more efficient
Does it end up being much simpler without relying on sscanf/%ms? I
would rather this code is portable across older glibcs (e.g. so it works
on older versions of RHEL).
> also, FYI my posts to the fedora list are being automatically rejected
> so if i say anything important that should be shared someone should
> forward it along :)
I've requested that you are whitelisted on the Fedora list. Thanks a lot
for reviewing the patch, greatly appreciated!
Forwarding since Sean's mail is not making it through.
----- Forwarded message from sean finney <seanius(a)debian.org> -----
From: sean finney <seanius(a)debian.org>
To: PHP Packagers <pkg-php-maint(a)lists.alioth.debian.org>,
Date: Wed, 15 Jul 2009 01:14:46 +0200
Subject: Re: [php-maint] Bug#535770: PHP system timezone patch for 5.3?
User-Agent: Mutt/1.5.20 (2009-06-14)
some quick notes/observations...
in your comments you say:
/* Parse an ISO-6709 date as used in zone.tab. Returns end of the
s/date/coordinate/ ? :)
but more seriously:
* in create_zone_index() it looks like
find_zone_info(system_zone_info...) is called but system_zone_info
isn't initialized until after create_zone_index has returned.
* i'm not sure but it seems like there might be a few corresponding free()'s
missing from the mallocs/strdups.
* it looks like load_zone_table() is called unconditionally, even if
it's not needed (i.e. a scall to localtime() will trigger it)
* is parsing the comments really necessary? i don't see any php functions
that use them.
also, before i came to the conclusion that some kind of intermediate
hashtable was needed to do the mapping (at which point i gave up and
figured i'd wait to hear back from you :), i had done a slightly
simpler implementation of the parsing using a clean mmap'd buffer
and sscanf with "%ms" type formats which avoids a lot of the hard-coded
lengths etc. if you're interested i can hack that code into the
load_zone_info (and parse_iso6709) to make a leaner/cleaner/more efficient
also, FYI my posts to the fedora list are being automatically rejected
so if i say anything important that should be shared someone should
forward it along :)
 'm' being the sooner-or-later POSIX.1 adoption of the non-standard 'a'
GNU libc extension to automatically allocate the parsed strings, which
in turn conflicted with the C99 'a' (float) format specifier, hence
the new specifier. it requires glibc >= 2.7 btw.
----- End forwarded message -----
Le 16/07/2009 21:37, Christopher Stone a écrit :
> On Thu, Jul 16, 2009 at 1:35 AM, Mamoru
> Tasaka<mtasaka(a)ioa.s.u-tokyo.ac.jp> wrote:
>> Remi Collet wrote, at 07/16/2009 05:23 PM +9:00:
>>> have been approved.
>>> php-pear-Auth-HTTP, like all others pear extensions available in Fedora.
>> I strongly disagree with your proposition.
>> We _must_ honor the naming upstream developer uses as much as possible,
>> this will easily confuse people who will actually want to use these packages
>> on Fedora.
> Well, one thing is clear. We need to be consistent. We should
> identify which php-pear packages are renaming _ to - and which are
The first seems to be a "De facto" guidelines followed by all pear
Hi folks, I've attached "v5" of the system timezone patch, which is
updated for 5.3.0.
This contains the changes to parse zone.tab which I proposed in my
previous mail, and should fix the crash reported in the Debian bug
535770. It seems to work fine though I have not tested it exhaustively.
Further review and testing would be very welcome!
Looks like I failed to CC that as intended, copying for the archives.
----- Forwarded message from Joe Orton <jorton(a)redhat.com> -----
From: Joe Orton <jorton(a)redhat.com>
To: sean finney <seanius(a)debian.org>
Cc: PHP Packagers <pkg-php-maint(a)lists.alioth.debian.org>,
Date: Tue, 7 Jul 2009 10:10:06 +0100
Subject: Re: [php-maint] Bug#535770: PHP system timezone patch for 5.3?
User-Agent: Mutt/1.5.18 (2008-05-17)
Hi Sean - adding the Fedora PHP devel list to CC. Thanks for getting in
touch. For Fedora folks, Sean has referenced this Debian bug report:
Quoting in full:
On Tue, Jul 07, 2009 at 12:05:31AM +0200, sean finney wrote:
> ...and hi again :)
> On Mon, Jul 06, 2009 at 10:51:40PM +0200, sean finney wrote:
> > specifically timezone_identifiers_list() was extended to include country
> > codes as a filter for the returned values and it doesn't look like this
> > is covered by the patch and results in a segfault.
> looking a bit more at the specifics, it doesn't seem so desirable to have
> to mmap and read every file to get all the country codes. here's some
> options i see:
> - read all the files anyway. performance will be dismal but it's hopefully
> a corner case except for someone running a public world clock app or
> something :)
> - fall back to the internal timezonedb for listing country names. it would
> be a bit ugly and there's maybe a possibility that timezone names might
> get out of sync.
> - pre-cache the tzdata at install time (and make use of triggers to
> catch updates to the tzdata files). nice performance but adds extra
> complexity/dependencies outside of the engine.
> - use zone.tab for country code -> tzdata mapping? it seems to have the
> info we'd want all in one file (assuming it's correct and consistant),
> and we'd only have to read it in the specific cases that it was needed.
> - disable the api extension. not desirable.
> i guess zone.tab seems a nice easy and boring fix for this, what do you think?
It looks like they are (as threatened) shipping a mutated version of the
zoneinfo data in 5.3. Changes:
1) first four bytes are "PHP1" not "TZif"
2) in the "reserved" 16 bytes which follow, they have:
ONE BYTE: "BC" flag
TWO BYTES: country code
... those are being used by the code you mention above
3) after the end of the data, they have:
TWO BYTES: latitude and longtitude
ONE BYTE: length N
(N) BYTES: "comment"
Whereas we have the 64-bit TZif2 block at the end of the TZif data. I
think that's everything that's different.
The "BC" flag seems to be an indication that the zone name is a
deprecated form, and it's the case that BC is 0 if and only if the
country code is '??', otherwise BC is 1; with 'UTC' as a special-case
(BC=1, code is '??').
I believe it is also the case that, other than UTC, there is a
one-to-one mapping between zones *not* listed in zone.tab, and zones
which PHP designates with country code '??' (and hence BC=0).
zone.tab also has the lat/long data, and the country codes, so, yup,
parsing that at startup seems to be the way to go. I'll have a crack at
a patch later today hopefully.
----- End forwarded message -----