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

rust-timer generated PRs include more than specific PR #832

Open
Mark-Simulacrum opened this issue Jan 26, 2021 · 2 comments
Open

rust-timer generated PRs include more than specific PR #832

Mark-Simulacrum opened this issue Jan 26, 2021 · 2 comments
Labels
C-bug A bug

Comments

@Mark-Simulacrum
Copy link
Member

For example, rust-lang/rust#81207 included more than just the requested PR, instead also including the PRs in the "head" of the rollup. This is likely because we're reverting up to master before the rollup, but should instead revert to the parent commit of the merge commit for a specific PR in the rollup.

@tgnottingham
Copy link
Contributor

This tests were also affected:

rust-lang/rust#81420
rust-lang/rust#81933

I don't think the potential regressions were looked into once it was noticed that this bug was occurring. I just want to make sure there's follow up on those when this issue is fixed, or manual follow up prior to that.

@rylev rylev added the C-bug A bug label Jul 29, 2021
@tgnottingham
Copy link
Contributor

I think the current approach is not at all testing what's intended, beyond the issue here. Since we use @rust-timer queue on the generated PR, we're comparing the performance of the try commit versus its master parent. But look at their contents:

Try commit content

The tip of the generated PR always has the exact content of the "Rollup merge" (well, whatever commit is given to @rust-timer make-pr-for, but that's what's expected I think) of the requested PR within the rollup PR, because that's the tree object we specify for its last commit. The try commit itself has the exact same content (barring a race condition where master gets more commits between the time the PR's commits are generated and the time the try build starts). The preceding revert commit we create has no effect, AFAICT.

That "Rollup merge" commit, and therefore the try commit, contains the requested PR, as well as all the PRs that were included in the rollup before it, and nothing that has merged to master since.

Try commit parent content

On the other hand, the try's master parent is the tip of master at the time of the try build, which contains the entire rollup, along with everything that has merged to master since the rollup PR was created.

So, not only is this testing way more changes than we'd like, it's also not testing the requested PR, since both sides of the comparison contain it.


I'm not sure exactly how we'd like to test the requested PR, but any solution that compares to the latest master has the problem of master already including the PR. So, if we're using the latest master as a comparison point, the only way to do it is to do a revert comparion by doing a proper git-revert of the commits in the requested PR. That can result in conflicts and require manual intervention, however. But maybe that's good enough.

A solution that would never result in git conflicts is to test the tip of the original PR against the master commit upon which it was based. Also, the tip of the original PR should almost always pass CI tests, since the rollup did.

I'm not familiar enough with the build infrastructure to know how much work this would be to integrate though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug A bug
Projects
None yet
Development

No branches or pull requests

3 participants