Let's imagine this scenario:

(master history)
abcd - some commit
hijk - change something

(pull request history)
abcd - some commit
defg - add feature


      master                                  pull request

abcd - some commit           -------      defg - add feature
hijk - change something


Currently in the CI we checkout defg and run the tests. If hijk introduces some change that breaks defg, we only know about it after defg is in master.

I propose pull requests to be automatically rebased on top of master, so that the resulting story becomes:

abcd - some commit
hijk - change something
defg - add feature

Now defg is properly tested :)

This may not be an issue in smaller projects, but in cpython where most things are a moving target, it should make a difference. Here I am referring to master but applies to all other maintained branches.

Github can't merge if the PR doesn't cleanly rebase to master, or rather, it will ask you to resolve conflicts, so I don't think there should be any problem.

Keep in mind that this only works at the time the pull request is created. To check against the latest master the build needs to be retriggered, which you should be able to do manually.
Even if we don't check against the latest master when the PR is merged, this is already an improvement. After this we could look into maybe automatically retriggering the CI on approvals, or better, managing merges with a bot.

I don't think this is a real issue we should solve:

* Travis-CI, at least, does test against "merge commit", not HEAD of PR branch.
* As you said already, master branch grow after PR is created anyway.
* We run CI and buildbots against master branch too.  So we can find breakage soon when something go wrong.

See also https://mail.python.org/archives/list/python-committers@python.org/thread/WEU5CQKIA4LIHWHT53YA7HHNUY5H2FUT/.  This was a problem with other CI GitHub actions when a change had to be merged to master with which all PRs need to be manually rebased to pass.

>  See also https://mail.python.org/archives/list/python-committers@python.org/thread/WEU5CQKIA4LIHWHT53YA7HHNUY5H2FUT/.  This was a problem with other CI GitHub actions when a change had to be merged to master with which all PRs need to be manually rebased to pass.

This is an example of "master branch grow after PR is created anyway."

This issue proposes " this only works at the time the pull request is created."  It doesn't help that case.

> * Travis-CI, at least, does test against "merge commit", not HEAD of PR branch.

Where? https://github.com/python/cpython/blob/master/.travis.yml

> * As you said already, master branch grow after PR is created anyway.

> This issue proposes " this only works at the time the pull request is created."  It doesn't help that case.

Please note that the proposed fix is just that. I detailed that we should do something else, but this was the first step.

> * We run CI and buildbots against master branch too.  So we can find breakage soon when something go wrong.

Right, but then it might not be trivial to add a fix or revert changes. If more changes are merged before someone deals with the breakage, it might become very difficult to fix, at this point you are playing whack-a-mole reverting patches...

So, I disagree with you. This is a real issue. It's not a matter of if, it's a matter of when. And a matter of, how bad will it be when it happens?

There are two ways we could go about this:

* Automatically
No-one merges anything manually, they add a tag or comment to trigger the build bot. The bot will restart the tests and merge if they pass.

* Semi-automatically
When someone approves the patch, the tests will automatically be re-triggered, it then is on the developer to make sure there are no major differences from master at the time of merging, to avoid any issues. The tests can be triggered manually, but perhaps it would be best to introduce a `/test` command in the bots to do this, avoiding the developer to go through a few UI jumps.
If this is chosen, all current developers should be notified, and this added to the dev wiki. It also should be noted during the onboard process for new developers.

Please note in both situations, automatically rebasing PRs *is* the first step. After that, it's just figuring out a suitable model to re-trigger the checks before merging.

On Sat, May 9, 2020 at 6:08 PM Filipe Laíns <report@bugs.python.org> wrote:
>
> > * Travis-CI, at least, does test against "merge commit", not HEAD of PR branch.
>
> Where? https://github.com/python/cpython/blob/master/.travis.yml

https://docs.travis-ci.com/user/pull-requests/

"Rather than build the commits that have been pushed to the branch the
pull request is from, we build the merge between the source branch and
the upstream branch."

> * Automatically
> * Semi-automatically

Both are fine, but...

>
> Please note in both situations, automatically rebasing PRs *is* the first step. After that, it's just figuring out a suitable model to re-trigger the checks before merging.

Bot shouldn't use rebase, but merge master branch.
Rebase in pull request branch break the pull request sometime.
See this thread:
https://discuss.python.org/t/info-rebase-origin-master-push-f-workflow-corrupts-pull-request-rarely/2558/7

> "Rather than build the commits that have been pushed to the branch the
pull request is from, we build the merge between the source branch and
the upstream branch."

Okay, great. But this still only happens for Linux. And still has the same problem, the build needs to be re-triggered.

> Both are fine, but...

...

> Bot shouldn't use rebase, but merge master branch.
Rebase in pull request branch break the pull request sometime.
See this thread:
https://discuss.python.org/t/info-rebase-origin-master-push-f-workflow-corrupts-pull-request-rarely/2558/7

Okay, so maybe I was not clear enough here. The PR would be rebased locally in the CI runs before building and running the tests. Nothing would ever be pushed. We don't really want users have to pull their branch before making changes, also the users might not give us write access to their branch.

What I am proposing is to do here the same as Travis apparently does, and then maybe figure out a workflow where we (semi-/)automatically  re-trigger the CI, to make everything works correctly with master.

This is a low risk change. Rethinking the core workflow is the difficult part.

> re-trigger the CI, to make everything works correctly with master.

*re-trigger the CI, to make *sure* everything works correctly with master.

Just like Travis, Github actions also automatically rebases pull requests onto the latest master. As you can see from a recent workflow run, it uses the special ref: "refs/remote/pull/20047/merge" https://github.com/python/cpython/pull/20047/checks?check_run_id=665377507#step:2:898

What is this ref that Github provides?

https://git-scm.com/book/en/v2/GitHub-Maintaining-a-Project
> There’s also a refs/pull/#/merge ref on the GitHub side,
> which represents the commit that would result if you push
> the “merge” button on the site. This can allow you to test
> the merge before even hitting the button


Unless there's real world examples of ongoing problems with PRs being merged with outdated test runs, I don't think this is worth complicating our CIs for.

As Inada-san mentioned, the occasional slip through will eventually be caught by the buildbots and promptly reverted. Re-triggering the CI for very stale pull requests might be worth doing. I'm kinda opposed to an automated system because this creates an undue burden on our CI providers, Travis graciously provides us with extra time for our coverage builds to finish already. Re-running pull requests that might never be merged is a lot of added work for them. Maybe we can get one of the bots to tag old pull requests and then have some guidance in the core-dev guide to re-run the CIs manually when merging those PRs.

> Just like Travis, Github actions also automatically rebases pull requests onto the latest master. As you can see from a recent workflow run, it uses the special ref: "refs/remote/pull/20047/merge" https://github.com/python/cpython/pull/20047/checks?check_run_id=665377507#step:2:898

Thanks for letting me know. I did search through the documentation but I hadn't found anything regarding this. Well, I guess [1] hinted it but I did not pay enough attention :D

> Unless there's real world examples of ongoing problems with PRs being merged with outdated test runs, I don't think this is worth complicating our CIs for.

Although I understand your point, I think this is one of those cases where things can easily get out of hand. And when that happens, it could be very difficult to revert unless you catch it right away. There's too much noise for me to find examples, sorry.

> As Inada-san mentioned, the occasional slip through will eventually be caught by the buildbots and promptly reverted. Re-triggering the CI for very stale pull requests might be worth doing. I'm kinda opposed to an automated system because this creates an undue burden on our CI providers, Travis graciously provides us with extra time for our coverage builds to finish already. Re-running pull requests that might never be merged is a lot of added work for them.

Well, I was not really proposing that. I suggested to retrigger on approvals (and by this I mean approvals from core devs) or have a bot retriggering and merging automatically.

> Maybe we can get one of the bots to tag old pull requests and then have some guidance in the core-dev guide to re-run the CIs manually when merging those PRs.

Yes, this seems reasonable, and could be expanded in the future. I think we should document this and maybe add a bot command to allow devs to retrigger more easily (I was thinking they could just comment with /test).

[1] https://github.com/actions/checkout#Checkout-pull-request-HEAD-commit-instead-of-merge-commit