[WIP] Migrate more checks to the pkg-config macro #3654

eli-schwartz · 2018-11-07T18:37:43Z

Followup on #3632.

Now that there's a nice pkg.m4 macro to use, start using it more consistently.

Currently implemented:

curl (now requires pkg-config)
openssl (now requires pkg-config)
pcre (migrate current pkg-config checks to the macro, do not drop pcre-config code)
pdo_pgsql's openssl support is fake and can be dropped

eli-schwartz · 2018-11-07T18:39:09Z

Will look at doing even more things.

/cc @nikic

ext/pdo_pgsql/config.m4

nikic · 2018-11-07T18:44:30Z

The travis/compile.sh file needs to be changed to use the new option names. Also missed that for --enable-freetype, oops.

ext/curl/config.m4

nikic · 2018-11-07T18:53:34Z

ext/pcre/config0.m4

@@ -11,21 +11,10 @@ PHP_ARG_WITH(pcre-jit,,[  --with-pcre-jit         Enable PCRE JIT functionality

  if test "$PHP_PCRE_REGEX" != "yes" && test "$PHP_PCRE_REGEX" != "no"; then

+    # FIXME: don't define "I'm not bundled" as only versions installed in /usr


Not sure what the best way to do this is. PCRE is always present and should default to the bundled lib. Maybe replace the existing --with-pcre-regex=DIR by a --disable-bundled-pcre flag?

eli-schwartz · 2018-11-07T18:55:23Z

Why does acinclude.m4's PHP_SETUP_OPENSSL check, use --cflags-only-I, then save the exact same thing to "INCDIR" without the leading -I?

This could use PKG_CHECK_VAR for the latter, but it sort of complicates the logic on both counts...

nikic · 2018-11-07T19:11:05Z

@eli-schwartz From what I can tell INCDIR is not necessary if this is reduced just to the pkg-config code. It should be fine to drop it and only use INCS.

ext/curl/config.m4

curl 7.15.1 in December 2006 first added pkg-config support, which is earlier than the minimum supported version for php. This should therefore be uiversally supported.

logic is still entwined with bundling code

This check was added in 0db3738 and greps for a private implementation detail of the postgres headers, removed in postgres/postgres@3c4768d It hasn't worked as intended for 12 years, and can safely be assumed to not be needed.

eli-schwartz · 2018-11-09T14:58:18Z

I have no clue why the openssl change fails to compile, when it works locally... also forcing travis to test each commit is painful. :(

acinclude.m4

openssl 0.9.8 in July 2005 first added pkg-config support, which is earlier than the minimum supported version for php. This should therefore be uiversally supported.

nikic · 2018-11-09T16:18:06Z

One thing I'm wondering about is the option naming. Should these options stay as --with-curl etc or become --enable-curl? It's unclear to me whether our convention is that --with should be used for anything involving a library, or only if the --with option accepts a path. Keeping the --with names will certainly reduce ecosystem churn, as people who use them without an explicit path (which should be the majority) will not have to adjust their build configuration. On the other hand it might be surprising if these options no longer accept a path but keep the old name.

@weltling @cmb69 What do you think about this?

By convention it probably makes sense to stick with this even when dropping the *-dir=DIR part. Fix the travis script, which is either way using the wrong version, with *-dir See: php#3632 (comment) https://autotools.io/autoconf/arguments.html

eli-schwartz · 2018-11-09T16:36:47Z

It really should be switched back: https://autotools.io/autoconf/arguments.html

petk · 2018-11-09T17:23:52Z

Convention so far was that --enable-foo didn't require anything to have from the system, and --with-foo required possible system dependency. It's probably most simple to understand it like --enable-feature, --with-package.

Where the package was bundled and shipped with php-src (like ~~pcre~~, zip redacted) the --enable-foo was also used.

P.S.: I'm in favor of improvements and moving the build system further otherwise. It's essential, and pkg-config seems very good addition in general otherwise :)

cmb69 · 2018-11-09T18:07:24Z

I was under the impression, that we're using --enable-* if no argument is supported, and --with-* otherwise (this might be the convention for our Windows builds). This was obviously wrong. Following a common convention appears to be reasonable, but we'd have to change quite some stuff, since several extensions use --with-* to get enabled (e.g. --with-sodium, --with-tidy, --with-gd). Some others (e.g. ext/soap) are enabled with --enable-* (looking for a default libxml2), and have an additional --with-libxml-dir option to specify a custom path. It seems to me that we should make all bundled extensions consistent, if we change anything. Not sure if we'd better wait for PHP 8 with this.

Wrt. --enable-freetype I agree that we'd better call it --with-freetype (but certainly not --with-freetype-dir as before).

nikic · 2018-11-13T16:19:17Z

acinclude.m4

-
-    if test "$PHP_OPENSSL_DIR" = "yes"; then
-      PHP_OPENSSL_DIR="/usr/local/ssl /usr/local /usr /usr/local/openssl"
+    if test -n "$OPENSSL_CFLAGS"; then


These test -n calls are probably redundant, PHP_EVAL_LIBLINE/PHP_EVAL_INCLUDE should be able to handle an empty string.

That's convenient. :)

nikic · 2018-11-13T16:24:24Z

ext/pcre/config0.m4

-        PCRE2_INC=`$PKG_CONFIG --cflags libpcre2-8`
-      fi
+      PKG_CHECK_MODULES([PCRE2], [libpcre2-8 >= 10.30], ,
+        AC_MSG_ERROR(PCRE2 version 10.30 or later is required to compile php with PCRE2 support))
    fi

    dnl PCRE2 in a non standard  prefix should still have its config tool.


The /usr check above and the whole if below can be dropped, as that's also fallback code.

So this would automatically try to find the system version, then fallback on bundled code only if the system one isn't found?

There's two level of checks there, first for yes/no, then for /usr, so unless --with-pcre-regex is specified with a directory, it would still use the bundled library.

And then it ignores the argument entirely? I'm thinking it's best to check for "external" here.

Or... yes would look for external libraries and fall back on the bundled one, external/internal could force one or the other. If we want to get fancy, that is.

nikic · 2018-11-13T16:32:04Z

Any opinions on how to handle the option for bundled PCRE? Should we replace --with-pcre-dir with --with-external-pcre? Or something like --with-pcre=external? Or --disable-bundled-pcre?

eli-schwartz · 2018-11-13T16:38:08Z

If we go with the first or third, we need two options, to also allow --with-pcre=[yes|no]

nikic · 2018-11-13T16:43:14Z

@eli-schwartz PCRE in particular is a required extension, which cannot be disabled.

Though we should also consider what we'll do with extensions that have bundled libraries but are optional. For example we have --disable-mbregex for mbregex and --with-onig to control external libonig. On the other hand for xmlrpc we only have --with-xmlrpc=DIR.

nikic · 2018-11-13T16:47:24Z

Overall I like --with-pcre=[yes|no|external]. For PCRE in particular the no case would generate an error.

eli-schwartz · 2018-11-13T16:59:14Z

"Include Perl Compatible Regular Expressions support." ==> "sorry, you tried to disable this option that you're allowed to disable, so we will now abort with fatal configure errors".

Yeah, that should be fixed. :p Let's go with --enable/--disable-bundled-regex.

petk · 2018-11-13T17:18:45Z

+1 for --with-pcre=[yes|/path/to/custom/build/pcrelib|no]

nikic · 2018-11-21T19:22:07Z

@petk The option to specify a path will no longer exist, because the path will now be provided through PKG_CONFIG_PATH or a similar environment variable. That's why the question is coming up at all :)

I don't strongly care either way how this is called, I'm fine with both --with-pcre=yes|no|external and --disable-bundled-pcre. We can adjust this later if the naming turns out to be inconsistent after more parts are migrated to pkg-config.

petk · 2018-11-26T02:12:52Z

Got it. Took me a while but yes, then the new key sounds like a good way to go. :) Basically, php configure options already use shared (--with-extension=shared or --enable-extension=shared) when the extension needs to be built as a shared (.so or .dll file), so adding more keys probably is one of the ways.

nikic · 2018-12-09T16:30:10Z

Applied pgsql change in bdd4eb2. Applied curl change in 78e4f04 with a tweak to remove the custom error message -- I think we should use the default error messages, which are more explicit about what you can do.

nikic · 2018-12-09T16:30:55Z

acinclude.m4

-    fi
+  if test "$PHP_OPENSSL" = "yes"; then
+    PKG_CHECK_MODULES([OPENSSL], [openssl >= 1.0.1], [found_openssl=yes],
+      [AC_MSG_ERROR([OpenSSL version 1.0.1 or greater required.])])


Something I missed before... PHP_SETUP_OPENSSL is also used in places where openssl is an optional extension, while this will make it a hard error. This should be using $2 and $3 as the success/failure actions.

nikic · 2018-12-26T12:54:26Z

Applied --with-freetype rename in 285a077 and PCRE migration in c1a22f3. I went with the --with-external-pcre name in the end.

nikic · 2019-01-11T10:51:16Z

openssl part merged in fe8fdfa, with tweaks to not always make this an error. I hope I got that right, as this ifelse() stuff is magic to me.

And with that, everything in this PR should be merged... we still have quite a few extensions to migrate to pkg-config though.

eli-schwartz · 2019-01-11T16:07:07Z

Awesome! Thanks.

May or may not get around to porting the rest in a third PR, but time will tell. :)

eli-schwartz commented Nov 7, 2018

View reviewed changes

ext/pdo_pgsql/config.m4 Outdated Show resolved Hide resolved

nikic reviewed Nov 7, 2018

View reviewed changes

ext/curl/config.m4 Outdated Show resolved Hide resolved

nikic reviewed Nov 7, 2018

View reviewed changes

petk added the Category: Build System label Nov 7, 2018

nikic reviewed Nov 7, 2018

View reviewed changes

ext/curl/config.m4 Outdated Show resolved Hide resolved

eli-schwartz force-pushed the pkg-config branch 5 times, most recently from bae4a43 to de96953 Compare November 7, 2018 20:32

eli-schwartz closed this Nov 7, 2018

eli-schwartz deleted the pkg-config branch November 7, 2018 20:33

eli-schwartz restored the pkg-config branch November 7, 2018 20:33

eli-schwartz reopened this Nov 7, 2018

eli-schwartz force-pushed the pkg-config branch from eb267ab to 4800539 Compare November 7, 2018 21:53

eli-schwartz added 3 commits November 8, 2018 14:55

ext/curl: port to pkg-config macro and consistently require its use

941a1da

curl 7.15.1 in December 2006 first added pkg-config support, which is earlier than the minimum supported version for php. This should therefore be uiversally supported.

ext/pcre: simplify pkg-config routine to use standard macro

5e81cf6

logic is still entwined with bundling code

ext/pdo_pgsql: drop unneeded code

d79ef88

This check was added in 0db3738 and greps for a private implementation detail of the postgres headers, removed in postgres/postgres@3c4768d It hasn't worked as intended for 12 years, and can safely be assumed to not be needed.

eli-schwartz force-pushed the pkg-config branch 2 times, most recently from d79ef88 to 941a1da Compare November 9, 2018 04:07

nikic reviewed Nov 9, 2018

View reviewed changes

acinclude.m4 Outdated Show resolved Hide resolved

ext/openssl: port to pkg-config macro and consistently require its use

708d739

openssl 0.9.8 in July 2005 first added pkg-config support, which is earlier than the minimum supported version for php. This should therefore be uiversally supported.

ext/gd: use --with instead of --enable

aa93cc0

By convention it probably makes sense to stick with this even when dropping the *-dir=DIR part. Fix the travis script, which is either way using the wrong version, with *-dir See: php#3632 (comment) https://autotools.io/autoconf/arguments.html

eli-schwartz force-pushed the pkg-config branch from 23f76f4 to aa93cc0 Compare November 9, 2018 16:36

nikic reviewed Nov 13, 2018

View reviewed changes

cmb69 mentioned this pull request Nov 27, 2018

Correctly check for pkg-config #1885

Closed

cmb69 mentioned this pull request Dec 5, 2018

[Bug #77241] ext/intl: Use pkg-config to detect icu #3701

Closed

nikic reviewed Dec 9, 2018

View reviewed changes

nikic closed this Jan 11, 2019

morozov mentioned this pull request Dec 17, 2019

Add PKG_CONFIG_PATH environment variables to phpbrew build process phpbrew/phpbrew#1085

Merged

		@@ -11,21 +11,10 @@ PHP_ARG_WITH(pcre-jit,,[ --with-pcre-jit Enable PCRE JIT functionality

		if test "$PHP_PCRE_REGEX" != "yes" && test "$PHP_PCRE_REGEX" != "no"; then

		# FIXME: don't define "I'm not bundled" as only versions installed in /usr

[WIP] Migrate more checks to the pkg-config macro #3654

[WIP] Migrate more checks to the pkg-config macro #3654

Uh oh!

Conversation

eli-schwartz commented Nov 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eli-schwartz commented Nov 7, 2018

Uh oh!

Uh oh!

nikic commented Nov 7, 2018

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eli-schwartz commented Nov 7, 2018

Uh oh!

nikic commented Nov 7, 2018

Uh oh!

Uh oh!

eli-schwartz commented Nov 9, 2018

Uh oh!

Uh oh!

nikic commented Nov 9, 2018

Uh oh!

eli-schwartz commented Nov 9, 2018

Uh oh!

petk commented Nov 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cmb69 commented Nov 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikic commented Nov 13, 2018

Uh oh!

eli-schwartz commented Nov 13, 2018

Uh oh!

nikic commented Nov 13, 2018

Uh oh!

nikic commented Nov 13, 2018

Uh oh!

eli-schwartz commented Nov 13, 2018

Uh oh!

petk commented Nov 13, 2018

Uh oh!

nikic commented Nov 21, 2018

Uh oh!

petk commented Nov 26, 2018

Uh oh!

nikic commented Dec 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikic commented Dec 26, 2018

Uh oh!

nikic commented Jan 11, 2019

Uh oh!

eli-schwartz commented Jan 11, 2019

Uh oh!

Uh oh!

eli-schwartz commented Nov 7, 2018 •

edited

Loading

petk commented Nov 9, 2018 •

edited

Loading