Welcome! Log In Create A New Profile

Advanced

[PHP-DEV] RE: [PHP-CVS] com php-src: fix default args for --with-pcre-valgrind: ext/pcre/config0.m4

Posted by Anatol Belski 
Hi Chris,

> -----Original Message-----
> From: Christopher Jones [mailto:[email protected]]
> Sent: Thursday, July 20, 2017 2:11 AM
> To: Anatol Belski <[email protected]>; php-cvs@lists.php.net; Tianfang Yang
> <[email protected]>
> Subject: Re: [PHP-CVS] com php-src: fix default args for --with-pcre-valgrind:
> ext/pcre/config0.m4
>
> Hi Anatol,
>
> 7.2 & 7.3 builds now fails for me on macOS and Oracle Linux 7 with:
>
> checking for Valgrind headers location... configure: error: Could not find
> valgrind/memcheck.h
> Configure failed
>
> Do we really want users to have to explicitly disable PCRE valgrind when they are
> using --enable-debug?
>
It wasn't by default in the first version, later it was suggested by Nikita and for debug mode certainly makes sense, as for me. The reason behind this option is the upgrade of the bundled PCRE, which shows a lot of false positives without the integrated Valgrind support. Also I was basing on the fact the debug mode is actually developers only, normal use wouldn't be affected therefore. Why would users do debug builds?

On a dev machine Valgrind would be anyway present. For developers it's best to have it by default, as PCRE usage is spread around the core here and there. Bug fixing also does normally involve valgrind checks. I wouldn't see the option itself as a big question, either way. If many people find the default enablement unsuitable, so the behavior can be reversed.

Regards

Anatol

> Chris
>
> A sample macOS config.nice:
>
> '/Users/cjones/php-7.2/configure' \
> '--enable-debug' \
> '--disable-phpdbg' \
> '--prefix=/Users/cjones/p/php72-ic121' \ '--with-
> apxs2=/Users/cjones/opt/apache24/bin/apxs' \ '--enable-fpm' \ '--disable-cgi' \
> '--enable-pdo' \ '--with-pdo-oci=instantclient,/Users/cjones/instantclient_12_1'
> \ '--with-oci8=shared,instantclient,/Users/cjones/instantclient_12_1' \ '--with-
> pdo-mysql=mysqlnd' \ '--with-mysqli=mysqlnd' \ '--enable-opcache' \ '--without-
> iconv' \ '--with-zlib' \ '--enable-mbstring' \ '--with-gd' \ '--with-jpeg-dir' \ '--with-
> png-dir' \ '--enable-gd-native-ttf' \ "[email protected]"
>
>
> On 14/7/17 4:28 am, Anatol Belski wrote:
> > Commit: 24de0fe9f4f92178adba27e2f1353e97a956b4a8
> > Author: Anatol Belski <[email protected]> Thu, 13 Jul 2017 20:28:10 +0200
> > Parents: 850bb998d9664e849ed743ea031dd0ee2a64cc9d
> > Branches: master
> >
> > Link: https://urldefense.proofpoint.com/v2/url?u=http-3A__git.php.net_-
> 3Fp-3Dphp-2Dsrc.git-3Ba-3Dcommitdiff-3Bh-
> 3D24de0fe9f4f92178adba27e2f1353e97a956b4a8&d=DwIFaQ&c=RoP1YumCXC
> gaWHvlZYR8PQcxBKCX5YTpkKY057SbK10&r=lLpUdeB4xTiOOWD6yGzxPFv2SHvP
> zg3yLT7kvD-
> ZfyU&m=SawjNjtteypZYYe2cUbXcW453R3ae9ZjHKFTbzos0x4&s=9rNX9_T0K9PlL
> vcxLSIvOtmCyyyAuCNztHLuvPMzLKs&e=
> >
> > Log:
> > fix default args for --with-pcre-valgrind
> >
> > Changed paths:
> > M ext/pcre/config0.m4
> >
> >
> > Diff:
> > diff --git a/ext/pcre/config0.m4 b/ext/pcre/config0.m4 index
> > aa8cd30..cc9f1b2 100644
> > --- a/ext/pcre/config0.m4
> > +++ b/ext/pcre/config0.m4
> > @@ -78,15 +78,20 @@ PHP_ARG_WITH(pcre-jit,,[ --with-pcre-jit Enable
> PCRE JIT functionality]
> > fi
> > fi
> >
> > -PHP_ARG_WITH(pcre-valgrind,,[ --with-pcre-valgrind=DIR
> > - Enable PCRE valgrind support. Developers only!], $PHP_DEBUG,
> no)
> > + if test "$PHP_DEBUG" != "no" && test "$PHP_DEBUG" != "0"; then
> > + PHP_ARG_WITH(pcre-valgrind,,[ --with-pcre-valgrind=DIR
> > + Enable PCRE valgrind support. Developers
> > + only!], yes, no) else
> > + PHP_ARG_WITH(pcre-valgrind,,[ --with-pcre-valgrind=DIR
> > + Enable PCRE valgrind support. Developers
> > + only!], no, no) fi
> >
> > if test "$PHP_PCRE_REGEX" != "yes" && test "$PHP_PCRE_REGEX" != "no";
> then
> > AC_MSG_WARN([PHP is going to be linked with an external PCRE, --with-
> pcre-valgrind has no effect])
> > else
> > - if test "$PHP_PCRE_VALGRIND" = "no" && test "$PHP_DEBUG" != "no";
> then
> > + if test "$PHP_PCRE_VALGRIND" = "no" && test "$PHP_DEBUG" != "0";
> > + then
> > AC_MSG_NOTICE([PCRE Valgrind support is disabled for debug build])
> > - elif test "$PHP_PCRE_VALGRIND" != "no" || test "$PHP_DEBUG" != "no";
> then
> > + elif test "$PHP_PCRE_VALGRIND" != "no" || test "$PHP_DEBUG" !=
> > + "0"; then
> > PHP_PCRE_VALGRIND_INCDIR=
> > AC_MSG_CHECKING([for Valgrind headers location])
> > for i in $PHP_PCRE_VALGRIND $PHP_PCRE_VALGRIND/include
> > $PHP_PCRE_VALGRIND/local/include /usr/include /usr/local/include; do
> >
> >
> > --
> > PHP CVS Mailing List
> > (https://urldefense.proofpoint.com/v2/url?u=http-3A__www.php.net_&d=Dw
> >
> IFaQ&c=RoP1YumCXCgaWHvlZYR8PQcxBKCX5YTpkKY057SbK10&r=lLpUdeB4xTi
> OOWD6y
> > GzxPFv2SHvPzg3yLT7kvD-
> ZfyU&m=SawjNjtteypZYYe2cUbXcW453R3ae9ZjHKFTbzos0
> > x4&s=tF4RZ_40bq3PPS5hVSOQrsVCRVqvVTHQTov804ZP3yY&e= ) To
> unsubscribe,
> > visit:
> > https://urldefense.proofpoint.com/v2/url?u=http-3A__www.php.net_unsub.
> >
> php&d=DwIFaQ&c=RoP1YumCXCgaWHvlZYR8PQcxBKCX5YTpkKY057SbK10&r=lL
> pUdeB4x
> > TiOOWD6yGzxPFv2SHvPzg3yLT7kvD-
> ZfyU&m=SawjNjtteypZYYe2cUbXcW453R3ae9ZjH
> > KFTbzos0x4&s=3ms6yngzhpGmsTx_FB72A4Ples9VsY1TU9hEL_BpyCc&e=
> >
>
> --
> http://twitter.com/ghrd
>
>
> --
> PHP CVS Mailing List (http://www.php.net/)
> To unsubscribe, visit: http://www.php.net/unsub.php
On 20/7/17 10:31 am, Anatol Belski wrote:
> Hi Chris,
>
>> -----Original Message-----
>> From: Christopher Jones [mailto:[email protected]]
>> Sent: Thursday, July 20, 2017 2:11 AM
>> To: Anatol Belski <[email protected]>; php-cvs@lists.php.net; Tianfang Yang
>> <[email protected]>
>> Subject: Re: [PHP-CVS] com php-src: fix default args for --with-pcre-valgrind:
>> ext/pcre/config0.m4
>>
>> Hi Anatol,
>>
>> 7.2 & 7.3 builds now fails for me on macOS and Oracle Linux 7 with:
>>
>> checking for Valgrind headers location... configure: error: Could not find
>> valgrind/memcheck.h
>> Configure failed
>>
>> Do we really want users to have to explicitly disable PCRE valgrind when they are
>> using --enable-debug?
>>
> It wasn't by default in the first version, later it was suggested by Nikita and for debug mode certainly makes sense, as for me. The reason behind this option is the upgrade of the bundled PCRE, which shows a lot of false positives without the integrated Valgrind support. Also I was basing on the fact the debug mode is actually developers only, normal use wouldn't be affected therefore. Why would users do debug builds?
>
> On a dev machine Valgrind would be anyway present. For developers it's best to have it by default, as PCRE usage is spread around the core here and there. Bug fixing also does normally involve valgrind checks. I wouldn't see the option itself as a big question, either way. If many people find the default enablement unsuitable, so the behavior can be reversed.
>
> Regards
>
> Anatol
>
I'm not totally convinced merging debug-level arguments makes sense (why not always enable --enable-phpdbg-debug too?) But there are more important
things to worry about, so I won't stress.

BTW, './configure --help' prints the --with-pcre-valgrind=DIR help twice.

Chris

--
http://twitter.com/ghrd


--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
On 25/07/17 00:31, Christopher Jones wrote:
>
>
> On 20/7/17 10:31 am, Anatol Belski wrote:
>> Hi Chris,
>>
>>> -----Original Message-----
>>> From: Christopher Jones [mailto:[email protected]]
>>> Sent: Thursday, July 20, 2017 2:11 AM
>>> To: Anatol Belski <[email protected]>; php-cvs@lists.php.net; Tianfang Yang
>>> <[email protected]>
>>> Subject: Re: [PHP-CVS] com php-src: fix default args for
>>> --with-pcre-valgrind:
>>> ext/pcre/config0.m4
>>>
>>> Hi Anatol,
>>>
>>> 7.2 & 7.3 builds now fails for me on macOS and Oracle Linux 7 with:
>>>
>>>     checking for Valgrind headers location... configure: error: Could
>>> not find
>>> valgrind/memcheck.h
>>>     Configure failed
>>>
>>> Do we really want users to have to explicitly disable PCRE valgrind
>>> when they are
>>> using --enable-debug?
>>>
>> It wasn't by default in the first version, later it was suggested by
>> Nikita and for debug mode certainly makes sense, as for me. The reason
>> behind this option is the upgrade of the bundled PCRE, which shows a
>> lot of false positives without the integrated Valgrind support. Also I
>> was basing on the fact the debug mode is actually developers only,
>> normal use wouldn't be affected therefore. Why would users do debug
>> builds?
>>
>> On a dev machine Valgrind would be anyway present. For developers it's
>> best to have it by default, as PCRE usage is spread around the core
>> here and there. Bug fixing also does normally involve valgrind checks.
>> I wouldn't see the option itself as a big question, either way. If
>> many people find the default enablement unsuitable, so the behavior
>> can be reversed.
>>
>> Regards
>>
>> Anatol
>>
> I'm not totally convinced merging debug-level arguments makes sense (why
> not always enable --enable-phpdbg-debug too?)  But there are more
> important things to worry about, so I won't stress.
>
> BTW, './configure --help' prints the --with-pcre-valgrind=DIR help twice.
>
> Chris
>

Just stumbled over this one, too.

I don't think it's very nice to suddenly let ./configure --enable-debug
die when valgrind headders are not present, and I didn't request
valgrind support in ext/pcre.


Suddenly all of my travis builds failed, for no apparent reason, despite
--disable-all

I think it should only fail hard, if explicitly requested.


--
Regards,
Mike
Sorry, only registered users may post in this forum.

Click here to login