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

x/build: trybots should rebase when testing #9858

Open
bradfitz opened this Issue Feb 12, 2015 · 13 comments

Comments

Projects
None yet
4 participants
@bradfitz
Member

bradfitz commented Feb 12, 2015

Currently with the trybot code, if Go's master is A and we have pending CLs B and C on A, currently we test B-on-A and C-on-A, but then when B is merged, we're still testing C on A, instead of C-on-B.

We should do:

https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#rebase-change

... which will add a patch set to the change, rebased. It could get spammy, though. Shawn says "don't complain when you have patch set 85 on a trivial one line change". So maybe we want to be careful when we do it. Maybe only for auto-submit.

@bradfitz bradfitz added the builder label Feb 12, 2015

@minux

This comment has been minimized.

Member

minux commented Feb 12, 2015

@bradfitz

This comment has been minimized.

Member

bradfitz commented Feb 12, 2015

We could, except that the trybot doesn't use any git right now. It pulls tar.gz files from Gerrit of a commit. We don't even have the "git" command on the builders.

@minux

This comment has been minimized.

Member

minux commented Feb 12, 2015

@bradfitz

This comment has been minimized.

Member

bradfitz commented Feb 12, 2015

I wouldn't do anything to the buildlet for this. We could apply the patch using a new service in a Docker container under the coordinator, using real git and/or real patch, and then extract the resulting tarball to push to the buildlets, like normal.

@minux

This comment has been minimized.

Member

minux commented Feb 12, 2015

@josharian

This comment has been minimized.

Contributor

josharian commented Feb 17, 2015

This'll need to respect the branch as well to keep trybot useful for release branch testing.

@bradfitz

This comment has been minimized.

Member

bradfitz commented Feb 26, 2015

@rsc says in email "Please keep using the exact commit. Thanks."

Russ, can you elaborate on what you'd like to see long-term? Would it be useful to also try the commit cherry-picked on top of tip?

@minux

This comment has been minimized.

Member

minux commented Mar 8, 2015

Please see https://golang.org/cl/6802/ as an extreme example of why this
issue should be fixed.

All trybots are fine with that CL, but because the CL is not rebased to the
latest master, after submission, it fails all builds.

@minux

This comment has been minimized.

Member

minux commented Mar 8, 2015

Sorry for the false alert. The problem I mentioned in the last comment is
caused by me running trybot on the wrong patch set. Will file a separate
issue.

Update: the actual reason is not me running on the wrong patch set.
It's because the auto-merge feature of Gerrit failed to merge the
change correctly. How could that be possible? (Note patch set 2 is created
by Gerrit, not by the author of the CL.)

So again, trybot should use rebase to test the patch in order to catch this
problem.

@minux

This comment has been minimized.

Member

minux commented Mar 13, 2015

Trybot really should rebase before testing, two more cases:

  1. CL 7032 removes one argument to gc.Naddr, trybot passes, but
    the build breaks after it's submitted because master contains cmd/7g,
    but that CL is based on a commit before the introduction of 7g, so
    it doesn't change 7g.
  2. trybot reports failure for CL 7080 on nacl/amd64p32, however,
    that's because the CL is based on a commit before the nacl build fix.
    The trybot messages asks to see the build dashboard if the failure
    is a new one, and according to the dashboard, it's indeed a new
    failure, but actually it's not.

@rsc rsc added this to the Unplanned milestone Apr 10, 2015

@rsc rsc changed the title from build: trybots should rebase when testing to x/build: trybots should rebase when testing Apr 14, 2015

@rsc rsc modified the milestones: Unreleased, Unplanned Apr 14, 2015

@rsc rsc removed the builder label Apr 14, 2015

@rsc

This comment has been minimized.

Contributor

rsc commented Apr 29, 2015

Attempting to rebase completely breaks any testing of multi-CL changes, since the CL will be considered in isolation instead of the work that has come before. And at least some of us do pretty much nothing but multi-CL changes.

In the case of a commit hash where the parent is on origin/master (so it's either the bottom of or not part of a multi-CL change) then rebasing would be fine. But it's very important not to rebase otherwise, unless somehow you rebase the entire sequence.

@minux

This comment has been minimized.

Member

minux commented Apr 29, 2015

@bradfitz

This comment has been minimized.

Member

bradfitz commented Jun 9, 2015

Okay, this is now much easier.

We're now using our own Git rev -> .tar.gz server, so we can do things like this more easily, since we don't depend on Gerrit to generate the .tar.gz.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment