Add dependency tracking for header files #6693

rioderelfte · 2021-02-14T19:19:52Z

This ensures all .c files which include a header file directly or indirectly are rebuilt whenever the header file is changed.

By adding the flags to PHP_ADD_SOURCES_X this change should also work for out of tree extensions, though this was not tested.

I used the .dep extension instead of the default extension .d since there are already some dtrace files with extension .d which would clash with the dependency files.

At least the change to the .gitignore should probably be ported to the other branches as well. Otherwise there will be a lot of untracked files after building master and changing to a different branch.

nikic · 2021-02-15T10:20:56Z

Huh, this is a lot simpler than I expected. Works fine for me with both in-tree and out-of-tree builds. This doesn't work for out-of-tree extension builds, because $ac_cv_mmd_support will not be set in that case. This could be fixed by repeating the AX_CHECK_COMPILE_FLAG check inside phpize.m4. Though I kind of wonder whether we could get away with not checking it entirely -- are there any compilers that don't support -MMD?

nikic · 2021-02-15T10:22:18Z

With this patch, this hack can be removed as well:

php-src/build/Makefile.global

Lines 159 to 161 in 549235d

    
           # As we don't track includes, this is just a heuristic 
        
           %.c: %_arginfo.h 
        
           	@touch $@

This ensures all .c files which include a header file directly or indirectly are rebuilt whenever the header file is changed.

rioderelfte · 2021-02-15T20:19:14Z

Thank you very much for the feedback. I removed the feature check. I don't know of any compiler which does not support those flags. So I think this should be safe.

I also removed the rule from the Makefile.global.

nikic · 2021-02-16T09:51:58Z

I've landed this in master. If it doesn't cause issues for anyone, we should probably backport it to earlier versions, as the change is so simple.

dzuelke · 2021-12-09T23:09:44Z

So here is a use case that no longer works due to this change. Imagine you want to build PHP for distributing it in some way, and you're installing it into /usr/local/php. You want separate archives for PHP itself, and for each PECL extension you're also packaging.

So you do this for PHP:

download/untar source
./configure --prefix=/usr/local/php
make && make install
tar up /usr/local/php and upload it somewhere

Now, a PECL extension:

download/untar the PHP built earlier to /usr/local/php and add it to $PATH (PHP is a build dependency for the extension, obviously)
download/untar extension source
phpize
./configure --prefix=/usr/local/php
make
rm -rf /usr/local/php/* (because PHP, as a build dependency, is there, but we only want to archive the extension that's been compiled in the previous step)
make install (this "installs" the extension .so to /usr/local/php/lib/php/extensions/no-debug-non-zts-…/, and its headers to /usr/local/php/include/)
tar up /usr/local/php and upload it somewhere

The problem is that step 7 no longer works - the -include listing all the headers (e.g. for ext-raphf here) contains all of PHP's headers, too:

src/php_raphf_api.lo: /tmp/bob-2eBC7e/raphf-2.0.1/src/php_raphf_api.c \
 config.h /app/.heroku/php/include/php/main/php.h \
 /app/.heroku/php/include/php/main/php_version.h \
 /app/.heroku/php/include/php/Zend/zend_stream.h \
 /app/.heroku/php/include/php/main/streams/php_stream_context.h \
… (many more from main/ and Zend/ here)
 /app/.heroku/php/include/php/ext/standard/info.h php_raphf.h \
 php_raphf_api.h php_raphf.h

This results in the make install failing if, like in our step 6 above, PHP itself has been (re-)moved since the make (output that follows is from a make -d install):

      Considering target file 'src/php_raphf_api.lo'.
…
        Considering target file '/app/.heroku/php/include/php/main/php.h'.
         File '/app/.heroku/php/include/php/main/php.h' does not exist.
         Looking for an implicit rule for
'/app/.heroku/php/include/php/main/php.h'.
         Trying pattern rule with stem 'php'.
         Trying implicit prerequisite
'/app/.heroku/php/include/php/main//tmp/bob-1830jl/raphf-2.0.1/src/php.h'.
         Trying pattern rule with stem 'php.h'.
         Trying implicit prerequisite
'/app/.heroku/php/include/php/main/php.h,v'.
         Trying pattern rule with stem 'php.h'.
         Trying implicit prerequisite
'/app/.heroku/php/include/php/main/RCS/php.h,v'.
         Trying pattern rule with stem 'php.h'.
         Trying implicit prerequisite
'/app/.heroku/php/include/php/main/RCS/php.h'.
         Trying pattern rule with stem 'php.h'.
         Trying implicit prerequisite
'/app/.heroku/php/include/php/main/s.php.h'.
         Trying pattern rule with stem 'php.h'.
         Trying implicit prerequisite
'/app/.heroku/php/include/php/main/SCCS/s.php.h'.
         Trying pattern rule with stem 'php'.
         Trying implicit prerequisite
'/app/.heroku/php/include/php/main//tmp/bob-1830jl/raphf-2.0.1/src/php.h'.
         Looking for a rule with intermediate file
'/app/.heroku/php/include/php/main//tmp/bob-1830jl/raphf-2.0.1/src/php.h'.
          Avoiding implicit rule recursion.
          Trying pattern rule with stem 'php.h'.
          Trying implicit prerequisite
'/app/.heroku/php/include/php/main//tmp/bob-1830jl/raphf-2.0.1/src/php.h,v'.
          Trying pattern rule with stem 'php.h'.
          Trying implicit prerequisite
'/app/.heroku/php/include/php/main//tmp/bob-1830jl/raphf-2.0.1/src/RCS/php.h,v'.
          Trying pattern rule with stem 'php.h'.
          Trying implicit prerequisite
'/app/.heroku/php/include/php/main//tmp/bob-1830jl/raphf-2.0.1/src/RCS/php.h'.
          Trying pattern rule with stem 'php.h'.
          Trying implicit prerequisite
'/app/.heroku/php/include/php/main//tmp/bob-1830jl/raphf-2.0.1/src/s.php.h'.
          Trying pattern rule with stem 'php.h'.
          Trying implicit prerequisite
'/app/.heroku/php/include/php/main//tmp/bob-1830jl/raphf-2.0.1/src/SCCS/s.php.h'.
         No implicit rule found for '/app/.heroku/php/include/php/main/php.h'.
         Finished prerequisites of target file
'/app/.heroku/php/include/php/main/php.h'.
        Must remake target '/app/.heroku/php/include/php/main/php.h'.
make: *** No rule to make target
'/app/.heroku/php/include/php/main/php.h', needed by
'src/php_raphf_api.lo'.  Stop.

Building extensions for 8.0 or lower doesn't generate such a src/php_EXTNAME_api.dep file; thanks to @Danack's great help, I found this PR.

Is this side effect for extensions intentional? The previous behavior was very useful for quickly and cleanly packaging extensions.

The workaround is to

make INSTALL_ROOT=$SOMETEMPDIR install

and archive that instead, although I wish that instead of INSTALL_ROOT, PHP used the more "standard" DESTDIR variable recommended by GNU, since it'd allow build tooling to be a bit more generic (it's what most other programs I've seen are using).

Any thoughts, @rioderelfte and @nikic ?

rioderelfte · 2021-12-10T09:45:09Z

That side effect was not intentional, no. Thanks for bringing it up.

I think this can be worked around by adding -MP as an additional compile flag. That tells the compiler to generate empty phony make rules for all header files. Could you verify that this fixes your workflow? If it fixes the problem I am happy to prepare a pull request to add the flag.

dzuelke · 2021-12-10T16:40:51Z

Will check @rioderelfte !

dzuelke · 2021-12-10T16:57:57Z

The idea is to make CPPFLAGS=-MP, correct, @rioderelfte?

If so, then that doesn't help:

$ ./configure --prefix=$PHPBASEDIR
…
$ make CPPFLAGS=-MP -s -j 9
…
 cc -I. -I/tmp/bob-ST2z9k/pcov-1.0.10 -I/tmp/bob-ST2z9k/pcov-1.0.10/include -I/tmp/bob-ST2z9k/pcov-1.0.10/main -I/tmp/bob-ST2z9k/pcov-1.0.10 -I/app/.heroku/php/include/php -I/app/.heroku/php/include/php/main -I/app/.heroku/php/include/php/TSRM -I/app/.heroku/php/include/php/Zend -I/app/.heroku/php/include/php/ext -I/app/.heroku/php/include/php/ext/date/lib -MP -g -O2 -DZEND_ENABLE_STATIC_TSRMLS_CACHE=1 -c /tmp/bob-ST2z9k/pcov-1.0.10/pcov.c -MMD -MF pcov.dep -MT pcov.lo  -fPIC -DPIC -o .libs/pcov.o
cc -shared  .libs/pcov.o   -Wl,-soname -Wl,pcov.so -o .libs/pcov.so
creating pcov.la
(cd .libs && rm -f pcov.la && ln -s ../pcov.la pcov.la)
cp ./.libs/pcov.so /tmp/bob-ST2z9k/pcov-1.0.10/modules/pcov.so
cp ./.libs/pcov.lai /tmp/bob-ST2z9k/pcov-1.0.10/modules/pcov.la
PATH="$PATH:/sbin" ldconfig -n /tmp/bob-ST2z9k/pcov-1.0.10/modules
----------------------------------------------------------------------
Libraries have been installed in:
   /tmp/bob-ST2z9k/pcov-1.0.10/modules

If you ever happen to want to link against installed libraries
in a given directory, LIBDIR, you must either use libtool, and
specify the full pathname of the library, or use the `-LLIBDIR'
flag during linking and do at least one of the following:
   - add LIBDIR to the `LD_LIBRARY_PATH' environment variable
     during execution
   - add LIBDIR to the `LD_RUN_PATH' environment variable
     during linking
   - use the `-Wl,--rpath -Wl,LIBDIR' linker flag
   - have your system administrator add LIBDIR to `/etc/ld.so.conf'

See any operating system documentation about shared libraries for
more information, such as the ld(1) and ld.so(8) manual pages.
----------------------------------------------------------------------

Build complete.
Don't forget to run 'make test'.

$ rm -rf $PHPBASEDIR/*

$ make install -s
 cc -I. -I/tmp/bob-ST2z9k/pcov-1.0.10 -I/tmp/bob-ST2z9k/pcov-1.0.10/include -I/tmp/bob-ST2z9k/pcov-1.0.10/main -I/tmp/bob-ST2z9k/pcov-1.0.10 -I/app/.heroku/php/include/php -I/app/.heroku/php/include/php/main -I/app/.heroku/php/include/php/TSRM -I/app/.heroku/php/include/php/Zend -I/app/.heroku/php/include/php/ext -I/app/.heroku/php/include/php/ext/date/lib -DHAVE_CONFIG_H -g -O2 -DZEND_ENABLE_STATIC_TSRMLS_CACHE=1 -c /tmp/bob-ST2z9k/pcov-1.0.10/pcov.c -MMD -MF pcov.dep -MT pcov.lo  -fPIC -DPIC -o .libs/pcov.o
/tmp/bob-ST2z9k/pcov-1.0.10/pcov.c:23:10: fatal error: php.h: No such file or directory
 #include "php.h"
          ^~~~~~~
compilation terminated.
Makefile:201: recipe for target 'pcov.lo' failed
make: *** [pcov.lo] Error 1

FWIW, this is different from the previous output of make install:

$ make install -s
make: *** No rule to make target '/app/.heroku/php/include/php/main/php.h', needed by 'pcov.lo'.  Stop.

dzuelke · 2021-12-13T16:14:23Z

Or did I misunderstand you, @rioderelfte ?

rioderelfte · 2021-12-13T16:30:09Z

No, you did not misunderstand me – that was exactly what I had in mind. But it makes sense that it does not work, now that I think about it. The newly generated targets for the header files result in the compile target being considered outdated by make. So make tries to recompile the code during install and this fails with the headers missing.

I can't think of any obvious fixes then, unfortunately.

Why does using INSTALL_ROOT fix the problem for you? Does it work because it enables you to do the make install before the rm -rf /usr/local/php/*? I could try to add support for DESTDIR to the build system if that fixes your issue. I would probably add it as an alias for INSTALL_ROOT so it does not break backward compatibility.

An alternative would be to add a configure flag to disable the dependency tracking.

dzuelke · 2021-12-17T17:17:08Z

Yes, exactly. The objective is to have an archive of only the built extension, without any build-time dependencies that might be sharing parts of the path.

It's not really a huge issue; I can just use INSTALL_ROOT as an alternative approach, but I remember seeing this technique (make, rm -rf $installdir, make install) somewhere before, maybe Homebrew or Debian, for PHP extensions.

I think DESTDIR would be useful to have as an alias since that's what most programs and libraries I've seen use.

An alternative might be to have a cleanup task that only removes the .dep files, as suggested in http://make.mad-scientist.net/papers/advanced-auto-dependency-generation/ - wdyt?

Add dependency tracking for header files

581f718

This ensures all .c files which include a header file directly or indirectly are rebuilt whenever the header file is changed.

rioderelfte force-pushed the header-dependency-tracking branch from 323a763 to 581f718 Compare February 15, 2021 20:17

nikic approved these changes Feb 16, 2021

View reviewed changes

php-pulls closed this in c4d508c Feb 16, 2021

nikic mentioned this pull request Feb 16, 2021

Detect header changes in build system php/php-tasks#1

Closed

rioderelfte deleted the header-dependency-tracking branch February 16, 2021 09:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add dependency tracking for header files #6693

Add dependency tracking for header files #6693

Uh oh!

rioderelfte commented Feb 14, 2021 •

edited

Loading

Uh oh!

nikic commented Feb 15, 2021

Uh oh!

nikic commented Feb 15, 2021

Uh oh!

rioderelfte commented Feb 15, 2021

Uh oh!

nikic commented Feb 16, 2021

Uh oh!

dzuelke commented Dec 9, 2021

Uh oh!

rioderelfte commented Dec 10, 2021

Uh oh!

dzuelke commented Dec 10, 2021

Uh oh!

dzuelke commented Dec 10, 2021

Uh oh!

dzuelke commented Dec 13, 2021

Uh oh!

rioderelfte commented Dec 13, 2021

Uh oh!

dzuelke commented Dec 17, 2021

Uh oh!

Uh oh!

Add dependency tracking for header files #6693

Add dependency tracking for header files #6693

Uh oh!

Conversation

rioderelfte commented Feb 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nikic commented Feb 15, 2021

Uh oh!

nikic commented Feb 15, 2021

Uh oh!

rioderelfte commented Feb 15, 2021

Uh oh!

nikic commented Feb 16, 2021

Uh oh!

dzuelke commented Dec 9, 2021

Uh oh!

rioderelfte commented Dec 10, 2021

Uh oh!

dzuelke commented Dec 10, 2021

Uh oh!

dzuelke commented Dec 10, 2021

Uh oh!

dzuelke commented Dec 13, 2021

Uh oh!

rioderelfte commented Dec 13, 2021

Uh oh!

dzuelke commented Dec 17, 2021

Uh oh!

Uh oh!

rioderelfte commented Feb 14, 2021 •

edited

Loading