Skip to content

Verify bundled - add internal generator script - #25229

Open
mvorisek wants to merge 7 commits into
php:masterfrom
mvorisek:verify_bundled_generator_script
Open

mvorisek wants to merge 7 commits into
php:masterfrom
mvorisek:verify_bundled_generator_script

Conversation

@mvorisek

@mvorisek mvorisek commented Nov 6, 2026

Copy link
Copy Markdown
Contributor

part of #19802

In the generator there is a manual how to use it.

The script allows:

  • easy additon of a new bundled source,
  • maintains the workflow file (.github/workflows/verify-bundled-files.yml) fully
  • and maintains the template of each .github/scripts/download-bundled/*.sh download script

No AI was used :)

@mvorisek

mvorisek commented Nov 6, 2026

Copy link
Copy Markdown
Contributor Author

@arnaud-lb would you mind to review and merge?

@arnaud-lb arnaud-lb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really understand the benefits of this script. Generating adds complexity and reduces flexibility.

If we want to de-duplicate the steps in .github/workflows/verify-bundled-files.yml, maybe we can factor them in some way?

For the if condition:

    env:
      CHECK_ALL: ${{ github.event_name == 'schedule' || github.event_name == 'workflow_dispatch' }}
    steps:
      - name: xxhash
        if: ${{ !cancelled() && (steps.changes.outputs.xxhash == 'true' || env.CHECK_ALL == 'true') }}

For the run script:

run: .github/scripts/download-and-verify.sh xxhash.sh xxhash.sh

@mvorisek

mvorisek commented Nov 6, 2026

Copy link
Copy Markdown
Contributor Author

I have addressed your feedback, thank you for it.

About your question above - the workflow file needs the paths/names on several places and this is impossible to achieve by YAML grammar only without specifying it manually on several places. Even with few bundled sources now it saved me a lot of time and typos. And there are many more sources that can be added :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants