Uh oh!
There was an error while loading. Please reload this page.
Verify bundled sources using CI - boost.context & uriparser - #20438
Conversation
5a94e72 to
623876dCompare39a8c6f to
0a2d4f7Comparekocsismate
commented
Nov 13, 2025
I love the concept, thanks! Can you show an example failure?
I find it the opposite way: in its current form, I find it very difficult to understand now. |
Thank you ❤ Using mvorisek@738c667 commit a failure is simulated - see CI results in https://github.com/mvorisek/php-src/actions/runs/19343856007/job/55338921670.
Why, how can it be done better? Maintaining the yml file by hand would not be practical as names/paths are copied in several places. Adding a new bundle is easy - add 1 line to https://github.com/php/php-src/blob/94cdb07789/.github/scripts/download-bundled/make-workflow-file.php#L8-L12. The script also generates partly the download scripts with canonical temporary directory names. In case you would do any manual change to the yml by mistake, the CI at https://github.com/php/php-src/blob/94cdb07789/.github/actions/verify-generated-files/action.yml#L16 will warn you about. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
95e4fac to
e065fc1Compareiluuu1994
commented
Nov 23, 2025
As mentioned, 2/3 of this PR is code that isn't needed. I don't think this should be merged in this state. There are other mechanisms to avoid duplicate code. |
Thank you for your message. I however do not think you can manage using anything than templating. The only thing that can be deduplicated using "other mechanisms to avoid duplicate code" is the download/verify step. Given all a), b), c) (or at least some of them) has no other solution, if we want to avoid mistakes and actual code duplication, the script is needed. If you know how to avoid it without having to specify the paths/names more than once or twice only, please tell me. Also please see #20487 and try adding a new definition/line into https://github.com/php/php-src/blob/94cdb07789/.github/scripts/download-bundled/make-workflow-file.php#L8-L12. You will see very quickly the power and benefits the templating script have. |
e065fc1 to
e74fb6eCompare…s on Windows vs. linux
e74fb6e to
993480bComparemvorisek
commented
Dec 3, 2025
@iluuu1994 I have rebased this PR and I would like to get it merged to continue with other bundled sources. You seems to be not fully convinced with the generator script. Please tell me what to do. I do not think it is possible to remove it as the paths would have to be duplicated on many places manually. If you do not fully see the value now, I can drop the generator script and add it in next PR where with more sources the advantage would be evident. |
mvorisek
commented
Dec 7, 2025
@iluuu1994 the generator script has been removed, is there anything else to address? |
mvorisek
commented
Dec 10, 2025
@iluuu1994 pinging you kindly again to move this PR forward |
iluuu1994
commented
Dec 10, 2025
Please stop pinging, I know this PR is open. |
mvorisek
commented
Dec 29, 2025
I would be happy to move this PR forward. Please let me know if there is anything I can do from my side to move it forward. Thank you. |
| tmp_dir=/tmp/php-src-download-bundled/uriparser | ||
| rm -rf "$tmp_dir" | ||
| revision=c3b49569f1f25550a16d9a18207e498d77458b27 # refs/tags/uriparser-0.9.9 with https://github.com/uriparser/uriparser/pull/276 |
There was a problem hiding this comment.
I believe we are now using revision 61c981a470401b477f5cd833fb18d61da54e474b (I'm not 100% sure)
There was a problem hiding this comment.
Revision adjusted, thank you for the support and if you can, please approve the changes in Github.
kocsismate
commented
Feb 11, 2026
@iluuu1994 I think this PR is good now except for the outdated uriparser revision. What do you think? |
61de38c to
a94db7eCompareiluuu1994
commented
Feb 11, 2026
@kocsismate I don't object to this PR in its current form, so feel free to merge if you're happy with it. |
Uh oh!
There was an error while loading. Please reload this page.
part of #19802
assert correctness of bundled #20375 and #20437 sources using CI