RFC: TestBot priorities
I'm quite set on the number one priority but after that it's more fuzzy. So if you feel something's important you can ask for it to be moved up the list. 1. Flaky failures blacklisting -> Get rid of false positives. https://www.winehq.org/pipermail/wine-devel/2020-March/163383.html 2. Use results from timed out WineTest runs -> Currently these have to be ignored. But some VMs (e.g. w1064v1809_ja and w1064v1809_hi) time out most of the time so there are not enough reference WineTest results to reliably detect intermittent failures. But with test unit blacklisting it would likely be possible to just blacklist intermittent failures so this priority may be lowered (at the cost of already slipping towards a long blacklist). Another alternative would be for Wine developers to fix the timeouts while I do something else. 3. Figure out / fix the occasional long delays in tests -> They cause a number of test intermittent failures (timeouts in short WaitForSingleObject() calls). If they can be fixed by tweaking the VM clock configuration then I'd rather figure that out before restoring VMs so I only have one pass to make through them. But initial tests have been inconclusive so if it's going to take long or end up requiring a complex fix like CPU pinning it will have to be delayed. https://www.winehq.org/pipermail/wine-devel/2020-March/162846.html 4. Add a Windows 10 1909 VM and rebalance the test configurations. -> w1064 has too many configurations to test (7+6) which increases the time it takes to proces jobs. I already created a new w10pro64 VM but it's not fully configured yet. Once set up the Windows 10 tests will be spread between the vm3 and vm4 hosts, for an ~2x speedup of the Windows 10 tests. 5. Restore the Vista VMs -> I've been asked about these a couple of times. So maybe restoring them should be higher priority. 6. Fix the Windows 8 VM WineTest crashes -> Most WineTest results are incomplete (but the results are still used which is actually wrong until point 2 is fixed and could cause false positives). 7. Continue fixing miscellaneous issues -> Complete automation of the TestBot test suite and then go through it: threaded patch series support, shared Gecko / Mono support, fix WinePrefix update issues, patches site interaction, .com program tests, etc. -- Francois Gouget <fgouget(a)codeweavers.com>
Francois Gouget <fgouget(a)codeweavers.com> wrote:
I'm quite set on the number one priority but after that it's more fuzzy. So if you feel something's important you can ask for it to be moved up the list.
Please remove the recently added "feature" to use -Werror when compiling the tests. It's the worst thing you get when you just want to run simple test: https://testbot.winehq.org/JobDetails.pl?Key=68772 -- Dmitry.
On Fri, 3 Apr 2020, Dmitry Timoshkov wrote: [...]
Please remove the recently added "feature" to use -Werror when compiling the tests. It's the worst thing you get when you just want to run simple test: https://testbot.winehq.org/JobDetails.pl?Key=68772
My understanding is that Alexandre compiles Wine with -Werror (as per the maintainer mode and patch rejections due to compilation warnings). So any patch that fails to compile with -Werror should be rejected by the TestBot. Now I can understand it can be annoying for manually submitted jobs if you don't plan on submitting the patch but having separate trees so the TestBot can compile either with or without -Werror is not very practical. -- Francois Gouget <fgouget(a)codeweavers.com>
Francois Gouget <fgouget(a)codeweavers.com> wrote:
On Fri, 3 Apr 2020, Dmitry Timoshkov wrote: [...]
Please remove the recently added "feature" to use -Werror when compiling the tests. It's the worst thing you get when you just want to run simple test: https://testbot.winehq.org/JobDetails.pl?Key=68772
My understanding is that Alexandre compiles Wine with -Werror (as per the maintainer mode and patch rejections due to compilation warnings). So any patch that fails to compile with -Werror should be rejected by the TestBot.
Tesbot should not pretend that it's the maintainer, all it needs to do is just execute the tests.
Now I can understand it can be annoying for manually submitted jobs if you don't plan on submitting the patch but having separate trees so the TestBot can compile either with or without -Werror is not very practical.
It's actually very harmful, it's a pure deny of service for no reason. -- Dmitry.
On 4/3/20 11:50 AM, Dmitry Timoshkov wrote:
Francois Gouget <fgouget(a)codeweavers.com> wrote:
On Fri, 3 Apr 2020, Dmitry Timoshkov wrote: [...]
Please remove the recently added "feature" to use -Werror when compiling the tests. It's the worst thing you get when you just want to run simple test: https://testbot.winehq.org/JobDetails.pl?Key=68772 My understanding is that Alexandre compiles Wine with -Werror (as per the maintainer mode and patch rejections due to compilation warnings). So any patch that fails to compile with -Werror should be rejected by the TestBot. Tesbot should not pretend that it's the maintainer, all it needs to do is just execute the tests.
Now I can understand it can be annoying for manually submitted jobs if you don't plan on submitting the patch but having separate trees so the TestBot can compile either with or without -Werror is not very practical. It's actually very harmful, it's a pure deny of service for no reason.
I think we can only enable -Werror for patches coming from the wine-devel. It's indeed making tests more inconvenient. Sometimes, you just want to comment out a large part of code and only test a specific spot.
Zhiyi Zhang <zzhang(a)codeweavers.com> wrote:
Please remove the recently added "feature" to use -Werror when compiling the tests. It's the worst thing you get when you just want to run simple test: https://testbot.winehq.org/JobDetails.pl?Key=68772 My understanding is that Alexandre compiles Wine with -Werror (as per the maintainer mode and patch rejections due to compilation warnings). So any patch that fails to compile with -Werror should be rejected by the TestBot. Tesbot should not pretend that it's the maintainer, all it needs to do is just execute the tests.
Now I can understand it can be annoying for manually submitted jobs if you don't plan on submitting the patch but having separate trees so the TestBot can compile either with or without -Werror is not very practical. It's actually very harmful, it's a pure deny of service for no reason.
I think we can only enable -Werror for patches coming from the wine-devel. It's indeed making tests more inconvenient. Sometimes, you just want to comment out a large part of code and only test a specific spot.
Even the patches from wine-devel should not be punished by a simple testing service. It should do its job of running the tests, everything else is not its business. -- Dmitry.
On Fri, 3 Apr 2020, Dmitry Timoshkov wrote: [...]
Even the patches from wine-devel should not be punished by a simple testing service. It should do its job of running the tests, everything else is not its business.
You need to remember the point of the TestBot: it is to automate testing of the patches so Alexandre can save time by not having to deal with patches that are wrong. That means: * Verifying the patch applies using the same parameters (fuzzy) that Alexandre uses. * Verifying that the patch compiles the way Alexandre wants. * Verifying that the patch does not introduce new failures. If any of these applies different criteria from what Alexandre does then it means he has to manually duplicate the work of the TestBot. -- Francois Gouget <fgouget(a)codeweavers.com>
Francois Gouget <fgouget(a)codeweavers.com> wrote:
On Fri, 3 Apr 2020, Dmitry Timoshkov wrote: [...]
Even the patches from wine-devel should not be punished by a simple testing service. It should do its job of running the tests, everything else is not its business.
You need to remember the point of the TestBot: it is to automate testing of the patches so Alexandre can save time by not having to deal with patches that are wrong.
That's a somewhat distorted vision of the original Wine testbot: run the tests, and do this job the best way it could do.
That means: * Verifying the patch applies using the same parameters (fuzzy) that Alexandre uses. * Verifying that the patch compiles the way Alexandre wants. * Verifying that the patch does not introduce new failures.
If any of these applies different criteria from what Alexandre does then it means he has to manually duplicate the work of the TestBot.
Are there no other priorites so it's started to look like a project of some really strange and random things? Is it possible to make testbot really useful for *developers*? Otheriwise posting priority requests to wine-devel seem to be a waste of time, if the target tesbot audience is actually only Alexandre. -- Dmitry.
On Fri, 3 Apr 2020, Zhiyi Zhang wrote: [...]
I think we can only enable -Werror for patches coming from the wine-devel.
Do you have a concrete proposal?
It's indeed making tests more inconvenient. Sometimes, you just want to comment out a large part of code and only test a specific spot.
That's where if (0) comes in. -- Francois Gouget <fgouget(a)codeweavers.com>
On 4/3/20 6:09 PM, Francois Gouget wrote:
On Fri, 3 Apr 2020, Zhiyi Zhang wrote: [...]
I think we can only enable -Werror for patches coming from the wine-devel. Do you have a concrete proposal? We could consider switching to clean build with ccache set up. So a separate tree is not needed. That would solve the problem of having to wait for nightly VM updates to complete for some patches as well since we don't have to update those VMs then. This will of course increase the TestBot workload a lot. But should be feasible after the 3900x upgrade.
Thanks, Zhiyi
It's indeed making tests more inconvenient. Sometimes, you just want to comment out a large part of code and only test a specific spot. That's where if (0) comes in.
On 03/04/2020 14:39, Zhiyi Zhang wrote:
On 4/3/20 6:09 PM, Francois Gouget wrote:
On Fri, 3 Apr 2020, Zhiyi Zhang wrote: [...]
I think we can only enable -Werror for patches coming from the wine-devel. Do you have a concrete proposal? We could consider switching to clean build with ccache set up. So a separate tree is not needed. That would solve the problem of having to wait for nightly VM updates to complete for some patches as well since we don't have to update those VMs then. This will of course increase the TestBot workload a lot. But should be feasible after the 3900x upgrade.
Thanks, Zhiyi
It's indeed making tests more inconvenient. Sometimes, you just want to comment out a large part of code and only test a specific spot. That's where if (0) comes in.
An alternative is to just print a summary of warnings and not actually fail the test because of them. Whoever is interested in the warnings can then easily spot them. It requires more work, though.
On Fri, 3 Apr 2020, Gabriel Ivăncescu wrote: [...]
An alternative is to just print a summary of warnings and not actually fail the test because of them. Whoever is interested in the warnings can then easily spot them. It requires more work, though.
I think that's more or less the way to go. 1. The builds would happen without -Werror. 2. GetLogLineCategory() would identify the warnings and add them to LogInfo but distinguish them from errors (replace IsNew with a Category field?). 3. Compare to the reference build to distinguish new warnings from old ones. This is necessary in case the compiler used by the TestBot produces warnings on the unpatched Wine tree. It will require storing the build logs in latest too. 4. The warnings would be saved in the errors files. The new line types could be 'w' and 'W' for old and new warnings respectively. 5. This will let the WineRun*() scripts know about both warnings and errors. They can then decide what to do depending on the type of job and whether the warnings are new or not. - Old warnings would always be ignored. - If a build task has new warnings, instead of marking it as 'badbuild' it could be marked as 'completed' as usual but with a non-zero number of failures. This would probably allow running the tests on Windows but I think the patch would still be considered bad since the job would end up with a non-zero number of failures. - For Wine tasks the number of warnings would be added to the Failures field for the same overall result. - So in both cases the non-zero number of failures would only impact wine-devel patches. 6. The JobDetails page would show the warnings in summary mode and highlight them in the full build log. -- Francois Gouget <fgouget(a)codeweavers.com>
On Fri, 3 Apr 2020, Zhiyi Zhang wrote:
On 4/3/20 6:09 PM, Francois Gouget wrote:
On Fri, 3 Apr 2020, Zhiyi Zhang wrote: [...]
I think we can only enable -Werror for patches coming from the wine-devel. Do you have a concrete proposal? We could consider switching to clean build with ccache set up.
The build and Wine VMs already use ccache. The TestBot does not need to wipe the build tree for each patch. So only files that have changed need to be rebuilt. But alternating between -Werror and no -Werror requires rerunning configure which takes about 30 seconds. A test I have done shows that such a basic rebuild takes about 130 seconds, 6 seconds of which are in make (and thus parallelisable). In comparison a basic build currently takes about 20 seconds.
So a separate tree is not needed. That would solve the problem of having to wait for nightly VM updates to complete for some patches as well since we don't have to update those VMs then.
I don't understand. A patch can cause the build process to run arbitrary commands. This means the VM must be reverted to a clean state after each build and this also wipes the ccache data. (That's why the Build/Wine updates are special tasks) Storing the ccache data out of the VM would not work because the VM would have write access to that persistent data and could poison the content of that cache. What could work would be to use distcc[1] so the compilation is performed outside the VM (and potentially distributed across multiple machines). The compilation server(s) could then use ccache. That would be safe as long as there is no security vulnerability in the compiler(s). I would probably want to place the compilers in a chroot sandbox or some such anyway. And we'd need to also ensure that all hosts that perform compilations (possibly including the VM) has consistent compiler versions. This would actually help a lot with patch series that start by modifying a global Wine header ;-) But that seems like way more complexity than I want to entertain for now. [1] https://distcc.github.io/ -- Francois Gouget <fgouget(a)codeweavers.com>
On 4/3/20 4:38 PM, Francois Gouget wrote:
The build and Wine VMs already use ccache.
The TestBot does not need to wipe the build tree for each patch. So only files that have changed need to be rebuilt. But alternating between -Werror and no -Werror requires rerunning configure which takes about 30 seconds. As Alexandre taught me on irc configure -C or better if you wipe the whole build dir configure --cache-file=<some-other-location/config.cache>
That gets configure for me down from also 30 s to about 4 s. bye michael
participants (5)
-
Dmitry Timoshkov -
Francois Gouget -
Gabriel Ivăncescu -
Michael Stefaniuc -
Zhiyi Zhang