Update include declaration in Dom css selectors #14752

nielsdos · 2024-07-01T15:36:47Z

May fix 88da914#r143708340

@drupol Can you please check whether this resolves your issue? Thanks.

May fix php@88da914#r143708340

drupol · 2024-07-01T15:47:36Z

Hi Niels,

I have included the patch in the local builds, I have another issue now:

       >  gcc -I. -I/build/source/ext/dom -I/nix/store/pazf5vvb8yjv20x9zhmwg4qj0p48d0g8-php-8.4.999-042ae15-dev/include/php -I/nix/store/pazf5vvb8yjv20x9zhmwg4qj0p48d0g8-php-8.4.999-042ae15-dev/include/php/main -I/nix/store/pazf5vvb8yjv20x9zhmwg4qj0p48d0g8-php-8.4.999-042ae15-dev/include/php/TSRM -I/nix/store/pazf5vvb8yjv20x9zhmwg4qj0p48d0g8-php-8.4.999-042ae15-dev/include/php/Zend -I/nix/store/pazf5vvb8yjv20x9zhmwg4qj0p48d0g8-php-8.4.999-042ae15-dev/include/php/ext -I/nix/store/pazf5vvb8yjv20x9zhmwg4qj0p48d0g8-php-8.4.999-042ae15-dev/include/php/ext/date/lib -I/nix/store/9szcp4b13ym7saivwfmky9dzm2zx226v-libxml2-2.12.7-dev/include/libxml2 -DHAVE_CONFIG_H -g -O2 -D_GNU_SOURCE -I/build/source/ext/dom/lexbor -DLEXBOR_STATIC -DZEND_COMPILE_DL_EXT=1 -c /build/source/ext/dom/lexbor/lexbor/ns/ns.c -MMD -MF lexbor/lexbor/ns/ns.dep -MT lexbor/lexbor/ns/ns.lo  -fPIC -DPIC -o lexbor/lexbor/ns/.libs/ns.o
       > mkdir lexbor/lexbor/tag/.libs
       >  gcc -I. -I/build/source/ext/dom -I/nix/store/pazf5vvb8yjv20x9zhmwg4qj0p48d0g8-php-8.4.999-042ae15-dev/include/php -I/nix/store/pazf5vvb8yjv20x9zhmwg4qj0p48d0g8-php-8.4.999-042ae15-dev/include/php/main -I/nix/store/pazf5vvb8yjv20x9zhmwg4qj0p48d0g8-php-8.4.999-042ae15-dev/include/php/TSRM -I/nix/store/pazf5vvb8yjv20x9zhmwg4qj0p48d0g8-php-8.4.999-042ae15-dev/include/php/Zend -I/nix/store/pazf5vvb8yjv20x9zhmwg4qj0p48d0g8-php-8.4.999-042ae15-dev/include/php/ext -I/nix/store/pazf5vvb8yjv20x9zhmwg4qj0p48d0g8-php-8.4.999-042ae15-dev/include/php/ext/date/lib -I/nix/store/9szcp4b13ym7saivwfmky9dzm2zx226v-libxml2-2.12.7-dev/include/libxml2 -DHAVE_CONFIG_H -g -O2 -D_GNU_SOURCE -I/build/source/ext/dom/lexbor -DLEXBOR_STATIC -DZEND_COMPILE_DL_EXT=1 -c /build/source/ext/dom/lexbor/lexbor/tag/tag.c -MMD -MF lexbor/lexbor/tag/tag.dep -MT lexbor/lexbor/tag/tag.lo  -fPIC -DPIC -o lexbor/lexbor/tag/.libs/tag.o
       > In file included from /build/source/ext/dom/lexbor/lexbor/selectors-adapted/selectors.c:19:
       > ./php_dom.h:68:9: error: unknown type name 'php_dom_xpath_callbacks'
       >    68 |         php_dom_xpath_callbacks xpath_callbacks;
       >       |         ^~~~~~~~~~~~~~~~~~~~~~~
       > make: *** [Makefile:817: lexbor/lexbor/selectors-adapted/selectors.lo] Error 1
       > make: *** Waiting for unfinished jobs....
       For full logs, run 'nix log /nix/store/kzr7n941rprjqvfg5ygcg2kr5z7033j9-php-dom-8.4.999-042ae15.drv'.

nielsdos · 2024-07-01T15:50:48Z

I've pushed another commit that may fix the additional issue.

drupol · 2024-07-01T16:10:27Z

Yes, that fixed the issue. Thank you so much :)

drupol · 2024-07-01T16:15:59Z

Niels,

For my understanding, could you explain why these issues are not detected by the CI? Why are we able to identify such issues when building with Nix?

nielsdos · 2024-07-01T17:06:46Z

For my understanding, could you explain why these issues are not detected by the CI? Why are we able to identify such issues when building with Nix?

It seems you're building the extensions separately from each other and in a different directory structure.
Normally we pass -I. so the ext/dom/... include path should've worked with that.
And the fact you build extension separately causes not all HAVE_* constants to be set, but we shouldn't rely on that anyway for headers.

drupol · 2024-07-01T17:09:21Z

Fair enough, thanks!

petk · 2024-07-01T17:36:37Z

@drupol the root (php-src), main, TSRM and Zend are always required include directories for extensions. This change realistically also doesn't make sense from the separate build. Because including the ext/dom file in the middle of the library should ideally be done either with relative path #include "../../foo.h" or with #include <ext/dom/foo.h>. But for now we still use also this syntax #include "ext/dom/foo.h". I think that you'll sooner or later run into other issues.

nielsdos · 2024-07-01T17:45:05Z

@petk shall I change the includes to relative paths then? I guess that would work too and also avoid the change I had to make to the windows build file.

petk · 2024-07-01T17:46:39Z

Yes, now I think understand. Separate build doesn't have the ext/dom headers available yet (if I understand this correctly). This will be issue also for phpize build. And the lexbor library could do the relative includes, yes. I think the relative includes are the way to go here. Good catch overall also for rechecking the libbcmath, pcrelib, libmbfl, and other bundled libraries.

Edit: confirming the phpize build has the same issue. So, yes the relative includes will be better.

nielsdos · 2024-07-01T17:49:30Z

Thanks for the insight.

drupol · 2024-07-01T17:58:12Z

Using the very latest version of the branch has introduced a failing test:

php-dom> PASS XPath: basic types evaluation [tests/xpath_evaluate_basic_types.phpt]
php-dom> =====================================================================
php-dom> TIME END 2024-07-01 17:57:13
php-dom> =====================================================================
php-dom> TEST RESULT SUMMARY
php-dom> ---------------------------------------------------------------------
php-dom> Exts skipped    :     0
php-dom> Exts tested     :    12
php-dom> ---------------------------------------------------------------------
php-dom> Number of tests :   718               702
php-dom> Tests skipped   :    16 (  2.2%) --------
php-dom> Tests warned    :     0 (  0.0%) (  0.0%)
php-dom> Tests failed    :     1 (  0.1%) (  0.1%)
php-dom> Tests passed    :   701 ( 97.6%) ( 99.9%)
php-dom> ---------------------------------------------------------------------
php-dom> Time taken      : 8.921 seconds
php-dom> =====================================================================
php-dom> =====================================================================
php-dom> FAILED TEST SUMMARY
php-dom> ---------------------------------------------------------------------
php-dom> GH-14698 crash on DOM node dereference [tests/gh14698.phpt]
php-dom> =====================================================================
php-dom> make: *** [Makefile:131: test] Error 1

nielsdos · 2024-07-01T17:59:23Z

@drupol Can you share the test output please?

drupol · 2024-07-01T18:01:30Z

@drupol Can you share the test output please?

I wish I could, how do you do that?

nielsdos · 2024-07-01T18:02:37Z

If you pass --show-output --show-diff to run-tests.php, It will show the output of the test and the diff with the expected output.

petk · 2024-07-01T18:13:07Z

Just for the side info: other extensions are ok and all use the includes kind of ready for separate builds. Except ext/standard but this one is not problematic for now.

drupol · 2024-07-01T18:13:49Z

Here's the issue:

php-dom> TEST 416/718 [tests/gh14698.phpt]
php-dom> ========DIFF========
php-dom> 001- DONE
php-dom> 001+ Fatal error: Uncaught Error: Call to undefined function simplexml_import_dom() in /build/source/ext/dom/tests/gh14698.php:4
php-dom> 002+ Stack trace:
php-dom> 003+ #0 {main}
php-dom> 004+   thrown in /build/source/ext/dom/tests/gh14698.php on line 4
php-dom> ========DONE========
php-dom> FAIL GH-14698 crash on DOM node dereference [tests/gh14698.phpt]

nielsdos · 2024-07-01T18:19:09Z

@drupol Thanks, looks like it's just missing an entry in the EXTENSIONS section, easy to fix up.
EDIT: fixed in 1d90544

drupol · 2024-07-01T18:32:07Z

Excellent. I'll have an eye on tomorrow morning's build and see if they passes.

question: why 1d90544 was not part of this PR ?

nielsdos · 2024-07-01T18:35:06Z

question: why 1d90544 was not part of this PR ?

I didn't want to restart CI, and it was independent of fixing include-related problems.

drupol · 2024-07-01T18:35:56Z

Fair enough, thanks for your great assistance!

drupol · 2024-07-01T18:41:42Z

The next build is foreseen tomorrow at 5am, see it live in this repo: https://github.com/loophp/php-src-nix

nielsdos · 2024-07-01T18:48:24Z

Fair enough, thanks for your great assistance!

Thank you as well!

drupol · 2024-07-02T08:30:39Z

Everything compiled successfully and has been automatically merged! loophp/php-src-nix#173

I really think having such a Nix based workflow directly in this repo could definitely be beneficial for contributors, to catch issues preemptively.

nielsdos · 2024-07-02T23:49:15Z

I really think having such a Nix based workflow directly in this repo could definitely be beneficial for contributors, to catch issues preemptively.

Perhaps, although it's probably difficult to adapt what we have now to a different build system.

Anyway thanks for checking.

drupol · 2024-07-05T06:49:00Z

Perhaps, although it's probably difficult to adapt what we have now to a different build system.

Anyway thanks for checking.

Just by adding a new Github Action workflow and a flake.nix file within this repo it could be done. I don't say it would be trivial to do, but it would definitely be a good thing to do.

Update include declaration in Dom css selectors

7a3d0c8

May fix php@88da914#r143708340

github-actions bot added the Extension: dom label Jul 1, 2024

Remove pointless guards

950ee74

github-actions bot added the Category: Build System label Jul 1, 2024

Fix Windows build

3f02815

Use relative path

981e6af

nielsdos merged commit 0c5d3db into php:master Jul 1, 2024
11 checks passed

Update include declaration in Dom css selectors #14752

Update include declaration in Dom css selectors #14752

Uh oh!

Conversation

nielsdos commented Jul 1, 2024

Uh oh!

drupol commented Jul 1, 2024

Uh oh!

nielsdos commented Jul 1, 2024

Uh oh!

drupol commented Jul 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

drupol commented Jul 1, 2024

Uh oh!

nielsdos commented Jul 1, 2024

Uh oh!

drupol commented Jul 1, 2024

Uh oh!

petk commented Jul 1, 2024

Uh oh!

nielsdos commented Jul 1, 2024

Uh oh!

petk commented Jul 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nielsdos commented Jul 1, 2024

Uh oh!

drupol commented Jul 1, 2024

Uh oh!

nielsdos commented Jul 1, 2024

Uh oh!

drupol commented Jul 1, 2024

Uh oh!

nielsdos commented Jul 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

petk commented Jul 1, 2024

Uh oh!

drupol commented Jul 1, 2024

Uh oh!

nielsdos commented Jul 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

drupol commented Jul 1, 2024

Uh oh!

nielsdos commented Jul 1, 2024

Uh oh!

drupol commented Jul 1, 2024

Uh oh!

drupol commented Jul 1, 2024

Uh oh!

nielsdos commented Jul 1, 2024

Uh oh!

drupol commented Jul 2, 2024

Uh oh!

nielsdos commented Jul 2, 2024

Uh oh!

drupol commented Jul 5, 2024

Uh oh!

Uh oh!

drupol commented Jul 1, 2024 •

edited

Loading

petk commented Jul 1, 2024 •

edited

Loading

nielsdos commented Jul 1, 2024 •

edited

Loading

nielsdos commented Jul 1, 2024 •

edited

Loading