Preferably include from build dir

[arnaud-lb]

This fixes out of tree builds by ensuring that configure artifacts are included from the build dir.

Before, out of tree builds would preferably include files from the src dir, as the include path was defined as follows (ignoring includes from ext/ and sapi/) :

-I$(top_builddir)/main
-I$(top_srcdir)
-I$(top_builddir)/TSRM
-I$(top_builddir)/Zend
-I$(top_srcdir)/main
-I$(top_srcdir)/Zend
-I$(top_srcdir)/TSRM
-I$(top_builddir)/

As a result, an out of tree build would include configure artifacts such as main/php_config.h from the src dir. I've been hit by this a few times when building from VMs.

After this change, the include path is defined as follows:

-I$(top_builddir)/main
-I$(top_builddir)
-I$(top_srcdir)/main
-I$(top_srcdir)
-I$(top_builddir)/TSRM
-I$(top_builddir)/Zend
-I$(top_srcdir)/Zend
-I$(top_srcdir)/TSRM

Edit: This also updates ext/ and ext/skeleton to include config.h with the brackets form (#include <config.h>), otherwise the file is always searched in the directory of the including file first.

[petk]

Thanks for noticing and fixing.

I see. If there is, for example, an existing php-src/main/php_config.h created during the configure step and then Git repository is not cleaned up, and a new build starts in the out-of-source build directory the php-out-of-src/main/php_config.h is not included due to existing php-src/main/php_config.h and similar. Sounds like a good idea to resort these also, yes.

The DEFS variable can be removed, yes. Otherwise, name comes from the Autoconf variable. It would be ideal, to not introduce a new Makefile variable for this, but probably complicates things a bit, and is ok, I guess.

I think, the phpize build will also then need a few adjustments, but I'll recheck this soon again...

[arnaud-lb]

The DEFS variable can be removed, yes. Otherwise, name comes from the Autoconf variable. It would be ideal, to not introduce a new Makefile variable for this, but probably complicates things a bit, and is ok, I guess.

Yes I removed DEFS and replaced it with CORE_INCLUDES because I figured we were not using DEFS as intended. Also it was hard coded so it shouldn't break anyone's extension or build script. As to avoid introducing a new variable, I'm not sure how to proceed. If we just prepend the two pathes to INCLUDES it would be a behavior change (slight, but I wanted to be cautious).

I think, the phpize build will also then need a few adjustments, but I'll recheck this soon again...

Ah right, extensions appear to have the same issue. We need to change PHP_ADD_SOURCES_X. Also, ext/skeleton/skeleton.c includes config.h with the quotes form, so it will always include from the source tree. I've pushed a new commit with these changes, WDYT?

[petk]

I'll check if this can be made a bit more concise by using PHP_ADD_INCLUDE() in configure.ac.

PHP_ADD_INCLUDE([$abs_builddir/Zend], [1])
PHP_ADD_INCLUDE([$abs_builddir/TSRM], [1])
PHP_ADD_INCLUDE([$abs_builddir/main], [1])
PHP_ADD_INCLUDE([$abs_builddir], [1])

if test "$abs_srcdir" != "$abs_builddir"; then
  PHP_ADD_INCLUDE([$abs_srcdir/Zend], [1])
  PHP_ADD_INCLUDE([$abs_srcdir/TSRM], [1])
  PHP_ADD_INCLUDE([$abs_srcdir/main], [1])
  PHP_ADD_INCLUDE([$abs_srcdir], [1])
fi

This is just example, and these need to be sorted properly of course...

Issue is mostly this COMMON_FLAGS is actually used in phpize's Makefile also. And there the -I$(top_builddir)/main and -I$(top_srcdir) are prepended. The main is no problem. For example, by coincidence I've even found a BC break in one extension out there I've introduced with that PHP_DEFINE macro removal (PR fix already sent), so this PR and the whole includes recheck is very much in place actually. 😄 Otherwise, these DEFS flags are anomaly in phpize and weren't intentional to be prepended anyway there. So no worries.

From what I've gathered so far, mostly the includes order should be done in the following order for some build scope (when doing the gcc ... step) :

• current build directory
• current source directory
• top build directories (root, main, Zend, TSRM - order not important)
• top source directories (root, main, Zend, TSRM and one after the other)
• system includes

[petk]

If it helps here a bit and if I'm not mistaken, this is what is now in the master branch converted to the removal of that DEFS and the some-extension/main for the phpize builds. I'd go something like this and then adjust this to have the build directories before the src directories... This still needs to be tested a bit if phpize builds can be done like this. According to a quick test it was ok. And the extension build directory is already before the src directory in the php.m4, I think.

diff --git a/build/Makefile.global b/build/Makefile.global
index 7149401596..e0d05f1061 100644
--- a/build/Makefile.global
+++ b/build/Makefile.global
@@ -2,8 +2,7 @@ mkinstalldirs = $(top_srcdir)/build/shtool mkdir -p
 INSTALL = $(top_srcdir)/build/shtool install -c
 INSTALL_DATA = $(INSTALL) -m 644
 
-DEFS = -I$(top_builddir)/main -I$(top_srcdir)
-COMMON_FLAGS = $(DEFS) $(INCLUDES) $(EXTRA_INCLUDES) $(CPPFLAGS) $(PHP_FRAMEWORKPATH)
+COMMON_FLAGS = $(INCLUDES) $(EXTRA_INCLUDES) $(CPPFLAGS) $(PHP_FRAMEWORKPATH)
 
 all: $(all_targets)
 	@echo
diff --git a/configure.ac b/configure.ac
index 2a62e9ce0d..3792f49c28 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1349,13 +1349,14 @@ LIBZEND_BASIC_CHECKS
 LIBZEND_DLSYM_CHECK
 LIBZEND_OTHER_CHECKS
 
-INCLUDES="$INCLUDES -I\$(top_builddir)/TSRM"
-INCLUDES="$INCLUDES -I\$(top_builddir)/Zend"
-
-if test "$abs_srcdir" != "$abs_builddir"; then
-  INCLUDES="$INCLUDES -I\$(top_srcdir)/main -I\$(top_srcdir)/Zend"
-  INCLUDES="$INCLUDES -I\$(top_srcdir)/TSRM -I\$(top_builddir)/"
-fi
+PHP_ADD_INCLUDE([$abs_builddir/TSRM])
+PHP_ADD_INCLUDE([$abs_builddir/Zend])
+PHP_ADD_INCLUDE([$abs_srcdir], [1])
+PHP_ADD_INCLUDE([$abs_builddir/main], [1])
+PHP_ADD_INCLUDE([$abs_srcdir/main])
+PHP_ADD_INCLUDE([$abs_srcdir/Zend])
+PHP_ADD_INCLUDE([$abs_srcdir/TSRM])
+PHP_ADD_INCLUDE([$abs_builddir])
 
 ZEND_EXTRA_LIBS="$LIBS"
 unset LIBS 

[arnaud-lb]

Thank you for looking into this and for the suggestions. However I was originally going for a bug fix and I'm not familiar enough with the build system to cary the suggested changes.

The current branch fixes out-of-tree builds for php-src with minimal risk of breaking things. For extensions, the include path was already correct but the skeleton was not, so I've updated it.

WDYT of merging this as-is, and refactoring separately?

[petk]

Somehow, I can't add suggestions to pull request - suggestions button is locked and something wrong with GitHub, I guess.

Then, I would suggest to add this instead of introducing a new CORE_INCLUDES Makefile variable:

diff --git a/build/Makefile.global b/build/Makefile.global
index 7149401596..e0d05f1061 100644
--- a/build/Makefile.global
+++ b/build/Makefile.global
@@ -2,8 +2,7 @@ mkinstalldirs = $(top_srcdir)/build/shtool mkdir -p
 INSTALL = $(top_srcdir)/build/shtool install -c
 INSTALL_DATA = $(INSTALL) -m 644
 
-DEFS = -I$(top_builddir)/main -I$(top_srcdir)
-COMMON_FLAGS = $(DEFS) $(INCLUDES) $(EXTRA_INCLUDES) $(CPPFLAGS) $(PHP_FRAMEWORKPATH)
+COMMON_FLAGS = $(INCLUDES) $(EXTRA_INCLUDES) $(CPPFLAGS) $(PHP_FRAMEWORKPATH)
 
 all: $(all_targets)
        @echo
diff --git a/configure.ac b/configure.ac
index 564f9c90c2..21c96140f2 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1351,13 +1351,14 @@ LIBZEND_BASIC_CHECKS
 LIBZEND_DLSYM_CHECK
 LIBZEND_OTHER_CHECKS
 
-INCLUDES="$INCLUDES -I\$(top_builddir)/TSRM"
-INCLUDES="$INCLUDES -I\$(top_builddir)/Zend"
-
-if test "$abs_srcdir" != "$abs_builddir"; then
-  INCLUDES="$INCLUDES -I\$(top_srcdir)/main -I\$(top_srcdir)/Zend"
-  INCLUDES="$INCLUDES -I\$(top_srcdir)/TSRM -I\$(top_builddir)/"
-fi
+PHP_ADD_INCLUDE([$abs_srcdir], [1])
+PHP_ADD_INCLUDE([$abs_srcdir/main], [1])
+PHP_ADD_INCLUDE([$abs_builddir], [1])
+PHP_ADD_INCLUDE([$abs_builddir/main], [1])
+PHP_ADD_INCLUDE([$abs_builddir/TSRM])
+PHP_ADD_INCLUDE([$abs_builddir/Zend])
+PHP_ADD_INCLUDE([$abs_srcdir/Zend])
+PHP_ADD_INCLUDE([$abs_srcdir/TSRM])

This will sort the include directories the same way as in your PR. See this diff as an example (meaning they are the same):

diff ../../Makefile ./Makefile
90,91c90
< CORE_INCLUDES = -I$(top_builddir)/main -I$(top_builddir) -I$(top_srcdir)/main -I$(top_srcdir)
< INCLUDES = -I/path/to/php-src/php-build/ext/date/lib -I/path/to/php-src/ext/date/lib -I/usr/include/libxml2 -I$(top_builddir)/TSRM -I$(top_builddir)/Zend -I$(top_srcdir)/Zend -I$(top_srcdir)/TSRM
---
> INCLUDES = -I/path/to/php-src/php-build/main -I/path/to/php-src/php-build -I/path/to/php-src/main -I/path/to/php-src -I/path/to/php-src/php-build/ext/date/lib -I/path/to/php-src/ext/date/lib -I/usr/include/libxml2 -I/path/to/php-src/php-build/TSRM -I/path/to/php-src/php-build/Zend -I/path/to/php-src/Zend -I/path/to/php-src/TSRM
115c114
< COMMON_FLAGS = $(CORE_INCLUDES) $(INCLUDES) $(EXTRA_INCLUDES) $(CPPFLAGS) $(PHP_FRAMEWORKPATH)
---
> COMMON_FLAGS = $(INCLUDES) $(EXTRA_INCLUDES) $(CPPFLAGS) $(PHP_FRAMEWORKPATH)

About changing the skeleton, I guess we can do that. Otherwise, all php-src extensions also use #include "config.h".

[arnaud-lb]

Thank you. I've applied your suggestion. One difference with this, is that PHP_ADD_INCLUDE(..., [1]) in extensions will now place the include path before / and /main. With DEPS / CORE_INCLUDES, / and /main are always first.

I've also updated all config.h includes to use the <...> form.

[petk]

One difference with this, is that PHP_ADD_INCLUDE(..., [1]) in extensions will now place the include path before / and /main. With DEPS / CORE_INCLUDES, / and /main are always first.

In the generated Makefile by configure script or the phpize? In the generated Makefile the INCLUDES variable is only used in the COMMON_FLAGS and this one is then passed to building all those C objects. For the phpize builds, the main directory won't be included and that's fine because extension source code from what I've checked quickly don't use main directories in their sources. It's sort of anomaly. The system /usr/include/php/.../main will be normally included as before. Do you have some Makefile line example where you think this happens?

Edit: And these PHP_ADD_INCLUDE changes in configure.ac file are happening after extensions are already parsed above.

[petk]

For example, if we try this in bcmath extension:

diff --git a/ext/bcmath/config.m4 b/ext/bcmath/config.m4
index ac654aba00..d76d837959 100644
--- a/ext/bcmath/config.m4
+++ b/ext/bcmath/config.m4
@@ -12,4 +12,5 @@ libbcmath/src/rmzero.c libbcmath/src/str2num.c,
           $ext_shared,, -DZEND_ENABLE_STATIC_TSRMLS_CACHE=1)
   PHP_ADD_BUILD_DIR($ext_builddir/libbcmath/src)
   AC_DEFINE(HAVE_BCMATH, 1, [Whether you have bcmath])
+  PHP_ADD_INCLUDE([$ext_builddir/libbcmath/src], [1])
 fi

./configure --enable-bcmath will add it the libbcmath/src after main and /.

[arnaud-lb]

And these PHP_ADD_INCLUDE changes in configure.ac file are happening after extensions are already parsed above

This is what I didn't realize. Thank you :)

[petk]

I've retested this a bit and it seems to be working ok. Otherwise, there needs to be slight caution now if some config.h file, which is quite common name, will be present somewhere in the -I paths. When using #include <...> the -I takes precedence before the local file included with relative way. For example:

• ext/fileinfo/libmagic/config.h
• ext/pcre/pcre2lib/config.h
• ext/bcmath/libbcmath/src/config.h

For the community PHP extensions, this isn't issue. It would be good if someone else tests this also and sees anything.

[Girgias]

I don't understand build systems but this looks fine?

[petk]

@arnaud-lb what kind of issue have you run into here? Was it the php_config.h file maybe or something else? Because, then perhaps this can be simplified a bit without changing these config.h file include styles. The config.h file is generated only when using phpize and maybe we also don't need to cover this case.

This is quite common issue in all C projects. And people mostly solve it by using separate build directories. If there are generated in-source build files, I'm not sure what would be the policy regarding this. It is kind of expected that source directory doesn't contain anything generated at the build time 🤔

I'll leave this to be decided by other reviewers. Because, includes across the php-src should be updated and cleaned up anyway and I'm not trying to add fuel to the fire from previous similar discussions and RFC created over the include issue. I still find the include-what-you-use tool really cool option to be considered one day also to enhance the php-src includes.

[arnaud-lb]

I usually build in the source tree during development because it's convenient, but sometimes I need to build in a slightly different way, and in this case I build out of tree.

So, usually I do something like this:

./configure && make

and occasionally I do this:

cd build
../php-src/configure && make

The issue I've ran into is that the out of tree build includes the php_config.h of the source tree. This is wrong because this is not the one generated by the out of tree configure run. This leads to unexpected results, and I've lost time on multiple occasions because of this.

php_config.h is generated by configure in the build tree, so it seems logical to include from there in priority.

The config.h change in ext/ fixes exactly the same issue for extensions (#include <config.h> gives priority to the include path, #include "config.h" includes from the source dir first, which is wrong). As you said this affects only phpize/standalone builds of extensions (which should be rare, but sometimes useful), because HAVE_CONFIG_H is not defined otherwise, and config.h is not included.

I you think this is risky, I can revert this part.

The config.h change in ext/skeleton ensures that new extensions won't have this problem.

Note that I'm not trying to reorganize headers, I'm just trying to fix the issue described above, that leads to broken builds and waste of time.

[petk]

Then, if everyone is fine with this, I'm merging this one in the following days. I'll recheck it again if all looks ok. Yes, angle brackets includes are more appropriate for the generated files in this case. Otherwise, this might not make sense to someone looking at the code from the point of the restriction to not build C project in-source. Some projects even restrict the in-source builds... But don't worry about that. We can always refactor these requirements/code styles in the future also if needed.

[arnaud-lb]

Thank you for your help! You should add yourself as co-author when merging

[petk]

So far I've only found the /usr/include/valgrind/config.h which has caused issues in the past in the ext/mbstring extension with libmbfl but with the include order of having PHP directories in front and everything this shouldn't be a problem. It also works in phpize builds no matter how hard I try to hack it with those PHP_ADD_INCLUDE in the extension or some weird configuration settings. I think this really shouldn't be problematic since it is only php-src changes. For the PHP extensions out there, they can do that by their preference and schedule. I'll still recheck a bit the Windows build.

[arnaud-lb]

@petk is anything blocking?

[petk]

@petk is anything blocking?

I'll have to write myself some docs about these headers. Because I'm now not sure anymore why are they important to be in this order (those main, Zend, TSRM, ext) :D

Otherwise, I have no idea ATM yet here. Probably, it's ok but it's many files to be completely sure. It would require rechecking this a bit. There are a couple of relative includes of the generated files left (timelib_config.h), but I still find those better to be included in a relative way because then there is no need to pass additional include to compiler.

Issue is probably more that I'm looking at this from the point of installed PHP on the system, because it can complicate libphp and building extension, and you find it important from the development PoV, which I understand totally.

[petk]

Hm, I found one. There is zend_config.h generated with content: <../main/php_config.h> (relative include to php_config.h). Therefore, the $abs_builddir/main needs to be the first include so it doesn't accidentally pick the generated main/php_config.h from some other in-source build ... :/

I'm still checking why then this couldn't work ok:

PHP_ADD_INCLUDE([$abs_srcdir/Zend], [1])
PHP_ADD_INCLUDE([$abs_builddir/Zend], [1])
PHP_ADD_INCLUDE([$abs_srcdir/TSRM], [1])
PHP_ADD_INCLUDE([$abs_builddir/TSRM], [1])
PHP_ADD_INCLUDE([$abs_srcdir], [1])
PHP_ADD_INCLUDE([$abs_builddir], [1])
PHP_ADD_INCLUDE([$abs_srcdir/main], [1])
PHP_ADD_INCLUDE([$abs_builddir/main], [1])

-I...php-src/php-build/main
-I...php-src/main
-I....php-src/php-build
-I...php-src
-I...php-src/php-build/TSRM
-I...php-src/TSRM
-I...php-src/php-build/Zend
-I...php-src/Zend
-I...<includes added by extensions, for example -I/usr/include/libxml2>

Probably I'm missing something but the order of root, main, TSRM and Zend shouldn't matter. It would be kind of expected that including some PHP header is not dependent on this order (here I mean only these pairs, not the build and src - build dir needs to be in front indeed).

[petk]

If I'm not mistaken, building with mbstring (--enable-mbstring) would cause same issues. And there is one more build-defs.h (not critical though). So then something like this:

diff --git a/ext/mbstring/config.m4 b/ext/mbstring/config.m4
index 2a43e6ad2f..5fbe487724 100644
--- a/ext/mbstring/config.m4
+++ b/ext/mbstring/config.m4
@@ -35,14 +35,14 @@ AC_DEFUN([PHP_MBSTRING_EXTENSION], [
     PHP_ADD_INCLUDE([$ext_builddir/$dir])
   done
 
-  out="php_config.h"
+  out="<php_config.h>"
 
   if test "$ext_shared" != "no" && test -f "$ext_builddir/config.h.in"; then
-    out="$abs_builddir/config.h"
+    out="\"$abs_builddir/config.h\""
   fi
 
   cat > $ext_builddir/libmbfl/config.h <<EOF
-#include "$out"
+#include $out
 EOF
 
   PHP_MBSTRING_ADD_INSTALL_HEADERS([mbstring.h])
diff --git a/sapi/fpm/fpm/fpm_php.h b/sapi/fpm/fpm/fpm_php.h
index d61857c5e0..254790af72 100644
--- a/sapi/fpm/fpm/fpm_php.h
+++ b/sapi/fpm/fpm/fpm_php.h
@@ -6,7 +6,9 @@
 #include <TSRM.h>
 
 #include "php.h"
-#include "build-defs.h" /* for PHP_ defines */
+#ifdef HAVE_BUILD_DEFS_H
+#include <main/build-defs.h> /* for PHP_ defines */
+#endif
 #include "fpm/fpm_conf.h"
 
 #define FPM_PHP_INI_TO_EXPAND \
(END)

And I'm also still not sure about all the config.w32.h includes. Perhaps on Windows this is less important to be able to build simultaneously in-source and out-of source. This is generated into main/config.w32.h from win32/build/config.w32.h.in. And, I've also rechecked the include order from above. Those Zend and TSRM pairs can be in any place. Before, after, middle...

[petk]

Yes, now I'm seeing this issue from another PoV. I've started hitting it too. The way php-src is currently comfortably possible to build in-source and out-of-source with Autotools you quickly need to do some build, then remember you want to compare the build result of something else and do a new build in a subdirectory and then these config headers get messed up. I think we can break this into more PRs like with other similar issues then. Merge here coming up with some minor adjustments of those -I flags sort order. And other changes in separate PRs. Windows build I guess is not so much relevant yet with current Windows build system for this to realistically happen because it already builds in out-of-source.

[petk]

All seems to be working at this point. Merged to master. I'll see what to adjust in the following PRs/commits. Basically that libmbfl config file and the include flags order to make it more clear that the order is irrelevant.

[arnaud-lb]

Thank you @petk !