-
Notifications
You must be signed in to change notification settings - Fork 8k
Verify bundled sources using CI - boost.context & uriparser #20438
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
5a94e72 to
623876d
Compare
39a8c6f to
0a2d4f7
Compare
|
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. |
| # use the repo root directory as "--git-dir" | ||
| cd "$(dirname "$0")/../.." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI only: This is important as the original cd "$(dirname "$0")/../../$1" did not detected any changes in case of the checked directory was containing .git directory.
Example: git clone https://github.com/derickr/timelib.git ext/date/lib
|
@TimWolla @iluuu1994 can this PR be merged? |
…s on Windows vs. linux
b92b445 to
95e4fac
Compare
95e4fac to
e065fc1
Compare
|
Can this PR be merged please? The MacOS job failure is unrelated. I would like to continue another PR for more sources. |
|
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. |
part of #19802
assert #20375 and #20437 using CI
The workflow YML file is generated to make adding/maintaining it easier.