Raphael,
I went over your patches from
http://patch-tracker.debian.org/package/php5/5.3.1-2 and did quick
reviews (didn't apply / test them or anything ...)
Here some comments:
004-ldap_fix.patch:
Do you have a test for this? when does it happen that ldap_value is
NULL happen? - If that's an issue we should add it to our tree...
013-force_getaddrinfo.patch:
Why doesn't the check work on Debian out of the box? On my (not
Debian) Linux boxes it works. anything needed for people compiling PHP
on Debian themselves?
019-z_off_t_as_long.patch
Certainly nothing for our tree
034-apache2_umask_fix.patch
036-fd_setsize_fix.patch
Any reproduce case where this is an issue?
043-recode_size_t.patch
Why is that needed? The name of the patch suggests that str_len is
used in a place where an size_t is expected but then the location
of this check is wrong. Additionally in case this check is tirggered
the function will return without error message.
045-exif_nesting_level.patch
How did you decide on the limit of 250?
047-zts_with_dl.patch
I don't support this. dl() can be used by CLI users for loading Gtk or
such on demand but avoiding to load (and inititalize) all the Gtk
stuff every time they use PHP and avoid messing with config files...
052-phpinfo_no_configure.patch
Neither do I support this. Users might benefit when building own
modules or might be interested in the configure line for other
reasons ...
053-extension_api.patch
When adding random options to command line tools please mark them as
Debian specific. Additionally the man page should be updated.
100-recode_is_shared.patch
The conflict between MySQL and recode should only happen with an old
libmysql (3.23?) not sure about imap ... but in your case the patch
might make sense, while I won't directly apply it to our tree as
usually people will build extensions to load them together...
101-sqlite_is_shared.patch
Not sure if we do any modifications to SQLite but in general we prefer
to use the bundled lib so users get the same behavior on all
platforms.
108-64_bit_datetime.patch
116-posixness_fix.patch
Why is that needed on our system? - I never saw an issue about these
and if they are missing there should be build issues.
use_embedded_timezonedb.patch
Like with the SQLite note above we prefer to have the same behavior
over all platforms by using a common time database.
force_libmysqlclient_r.patch
Why are you using the reentrant version of this library which is
slower than the "regular" one? Did you consider mysqlnd?
009_ob-memory-leaks.patch
Any test case? (While this looks good on first sight)
exif_read_data-segfault.patch
Any sample case for this? Any bug report?
sybase-alias.patch
I have no idea about MS SQL and Sybase but "randomly" aliassing looks
bad to me.
strcmp_null-OnUpdateErrorLog.patch
You should at least ad an "echo 'done';" or such to the expect
section to make sure it really works ...
I skipped the patches related to build system and such and maybe skipped
one or two by accident ....
johannes
Johannes,
Johannes Schlüter wrote:
Raphael,
I went over your patches from
http://patch-tracker.debian.org/package/php5/5.3.1-2 and did quick
reviews (didn't apply / test them or anything ...)
Although I would have preferred you wait for me to submit each patch
individually with enough information (because I've only been like two years
co-maintaining the packages and most patches were added by others, and most
before I joined), thanks. I'll try to comment on each but I don't know about
them all and can't check them right now (I'm short on time).
Here some comments:
004-ldap_fix.patch:
Do you have a test for this? when does it happen that ldap_value is
NULLhappen? - If that's an issue we should add it to our tree...
I don't have a test, but here's the bug report that lead to that patch:
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=205405
013-force_getaddrinfo.patch:
Why doesn't the check work on Debian out of the box? On my (not
Debian) Linux boxes it works. anything needed for people compiling PHP
on Debian themselves?
A 2004 changelog entry says:
- Add 013-force_getaddrinfo.patch, so that getaddrinfo support is
always enabled (instead of doing check during build).
So it looks it might be ok to disable it.
019-z_off_t_as_long.patch
Certainly nothing for our tree
I found these two entries:
-
Re-enable 019-z_off_t_as_long.patch, which is needed to make sure
that LFS-enabled SAPIs can still use zlib file functions correctly. -
Add 019-z_off_t_as_long.patch, including local headers for zlib,
forcing off_t = long for gzip file functions, however disable it
for now, as we'll only need it if we reenable LFS (closes: #208608)
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=208608
(N.B. at Debian we do enable LFS -- see below)
034-apache2_umask_fix.patch
036-fd_setsize_fix.patch
Any reproduce case where this is an issue?
- Added several patches, yanked from the Fedora PHP sources:
- 034-apache2_umask_fix.patch, fixes umask not being properly reset
after each request (closes: #286225) - 036-fd_setsize_fix.patch, fixes misuse of FD_SET()
- 034-apache2_umask_fix.patch, fixes umask not being properly reset
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=286225
043-recode_size_t.patch
Why is that needed? The name of the patch suggests that str_len is
used in a place where an size_t is expected but then the location
of this check is wrong. Additionally in case this check is tirggered
the function will return without error message.
- Add 043-recode_size_t.patch to fix 32/64-bit issues causing the recode
extension to segfault on alpha/amd64/ia64 (closes: #294986)
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=294986
and later...
- zend_parse_parameters does not handle size_t's, causing issues with
043-recode_size_t.patch and segmentation faults for recode-using pages.
changed problematic parameters back to "int" and added an overflow
check.
thanks to Thomas Stegbauer, Tim Dijkstra, Bart Cortooms, Sebastian
Göbel,
and Vincent Tondellier for their reports. closes: #459020.
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=459020
045-exif_nesting_level.patch
How did you decide on the limit of 250?
- Add 045-exif_nesting_level.patch to bump the exif header parsing max
nesting level to something that actually works with most JPEG images.
047-zts_with_dl.patch
I don't support this.dl()can be used by CLI users for loading Gtk or
such on demand but avoiding to load (and inititalize) all the Gtk
stuff every time they use PHP and avoid messing with config files...052-phpinfo_no_configure.patch
Neither do I support this. Users might benefit when building own
modules or might be interested in the configure line for other
reasons ...
I will later be commenting on these two.
053-extension_api.patch
When adding random options to command line tools please mark them as
Debian specific. Additionally the man page should be updated.
Indeed. In this case I'd prefer if this option is added upstream.
The --phpapi option is used to determine the path to the extensions
directory. In the case of architectures where LFS is enabled, the phpapi
value is appended '+lfs'.
100-recode_is_shared.patch
The conflict between MySQL and recode should only happen with an old
libmysql (3.23?) not sure about imap ... but in your case the patch
might make sense, while I won't directly apply it to our tree as
usually people will build extensions to load them together...
I will later be commenting on this one.
101-sqlite_is_shared.patch
Not sure if we do any modifications to SQLite but in general we prefer
to use the bundled lib so users get the same behavior on all
platforms.
We use the shared library to avoid extra work on security issues and general
maintenance.
108-64_bit_datetime.patch
116-posixness_fix.patch
Why is that needed on our system? - I never saw an issue about these
and if they are missing there should be build issues.
I will later be commenting on these two.
use_embedded_timezonedb.patch
Like with the SQLite note above we prefer to have the same behavior
over all platforms by using a common time database.
Same here, updating the tz data on PHP everytime would be a pain. And yes,
at some point we used the PECL extension but was later dropped in favour of
the above patch.
force_libmysqlclient_r.patch
Why are you using the reentrant version of this library which is
slower than the "regular" one? Did you consider mysqlnd?
- New patch: force_libmysqlclient_r.patch, forcing the build system
to link against the threadsafe libmysqlclient without having to enable
the other zts features in php. This is required since the apr libraries
are now linking against this as well and mysql exports the same symbols
from both libraries. Thanks to Stefan Fritsch (closes: #469081).
I don't think we are going to switch to mysqlnd as it would require more
maintenance from our part and more work in case of security issues.
009_ob-memory-leaks.patch
Any test case? (While this looks good on first sight)
I think there was one for the original patch (as you can see the patch was
taken from php). Later, when we refreshed the patches it was still applying
at a different part of the same file and a quick review demonstrated it
looked correct.
exif_read_data-segfault.patch
Any sample case for this? Any bug report?
Looks like somebody forgot to remove it, as the code is already upstream
(and the context that can be seen is exactly the same check). This is the
fix for CVE-2009-2687.
sybase-alias.patch
I have no idea about MS SQL and Sybase but "randomly" aliassing looks
bad to me.
Since the mssql and sybase extensions both use freetds their internal
behaviour is the same. As such, the aliases were added to preserve
compatibility with applications using the sybase set of functions.
strcmp_null-OnUpdateErrorLog.patch
You should at least ad an "echo 'done';" or such to the expect
section to make sure it really works ...
Could be added, yes, but in case it fails it would lead to a segfault.
I skipped the patches related to build system and such and maybe skipped
one or two by accident ....
Some of the other patches include:
libdb_is_-ldb
115-autoconf_ftbfs.patch
fix_broken_upstream_tests.patch
bad_whatis_entries.patch
Which should all be merged.
Here's an online copy of the latest changelog:
http://packages.debian.org/changelogs/pool/main/p/php5/current/changelog.html
Cheers,
Raphael Geissert - Debian Developer
www.debian.org - get.debian.net
16.1.2010 20:10, Raphael Geissert wrote:
Some of the other patches include:
libdb_is_-ldb
Why? Potentially breaks things when you assume db/ being correct place..
115-autoconf_ftbfs.patch
Hell no. You're breaking the configure again with this crap. I already reverted
the idiocy once, don't even think about doing this shit again. PHP configure
works properly only with autoconf-2.13 which was the last working autoconf.
fix_broken_upstream_tests.patch
The soap thing seems ok.
But why are you disabling ext/standard/tests/general_functions/phpinfo.phpt ??
That test passes fine for me everywhere. The patch says:
test suite's handling of "%s" is incompatible with this test"
Eh?
bad_whatis_entries.patch
Looks ok.
Which should all be merged
And next time, please include direct links to the patches, makes easier to
follow these and comment on them. :)
--Jani
115-autoconf_ftbfs.patch
Hell no. You're breaking the configure again with this crap. I already reverted the idiocy once, don't even think about doing this shit again. PHP configure works properly only with autoconf-2.13 which was the last working autoconf.
Which autoconf was the last "working" one is a matter of opinion at this point. The Autoconf people would love for us to stop using 2.13 - the only reason it still exists is because we use it. (Or so I'm told, anyway, I don't know this for a fact firsthand.) Personally, I'd rather see PHP go to CMake. CMake does have its own host of problems, but they could be dealt with. And yes, I would volunteer to revive the CMake effort.
-- Gwynne
115-autoconf_ftbfs.patch
Hell no. You're breaking the configure again with this crap. I already reverted the idiocy once, don't even think about doing this shit again. PHP configure works properly only with autoconf-2.13 which was the last working autoconf.Which autoconf was the last "working" one is a matter of opinion at this point. The Autoconf people would love for us to stop using 2.13 - the only reason it still exists is because we use it. (Or so I'm told, anyway, I don't know this for a fact firsthand.) Personally, I'd rather see PHP go to CMake. CMake does have its own host of problems, but they could be dealt with. And yes, I would volunteer to revive the CMake effort.
strong +1 from me
would be glad to help
17.1.2010 0:29, Alexey Zakhlestin wrote:
115-autoconf_ftbfs.patch
Hell no. You're breaking the configure again with this crap. I already reverted the idiocy once, don't even think about doing this shit again. PHP configure works properly only with autoconf-2.13 which was the last working autoconf.Which autoconf was the last "working" one is a matter of opinion at this point. The Autoconf people would love for us to stop using 2.13 - the only reason it still exists is because we use it. (Or so I'm told, anyway, I don't know this for a fact firsthand.) Personally, I'd rather see PHP go to CMake. CMake does have its own host of problems, but they could be dealt with. And yes, I would volunteer to revive the CMake effort.
strong +1 from me
would be glad to help
Strong -1000 from me, there's nothing wrong with autoconf.
--Jani
Jani Taskinen wrote:
16.1.2010 20:10, Raphael Geissert wrote:
Some of the other patches include:
libdb_is_-ldbWhy? Potentially breaks things when you assume db/ being correct place..
Do you have an example of any actual case?
115-autoconf_ftbfs.patch
Hell no. You're breaking the configure again with this crap. I already
reverted the idiocy once, don't even think about doing this shit again.
PHP configure works properly only with autoconf-2.13 which was the last
working autoconf.
Can you tell me what exactly we are breaking? divert calls should only be
used internally by autoconf and the, apparently useless, usage of them in
php makes it fail to build with any other autoconf.
fix_broken_upstream_tests.patch
The soap thing seems ok.
But why are you disabling
ext/standard/tests/general_functions/phpinfo.phpt ?? That test passes fine
for me everywhere. The patch says:test suite's handling of "%s" is incompatible with this test"
Eh?
I've never seen that test actually work but I don't remember the details
right now. Will check it next time and report.
Which should all be merged
And next time, please include direct links to the patches, makes easier to
follow these and comment on them. :)
Ok.
Cheers,
Raphael Geissert - Debian Developer
www.debian.org - get.debian.net
Raphael Geissert wrote:
Can you tell me what exactly we are breaking? divert calls should only be
used internally by autoconf and the, apparently useless, usage of them in
php makes it fail to build with any other autoconf.
Have a look in the archives. I tried getting the divert stuff working
correctly and it broke a bunch of stuff. The current attempt is in
HEAD, but it needs more work. And yes, the Debian patch the way it is
now definitely won't work in the general case.
-Rasmus
Jani Taskinen wrote:
16.1.2010 20:10, Raphael Geissert wrote:
Some of the other patches include:
libdb_is_-ldbWhy? Potentially breaks things when you assume db/ being correct place..
Do you have an example of any actual case?
Do you have an example where it's actually needed?
Can you tell me what exactly we are breaking? divert calls should only be
used internally by autoconf and the, apparently useless, usage of them in
php makes it fail to build with any other autoconf.
If you remove them, things break. I don't remember right now why, but it
did, Rasmus tried this already and failed. Search the mailing lists for
more. His commit message is a bit weird, I think it should say "2.6x is
broken in so many ways" rather than 2.13:
http://svn.php.net/viewvc?view=revision&revision=291414
--Jani
Jani Taskinen wrote:
Jani Taskinen wrote:
16.1.2010 20:10, Raphael Geissert wrote:
Some of the other patches include:
libdb_is_-ldbWhy? Potentially breaks things when you assume db/ being correct place..
Do you have an example of any actual case?
Do you have an example where it's actually needed?
Can you tell me what exactly we are breaking? divert calls should only be
used internally by autoconf and the, apparently useless, usage of them in
php makes it fail to build with any other autoconf.If you remove them, things break. I don't remember right now why, but it
did, Rasmus tried this already and failed. Search the mailing lists for
more. His commit message is a bit weird, I think it should say "2.6x is
broken in so many ways" rather than 2.13:
Not really. It is only maintaining backward compatibility with 2.13
that is broken. In HEAD where we can drop that version entirely, things
are fine. Since we can't do that in 5.3, I gave up on the changes for
this branch.
-Rasmus
Can you tell me what exactly we are breaking? divert calls should only be
used internally by autoconf and the, apparently useless, usage of them in
php makes it fail to build with any other autoconf.
If you remove them, things break. I don't remember right now why, but it did, Rasmus tried this already and failed. Search the mailing lists for more. His commit message is a bit weird, I think it should say "2.6x is broken in so many ways" rather than 2.13:
If I remember right, removing the diverts makes the ./configure --help output no longer pretty. And 2.6x and 2.13 are both broken in their own lists of ways. Though I thought the use of high-numbered diversions was actually a supported thing - or was that only in 2.13?
-- Gwynne
Gwynne Raskind wrote:
Though I thought the use of high-numbered diversions was
actually a supported thing - or was that only in 2.13?
That argument is not supported by the autoconf manual. Please see the
discussion at
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=542906#10
Cheers,
Raphael Geissert - Debian Developer
www.debian.org - get.debian.net
Raphael Geissert wrote:
Gwynne Raskind wrote:
Though I thought the use of high-numbered diversions was
actually a supported thing - or was that only in 2.13?That argument is not supported by the autoconf manual. Please see the
discussion at
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=542906#10
Well, the autoconf maintainers have a different opinion on that. They
don't see anything wrong with doing diverts if there is no other way to
accomplish something.
You can read this thread about my attempts to create a portable autoconf
setup that works both in autoconf-2.60+ and previous versions. There
were various ideas, but in the end none of them proved to be reliable.
We need diversions prior to 2.60 or whichever version introduced
AC_PRESERVE_HELP_ORDER. In autoconf versions that have
AC_PRESERVE_HELP_ORDER we can use that and we can drop the diversions
entirely.
Renumbering the diversions and the various other attempts people have
made doesn't work.
-Rasmus
-Rasmus
Rasmus Lerdorf wrote:
Raphael Geissert wrote:
Gwynne Raskind wrote:
Though I thought the use of high-numbered diversions was
actually a supported thing - or was that only in 2.13?That argument is not supported by the autoconf manual. Please see the
discussion at
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=542906#10Well, the autoconf maintainers have a different opinion on that. They
don't see anything wrong with doing diverts if there is no other way to
accomplish something.You can read this thread about my attempts to create a portable autoconf
setup that works both in autoconf-2.60+ and previous versions. There
were various ideas, but in the end none of them proved to be reliable.We need diversions prior to 2.60 or whichever version introduced
AC_PRESERVE_HELP_ORDER. In autoconf versions that have
AC_PRESERVE_HELP_ORDER we can use that and we can drop the diversions
entirely.Renumbering the diversions and the various other attempts people have
made doesn't work.
Oops, missed the thread link:
http://lists.gnu.org/archive/html/autoconf/2009-11/msg00100.html
Rasmus Lerdorf wrote:
Rasmus Lerdorf wrote:
You can read this thread about my attempts to create a portable autoconf
setup that works both in autoconf-2.60+ and previous versions. There
were various ideas, but in the end none of them proved to be reliable.We need diversions prior to 2.60 or whichever version introduced
AC_PRESERVE_HELP_ORDER. In autoconf versions that have
AC_PRESERVE_HELP_ORDER we can use that and we can drop the diversions
entirely.Renumbering the diversions and the various other attempts people have
made doesn't work.Oops, missed the thread link:
http://lists.gnu.org/archive/html/autoconf/2009-11/msg00100.html
I see, thanks for the pointer.
Cheers,
Raphael Geissert - Debian Developer
www.debian.org - get.debian.net
Jani Taskinen wrote:
Jani Taskinen wrote:
16.1.2010 20:10, Raphael Geissert wrote:
Some of the other patches include:
libdb_is_-ldbWhy? Potentially breaks things when you assume db/ being correct place..
Do you have an example of any actual case?
Do you have an example where it's actually needed?
Yes, every time a new libdb version is released we would have to patch the
config.m4 file. Instead, what this patch does is make the config.m4 pick the
versionless files.
For example, 5.3.1's ext/dba/config.m4 only knows up to db4.6 but at Debian
we have already moved to 4.7 and are switching to 4.8.
Can you tell me what exactly we are breaking? divert calls should only be
used internally by autoconf and the, apparently useless, usage of them in
php makes it fail to build with any other autoconf.If you remove them, things break. I don't remember right now why, but it
did, Rasmus tried this already and failed. Search the mailing lists for
more. His commit message is a bit weird, I think it should say "2.6x is
broken in so many ways" rather than 2.13:
Well, we use autoconf2.63 (2.13 is soon to be dropped) and we need those
changes to make it work.
Cheers,
Raphael Geissert - Debian Developer
www.debian.org - get.debian.net
Raphael Geissert wrote:
Johannes Schlüter wrote:
Although I would have preferred you wait for me to submit each patch
individually with enough information (because I've only been like two
years co-maintaining the packages and most patches were added by others,
and most before I joined), thanks. I'll try to comment on each but I don't
know about them all and can't check them right now (I'm short on time).
I haven't had much time to work on merging those patches, but will try to
eventually get to do it.
047-zts_with_dl.patch
I don't support this.dl()can be used by CLI users for loading Gtk or
such on demand but avoiding to load (and inititalize) all the Gtk
stuff every time they use PHP and avoid messing with config files...
The least I can say is that that patch is obsolete. We don't build the
apache modules with ZTS.
However, I don't understand what you are saying about Gtk. The patch doesn't
change the behaviour of the cli/cgi/embed SAPIs. It only re-enables dl() on
any SAPI even with ZTS.
052-phpinfo_no_configure.patch
Neither do I support this. Users might benefit when building own
modules or might be interested in the configure line for other
reasons ...
The main issue is that we build the different SAPIs with different configure
flags. This seems to have lead to bogus bug reports in the past and
confusion (--disable-/--without- in certain SAPIs). For example, the
shared extensions are built when building the apache SAPI, the cgi build
options are also massaged after a first build, etc.
053-extension_api.patch
When adding random options to command line tools please mark them as
Debian specific. Additionally the man page should be updated.Indeed. In this case I'd prefer if this option is added upstream.
The --phpapi option is used to determine the path to the extensions
directory. In the case of architectures where LFS is enabled, the phpapi
value is appended '+lfs'.
Is there any objection to making this change? (only to trunk, I assume.)
100-recode_is_shared.patch
The conflict between MySQL and recode should only happen with an old
libmysql (3.23?) not sure about imap ... but in your case the patch
might make sense, while I won't directly apply it to our tree as
usually people will build extensions to load them together...
Shouldn't checking for $PHP_IMAP_SHARED != "yes" (and the same for mysql)
work? if either recode or imap/mysql are being built as shared I don't think
there's any reason to abort the build. A warning should be enough, don't you
think?
108-64_bit_datetime.patch
116-posixness_fix.patch
Why is that needed on our system? - I never saw an issue about these
and if they are missing there should be build issues.I will later be commenting on these two.
Haven't had time to check, deferring.
Did you consider mysqlnd?
Leaving any security concern aside, there are at the moment some missing
features and behaviour changes that I don't feel very comfortable with. In a
future release we might switch, but won't happen for Squeeze.
Some of the other patches include:
libdb_is_-ldb
Not yet merged, but at least support for db-4.{7,8} was added.
115-autoconf_ftbfs.patch
as per discussion we will continue carrying it
fix_broken_upstream_tests.patch
not completely merged/resolved
bad_whatis_entries.patch
merged
Cheers,
Raphael Geissert
100-recode_is_shared.patch
The conflict between MySQL and recode should only happen with an old
libmysql (3.23?) not sure about imap ... but in your case the patch
might make sense, while I won't directly apply it to our tree as
usually people will build extensions to load them together...Shouldn't checking for $PHP_IMAP_SHARED != "yes" (and the same for mysql)
work? if either recode or imap/mysql are being built as shared I don't
think
there's any reason to abort the build. A warning should be enough, don't
you
think?
Hi.
I've just run into this issue.
with debian (and dotdeb) one can have both recode and imap configured, with
vanilla trunk, I get an "configure: error: recode extension can not be
configured together with: imap"
which is in sync with the docs:
http://www.php.net/manual/en/recode.installation.php
"The IMAP, recode, YAZ and Cyrus extensions cannot be used in conjuction,
because they share the same internal symbols. Note: Yaz 2.0 and above does
not suffer from this problem."
it seems that the guys at fedora also "fixed" this:
http://osdir.com/ml/fedora-extras-commits/2009-02/msg01651.html
could somebody look into this? I think we should either enable this (throw a
warning not an error), or we should tell the distros to don't do this.
Tyrael
use_embedded_timezonedb.patch
Like with the SQLite note above we prefer to have the same behavior
over all platforms by using a common time database.Same here, updating the tz data on PHP everytime would be a pain. And yes,
at some point we used the PECL extension but was later dropped in favour of
the above patch.
Please reinstate the PECL extension approach. This can go to volatile
just like the tzdata package.
regards,
Derick
--
http://derickrethans.nl | http://xdebug.org
Like Xdebug? Consider a donation: http://xdebug.org/donate.php
twitter: @derickr and @xdebug