re: Patch checking robot coming
Ambroz wrote:
To me, a script that builds just every patch is a serious security flaw; I suppose it wouldn't be very hard for someone to send a naughty patch that would take control of your machine.
True.
I suggest you make it build patches in a chroot as a regular user, and copy over the chroot from a template every time a new patch is being built.
Sounds great. Want to implement that and send it my way? It'll take me a while to get the kinks worked out of the script, it'd be nice to have a hand with the chroot. - Dan
On Sat, Aug 2, 2008 at 8:24 PM, Dan Kegel <dank(a)kegel.com> wrote:
Ambroz wrote:
To me, a script that builds just every patch is a serious security flaw; I suppose it wouldn't be very hard for someone to send a naughty patch that would take control of your machine.
True.
I suggest you make it build patches in a chroot as a regular user, and copy over the chroot from a template every time a new patch is being built.
Sounds great. Want to implement that and send it my way? It'll take me a while to get the kinks worked out of the script, it'd be nice to have a hand with the chroot. - Dan
Its really ironic that you post this today as just yesterday I was contemplating the same thing, and not only doing a compile check but also a run of the test suite and valgrind. Other than that I wanted to put in a +1 for sending an email to the author and to a status website.
On Sat, Aug 2, 2008 at 7:25 PM, Zachary Goldberg <zgold550(a)gmail.com> wrote:
Its really ironic that you post this today as just yesterday I was contemplating the same thing, and not only doing a compile check but also a run of the test suite and valgrind.
Yeah, that's the end goal. Gotta walk before you run, though.
Other than that I wanted to put in a +1 for sending an email to the author and to a status website.
Yeah, I'll probably do that. I'll probably only send the email in case of a failure; nobody needs extra email that just tells them what they already knew (that their patch was perfect :-) - Dan
Dan Kegel wrote:
On Sat, Aug 2, 2008 at 7:25 PM, Zachary Goldberg <zgold550(a)gmail.com> wrote:
Its really ironic that you post this today as just yesterday I was contemplating the same thing, and not only doing a compile check but also a run of the test suite and valgrind.
Yeah, that's the end goal. Gotta walk before you run, though.
Other than that I wanted to put in a +1 for sending an email to the author and to a status website.
Yeah, I'll probably do that. I'll probably only send the email in case of a failure; nobody needs extra email that just tells them what they already knew (that their patch was perfect :-) - Dan
Depending on how bogged-down the robot gets (compiler cache will help), it might be useful to have a visible status somewhere so I know that my patch was at least checked before bothering Alexandre to commit it :) Thanks, Scott Ritchie
On Sat, 2 Aug 2008, Zachary Goldberg wrote: [...]
Its really ironic that you post this today as just yesterday I was contemplating the same thing, and not only doing a compile check but also a run of the test suite
One problem is that you need an X server, preferably on real hardware to please the DirectX chaps. That may be tricky for the chroot, but maybe not. Another problem with running the test suite is that all it's going to tell you is that it failed. As I understand it, even on Alexandre's machine it fails intermittently. One way it could work would be to only have the robot complain about new failures. This is tricky, see below, but feasible. Let's just take a test failure as an example: visual.c:506: Test failed: Present failed: Got color 0x00ffffff, expected 0x00ff0000. The problem here is that if someone adds ten lines near the beginning of visual.c, this may turn into: visual.c:516: Test failed: Present failed: Got color 0x00ffffff, expected 0x00ff0000. Or if the colors are random (probably not the case here but happens for other tests), then the next time you may get the following message: visual.c:506: Test failed: Present failed: Got color 0x001e89df, expected 0x00ff0000. So how is a bot to know all these are the same error? With git-blame, that's how. Just use the filename and line number as imput to git-blame: $ git-blame -p -L506,506 dlls/d3d8/tests/visual.c | head -n 1 078523f73e7b708dab06e888c24a1595bb5a63d8 495 506 1 What this means is that this line was first introduced in commit 078523f7 and was line number 495 at the time. Now, if you commit a patch adding 10 lines and then ask about line 516 you will get the exact same result. So a unique identifier for this line is: 078523f73e7b708dab06e888c24a1595bb5a63d8:dlls/d3d8/tests/visual.c:495 So all the bot has to do is to run git-blame on each failure line to get its unique id, then check the unique id against the list of known/allowed (intermittent) failures, and only fail the patch if the failure is not in the list. Now what I'd like is to have something like that for the Windows test results...
and valgrind.
There you have to take into account that for the past week we've had an average of 56 patch submissions per day. So if you want the bot to keep up it means it must spend less than 25 minutes on each patch. But if you want to have a little bit of room for growth it limiting it to 15 minutes per patch (96 patches/day) would be better... My understanding is that Valgrinding alone takes much more than 25 minutes already. -- Francois Gouget <fgouget(a)free.fr> http://fgouget.free.fr/ Hiroshima '45 - Czernobyl '86 - Windows '95
On Mon, Aug 4, 2008 at 1:55 AM, Francois Gouget <fgouget(a)free.fr> wrote:
So all the bot has to do is to run git-blame on each failure line to get its unique id, then check the unique id against the list of known/allowed (intermittent) failures, and only fail the patch if the failure is not in the list.
Great idea, thanks! I had been toying with ideas like 'only alert to diffs from yesterday' (but that's noisy) or 'run failures repeatedly' (to help with flaky tests) and 'ignore line numbers', but your idea sounds a lot better.
and valgrind.
There you have to take into account that for the past week we've had an average of 56 patch submissions per day. So if you want the bot to keep up it means it must spend less than 25 minutes on each patch. But if you want to have a little bit of room for growth it limiting it to 15 minutes per patch (96 patches/day) would be better...
Right, but this is very parallelizable. Valgrinding on my dual core machine takes 90 minutes now, so we'd just need four equivalently powerful machines. Not scary at all. (It would help if Alexandre would commit the parallel conformance test patch, otherwise I'll probably need eight machines.) - Dan
Francois Gouget wrote:
There you have to take into account that for the past week we've had an average of 56 patch submissions per day. So if you want the bot to keep up it means it must spend less than 25 minutes on each patch. But if you want to have a little bit of room for growth it limiting it to 15 minutes per patch (96 patches/day) would be better...
My understanding is that Valgrinding alone takes much more than 25 minutes already.
Well, you could throw more hardware at it. Alternatively, if most patches are good, then you could test several patches at a time and only test specific ones if something breaks. Thanks, Scott Ritchie
On Mon, Aug 4, 2008 at 11:08 AM, Scott Ritchie <scott(a)open-vote.org> wrote:
My understanding is that Valgrinding alone takes much more than 25 minutes already.
Well, you could throw more hardware at it.
That's the option I like.
Alternatively, if most patches are good, then you could test several patches at a time and only test specific ones if something breaks.
That could work too, but the scripting would be more complex and brittle. I'm less scared of doing the scripting to distribute the work over four computers.
I was distracted by my trip to Linuxworld (trip report: http://lula.org/pipermail/lula_lula.org/2008-August/015218.html ) but am still slowly making progress on the patch robot. It now looks at all mime parts of a message, and properly flags messages with multiple patches attached as violating list policy (since we require one message per patch). Next step is probably to have it generate a status page and start uploading results to a web site.
Dan Kegel wrote:
Sounds great. Want to implement that and send it my way? It'll take me a while to get the kinks worked out of the script, it'd be nice to have a hand with the chroot. - Dan
OK, I'll try it. I have a lot of experience with the OS's architecture so it should be ready soon.
I've written some code for the chroot, though it has proven to be harder than I taught it would, especially because of all the development tools and libraries that need to be copied into the chroot. Right now it will only work on Gentoo, other distributions will require some fine-tuning of paths, especially the toolchain. Dan, if you tell me which distro you use, I'll try to get it working there as well. To use it, you first need a second user in the system (not the one you're using!) that has read-access to files you create with default permissions (but no write access). Specify that user in the file "config" and run "patchwatcher.sh initialize_chroot". If everything goes well you will be able to get a shell inside the chroot with "patchwatcher.sh chroot-shell" or "patchwatcher.sh chroot-shell-owner" after running some commands displayed as root. After having a working chroot, use the initialize_tree command to setup the wine source (it's inside the chroot), and the "run" option to start continuous building.
participants (5)
-
Ambroz Bizjak -
Dan Kegel -
Francois Gouget -
Scott Ritchie -
Zachary Goldberg