Verify bundled sources using CI - PCRE2 & SLJIT #20354

mvorisek · 2025-11-01T11:46:30Z

initial PR of #19802

The new CI asserts all bundled files are up-to-date and the CI code also provides trusted documentation of how the bundled deps were added.

On push/PR the CI is run only if the bundled files were changed.

The CI is based on shell scripts that can be easily modified for upgrade and run localy.

mvorisek · 2025-11-01T11:48:56Z

.github/scripts/download-bundled/pcre2.sh

+cp -R /tmp/php-src-bundled/pcre2/src ext/pcre/pcre2lib
+
+cd ext/pcre/pcre2lib
+git restore config.h


@ndossche might I ask you for the exact steps how #20268 was made?

There is no exact procedure as files get added and removed in the library. It mainly consists of copying over the c and h files,sometimes updating the config files, applying the diff of pcre.h, and updating the sljit includes so that it includes from the right location.

Please review, I reverse-engineered the patches.

ndossche

I'm fine with this, as this is right from a technical PoV, but deferring the action workflow judgement to @TimWolla / @iluuu1994

iluuu1994

I don't have strong opinions on this. I think it would be better if these files lived outside of .github and also helped updating the libraries themselves (i.e. downloading some revision, updating the hash somewhere, applying the patches, etc.). But I don't maintain any parts of PHP with external code that is pulled in, so I'll defer to those people whether that's actually necessary.

iluuu1994 · 2025-11-03T14:37:10Z

.github/scripts/download-bundled/pcre2-adjust-sljit-directory.patch

@@ -0,0 +1,13 @@
+diff --git a/ext/pcre/pcre2lib/pcre2_jit_compile.c b/ext/pcre/pcre2lib/pcre2_jit_compile.c


We have some existing patch files, e.g. ext/fileinfo/libmagic.patch. They should live in a consistent place. Not necessarily the same place, just consistent.

Yes, to be consistent.

I personally prefer to keep it here for now as there are dozens of bundled sources and maintaining the update files is much easier if they are kept at shared place.

iluuu1994 · 2025-11-03T14:41:54Z

.github/workflows/verify-bundled-files.yml

+name: Verify Bundled Files
+
+on:
+  push:


We should only run this when the files have actually changed. https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-syntax#onpushpull_requestpull_request_targetpathspaths-ignore

This is per workflow, not per step. It is however handled using dorny/paths-filter@v3 step. The workflow will skip all downloads if nothing has changed.

in that case, I assume the intention is that this workflow will be used for other bundled files, in which case the VERIFY_BUNDLED_FILES job should be renamed to maybe VERIFY_BUNDLED_PCRE2 or something? If there will be multiple jobs, the job name should match what is tested, and if there is only one job, then we can apply the filter for the overall workflow

see https://github.com/mvorisek/php-src/blob/778cf78cdbf1be73f63889e518f78e4d89a8b71d/.github/workflows/verify-bundled-files.yml#L32-L38 for added bundle - not a separate workflow/job, but instead of separate step (2 steps) to safe CI resources.

iluuu1994 · 2025-11-03T14:47:55Z

.github/scripts/download-bundled/pcre2.sh

+set -ex
+cd "$(dirname "$0")/../../.."
+
+commit=b2bd4254b379b9d7dc9a3dda060a7e27009ccdff # 10.46 release


Why not use the tag? This is more self-documenting.

I like it 👍

DanielEScherzer · 2025-11-03T20:06:53Z

.github/scripts/download-bundled/pcre2.sh

+rm pcre2test.c
+
+# move renamed files
+mv config.h.generic config.h


what is the point of moving a file into config.h just to restore the original one that was in git via git restore config.h down below - why not just rm config.h.generic entirely, and document that config.h is not something that is bundled and so should not be used for the comparison?

I designed it this way. The actual git restore config.h command is the one that can be removed in the future. The config.h is currently very out of sync with config.h.generic but that is because a lot of lines are commented out, so the real patch should not be that big...

DanielEScherzer · 2025-11-03T20:09:30Z

.github/scripts/test-directory-unchanged.sh

+git status
+
+# display & detect all changes
+git add . -N && git diff --cached -a --exit-code . && git diff -a --exit-code .


the existing workflow to verify generated files uses git add . -N && git diff -a --exit-code - here you use something different. If this is more correct, we should update that, but if not I suggest we match

I updated the "verify generated files" and deduplicated it.

DanielEScherzer · 2025-11-03T20:10:38Z

.github/workflows/verify-bundled-files.yml

+name: Verify Bundled Files
+
+on:
+  push:


in that case, I assume the intention is that this workflow will be used for other bundled files, in which case the VERIFY_BUNDLED_FILES job should be renamed to maybe VERIFY_BUNDLED_PCRE2 or something? If there will be multiple jobs, the job name should match what is tested, and if there is only one job, then we can apply the filter for the overall workflow

mvorisek added 3 commits November 1, 2025 12:32

working filter and empty scripts

74aefc4

DEBUG FORCE CHANGE/CI

ae3ccb9

wip update local bundle from upstream

ff07fc5

github-actions bot added the Extension: pcre label Nov 1, 2025

mvorisek commented Nov 1, 2025

View reviewed changes

mvorisek added 5 commits November 1, 2025 14:21

--recurse-submodules

5fbbe62

Revert "DEBUG FORCE CHANGE/CI"

bd1bda4

fix outdated pcre2_chartables.c (pcre2_chartables.c.dist)

60385d5

fix outdated pcre2.h (pcre2.h.generic)

d739629

impl

95a110a

github-actions bot added the ABI break label Nov 2, 2025

mvorisek marked this pull request as ready for review November 2, 2025 01:32

mvorisek requested a review from TimWolla as a code owner November 2, 2025 01:32

ndossche approved these changes Nov 2, 2025

View reviewed changes

iluuu1994 reviewed Nov 3, 2025

View reviewed changes

mvorisek added 2 commits November 3, 2025 16:26

run if download script changes

8d00dc3

reference using tag

8a9e828

mvorisek requested a review from iluuu1994 November 3, 2025 16:31

DanielEScherzer reviewed Nov 3, 2025

View reviewed changes

dedup changed files testing

66c5bfa

mvorisek requested a review from DanielEScherzer November 3, 2025 20:43

mvorisek added 2 commits November 4, 2025 08:52

Merge branch 'master' - merge PCRE 10.44 downgrade

4227f15

adjust to downgrade (PR 20380)

a120ad9

		@@ -0,0 +1,13 @@
		diff --git a/ext/pcre/pcre2lib/pcre2_jit_compile.c b/ext/pcre/pcre2lib/pcre2_jit_compile.c

Verify bundled sources using CI - PCRE2 & SLJIT #20354

Are you sure you want to change the base?

Verify bundled sources using CI - PCRE2 & SLJIT #20354

Conversation

mvorisek commented Nov 1, 2025

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!

ndossche left a comment

Choose a reason for hiding this comment

Uh oh!

iluuu1994 left a comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants