GeezerVison

Thanks for your contribution.

I looked into adding concurrency for our jobs before but it has a major shortcoming that you cannot limit it to branches. That will lead to jobs on main and release/* branches being cancelled which is not what we want. We only want this for pull requests and then for all workflows.

Also see https://trac.cppcheck.net/ticket/12325.

I am curious though why the workflow is being triggered for random branches in your case since we limited the branches this is triggered on:

on:
  push:
    branches:
      - 'main'
      - 'releases/**'
      - '2.*'
    tags:
      - '2.*'
  pull_request:

Thanks for your prompt reply.

I think what you describe can be achieved by the following syntax (given that we insert it in all .yml files), but I will test it just to be sure and then get back to you:

concurrency:
  group: ${{ github.event_name }}-${{ github.workflow }}-${{ github.head_ref || github.ref }}
  cancel-in-progress: ${{ github.event_name == 'pull_request' }} # this will cancel only PRs

Would you mind explaining why you do not want this:

That will lead to jobs on main and release/* branches being cancelled which is not what we want
It would seem that this is a similar case as in the PR: if a newer commit on the same branch arrives, why keep running the workflow for the old commit?

Thanks a lot!

I think what you describe can be achieved by the following syntax (given that we insert it in all .yml files)

That is so obvious and simple I am flabbergasted why this couldn't be done back then. I will see if I can find my old WIP code tomorrow and see if that gives me a hint why.

Would you mind explaining why you do not want this:

That will lead to jobs on main and release/* branches being cancelled which is not what we want
It would seem that this is a similar case as in the PR: if a newer commit on the same branch arrives, why keep running the workflow for the old commit?

We want the CI "all green" on code which will be used. If we would cancel builds this might not be the case if a lot of stuff is merged and would make it harder to determine which commit actually caused the issue.

Also most of the time in the CI is currently "wasted" by our code because of performance issues. There are WIP PRs to improve things.

Thanks for working on this. I envision that this could mean that fewer pipelines will be queued and that you will get the CI result a little faster for your latest push. That is good!

That will lead to jobs on main and release/* branches being cancelled which is not what we want

I also agree with @firewave.

We want the CI "all green" on code which will be used. If we would cancel builds this might not be the case if a lot of stuff is merged and would make it harder to determine which commit actually caused the issue.

This is interesting and makes sense.

I envision that this could mean that fewer pipelines will be queued and that you will get the CI result a little faster for your latest push

Exactly!

That is so obvious and simple I am flabbergasted why this couldn't be done back then. I will see if I can find my old WIP code tomorrow and see if that gives me a hint why.

I found my old changes and I think the problem I was facing was finding a solution to generate groups which work with pushes and PRs which you did solve with ${{ github.head_ref || github.ref }}.

I was not aware that strings could be used in concatenation code - looking at it I would have assume that the expression would resolve to true.

And I also did not think about just leveraging github.event_name in cancel-in-progress.

Regrading some question I had about this back then - can you confirm the the groups are just enforced across repos/forks and not globally? Otherwise the group needs to add ${{ github.repository }}.

I will also do some tests later on to see if it behaves as expected.

can you confirm the the groups are just enforced across repos/forks and not globally? Otherwise the group needs to add ${{ github.repository }}

I confirm; workflows in this repo's .github/workflows/ will only apply here, not globally.

I have just pushed the improved version, let me know if I can do anything else. For example, I could add it to all workflows that trigger on PR if you wish.

I have just pushed the improved version, let me know if I can do anything else. For example, I could add it to all workflows that trigger on PR if you wish.

That would be great. But let me run some tests first. Will do later today.

We want the CI "all green" on code which will be used. If we would cancel builds this might not be the case if a lot of stuff is merged and would make it harder to determine which commit actually caused the issue.

This is interesting and makes sense.

Another point is that we do not require PRs to be at HEAD to be merged. So even though no merge conflicts are shown the changes from various PRs might not be compatible with each other. That's why we also need to do the workflows for the main branch. Otherwise we could probably disable all workflow outside of PRs.

Also a note on why we filter the workflow. We had the problem that when you create a PR from a branch in the same repo you want to merge it into the builds would run twice. Once for the PR and once for the push to the branch. This is making it harder to test changes before you open a PR even if you are not working on the repo the PR will merge into.

even though no merge conflicts are shown the changes from various PRs might not be compatible with each other

Oh, I see, that makes sense!

We had the problem that when you create a PR from a branch in the same repo you want to merge it into the builds would run twice

Right, we were also thinking about this problem and how to propose an optimization to overcome it, but we couldn't come up with something that can be applied automatically to an arbitrary yaml file. What you have done here seems to work great.

Sorry for not testing the changes yet, but something else came up.

We had the problem that when you create a PR from a branch in the same repo you want to merge it into the builds would run twice

Right, we were also thinking about this problem and how to propose an optimization to overcome it, but we couldn't come up with something that can be applied automatically to an arbitrary yaml file. What you have done here seems to work great.

Except for the shortcoming. And I am not too happy about it since i affects all forks. But so far "it works".

An optimizations we also implemented is using ccache (but apparently the configuration is flawed - see #7472 (review)). We did not use path matching to limit the execution of certain workflows because things are too intertwined and we essentially just have a single output (i.e. the main executable).

Okay, I gave it a spin.

This prevents the workflows from running more than once on main. In our case that is fine and it might cause delays (in some cases when there is a flakey we might restart some worksflows/jobs). And if there are workflows with jobs that can run in parallel from different pushes this would also prevent it. So the concurrency should be defined on a job level to enable that parallelism.

But that really doesn't matter because in our case the concurrency should not be applied when in the non-PR case at all (I have the feeling that this is what I was actually hitting on back then). I wonder if an empty group would disable it (also something I raised back then)?

We can make this work this by making the concurrency a unique ID for the non-PR case:

concurrency:
  group: ${{ github.event_name }}-${{ github.workflow }}-${{ github.head_ref || github.sha }}
  cancel-in-progress: ${{ github.event_name == 'pull_request' }} # this will cancel only PRs

We can make this work this by making the concurrency a unique ID for the non-PR case:

In that case we might not even need the conditional value for cancel-in-progress because you cannot restart any workflow until all jobs have finished. We could keep it for being explicit.

We can make this work this by making the concurrency a unique ID for the non-PR case:

Oh no - that is all wrong (it's too warm). That is not set for PRs. It should be:

concurrency:
  group: ${{ github.event_name }}-${{ github.workflow }}-${{ github.event.pull_request.number || github.sha }}
  cancel-in-progress: ${{ github.event_name == 'pull_request' }} # this will cancel only PRs

It should be

This will have the same effect with the changes in this PR, under the condition that from a single branch, only one PR can be opened.

This will have the same effect with the changes in this PR, under the condition that from a single branch, only one PR can be opened.

          echo "${{ github.event_name }}-${{ github.workflow }}-${{ github.head_ref }}"
          echo "${{ github.event_name }}-${{ github.workflow }}-${{ github.ref }}"
          echo "${{ github.event_name }}-${{ github.workflow }}-${{ github.sha }}"
          echo "${{ github.event_name }}-${{ github.workflow }}-${{ github.event.pull_request.number }}"

main (push)

push-1-
push-1-refs/heads/main
push-1-ae509c0aabf761f0cb48bd3d157fffa7168101d1
push-1-

branch (push)

push-1-
push-1-refs/heads/branch
push-1-1c7b78983841abf7d973c9bfa357e55ad387614c
push-1-

branch (pull_request)

pull_request-1-branch
pull_request-1-refs/pull/1/merge
pull_request-1-6ed8b7e5f564c94cb90a13e310ae31d944ea88ef
pull_request-1-1

I cannot think right now - it is just too warm ...

I cannot think right now - it is just too warm ...

:D

Thanks for the echo results. I will also have another look and try to prepare a couple of illustrative examples.

In the meantime, you may also have a look at this one: #7641

The change is much simpler, we would be curious to see what you think of the idea.

We can make this work this by making the concurrency a unique ID for the non-PR case:

Oh no - that is all wrong (it's too warm). That is not set for PRs. It should be:

concurrency:
  group: ${{ github.event_name }}-${{ github.workflow }}-${{ github.event.pull_request.number || github.sha }}
  cancel-in-progress: ${{ github.event_name == 'pull_request' }} # this will cancel only PRs

I had another look in this, and I think your suggestion is better.

It also works in the case where from branch feature-X you open two PRs (I can't think of a reason why someone would do it though), PR 1 and PR 2.

In your poposal, the pr_number would result in two concurrency groups, thus the CI/CD would run in both PRs.
In my proposal, the head_branch would be the same, so the later PR would cancel the CI of the former.

Given that, I could apply your proposal if you wish.

Will have another look later today.

It also works in the case where from branch feature-X you open two PRs (I can't think of a reason why someone would do it though), PR 1 and PR 2.

I think this is impossible in GitHub and there is a 1-to-1 relation between branches and PRs. A look at https://github.com/danmar/cppcheck/branches seems to support that.

I think this is impossible in GitHub and there is a 1-to-1 relation between branches and PRs.

It is possible only if the target branch is different: I can open a PR from feature/A to main and another PR from feature/B to develop.

I just tried it by attempting to open a PR from the branch of this PR but targeting 2.9.x and not main, and it was possible:

It is possible only if the target branch is different: I can open a PR from feature/A to main and another PR from feature/B to develop.

That is very interesting. That should not affect our main repo but might forks. Bot something I think we should consider for this repo.

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@kitsiosk @firewave
as far as I see in the comments we were more or less ready to merge this?

Hi @danmar,

I see that nothing changed in the CI/CD file recently, so I believe the change proposed in this PR is still useful and can be merged.

@firewave can you please make the decision on this one?