Skip to content
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

[skip ci] Re-enable nightly redis build #16118

Merged
merged 2 commits into from
Oct 1, 2024
Merged

Conversation

cmb69
Copy link
Member

@cmb69 cmb69 commented Sep 29, 2024

Is there an easy way to test whether the build works again?

cc @iluuu1994

@cmb69 cmb69 requested a review from TimWolla as a code owner September 29, 2024 09:53
@iluuu1994
Copy link
Member

iluuu1994 commented Sep 29, 2024

@cmb69 Unfortunately, not without triggering the whole nightly suite (or copying the relevant job to the push.yml file).

Under "Run workflow":
https://github.com/php/php-src/actions/workflows/nightly.yml

It would be possible to add a dropdown or set of checkboxes (multi-choice is apparently not a thing), to restrict which jobs are executed. I could have used that on multiple occasions.

@cmb69
Copy link
Member Author

cmb69 commented Sep 29, 2024

The problem with manually triggering workflows is, to my knowledge, that you can only select branches on php-src, but not forks. This is why I usually push to winlibs directly when preparing a PR, instead of pushing to my fork; not a big deal there, but might not be the best idea for php-src (already so many branches).

@iluuu1994
Copy link
Member

@cmb69 You can just trigger the workflow in your fork.

@cmb69
Copy link
Member Author

cmb69 commented Sep 29, 2024

Oh, indeed. Just had to update my fork.

@cmb69
Copy link
Member Author

cmb69 commented Sep 30, 2024

See https://github.com/cmb69/php-src/actions/runs/11111548303. I'll submit a PR later. As I understand it, boths PRs should target PHP-8.4, right?

@iluuu1994
Copy link
Member

iluuu1994 commented Sep 30, 2024

@cmb69 As long as you're only modifying the nightly.yml file, sending it to master is sufficient. It's one of the quirks of GitHub actions: Cron is only triggered for the default branch, so the nightly configuration comes from master for all branches. The individual action files are used from the branch that is checked out.

@iluuu1994
Copy link
Member

iluuu1994 commented Sep 30, 2024

@cmb69 Can you disable xdebug for now, while you're at it? Or mark it as continue-on-error: true.

It currently fails already during configure for PHP >= 8.5.0.
@cmb69
Copy link
Member Author

cmb69 commented Sep 30, 2024

As long as you're only modifying the nightly.yml file, sending it to master is sufficient. It's one of the quirks of GitHub actions: Cron is only triggered for the default branch, so the nightly configuration comes from master for all branches. The individual action files are used from the branch that is checked out.

Ah, thanks for the explanation!

Can you disable xdebug for now, while you're at it?

Done, although that defeats the purpose of the nightly PECL job. Thus I've filed xdebug/xdebug#977. If the PR will not be accepted, we can still switch to doing a Windows build, since there is no such restriction in config.w32. ;)

@iluuu1994
Copy link
Member

Thinking about this a little more, since the workflow trigger does actually use the nightly file from the given branch, there might be benefits to keeping it synced. That said, since it already isn't in sync, we should do that in a separate PR if desired (simply copying the one from master).

@cmb69
Copy link
Member Author

cmb69 commented Oct 1, 2024

(simply copying the one from master).

I'm not sure if it's that simple. A lot of details might not work with master's nightly.yml for older branches.

If it's not about debugging/developing the nightly workflow, but rather to run e.g. ASan builds for older branches, I think we need to restructure (basically, have individual workflows in all branches, which can be triggered manually, and might be re-used by nightly.yml).

@iluuu1994
Copy link
Member

iluuu1994 commented Oct 1, 2024

I'm not sure if it's that simple. A lot of details might not work with master's nightly.yml for older branches.

We already need to keep it working for older branches, as otherwise the cron nightly wouldn't work. That's why there are version checks in it.

@cmb69
Copy link
Member Author

cmb69 commented Oct 1, 2024

It's easy to try it out, but I'm quite sure it wouldn't work. Already nightly-matrix.php may not work on older branches as expected, and the different actions which are used to set up stuff are also likely not compatible. As it's now, nightly.yml just checks out older branches, but completely uses the .github stuff from the master branch.

@iluuu1994
Copy link
Member

Already nightly-matrix.php may not work on older branches as expected

That is true. It should be kept up-to-date also.

but completely uses the .github stuff from the master branch

This is not accurate. It uses only .github/workflows/nightly.yml from master. The rest of the files come from the branch. Hence, we're already merging changes in the action files to older branches. We'd need to do this anyway for the push workflow.

@cmb69
Copy link
Member Author

cmb69 commented Oct 1, 2024

Indeed, you're right. Anyway, I'm going to merge this PR (that should bring a small improvement for the PECL job). We can backport later if we want to.

@cmb69 cmb69 merged commit 62a1eb9 into php:master Oct 1, 2024
2 checks passed
@cmb69 cmb69 deleted the cmb/nightly-redis branch October 1, 2024 14:38
@iluuu1994
Copy link
Member

iluuu1994 commented Oct 1, 2024

Sorry, I forgot to actually approve this. LGTM of course!

Already nightly-matrix.php may not work on older branches as expected

That is true. It should be kept up-to-date also.

Or, we could actually just specify the master branch in the checkout action for the GENERATE_MATRIX job. But it might be better to just backport everything to avoid additional confusion.

@cmb69
Copy link
Member Author

cmb69 commented Oct 1, 2024

Frankly, I still feel that we're doing something "wrong" here. Maybe it's just because I've never worked with cron triggers before; maybe it's because it never occured to me to run a workflow on a checkout of another branch. I wonder if reusable workflows could be a cleaner solution. Have you already considered this?

@iluuu1994
Copy link
Member

Maybe @TimWolla can chime in, but from my experience GH actions is just pretty bad in terms of reusability and configuration in general...

@cmb69
Copy link
Member Author

cmb69 commented Oct 1, 2024

I think, what we want is to run scheduled workflows for all branches. That is not supported by GH. Thus, we're running a single scheduled workflow from master, which checks out the required branches (simplified). I'd rather have a minimal "coordinator" in master, which then calls workflows on the other branches. When calling reusable workflows, you can specify a branch; I don't know, though, whether this branch is only used for checking out the workflow file, or whether that branch is targeted. Would need to try that out.

@TimWolla
Copy link
Member

TimWolla commented Oct 1, 2024

I'd rather have a minimal "coordinator" in master, which then calls workflows on the other branches.

That makes sense to me. I'm doing something similar in a private repository: I have a workflow to create release builds. This workflow is automatically triggered using the workflow_call: trigger (i.e. the reusable workflow trigger) after CI succeeds. But I can also manually trigger it using the workflow_dispatch: trigger. I did not use it to workflow_call: a workflow on a different branch, though.

I don't know, though, whether this branch is only used for checking out the workflow file, or whether that branch is targeted.

This distinction doesn't really matter. If you use the workflow file for branch X and then in the checkout action you also use the respective branch, then for all intents and purposes it's targeting that branch.


A simpler solution might be not using workflow_call: at all, but instead trigger workflow_dispatch: from CI using gh workflow run --ref branch nightly.yml, but I'm not sure if the automated CI access token may access the workflow_dispatch: API (to prevent infinite recursion).

@TimWolla
Copy link
Member

TimWolla commented Oct 1, 2024

I'll have a look if I can perform some tests regarding the options in a very simplified test repository of mine in the next days.

jorgsowa pushed a commit to jorgsowa/php-src that referenced this pull request Oct 1, 2024
* Re-enable nightly redis build (that should pass again)
* Disable Xdebug for now (it currently fails already during configure for PHP >= 8.5.0)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants