openwrt / actions-shared-workflows

4 stars 12 forks source link

CI: reusable_build: #18

Open systemcrash opened 2 months ago

systemcrash commented 2 months ago

I think the || logic here is wrong. All packages are checked even if only a single package has changed in a (force) push. It would seem that any push event always triggers make package/check (if [ "${{ github.event_name }}" = "push" ])


      - name: Download and check packages
        if: inputs.build_all_modules == true || inputs.build_all_kmods == true || inputs.build_full == true
        shell: su buildbot -c "sh -e {0}"
        working-directory: openwrt
        run: |
          # With push events or check_packages_list set to 'all', check all packages
          if [ "${{ github.event_name }}" = "push" ] || [ "${{ inputs.check_packages_list }}" = "all" ]; then
            make package/download package/check FIXUP=1 -j$(nproc) BUILD_LOG=1 || ret=$? .github/workflows/scripts/show_build_failures.sh
          # With every other event check only changed packages (if provided)
          elif [ -n "${{ inputs.check_packages_list }}" ]; then
            for package in ${{ inputs.check_packages_list }}; do
              make package/$package/download package/$package/check FIXUP=1 -j$(nproc) BUILD_LOG=1 || ret=$? .github/workflows/scripts/show_build_failures.sh
            done
          fi

Also, if only checksums have changed in a Makefile, package/check FIXUP=1 seems to do nothing locally (to warrant a git add blah; git commit --amend ; git push --force).

Ansuel commented 2 months ago

That is intended as the comment suggest. Push events on openwrt main repo are on main branch and stable branch and we want to check every package.

About the package/check not correctly fixing stuff, that is a bug i think ? probably FIXUP=1 is not propagated?

systemcrash commented 2 months ago

we want to check every package.

maybe better to have two separate event handlers in the yaml - one for pushes to main, and one for PRs and their (force) pushes. Then the check and build is much quicker for those PRs where only a single package changed.

About the package/check not correctly fixing stuff, that is a bug i think ? probably FIXUP=1 is not propagated?

🤷

Ansuel commented 2 months ago

maybe better to have two separate event handlers in the yaml - one for pushes to main, and one for PRs and their (force) pushes. Then the check and build is much quicker for those PRs where only a single package changed.

but push to pr (on our side) are pr event so the condition of checking all the package should not be triggered.

systemcrash commented 2 months ago

Hmm - my action logs for single package changes show they check every package.

Ansuel commented 2 months ago

@systemcrash can you link the action? Are you sure it's not run on your repository?

systemcrash commented 2 months ago

The runs are in my repo - but that should not matter, or?

Ansuel commented 2 months ago

It does as you push commits to a branch, push on your repository are not handled by openwrt runner but by your runner.

Check here

https://github.com/openwrt/openwrt/actions "Pull request https://github.com/openwrt/openwrt/pull/14448 synchronize by roshii"

For push we have instead Build Kernel #10217: Commit 12137cb pushed by openwrt-bot

But here (my repository) (ignore the gravestone of failed actions ahahha) https://github.com/ansuel/openwrt/actions?page=2

920: Commit 23df902 pushed by Ansuel

systemcrash commented 2 months ago

The Determine Changed Packages step correctly determines the only changed package. But the Download and check packages code in the first post checks everything. So something isn't right, or could be better.

( Irrespective of which repo or runner )