Re: [PATCH v5 1/5] iotests: remove 'linux' from default supported platforms
On 9/23/19 9:09 AM, Max Reitz wrote: > On 18.09.19 01:45, John Snow wrote: >> verify_platform will check an explicit whitelist and blacklist instead. >> The default will now be assumed to be allowed to run anywhere. >> >> For tests that do not specify their platforms explicitly, this has the >> effect of >> enabling these tests on non-linux platforms. For tests that always specified >> linux explicitly, there is no change. >> >> For Python tests on FreeBSD at least; only seven python tests fail: >> 045 147 149 169 194 199 211 >> >> 045 and 149 appear to be misconfigurations, >> 147 and 194 are the AF_UNIX path too long error, >> 169 and 199 are bitmap migration bugs, and >> 211 is a bug that shows up on Linux platforms, too. >> >> This is at least good evidence that these tests are not Linux-only. If >> they aren't suitable for other platforms, they should be disabled on a >> per-platform basis as appropriate. >> >> Therefore, let's switch these on and deal with the failures. > > What exactly do you mean by “deal with the failures”? Do you have a > reference to patches that deal with them, or are you or is someone else > working on them...? > > Apart from that, I am rather hesitant to take a patch through my tree > that not only may cause test failures on platforms that I will not or > actually cannot run tests on (like MacOS or Windows), but that actually > does introduce new failures as you describe. > > Well, at least it doesn’t introduce build failures because it appears > there is no Python test that’s in the auto group, so I suppose “rather > hesitant” is not an “I won’t”. > Think of it more like this: The failures were always there, but we hid them. I'm not "introducing new failures" as such O:-) I think that I have demonstrated sufficiently that it's not correct to prohibit python tests from running on other platforms wholesale, so I'd prefer we don't do that anymore. Further, iotests on FreeBSD already weren't 100% green, so I'm not causing a regression in that sense, either. I'm going to meekly push and ask that we stage this as-is, and when something bad happens you can remind me that I wanted this and make me do it. --js
Re: [PATCH v5 1/5] iotests: remove 'linux' from default supported platforms
On 28.09.19 01:35, John Snow wrote: > > > On 9/23/19 9:09 AM, Max Reitz wrote: >> On 18.09.19 01:45, John Snow wrote: >>> verify_platform will check an explicit whitelist and blacklist instead. >>> The default will now be assumed to be allowed to run anywhere. >>> >>> For tests that do not specify their platforms explicitly, this has the >>> effect of >>> enabling these tests on non-linux platforms. For tests that always specified >>> linux explicitly, there is no change. >>> >>> For Python tests on FreeBSD at least; only seven python tests fail: >>> 045 147 149 169 194 199 211 >>> >>> 045 and 149 appear to be misconfigurations, >>> 147 and 194 are the AF_UNIX path too long error, >>> 169 and 199 are bitmap migration bugs, and >>> 211 is a bug that shows up on Linux platforms, too. >>> >>> This is at least good evidence that these tests are not Linux-only. If >>> they aren't suitable for other platforms, they should be disabled on a >>> per-platform basis as appropriate. >>> >>> Therefore, let's switch these on and deal with the failures. >> >> What exactly do you mean by “deal with the failures”? Do you have a >> reference to patches that deal with them, or are you or is someone else >> working on them...? >> >> Apart from that, I am rather hesitant to take a patch through my tree >> that not only may cause test failures on platforms that I will not or >> actually cannot run tests on (like MacOS or Windows), but that actually >> does introduce new failures as you describe. >> >> Well, at least it doesn’t introduce build failures because it appears >> there is no Python test that’s in the auto group, so I suppose “rather >> hesitant” is not an “I won’t”. >> > > Think of it more like this: The failures were always there, but we hid > them. I'm not "introducing new failures" as such O:-) That is incorrect. As I have said, the conceptual problem is that the iotests now run as part of make check. As such, allowing auto tests to run on non-Linux platforms may introduce build failures that I cannot do anything about. And those are very much new failures. > I think that I have demonstrated sufficiently that it's not correct to > prohibit python tests from running on other platforms wholesale, so I'd > prefer we don't do that anymore. You have not. The actual argument to convince me is “This does not affect any tests in the auto group, so it will not introduce build failures at this time”. > Further, iotests on FreeBSD already weren't 100% green, so I'm not > causing a regression in that sense, either. My problem is twofold: (1) You claim “Sure, there are failures, but let’s just deal with them” and then you do not deal with them. Seems wrong to me. I’m fine with the argument “Sorry, royal ‘we’. But it just doesn’t help anyone to hide the errors. If someone’s on BSD and wants to run the iotests, let them.” That sounds good to me. (2) Maybe someone adds a Python test in the future that is in auto and that does not specify Linux as the only supported platform. Then I send a pull request and it breaks on macOS. Now what? Remove it from auto? Blindly put "macOS" in unsupported platforms? In any case, it’ll be a problem for no good reason. More on that in the next chunk. > I'm going to meekly push and ask that we stage this as-is, and when > something bad happens you can remind me that I wanted this and make me > do it. Make you do what? Deal with the fact that a pull request is rejected because a test fails on macOS? This is precisely the kind of problem I already had with adding the iotests to make check, and I’m already very much not happy about it. (In that case it was $DISPLAY not being set on Peter’s test system.) I’ll let you make the deduction of “The problem isn’t allowing the iotests to run on non-Linux platforms, but the fact that they run in make check” yourself, so that I no longer feel like I’m the only one who considers that having been a mistake. Max signature.asc Description: OpenPGP digital signature
Re: [PATCH v5 1/5] iotests: remove 'linux' from default supported platforms
On 01/10/2019 20.44, Max Reitz wrote: [...] > As I have said, the conceptual problem is that the iotests now run as > part of make check. As such, allowing auto tests to run on non-Linux > platforms may introduce build failures that I cannot do anything about. Well, simply run "make vm-build-openbsd", "make vm-build-freebsd", ... if something fails there, it likely should not be in the auto group. > And those are very much new failures. > >> I think that I have demonstrated sufficiently that it's not correct to >> prohibit python tests from running on other platforms wholesale, so I'd >> prefer we don't do that anymore. > > You have not. > > The actual argument to convince me is “This does not affect any tests in > the auto group, so it will not introduce build failures at this time”. I've applied the patch here and it works fine with FreeBSD and macOS: https://cirrus-ci.com/build/5169384718336000 https://travis-ci.com/huth/qemu/builds/129968676 It also works fine with "make vm-build-openbsd BUILD_TARGET=check-block" ... so I think you don't have to worry here. >> Further, iotests on FreeBSD already weren't 100% green, so I'm not >> causing a regression in that sense, either. > > My problem is twofold: > > (1) You claim “Sure, there are failures, but let’s just deal with them” > and then you do not deal with them. Seems wrong to me. > > I’m fine with the argument “Sorry, royal ‘we’. But it just doesn’t help > anyone to hide the errors. If someone’s on BSD and wants to run the > iotests, let them.” > > That sounds good to me. > > (2) Maybe someone adds a Python test in the future that is in auto and > that does not specify Linux as the only supported platform. Then I send > a pull request and it breaks on macOS. Now what? Remove it from auto? > Blindly put "macOS" in unsupported platforms? I think both solutions are good. If a test does not work on all systems, it either should not be in the "auto" group, or it needs to be modified to skip testing when running in an unsupported environment. > In any case, it’ll be a problem for no good reason. Really? Is "more test coverage" not a good reason? > More on that in the next chunk. > >> I'm going to meekly push and ask that we stage this as-is, and when >> something bad happens you can remind me that I wanted this and make me >> do it. > > Make you do what? Deal with the fact that a pull request is rejected > because a test fails on macOS? > > This is precisely the kind of problem I already had with adding the > iotests to make check, and I’m already very much not happy about it. > (In that case it was $DISPLAY not being set on Peter’s test system.) > > I’ll let you make the deduction of “The problem isn’t allowing the > iotests to run on non-Linux platforms, but the fact that they run in > make check” yourself, so that I no longer feel like I’m the only one who > considers that having been a mistake. Max, I can understand that you are a little bit annoyed that this "make check with iotests" caused some extra hurdles for you. But honestly, removing that again would be quite egoistic by you. Try to put yourself into the position of a "normal", non-blocklayer-maintainer QEMU developer. For me, iotests were a *constant* source of frustration. Often, when I ran them on top of my latest and greatest patches, to check whether I caused a regression, the iotests simply failed. Then I had to start debugging - did my patches cause the break, or is "master" broken, too? In almost all cases, there was an issue in the master branch already, either because they were failing on s390x, or because I was using ext4 instead of xfs, or because I was using an older distro than you, etc... . So while the iotests likely worked fine for the limited set of systems that you, Kevin and the other hard-core block layer developers are using, it's constantly broken for everybody else who is not using the very same setup as you. The only way of *not* getting upset about the iotests was to simply completely *ignore* them. Is it that what you want? Or maybe let me phrase it differently: Do you consider the iotests as something that is more or less "private" to the hard-core block layer developers, and it's ok if others completely ignore them and break them by accident (and you also don't expect the normal developers to fix the iotests afterwards)? Then sure, please go ahead and remove the iotests from "make check" again. Maybe I just understood the idea of having common tests in the repository wrong (or maybe the iotests should be moved to a separate repository instead, so that the normal QEMU developers do not get in touch with them anymore?) ... Otherwise, I think it was the right step to add them "make check" so that everybody now *has* to run at least a basic set of the iotests now before they can their patches merged. Thomas PS: Sorry, if my mail sounded a little bit harsh... but I really had quite some frustration with the iotests in the past already.
Re: [PATCH v5 1/5] iotests: remove 'linux' from default supported platforms
On 02.10.19 06:46, Thomas Huth wrote: > On 01/10/2019 20.44, Max Reitz wrote: > [...] >> As I have said, the conceptual problem is that the iotests now run as >> part of make check. As such, allowing auto tests to run on non-Linux >> platforms may introduce build failures that I cannot do anything about. > > Well, simply run "make vm-build-openbsd", "make vm-build-freebsd", ... > if something fails there, it likely should not be in the auto group. Then we come to Windows and macOS. What I’ve proposed to John on IRC was to simply skip the iotests in make check for non-Linux systems. >> And those are very much new failures. >> >>> I think that I have demonstrated sufficiently that it's not correct to >>> prohibit python tests from running on other platforms wholesale, so I'd >>> prefer we don't do that anymore. >> >> You have not. >> >> The actual argument to convince me is “This does not affect any tests in >> the auto group, so it will not introduce build failures at this time”. > > I've applied the patch here and it works fine with FreeBSD and macOS: > > https://cirrus-ci.com/build/5169384718336000 > https://travis-ci.com/huth/qemu/builds/129968676 > > It also works fine with "make vm-build-openbsd BUILD_TARGET=check-block" > ... so I think you don't have to worry here. Obviously, because as I’ve said it doesn’t affect any tests in the auto group. Yet. >>> Further, iotests on FreeBSD already weren't 100% green, so I'm not >>> causing a regression in that sense, either. >> >> My problem is twofold: >> >> (1) You claim “Sure, there are failures, but let’s just deal with them” >> and then you do not deal with them. Seems wrong to me. >> >> I’m fine with the argument “Sorry, royal ‘we’. But it just doesn’t help >> anyone to hide the errors. If someone’s on BSD and wants to run the >> iotests, let them.” >> >> That sounds good to me. >> >> (2) Maybe someone adds a Python test in the future that is in auto and >> that does not specify Linux as the only supported platform. Then I send >> a pull request and it breaks on macOS. Now what? Remove it from auto? >> Blindly put "macOS" in unsupported platforms? > > I think both solutions are good. If a test does not work on all systems, > it either should not be in the "auto" group, or it needs to be modified > to skip testing when running in an unsupported environment. > >> In any case, it’ll be a problem for no good reason. > > Really? Is "more test coverage" not a good reason? It isn’t when the solution is to just reduce the coverage again. Furthermore, the problem is that we get this additional coverage on systems that I will not support. >> More on that in the next chunk. >> >>> I'm going to meekly push and ask that we stage this as-is, and when >>> something bad happens you can remind me that I wanted this and make me >>> do it. >> >> Make you do what? Deal with the fact that a pull request is rejected >> because a test fails on macOS? >> >> This is precisely the kind of problem I already had with adding the >> iotests to make check, and I’m already very much not happy about it. >> (In that case it was $DISPLAY not being set on Peter’s test system.) >> >> I’ll let you make the deduction of “The problem isn’t allowing the >> iotests to run on non-Linux platforms, but the fact that they run in >> make check” yourself, so that I no longer feel like I’m the only one who >> considers that having been a mistake. > > Max, I can understand that you are a little bit annoyed that this "make > check with iotests" caused some extra hurdles for you. But honestly, > removing that again would be quite egoistic by you. Try to put yourself > into the position of a "normal", non-blocklayer-maintainer QEMU > developer. For me, iotests were a *constant* source of frustration. > Often, when I ran them on top of my latest and greatest patches, to > check whether I caused a regression, the iotests simply failed. Then I > had to start debugging - did my patches cause the break, or is "master" > broken, too? In almost all cases, there was an issue in the master > branch already, either because they were failing on s390x, or because I > was using ext4 instead of xfs, or because I was using an older distro > than you, etc... . So while the iotests likely worked fine for the > limited set of systems that you, Kevin and the other hard-core block > layer developers are using, it's constantly broken for everybody else > who is not using the very same setup as you. The only way of *not* > getting upset about the iotests was to simply completely *ignore* them. > Is it that what you want? It usually worked fine for me because it’s rather rare that non-block patches broke the iotests. I have to admit I actually didn’t think of other people wanting to run the iotests; but to be honest, your mail doesn’t sound like you want to run the iotests either. It rather sounds like you have to because otherwise I might complain. (The reason I didn’t think of it is because non-b
Re: [PATCH v5 1/5] iotests: remove 'linux' from default supported platforms
On 02/10/2019 13.57, Max Reitz wrote: > On 02.10.19 06:46, Thomas Huth wrote: >> [...] >> Max, I can understand that you are a little bit annoyed that this "make >> check with iotests" caused some extra hurdles for you. But honestly, >> removing that again would be quite egoistic by you. Try to put yourself >> into the position of a "normal", non-blocklayer-maintainer QEMU >> developer. For me, iotests were a *constant* source of frustration. >> Often, when I ran them on top of my latest and greatest patches, to >> check whether I caused a regression, the iotests simply failed. Then I >> had to start debugging - did my patches cause the break, or is "master" >> broken, too? In almost all cases, there was an issue in the master >> branch already, either because they were failing on s390x, or because I >> was using ext4 instead of xfs, or because I was using an older distro >> than you, etc... . So while the iotests likely worked fine for the >> limited set of systems that you, Kevin and the other hard-core block >> layer developers are using, it's constantly broken for everybody else >> who is not using the very same setup as you. The only way of *not* >> getting upset about the iotests was to simply completely *ignore* them. >> Is it that what you want? > > It usually worked fine for me because it’s rather rare that non-block > patches broke the iotests. > > I have to admit I actually didn’t think of other people wanting to run > the iotests; but to be honest, your mail doesn’t sound like you want to > run the iotests either. I *want* to run them. Occasionally - when I have new patches that might affect anything related to the block layer. But then I don't want to be in the situation where I first have to debug multiple other problems with the iotests first that are not related to my new patches. > (The reason I didn’t think of it is because non-block patches rarely > break them, so I wouldn’t run the iotests if I were a non-block > maintainer. Sorry.) Well, "rarely" is relative. They've been broken *completely* two times in the 4.1 development time frame, and IIRC at least once in the 4.0 time frame. [...] > Maybe my main problem is that I feel like now I have to deal with all of > the fallout, even though adding the iotests to make check wasn’t my idea > and neither me nor Kevin ever consented. (I don’t know whether Kevin > consented implicitly, but I don’t see his name in bdd95e47844f2d8b.) > > You can’t just give me more responsibility without my consent and then > call me egoistic for not liking it. Ok, sorry for that. I guess one part of my frustration was also that the patches to enable the iotests during "make check" have been on the list for weeks - or rather months - and I never ever got much feedback from you or Kevin on them. If you told me right in the beginning about your concerns, we would not be at this point now. Also partly my bad, I guess, since I could have reached out to you on IRC to discuss it, but at that point in time, I rather thought that you just don't really care. Thus it's good to have some conversation now, helps a lot to understand the different expectations. Maybe we can also have a discussion about this at KVM forum, I think it's easier to clarify some points of view verbally there instead of using mails. >> Or maybe let me phrase it differently: Do you consider the iotests as >> something that is more or less "private" to the hard-core block layer >> developers, and it's ok if others completely ignore them and break them >> by accident (and you also don't expect the normal developers to fix the >> iotests afterwards)? > > Well, that’s how it’s always worked, and that didn’t frustrate me. Ok ... you're the maintainer, so if that's really the way you prefer, I can send a patch to remove the iotests from "make check" again. > Honestly, it looks to me like you don’t even want to run the iotests. I > interpret most of what you’ve written as: > > - I don’t want to not run the iotests, because then people will hit me > for making them fail. > > - But they fail all the time, so I always need a baseline for what is > expected to sometimes fail and what isn’t. That’s very annoying. > Let’s introduce a baseline in the form of auto/qcow2, and then let > everyone verify that it works. > > So to me it looks like we’ve just added all tests that never fail to > auto. But if they never fail, then it’s like we haven’t run the tests > at all. I disagree. First, the complete iotest failures that have been merged during the 4.1 development time frame to the master branch would have been prevented, I think, so it's certainly not that "they never fail". Second, yes, the basic idea was to start with a small set of tests in the auto group which was already working, and then to increase that set over time, once other tests run more stable, too. But you know the iotests better than me, if you think that most of them can hardly brought into the right shape, then this wa
Re: [PATCH v5 1/5] iotests: remove 'linux' from default supported platforms
On 02.10.19 15:11, Thomas Huth wrote: > On 02/10/2019 13.57, Max Reitz wrote: >> On 02.10.19 06:46, Thomas Huth wrote: >>> [...] >>> Max, I can understand that you are a little bit annoyed that this "make >>> check with iotests" caused some extra hurdles for you. But honestly, >>> removing that again would be quite egoistic by you. Try to put yourself >>> into the position of a "normal", non-blocklayer-maintainer QEMU >>> developer. For me, iotests were a *constant* source of frustration. >>> Often, when I ran them on top of my latest and greatest patches, to >>> check whether I caused a regression, the iotests simply failed. Then I >>> had to start debugging - did my patches cause the break, or is "master" >>> broken, too? In almost all cases, there was an issue in the master >>> branch already, either because they were failing on s390x, or because I >>> was using ext4 instead of xfs, or because I was using an older distro >>> than you, etc... . So while the iotests likely worked fine for the >>> limited set of systems that you, Kevin and the other hard-core block >>> layer developers are using, it's constantly broken for everybody else >>> who is not using the very same setup as you. The only way of *not* >>> getting upset about the iotests was to simply completely *ignore* them. >>> Is it that what you want? >> >> It usually worked fine for me because it’s rather rare that non-block >> patches broke the iotests. >> >> I have to admit I actually didn’t think of other people wanting to run >> the iotests; but to be honest, your mail doesn’t sound like you want to >> run the iotests either. > > I *want* to run them. Occasionally - when I have new patches that might > affect anything related to the block layer. But then I don't want to be > in the situation where I first have to debug multiple other problems > with the iotests first that are not related to my new patches. OK. >> (The reason I didn’t think of it is because non-block patches rarely >> break them, so I wouldn’t run the iotests if I were a non-block >> maintainer. Sorry.) > > Well, "rarely" is relative. They've been broken *completely* two times > in the 4.1 development time frame, and IIRC at least once in the 4.0 > time frame. Hm, OK. So my memory is just shot. > [...] >> Maybe my main problem is that I feel like now I have to deal with all of >> the fallout, even though adding the iotests to make check wasn’t my idea >> and neither me nor Kevin ever consented. (I don’t know whether Kevin >> consented implicitly, but I don’t see his name in bdd95e47844f2d8b.) >> >> You can’t just give me more responsibility without my consent and then >> call me egoistic for not liking it. > > Ok, sorry for that. I guess one part of my frustration was also that the > patches to enable the iotests during "make check" have been on the list > for weeks - or rather months - and I never ever got much feedback from > you or Kevin on them. I think that’s for two reasons: (1) The time line in the block layer is unfortunately apparently different from what you’re used to. Or, rather, fortunately for you. It isn’t too rare for me to have patches up for a year or more. (2) We’ve had discussions on this before and the result was always the same. Yes, we’d like to run the iotests as part of make check, but we can’t imagine it would work right now (where “right now” is whenever we have the discussion). So as for me, I felt bad about plainly saying “no”. But I cannot say “yes” either. You’re right that I should’ve said something. There are more words than “yes” and “no”. Sorry again. I can imagine I had hoped Kevin would speak up (and thus take responsibility) or you’d become so annoyed with our silence that you’d angrily reach out to us and I’d have to do something. (Which is a horrible attitude, I realize that.) I do remember that I didn’t expect that you’d just find someone else to merge the patch. That took me by surprise. > If you told me right in the beginning about your > concerns, we would not be at this point now. Also partly my bad, I > guess, since I could have reached out to you on IRC to discuss it, but > at that point in time, I rather thought that you just don't really care. > Thus it's good to have some conversation now, helps a lot to understand > the different expectations. Maybe we can also have a discussion about > this at KVM forum, I think it's easier to clarify some points of view > verbally there instead of using mails. Well, I won’t be at KVM Forum, unfortunately. >>> Or maybe let me phrase it differently: Do you consider the iotests as >>> something that is more or less "private" to the hard-core block layer >>> developers, and it's ok if others completely ignore them and break them >>> by accident (and you also don't expect the normal developers to fix the >>> iotests afterwards)? >> >> Well, that’s how it’s always worked, and that didn’t frustrate me. > > Ok ... you're the maintainer, so if that's really the way you pref
Re: [PATCH v5 1/5] iotests: remove 'linux' from default supported platforms
On 02/10/2019 15.36, Max Reitz wrote: [...] > Or can we have some middle ground? Something that runs on some CI > systems (and notifies me and others) but won’t result in pull requests > being rejected or cause make check failures? Yes, I think that might be an option... Since many developers are using Travis, I think I'd make sense to add a "make check-block" to .travis.yml so that at least most people still get automatic test coverage, without blocking Peter's "make check" pull request criteria. I'll post a patch for doing that, too. Thomas signature.asc Description: OpenPGP digital signature
Re: [PATCH v5 1/5] iotests: remove 'linux' from default supported platforms
Not sure where in this thread to reply, but since my name is mentioned in this mail, it might at least be not the worst one. Am 02.10.2019 um 13:57 hat Max Reitz geschrieben: > On 02.10.19 06:46, Thomas Huth wrote: > > On 01/10/2019 20.44, Max Reitz wrote: > > [...] > >> As I have said, the conceptual problem is that the iotests now run as > >> part of make check. As such, allowing auto tests to run on non-Linux > >> platforms may introduce build failures that I cannot do anything about. > > > > Well, simply run "make vm-build-openbsd", "make vm-build-freebsd", ... > > if something fails there, it likely should not be in the auto group. > > Then we come to Windows and macOS. > > What I’ve proposed to John on IRC was to simply skip the iotests in make > check for non-Linux systems. I think this really makes sense. Or at least have a blacklist of OSes that we can't support iotests on, which would contain at least Windows and OS X. Windows because I'm pretty sure that it doesn't work anyway, and OS X because if something fails there, we have no way to reproduce. The occasional build failures on OS X that must be fixed by blindly guessing what could work without any way to test the fix are already bad enough. > > Max, I can understand that you are a little bit annoyed that this "make > > check with iotests" caused some extra hurdles for you. But honestly, > > removing that again would be quite egoistic by you. Try to put yourself > > into the position of a "normal", non-blocklayer-maintainer QEMU > > developer. For me, iotests were a *constant* source of frustration. > > Often, when I ran them on top of my latest and greatest patches, to > > check whether I caused a regression, the iotests simply failed. Then I > > had to start debugging - did my patches cause the break, or is "master" > > broken, too? In almost all cases, there was an issue in the master > > branch already, either because they were failing on s390x, or because I > > was using ext4 instead of xfs, or because I was using an older distro > > than you, etc... . So while the iotests likely worked fine for the > > limited set of systems that you, Kevin and the other hard-core block > > layer developers are using, it's constantly broken for everybody else > > who is not using the very same setup as you. The only way of *not* > > getting upset about the iotests was to simply completely *ignore* them. > > Is it that what you want? > > It usually worked fine for me because it’s rather rare that non-block > patches broke the iotests. I disagree. It happened all the time that someone else broke the iotests in master and we needed to fix it up. > Maybe my main problem is that I feel like now I have to deal with all of > the fallout, even though adding the iotests to make check wasn’t my idea > and neither me nor Kevin ever consented. (I don’t know whether Kevin > consented implicitly, but I don’t see his name in bdd95e47844f2d8b.) > > You can’t just give me more responsibility without my consent and then > call me egoistic for not liking it. I think I may have implicitly consented by helping Alex with the changes to 'check' that made its output fit better in 'make check'. Basically, my stance is that I share your dislike for the effects on me personally (also, I can't run 'make check' any more before a pull request without testing half of the iotests twice because I still need a full iotests run), but I also think that having iotests in 'make check' is really the right thing for the project despite the inconvenience it means for me. > I know you’ll say that we just need to ensure we can add more tests, > then. But for one thing, the most important tests are the ones that > take the longest, like 041. And the other of course is that adding any > more tests to make check just brings me more pain, so I won’t do it. That's something that can be fixed by tweaking the auto group. Can we run tests in 'auto' that require the system emulator? If so, let's add 030 040 041 to the default set. They take long, but they are among the most valuable tests we have. If this makes the tests take too much time, let's remove some less important ones instead. > [1] There is the recent case of Kevin’s pull request having been > rejected because his test failed on anything but x64. I’m torn here, > because I would have seen that failure on my -m32 build. So it isn’t > like it would have evaded our view for long. I messed up, so this pull request was rightly stopped. I consider this one a feature, not a bug. Kevin signature.asc Description: PGP signature
Re: [PATCH v5 1/5] iotests: remove 'linux' from default supported platforms
On 02.10.19 18:44, Kevin Wolf wrote: > Not sure where in this thread to reply, but since my name is mentioned > in this mail, it might at least be not the worst one. > > Am 02.10.2019 um 13:57 hat Max Reitz geschrieben: >> On 02.10.19 06:46, Thomas Huth wrote: >>> On 01/10/2019 20.44, Max Reitz wrote: >>> [...] As I have said, the conceptual problem is that the iotests now run as part of make check. As such, allowing auto tests to run on non-Linux platforms may introduce build failures that I cannot do anything about. >>> >>> Well, simply run "make vm-build-openbsd", "make vm-build-freebsd", ... >>> if something fails there, it likely should not be in the auto group. >> >> Then we come to Windows and macOS. >> >> What I’ve proposed to John on IRC was to simply skip the iotests in make >> check for non-Linux systems. > > I think this really makes sense. Or at least have a blacklist of OSes > that we can't support iotests on, which would contain at least Windows > and OS X. Windows because I'm pretty sure that it doesn't work anyway, > and OS X because if something fails there, we have no way to reproduce. > The occasional build failures on OS X that must be fixed by blindly > guessing what could work without any way to test the fix are already bad > enough. > >>> Max, I can understand that you are a little bit annoyed that this "make >>> check with iotests" caused some extra hurdles for you. But honestly, >>> removing that again would be quite egoistic by you. Try to put yourself >>> into the position of a "normal", non-blocklayer-maintainer QEMU >>> developer. For me, iotests were a *constant* source of frustration. >>> Often, when I ran them on top of my latest and greatest patches, to >>> check whether I caused a regression, the iotests simply failed. Then I >>> had to start debugging - did my patches cause the break, or is "master" >>> broken, too? In almost all cases, there was an issue in the master >>> branch already, either because they were failing on s390x, or because I >>> was using ext4 instead of xfs, or because I was using an older distro >>> than you, etc... . So while the iotests likely worked fine for the >>> limited set of systems that you, Kevin and the other hard-core block >>> layer developers are using, it's constantly broken for everybody else >>> who is not using the very same setup as you. The only way of *not* >>> getting upset about the iotests was to simply completely *ignore* them. >>> Is it that what you want? >> >> It usually worked fine for me because it’s rather rare that non-block >> patches broke the iotests. > > I disagree. It happened all the time that someone else broke the iotests > in master and we needed to fix it up. OK. (In my experience, it’s still mostly block patches, only that they tend to go through non-block trees.) >> Maybe my main problem is that I feel like now I have to deal with all of >> the fallout, even though adding the iotests to make check wasn’t my idea >> and neither me nor Kevin ever consented. (I don’t know whether Kevin >> consented implicitly, but I don’t see his name in bdd95e47844f2d8b.) >> >> You can’t just give me more responsibility without my consent and then >> call me egoistic for not liking it. > > I think I may have implicitly consented by helping Alex with the changes > to 'check' that made its output fit better in 'make check'. > > Basically, my stance is that I share your dislike for the effects on me > personally (also, I can't run 'make check' any more before a pull > request without testing half of the iotests twice because I still need a > full iotests run), but I also think that having iotests in 'make check' > is really the right thing for the project despite the inconvenience it > means for me. Then I expect you to NACK Thomas’s patch, and I take it to mean that I can rely on you to handle basically all make check iotests failures that are not caused by my own pull requests. Works for me. >> I know you’ll say that we just need to ensure we can add more tests, >> then. But for one thing, the most important tests are the ones that >> take the longest, like 041. And the other of course is that adding any >> more tests to make check just brings me more pain, so I won’t do it. > > That's something that can be fixed by tweaking the auto group. > > Can we run tests in 'auto' that require the system emulator? If so, > let's add 030 040 041 to the default set. They take long, but they are > among the most valuable tests we have. If this makes the tests take too > much time, let's remove some less important ones instead. > >> [1] There is the recent case of Kevin’s pull request having been >> rejected because his test failed on anything but x64. I’m torn here, >> because I would have seen that failure on my -m32 build. So it isn’t >> like it would have evaded our view for long. > > I messed up, so this pull request was rightly stopped. I consider this > one a feature, not a bug. Sure, that was a
Re: [PATCH v5 1/5] iotests: remove 'linux' from default supported platforms
On 02/10/2019 18.44, Kevin Wolf wrote: > Not sure where in this thread to reply, but since my name is mentioned > in this mail, it might at least be not the worst one. > > Am 02.10.2019 um 13:57 hat Max Reitz geschrieben: >> On 02.10.19 06:46, Thomas Huth wrote: >>> On 01/10/2019 20.44, Max Reitz wrote: >>> [...] As I have said, the conceptual problem is that the iotests now run as part of make check. As such, allowing auto tests to run on non-Linux platforms may introduce build failures that I cannot do anything about. >>> >>> Well, simply run "make vm-build-openbsd", "make vm-build-freebsd", ... >>> if something fails there, it likely should not be in the auto group. >> >> Then we come to Windows and macOS. >> >> What I’ve proposed to John on IRC was to simply skip the iotests in make >> check for non-Linux systems. > > I think this really makes sense. Or at least have a blacklist of OSes > that we can't support iotests on, which would contain at least Windows > and OS X. Windows because I'm pretty sure that it doesn't work anyway, > and OS X because if something fails there, we have no way to reproduce. Both, .travis.yml and .cirrus-ci.yml have a macOS test, so you can reproduce bugs there. It's just a PITA that you cannot do any interactive debugging there. [...] > Can we run tests in 'auto' that require the system emulator? Yes. In fact, tests/check-block.sh explicitly checks for the availability of a system emulator before running the iotests. > If so, let's add 030 040 041 to the default set. Sure, but let's check whether they work in Travis first. Thomas signature.asc Description: OpenPGP digital signature
Re: [PATCH v5 1/5] iotests: remove 'linux' from default supported platforms
On 10/1/19 2:44 PM, Max Reitz wrote: > On 28.09.19 01:35, John Snow wrote: >> >> >> On 9/23/19 9:09 AM, Max Reitz wrote: >>> On 18.09.19 01:45, John Snow wrote: verify_platform will check an explicit whitelist and blacklist instead. The default will now be assumed to be allowed to run anywhere. For tests that do not specify their platforms explicitly, this has the effect of enabling these tests on non-linux platforms. For tests that always specified linux explicitly, there is no change. For Python tests on FreeBSD at least; only seven python tests fail: 045 147 149 169 194 199 211 045 and 149 appear to be misconfigurations, 147 and 194 are the AF_UNIX path too long error, 169 and 199 are bitmap migration bugs, and 211 is a bug that shows up on Linux platforms, too. This is at least good evidence that these tests are not Linux-only. If they aren't suitable for other platforms, they should be disabled on a per-platform basis as appropriate. Therefore, let's switch these on and deal with the failures. >>> >>> What exactly do you mean by “deal with the failures”? Do you have a >>> reference to patches that deal with them, or are you or is someone else >>> working on them...? >>> >>> Apart from that, I am rather hesitant to take a patch through my tree >>> that not only may cause test failures on platforms that I will not or >>> actually cannot run tests on (like MacOS or Windows), but that actually >>> does introduce new failures as you describe. >>> >>> Well, at least it doesn’t introduce build failures because it appears >>> there is no Python test that’s in the auto group, so I suppose “rather >>> hesitant” is not an “I won’t”. >>> >> >> Think of it more like this: The failures were always there, but we hid >> them. I'm not "introducing new failures" as such O:-) > > That is incorrect. > > As I have said, the conceptual problem is that the iotests now run as > part of make check. As such, allowing auto tests to run on non-Linux > platforms may introduce build failures that I cannot do anything about. > > And those are very much new failures. > >> I think that I have demonstrated sufficiently that it's not correct to >> prohibit python tests from running on other platforms wholesale, so I'd >> prefer we don't do that anymore. > > You have not. > I feel as if I have. The tests can run on other platforms and I proved that most of them work. There's not good evidence that any of these tests are indeed "Linux-only", as only a handful experience problems not more easily explained by other factors. There's nothing inherent to the framework itself that makes it Linux-only. I think it's more than accurate to say that "it's not correct to prohibit python tests from running on other platforms wholesale." Maybe you want to talk about whether or not we should gate on these tests on those platforms, and that's fair, but it's not what got written. > The actual argument to convince me is “This does not affect any tests in > the auto group, so it will not introduce build failures at this time”. > >> Further, iotests on FreeBSD already weren't 100% green, so I'm not >> causing a regression in that sense, either. > > My problem is twofold: > > (1) You claim “Sure, there are failures, but let’s just deal with them” > and then you do not deal with them. Seems wrong to me. > > I’m fine with the argument “Sorry, royal ‘we’. But it just doesn’t help > anyone to hide the errors. If someone’s on BSD and wants to run the > iotests, let them.” > > That sounds good to me. > That's more or less what I want to convey. Enable them and see where the chips fall, but don't gate pull requests on non-Linux platforms. In the event that an "auto" group test did fail on an esoteric platform, I wanted to offer being the point of contact for that so I wasn't foisting work onto you. "Royal 'we', but with the expectation that I'd likely just re-blacklist certain tests we don't have the capacity to support." I felt like the burden wouldn't be too severe there, as all of the 'auto' tests appeared to pass without much of a fuss. > (2) Maybe someone adds a Python test in the future that is in auto and > that does not specify Linux as the only supported platform. Then I send > a pull request and it breaks on macOS. Now what? Remove it from auto? > Blindly put "macOS" in unsupported platforms? > > In any case, it’ll be a problem for no good reason. > I don't agree with the sentiment that it's "no good reason". We claim to support these platforms. If tests fail on them, I think we rather want to know. The fear, I think, is that it will be mostly boring platform differences that are unimportant to the actual functioning of QEMU: that it's just good at finding bugs in the test instead of the binary. I think that's a legitimate concern, but it doesn't move me enough to agree that we shouldn't run tests on platf
Re: [PATCH v5 1/5] iotests: remove 'linux' from default supported platforms
Am 02.10.2019 um 19:47 hat Max Reitz geschrieben: > On 02.10.19 18:44, Kevin Wolf wrote: > > Am 02.10.2019 um 13:57 hat Max Reitz geschrieben: > >> It usually worked fine for me because it’s rather rare that non-block > >> patches broke the iotests. > > > > I disagree. It happened all the time that someone else broke the iotests > > in master and we needed to fix it up. > > OK. > > (In my experience, it’s still mostly block patches, only that they tend > to go through non-block trees.) Fair enough, it's usually code that touches block code. I assumed "block patches" to mean patches that go through one of the block trees and for which iotests are run before sending a pull request. In the end, I don't care what code these patches touched. I do an innocent git pull, and when I finally see that it's master that breaks iotests and not my patches on top of it, I'm annoyed. > >> Maybe my main problem is that I feel like now I have to deal with all of > >> the fallout, even though adding the iotests to make check wasn’t my idea > >> and neither me nor Kevin ever consented. (I don’t know whether Kevin > >> consented implicitly, but I don’t see his name in bdd95e47844f2d8b.) > >> > >> You can’t just give me more responsibility without my consent and then > >> call me egoistic for not liking it. > > > > I think I may have implicitly consented by helping Alex with the changes > > to 'check' that made its output fit better in 'make check'. > > > > Basically, my stance is that I share your dislike for the effects on me > > personally (also, I can't run 'make check' any more before a pull > > request without testing half of the iotests twice because I still need a > > full iotests run), but I also think that having iotests in 'make check' > > is really the right thing for the project despite the inconvenience it > > means for me. > > Then I expect you to NACK Thomas’s patch, and I take it to mean that I > can rely on you to handle basically all make check iotests failures that > are not caused by my own pull requests. > > Works for me. Ah, we can also play this game the other way round. So if you merge that revert, when iotests break in master, I take it I can rely on you to fix it up before it impacts my working branch? > >> I know you’ll say that we just need to ensure we can add more tests, > >> then. But for one thing, the most important tests are the ones that > >> take the longest, like 041. And the other of course is that adding any > >> more tests to make check just brings me more pain, so I won’t do it. > > > > That's something that can be fixed by tweaking the auto group. > > > > Can we run tests in 'auto' that require the system emulator? If so, > > let's add 030 040 041 to the default set. They take long, but they are > > among the most valuable tests we have. If this makes the tests take too > > much time, let's remove some less important ones instead. > > > >> [1] There is the recent case of Kevin’s pull request having been > >> rejected because his test failed on anything but x64. I’m torn here, > >> because I would have seen that failure on my -m32 build. So it isn’t > >> like it would have evaded our view for long. > > > > I messed up, so this pull request was rightly stopped. I consider this > > one a feature, not a bug. > > Sure, that was a good thing. I just wonder whether that instance is > enough to justify the pain. Yes, making iotests stable on CI probably involves some pain, especially initially. However, if we don't fix them upstream, they'll end up on our desk again downstream because people run tests neither on your nor on my laptop. I think we might actually save time by fixing them once and for all, even if they are problems in the test cases and not in QEMU, and making new test cases portable from the start, instead of getting individual reports one at a time and having to look at the same test cases again and again. Kevin signature.asc Description: PGP signature
Re: [PATCH v5 1/5] iotests: remove 'linux' from default supported platforms
On 04.10.19 12:19, Kevin Wolf wrote: > Am 02.10.2019 um 19:47 hat Max Reitz geschrieben: >> On 02.10.19 18:44, Kevin Wolf wrote: >>> Am 02.10.2019 um 13:57 hat Max Reitz geschrieben: It usually worked fine for me because it’s rather rare that non-block patches broke the iotests. >>> >>> I disagree. It happened all the time that someone else broke the iotests >>> in master and we needed to fix it up. >> >> OK. >> >> (In my experience, it’s still mostly block patches, only that they tend >> to go through non-block trees.) > > Fair enough, it's usually code that touches block code. I assumed "block > patches" to mean patches that go through one of the block trees and for > which iotests are run before sending a pull request. > > In the end, I don't care what code these patches touched. I do an > innocent git pull, and when I finally see that it's master that breaks > iotests and not my patches on top of it, I'm annoyed. Hm. Part of my point was that this still happens all the time. Which is why I’d prefer more tests to run in CI than a handful of not very useful ones in make check. (Of course, running it in a CI won’t prevent iotest failures on your machine, but in practice neither does the current model.) Maybe my main problem is that I feel like now I have to deal with all of the fallout, even though adding the iotests to make check wasn’t my idea and neither me nor Kevin ever consented. (I don’t know whether Kevin consented implicitly, but I don’t see his name in bdd95e47844f2d8b.) You can’t just give me more responsibility without my consent and then call me egoistic for not liking it. >>> >>> I think I may have implicitly consented by helping Alex with the changes >>> to 'check' that made its output fit better in 'make check'. >>> >>> Basically, my stance is that I share your dislike for the effects on me >>> personally (also, I can't run 'make check' any more before a pull >>> request without testing half of the iotests twice because I still need a >>> full iotests run), -x auto for the qcow2 pass, by the way. >>> but I also think that having iotests in 'make check' >>> is really the right thing for the project despite the inconvenience it >>> means for me. >> >> Then I expect you to NACK Thomas’s patch, and I take it to mean that I >> can rely on you to handle basically all make check iotests failures that >> are not caused by my own pull requests. >> >> Works for me. > > Ah, we can also play this game the other way round. > > So if you merge that revert, when iotests break in master, I take it I > can rely on you to fix it up before it impacts my working branch? This is not a game, I’m talking purely from my standpoint: (I talked wrongly, but more on that below) Whenever make check fails, it’s urgent. Without iotests running in make check, we had some time to sort the situation out. That’s no longer the case. That means we need to take care of everything immediately. And I purely want help with that. I just did not have the impression that such help was there in the past months. (This impression may or may not have a correlation to reality.) I thought that was just because nobody really cared about the iotests in make check. Now you say you do, so that’s why I said you should NACK the revert and then I know that I can count on your help. Now where I was wrong: I didn’t say “your help” but “you handle it”. That was wrong. I’m sorry. That would mean I reap all the benefits and you have to pay the price. What I’m not so sure of is why you didn’t just say that’s unfair and instead reply with an impossible suggestion. That makes it a bit difficult for me to find out exactly what you plan to do. I take your point as “Exactly, this suggestion would be impossible. With the tests in make check, the worst that happens is CI breakage and rejected pull requests, and dealing with those is very much possible.”[1] As I’ve said, my POV is different, because I find CI breakage, make check breakage, and rejected pull requests to be worse because those are failures on other people’s machines. That puts me personally under much more stress than failures on my own machine. But it seems that you feel differently. [1] Or maybe you wanted to say that you find a rare intermittent failure that slips into make check less worse than 100 % failure of some test for everyone who runs the iotests. I don’t know. I know that the 100 % failures are annoying but generally easily fixed; whereas the intermittent ones are the ones that stick and hard to fix. And those intermittent ones are unlikely to cause pull requests to be rejected. I know you’ll say that we just need to ensure we can add more tests, then. But for one thing, the most important tests are the ones that take the longest, like 041. And the other of course is that adding any more tests to make check just brings me more pain, so I won’t do it. >>> >>> That's something that c
Re: [PATCH v5 1/5] iotests: remove 'linux' from default supported platforms
On Fri, 4 Oct 2019 at 13:45, Max Reitz wrote: > > In the end, I don't care what code these patches touched. I do an > > innocent git pull, and when I finally see that it's master that breaks > > iotests and not my patches on top of it, I'm annoyed. > > Hm. Part of my point was that this still happens all the time. > > Which is why I’d prefer more tests to run in CI than a handful of not > very useful ones in make check. > > (Of course, running it in a CI won’t prevent iotest failures on your > machine, but in practice neither does the current model.) If you want the tree to be defended against problems, then you need to run tests in 'make check'. Nothing else gets consistently run and has failures spotted either before code goes into the tree or quickly afterwards. 'make check' does have the restriction that we don't want the tests to take too long to run, but in general the block layer should be running some reasonable subset of tests in the project's standard "run the tests please" command. (I have no opinion on exactly what that subset ought to be, beyond that it would be good if that subset doesn't have known intermittent failures in it.) > I still think that I personally would prefer the iotests to not run as > part of make check, but just as CI. 'make check' *is* our CI gate, to a first approximation. Most of the various travis and other setups are simply a front-end for "build QEMU in various configurations and on various hosts and then run 'make check'". The travis setup at the moment is a bit odd, because it runs tests but it's not a gate on our merging changes. Ideally I would like to fix this so that rather than me personally running "make check" via a bunch of scripts we have one CI setup that we trust (gitlab seems the current favoured contender) and that gates changes flowing into master rather than me doing it manually. We might then turn travis off if it's not providing anything for us that the gitlab setup doesn't. thanks -- PMM
Re: [PATCH v5 1/5] iotests: remove 'linux' from default supported platforms
On 04.10.19 15:16, Peter Maydell wrote: > On Fri, 4 Oct 2019 at 13:45, Max Reitz wrote: >>> In the end, I don't care what code these patches touched. I do an >>> innocent git pull, and when I finally see that it's master that breaks >>> iotests and not my patches on top of it, I'm annoyed. >> >> Hm. Part of my point was that this still happens all the time. >> >> Which is why I’d prefer more tests to run in CI than a handful of not >> very useful ones in make check. >> >> (Of course, running it in a CI won’t prevent iotest failures on your >> machine, but in practice neither does the current model.) > > If you want the tree to be defended against problems, then > you need to run tests in 'make check'. Nothing else gets consistently > run and has failures spotted either before code goes into the > tree or quickly afterwards. Yes, but part of the problem is that what does get run as part of make check currently does not help much. So this doesn’t really defend the iotests from problems. > 'make check' does have the restriction > that we don't want the tests to take too long to run, but in > general the block layer should be running some reasonable subset > of tests in the project's standard "run the tests please" command. > (I have no opinion on exactly what that subset ought to be, beyond > that it would be good if that subset doesn't have known intermittent > failures in it.) Deciding the subset is difficult. My stance was that it’s better to not choose an arbitrary subset here but ensure that really everything gets run as part of CI, that is asynchronously so it doesn’t block anyone and can take as long as it wants. If we decide to get pull requests based on that, then that’d bring even more pain, but at least it’d be honest. But just running half of the qcow2 tests to me seems only like we want to pretend we ran the iotests. So for me, iotests still break, and we need to deal with make check failures on top. I’d at least prefer one or the other. I voted for dropping make check, because running all iotests doesn’t seem feasible to me. >> I still think that I personally would prefer the iotests to not run as >> part of make check, but just as CI. > > 'make check' *is* our CI gate, to a first approximation. Most of > the various travis and other setups are simply a front-end for > "build QEMU in various configurations and on various hosts > and then run 'make check'". The travis setup at the moment is > a bit odd, because it runs tests but it's not a gate on our > merging changes. Ideally I would like to fix this so that > rather than me personally running "make check" via a bunch > of scripts we have one CI setup that we trust (gitlab seems > the current favoured contender) and that gates changes flowing > into master rather than me doing it manually. We might then turn > travis off if it's not providing anything for us that the gitlab > setup doesn't. Hm, but make check also serves other purposes. I would suppose e.g. distributions generally require make check to pass when building packages. I assumed our CI included more things than just make check, so we could move the iotests from out of make check but keep them as part of CI. Max signature.asc Description: OpenPGP digital signature
Re: [PATCH v5 1/5] iotests: remove 'linux' from default supported platforms
On Fri, 4 Oct 2019 at 14:50, Max Reitz wrote: > On 04.10.19 15:16, Peter Maydell wrote: > > 'make check' does have the restriction > > that we don't want the tests to take too long to run, but in > > general the block layer should be running some reasonable subset > > of tests in the project's standard "run the tests please" command. > > (I have no opinion on exactly what that subset ought to be, beyond > > that it would be good if that subset doesn't have known intermittent > > failures in it.) > > Deciding the subset is difficult. My stance was that it’s better to not > choose an arbitrary subset here but ensure that really everything gets > run as part of CI, that is asynchronously so it doesn’t block anyone and > can take as long as it wants. I wonder if we have a terminology difference confusion here. To me "CI" means "we have tests which we automatically run before pushing commits to master, and if they fail then we don't push". So (a) they have to run synchronously and (b) there is a need for them to run in a reasonable period of time because otherwise it takes too long to run the tests before pushing to master and we get a backlog of unprocessed pullrequests and annoying delays. > If we decide to get pull requests based on that, then that’d bring even > more pain, but at least it’d be honest. But just running half of the > qcow2 tests to me seems only like we want to pretend we ran the iotests. > So for me, iotests still break, and we need to deal with make check > failures on top. I’d at least prefer one or the other. I think the ideal we're aiming for here is: (1) people doing active work in the block subsystem are probably often going to want to run all the iotests, and certainly the subsystem maintainers will want to do that as they put together pull requests. (2) but people who *don't* do active work in the block subsystem still sometimes touch it in passing as part of things like global refactorings or other changes that touch big parts of the tree, or accidentally break it with a change to some other bit of QEMU entirely. These people won't run the full iotests, but it is reasonable to expect them to run 'make check', and it would be good if that caught "whoops you broke the block subsystem". Similarly, "make check" has incredibly spotty coverage of various arm boards and devices, but it does do some basic checks which do catch accidental breakage. thanks -- PMM
Re: [PATCH v5 1/5] iotests: remove 'linux' from default supported platforms
On 04.10.19 16:05, Peter Maydell wrote: > On Fri, 4 Oct 2019 at 14:50, Max Reitz wrote: >> On 04.10.19 15:16, Peter Maydell wrote: >>> 'make check' does have the restriction >>> that we don't want the tests to take too long to run, but in >>> general the block layer should be running some reasonable subset >>> of tests in the project's standard "run the tests please" command. >>> (I have no opinion on exactly what that subset ought to be, beyond >>> that it would be good if that subset doesn't have known intermittent >>> failures in it.) >> >> Deciding the subset is difficult. My stance was that it’s better to not >> choose an arbitrary subset here but ensure that really everything gets >> run as part of CI, that is asynchronously so it doesn’t block anyone and >> can take as long as it wants. > > I wonder if we have a terminology difference confusion here. > To me "CI" means "we have tests which we automatically run > before pushing commits to master, and if they fail then we > don't push". So (a) they have to run synchronously and (b) there > is a need for them to run in a reasonable period of time because > otherwise it takes too long to run the tests before pushing > to master and we get a backlog of unprocessed pullrequests > and annoying delays. Hm, yes. In that case, there’s of course no point in moving it. I meant moving the tests to somewhere where they run asynchronously. >> If we decide to get pull requests based on that, then that’d bring even >> more pain, but at least it’d be honest. But just running half of the >> qcow2 tests to me seems only like we want to pretend we ran the iotests. >> So for me, iotests still break, and we need to deal with make check >> failures on top. I’d at least prefer one or the other. > > I think the ideal we're aiming for here is: > (1) people doing active work in the block subsystem are probably > often going to want to run all the iotests, and certainly the > subsystem maintainers will want to do that as they put together > pull requests. > (2) but people who *don't* do active work in the block subsystem > still sometimes touch it in passing as part of things like global > refactorings or other changes that touch big parts of the tree, > or accidentally break it with a change to some other bit of QEMU > entirely. These people won't run the full iotests, but it is > reasonable to expect them to run 'make check', and it would be good > if that caught "whoops you broke the block subsystem". > > Similarly, "make check" has incredibly spotty coverage of > various arm boards and devices, but it does do some basic > checks which do catch accidental breakage. Yes, it has its use, but at the same time it means that intermittent failure can appear in make check because some iotest is broken. (The past has shown that it’s hard to predict which tests will at some point start to exhibit such behavior.) This then requires immediate attention, and I simply want help with that from someone who really cares about having the test be part of make check, as I wrote in my reply to Thomas’s patch “iotests: Do not run the iotests during "make check" anymore”. The issues are just half as pressing when it isn’t make check that’s intermittently failing. (I hope nobody takes this as “He doesn’t want to fix intermittent failures”, because I do really try to do that. So I don’t consider this a good kind of pressure.) I suppose my main problem is that I’m just lucky that I can’t remember a case where the block subsystem was really broken and iotests in make check would have caught it; and that I’m naïve enough to expect this to continue into the future. Max signature.asc Description: OpenPGP digital signature
Re: [PATCH v5 1/5] iotests: remove 'linux' from default supported platforms
On 04/10/2019 14.44, Max Reitz wrote: > On 04.10.19 12:19, Kevin Wolf wrote: >> Am 02.10.2019 um 19:47 hat Max Reitz geschrieben: >>> On 02.10.19 18:44, Kevin Wolf wrote: Am 02.10.2019 um 13:57 hat Max Reitz geschrieben: > It usually worked fine for me because it’s rather rare that non-block > patches broke the iotests. I disagree. It happened all the time that someone else broke the iotests in master and we needed to fix it up. >>> >>> OK. >>> >>> (In my experience, it’s still mostly block patches, only that they tend >>> to go through non-block trees.) >> >> Fair enough, it's usually code that touches block code. I assumed "block >> patches" to mean patches that go through one of the block trees and for >> which iotests are run before sending a pull request. >> >> In the end, I don't care what code these patches touched. I do an >> innocent git pull, and when I finally see that it's master that breaks >> iotests and not my patches on top of it, I'm annoyed. > > Hm. Part of my point was that this still happens all the time. > > Which is why I’d prefer more tests to run in CI than a handful of not > very useful ones in make check. Ok, so let's try to add some more useful test to the "auto" group. Kevin mentioned 030, 040 and 041, and I think it should be ok to enable them (IIRC the only issue was that they run a little bit longer, but if they are very useful, we should include them anyway). Do you have any other tests in mind which could be very useful? [...] >> So if you merge that revert, when iotests break in master, I take it I >> can rely on you to fix it up before it impacts my working branch? > > This is not a game, I’m talking purely from my standpoint: > (I talked wrongly, but more on that below) > > Whenever make check fails, it’s urgent. Without iotests running in make > check, we had some time to sort the situation out. That’s no longer the > case. > > That means we need to take care of everything immediately. And I purely > want help with that. While that sounds noble at a first glance, I think you've maybe got too high expectations at yourself here? I mean, all other maintainers of other subsystems with tests have the same "problem" if the tests for their subsystem run in "make check". The "normal" behavior that I've seen so far (and which I think is the right way to deal with it): 1) If a pull request gets rejected due to a "make check" failure, simply drop the offending patches from the pull request, send a v2 pull req without them, and tell the author of the offending patches to fix the problem (unless the fix is completely trivial and could immediately be applied to the v2 pull req). You really don't have to fix each and every issue on your own as a maintainer and can redirect this to the patch authors again. 2) If a test fails occasionally during "make check" (due to race conditions or whatever), it gets disabled from "make check" if it can't be fixed easily (e.g. in the qtests it gets moved to the "SPEED=slow" category, or in iotests it would get removed from the "auto" group). >> Yes, making iotests stable on CI probably involves some pain, especially >> initially. However, if we don't fix them upstream, they'll end up on our >> desk again downstream because people run tests neither on your nor on my >> laptop. >> >> I think we might actually save time by fixing them once and for all, >> even if they are problems in the test cases and not in QEMU, and making >> new test cases portable from the start, instead of getting individual >> reports one at a time and having to look at the same test cases again >> and again. > > You should really know I’m all for that and that I’ve done my share of > work there. > > But from my perspective we’ve said and tried that for six years and we > aren’t there still. So to me “We should just fix it” of course sounds > correct, but I also don’t believe it’ll happen any time soon. Mostly > because we generally don’t know what to fix before it breaks, but also > because that’s exactly what we’ve tried to do for at least six years. Well, I guess we won't ever get there if we don't try. And the hurdles will rather get higher over the years since more and more tests are added ... > OTOH, keeping the iotests in make check means we have to act on failure > reports immediately. For example, someone™ needs to investigate the 130 > failure Peter has reported. I’ve just replied “I don’t know”, CC’d > Thomas, and waited whether anyone else would do anything. Nobody did, > and that can’t work. (I also wrote that I wouldn’t act on it, precisely > because I didn’t see the point. I assumed that if anyone disagreed with > that statement, they would have said something.) > > So acting on such reports means pain, too. I consider it greater than > the previous kind of pain, because I prefer “This sucks, I’ll have to > fix it this week or so” to “Oh crap, I’ll have to investigate now, > reproduce it, write an email as soo
Re: [PATCH v5 1/5] iotests: remove 'linux' from default supported platforms
On Mon, 7 Oct 2019 at 13:32, Thomas Huth wrote: > On 04/10/2019 14.44, Max Reitz wrote: > > Whenever make check fails, it’s urgent. Without iotests running in make > > check, we had some time to sort the situation out. That’s no longer the > > case. > > > > That means we need to take care of everything immediately. And I purely > > want help with that. > > While that sounds noble at a first glance, I think you've maybe got too > high expectations at yourself here? I mean, all other maintainers of > other subsystems with tests have the same "problem" if the tests for > their subsystem run in "make check". The "normal" behavior that I've > seen so far (and which I think is the right way to deal with it): > > 1) If a pull request gets rejected due to a "make check" failure, simply > drop the offending patches from the pull request, send a v2 pull req > without them, and tell the author of the offending patches to fix the > problem (unless the fix is completely trivial and could immediately be > applied to the v2 pull req). You really don't have to fix each and every > issue on your own as a maintainer and can redirect this to the patch > authors again. > > 2) If a test fails occasionally during "make check" (due to race > conditions or whatever), it gets disabled from "make check" if it can't > be fixed easily (e.g. in the qtests it gets moved to the "SPEED=slow" > category, or in iotests it would get removed from the "auto" group). I would agree with this and would also suggest that things tested in 'make check' should ideally be reducing work for the maintainer: if something fails 'make check' then that change won't get into the tree and the task of getting it to work is pushed back to the original patch submitter. If something causes failures but they're not caught by 'make check' then the patch will get into the tree and now it's likely the subsystem maintainer's job to track down and fix the bug after the fact. > So if you like, I can send a patch to remove 130 from the "auto" group > (personally, I'd rather wait to see if it fails anytime soon again, or > if this was maybe rather a one-time issue due to a non-clean test system > ...) FWIW I haven't seen that iotest 130 failure again... thanks -- PMM
Re: [PATCH v5 1/5] iotests: remove 'linux' from default supported platforms
On 07.10.19 14:16, Thomas Huth wrote: > On 04/10/2019 14.44, Max Reitz wrote: >> On 04.10.19 12:19, Kevin Wolf wrote: >>> Am 02.10.2019 um 19:47 hat Max Reitz geschrieben: On 02.10.19 18:44, Kevin Wolf wrote: > Am 02.10.2019 um 13:57 hat Max Reitz geschrieben: >> It usually worked fine for me because it’s rather rare that non-block >> patches broke the iotests. > > I disagree. It happened all the time that someone else broke the iotests > in master and we needed to fix it up. OK. (In my experience, it’s still mostly block patches, only that they tend to go through non-block trees.) >>> >>> Fair enough, it's usually code that touches block code. I assumed "block >>> patches" to mean patches that go through one of the block trees and for >>> which iotests are run before sending a pull request. >>> >>> In the end, I don't care what code these patches touched. I do an >>> innocent git pull, and when I finally see that it's master that breaks >>> iotests and not my patches on top of it, I'm annoyed. >> >> Hm. Part of my point was that this still happens all the time. >> >> Which is why I’d prefer more tests to run in CI than a handful of not >> very useful ones in make check. > > Ok, so let's try to add some more useful test to the "auto" group. Kevin > mentioned 030, 040 and 041, and I think it should be ok to enable them > (IIRC the only issue was that they run a little bit longer, but if they > are very useful, we should include them anyway). I agree on those. (Maybe not 040, but definitely 030 and 041.) Maybe one of the issues was the “path too long” thing for Unix sockets? > Do you have any other tests in mind which could be very useful? I’d like a test for iothreads, it doesn’t look like there currently is one in auto. (The problem of course is that they have a higher chance of being flaky, but I think they also have a higher probability of breaking because of non-block stuff.) 127 and 256 look promising to me. Also, I don’t see any migration tests in auto (156 doesn’t really count). The ones that looks interesting here are 091, 181 (but I seem to remember that 181 is flaky under load, I should investigate that), 183, and 203 (which also tests iothreads). Those are the two area that I spontaneously think of when considering what is more likely to be broken by non-block patches. Unfortunately, those are also the two areas with the tests that tend to be the flakiest, I guess... > [...] >>> So if you merge that revert, when iotests break in master, I take it I >>> can rely on you to fix it up before it impacts my working branch? >> >> This is not a game, I’m talking purely from my standpoint: >> (I talked wrongly, but more on that below) >> >> Whenever make check fails, it’s urgent. Without iotests running in make >> check, we had some time to sort the situation out. That’s no longer the >> case. >> >> That means we need to take care of everything immediately. And I purely >> want help with that. > > While that sounds noble at a first glance, I think you've maybe got too > high expectations at yourself here? I mean, all other maintainers of > other subsystems with tests have the same "problem" if the tests for > their subsystem run in "make check". The difference is that the iotests generally seem much less deterministic to me than the other tests we have. > The "normal" behavior that I've > seen so far (and which I think is the right way to deal with it): > > 1) If a pull request gets rejected due to a "make check" failure, simply > drop the offending patches from the pull request, send a v2 pull req > without them, and tell the author of the offending patches to fix the > problem (unless the fix is completely trivial and could immediately be > applied to the v2 pull req). You really don't have to fix each and every > issue on your own as a maintainer and can redirect this to the patch > authors again. > > 2) If a test fails occasionally during "make check" (due to race > conditions or whatever), it gets disabled from "make check" if it can't > be fixed easily (e.g. in the qtests it gets moved to the "SPEED=slow" > category, or in iotests it would get removed from the "auto" group). Well, we’ll see how it goes. I suppose in practice it won’t be too big of a problem to just temporarily remove tests from auto if the issue isn’t clear immediately but the test does seem important. (I don’t think there’s too much danger of forgetting them.) >>> Yes, making iotests stable on CI probably involves some pain, especially >>> initially. However, if we don't fix them upstream, they'll end up on our >>> desk again downstream because people run tests neither on your nor on my >>> laptop. >>> >>> I think we might actually save time by fixing them once and for all, >>> even if they are problems in the test cases and not in QEMU, and making >>> new test cases portable from the start, instead of getting individual >>> reports one at a time and
Re: [PATCH v5 1/5] iotests: remove 'linux' from default supported platforms
On 07/10/2019 14.52, Max Reitz wrote: > On 07.10.19 14:16, Thomas Huth wrote: >> On 04/10/2019 14.44, Max Reitz wrote: >>> On 04.10.19 12:19, Kevin Wolf wrote: Am 02.10.2019 um 19:47 hat Max Reitz geschrieben: > On 02.10.19 18:44, Kevin Wolf wrote: >> Am 02.10.2019 um 13:57 hat Max Reitz geschrieben: >>> It usually worked fine for me because it’s rather rare that non-block >>> patches broke the iotests. >> >> I disagree. It happened all the time that someone else broke the iotests >> in master and we needed to fix it up. > > OK. > > (In my experience, it’s still mostly block patches, only that they tend > to go through non-block trees.) Fair enough, it's usually code that touches block code. I assumed "block patches" to mean patches that go through one of the block trees and for which iotests are run before sending a pull request. In the end, I don't care what code these patches touched. I do an innocent git pull, and when I finally see that it's master that breaks iotests and not my patches on top of it, I'm annoyed. >>> >>> Hm. Part of my point was that this still happens all the time. >>> >>> Which is why I’d prefer more tests to run in CI than a handful of not >>> very useful ones in make check. >> >> Ok, so let's try to add some more useful test to the "auto" group. Kevin >> mentioned 030, 040 and 041, and I think it should be ok to enable them >> (IIRC the only issue was that they run a little bit longer, but if they >> are very useful, we should include them anyway). > > I agree on those. (Maybe not 040, but definitely 030 and 041.) > > Maybe one of the issues was the “path too long” thing for Unix sockets? Ah, right. I've applied John's "remove 'linux' from default" patch and added the three iotests to the "auto" group, and indeed, they fail now on cirrus-ci due to the "path too long" socket problem. "We" (royal we, I guess) should likely fix that first... >> Do you have any other tests in mind which could be very useful? > > I’d like a test for iothreads, it doesn’t look like there currently is > one in auto. (The problem of course is that they have a higher chance > of being flaky, but I think they also have a higher probability of > breaking because of non-block stuff.) > > 127 and 256 look promising to me. > > > Also, I don’t see any migration tests in auto (156 doesn’t really count). > > The ones that looks interesting here are 091, 181 (but I seem to > remember that 181 is flaky under load, I should investigate that), 183, > and 203 (which also tests iothreads). > > > Those are the two area that I spontaneously think of when considering > what is more likely to be broken by non-block patches. Unfortunately, > those are also the two areas with the tests that tend to be the > flakiest, I guess... Ok, thanks for the list. I'll have a look at them whether it's feasible to get them enabled... >>> OTOH, keeping the iotests in make check means we have to act on failure >>> reports immediately. For example, someone™ needs to investigate the 130 >>> failure Peter has reported. I’ve just replied “I don’t know”, CC’d >>> Thomas, and waited whether anyone else would do anything. Nobody did, >>> and that can’t work. (I also wrote that I wouldn’t act on it, precisely >>> because I didn’t see the point. I assumed that if anyone disagreed with >>> that statement, they would have said something.) >>> >>> So acting on such reports means pain, too. I consider it greater than >>> the previous kind of pain, because I prefer “This sucks, I’ll have to >>> fix it this week or so” to “Oh crap, I’ll have to investigate now, >>> reproduce it, write an email as soon as possible, and fix it”. >> >> I think that "I have to investigate now ..." mindset is not the right >> way to handle these kind of issues. If a test is shaky and can not be >> fixed easily, we should simply disable it from the "auto" group again. >> So if you like, I can send a patch to remove 130 from the "auto" group >> (personally, I'd rather wait to see if it fails anytime soon again, or >> if this was maybe rather a one-time issue due to a non-clean test system >> ...) > > Waiting for another failure sounds OK to me. (OTOH, 130 doesn’t seem > like something that needs to be in auto, in case we want to take > something out to save time for more important tests.) > > But that reminds me that iotest failures probably should be > automatically signaled to me and Kevin instead of Peter having to write > an email himself. Would that be possible? Not sure whether an automatic e-mail is feasible, but you could register a github account to test with Travis (and maybe Cirrus-CI) before sending pull requests to Peter, I guess that will catch most of the issues on other machines already. Thomas signature.asc Description: OpenPGP digital signature
Re: [PATCH v5 1/5] iotests: remove 'linux' from default supported platforms
On 07/10/2019 15.11, Thomas Huth wrote: > On 07/10/2019 14.52, Max Reitz wrote: >> On 07.10.19 14:16, Thomas Huth wrote: >>> On 04/10/2019 14.44, Max Reitz wrote: On 04.10.19 12:19, Kevin Wolf wrote: > Am 02.10.2019 um 19:47 hat Max Reitz geschrieben: >> On 02.10.19 18:44, Kevin Wolf wrote: >>> Am 02.10.2019 um 13:57 hat Max Reitz geschrieben: It usually worked fine for me because it’s rather rare that non-block patches broke the iotests. >>> >>> I disagree. It happened all the time that someone else broke the iotests >>> in master and we needed to fix it up. >> >> OK. >> >> (In my experience, it’s still mostly block patches, only that they tend >> to go through non-block trees.) > > Fair enough, it's usually code that touches block code. I assumed "block > patches" to mean patches that go through one of the block trees and for > which iotests are run before sending a pull request. > > In the end, I don't care what code these patches touched. I do an > innocent git pull, and when I finally see that it's master that breaks > iotests and not my patches on top of it, I'm annoyed. Hm. Part of my point was that this still happens all the time. Which is why I’d prefer more tests to run in CI than a handful of not very useful ones in make check. >>> >>> Ok, so let's try to add some more useful test to the "auto" group. Kevin >>> mentioned 030, 040 and 041, and I think it should be ok to enable them >>> (IIRC the only issue was that they run a little bit longer, but if they >>> are very useful, we should include them anyway). >> >> I agree on those. (Maybe not 040, but definitely 030 and 041.) >> >> Maybe one of the issues was the “path too long” thing for Unix sockets? > > Ah, right. I've applied John's "remove 'linux' from default" patch and > added the three iotests to the "auto" group, and indeed, they fail now > on cirrus-ci due to the "path too long" socket problem. "We" (royal we, > I guess) should likely fix that first... FWIW, 041 also fails on macOS on Travis (which does not have the "path too long" issue): https://travis-ci.com/huth/qemu/jobs/242942716#L8415 ... so we might need to declare this as "linux only" again after John's patch gets merged. Thomas signature.asc Description: OpenPGP digital signature
Re: [PATCH v5 1/5] iotests: remove 'linux' from default supported platforms
On 07.10.19 18:28, Thomas Huth wrote: > On 07/10/2019 15.11, Thomas Huth wrote: >> On 07/10/2019 14.52, Max Reitz wrote: >>> On 07.10.19 14:16, Thomas Huth wrote: On 04/10/2019 14.44, Max Reitz wrote: > On 04.10.19 12:19, Kevin Wolf wrote: >> Am 02.10.2019 um 19:47 hat Max Reitz geschrieben: >>> On 02.10.19 18:44, Kevin Wolf wrote: Am 02.10.2019 um 13:57 hat Max Reitz geschrieben: > It usually worked fine for me because it’s rather rare that non-block > patches broke the iotests. I disagree. It happened all the time that someone else broke the iotests in master and we needed to fix it up. >>> >>> OK. >>> >>> (In my experience, it’s still mostly block patches, only that they tend >>> to go through non-block trees.) >> >> Fair enough, it's usually code that touches block code. I assumed "block >> patches" to mean patches that go through one of the block trees and for >> which iotests are run before sending a pull request. >> >> In the end, I don't care what code these patches touched. I do an >> innocent git pull, and when I finally see that it's master that breaks >> iotests and not my patches on top of it, I'm annoyed. > > Hm. Part of my point was that this still happens all the time. > > Which is why I’d prefer more tests to run in CI than a handful of not > very useful ones in make check. Ok, so let's try to add some more useful test to the "auto" group. Kevin mentioned 030, 040 and 041, and I think it should be ok to enable them (IIRC the only issue was that they run a little bit longer, but if they are very useful, we should include them anyway). >>> >>> I agree on those. (Maybe not 040, but definitely 030 and 041.) >>> >>> Maybe one of the issues was the “path too long” thing for Unix sockets? >> >> Ah, right. I've applied John's "remove 'linux' from default" patch and >> added the three iotests to the "auto" group, and indeed, they fail now >> on cirrus-ci due to the "path too long" socket problem. "We" (royal we, >> I guess) should likely fix that first... I’m looking into that, by the way. I hope a SOCK_DIR that defaults to $(mktemp -d) will suffice... > FWIW, 041 also fails on macOS on Travis (which does not have the "path > too long" issue): > > https://travis-ci.com/huth/qemu/jobs/242942716#L8415 > > ... so we might need to declare this as "linux only" again after John's > patch gets merged. :/ Either that, or let make check skip the iotests generally on non-Linux systems. Max signature.asc Description: OpenPGP digital signature
Re: [PATCH v5 1/5] iotests: remove 'linux' from default supported platforms
On 18.09.19 01:45, John Snow wrote: > verify_platform will check an explicit whitelist and blacklist instead. > The default will now be assumed to be allowed to run anywhere. > > For tests that do not specify their platforms explicitly, this has the effect > of > enabling these tests on non-linux platforms. For tests that always specified > linux explicitly, there is no change. > > For Python tests on FreeBSD at least; only seven python tests fail: > 045 147 149 169 194 199 211 > > 045 and 149 appear to be misconfigurations, > 147 and 194 are the AF_UNIX path too long error, > 169 and 199 are bitmap migration bugs, and > 211 is a bug that shows up on Linux platforms, too. > > This is at least good evidence that these tests are not Linux-only. If > they aren't suitable for other platforms, they should be disabled on a > per-platform basis as appropriate. > > Therefore, let's switch these on and deal with the failures. What exactly do you mean by “deal with the failures”? Do you have a reference to patches that deal with them, or are you or is someone else working on them...? Apart from that, I am rather hesitant to take a patch through my tree that not only may cause test failures on platforms that I will not or actually cannot run tests on (like MacOS or Windows), but that actually does introduce new failures as you describe. Well, at least it doesn’t introduce build failures because it appears there is no Python test that’s in the auto group, so I suppose “rather hesitant” is not an “I won’t”. Max > Signed-off-by: John Snow > --- > tests/qemu-iotests/iotests.py | 16 +++- > 1 file changed, 11 insertions(+), 5 deletions(-) signature.asc Description: OpenPGP digital signature
Re: [PATCH v5 1/5] iotests: remove 'linux' from default supported platforms
On 9/23/19 9:09 AM, Max Reitz wrote: > On 18.09.19 01:45, John Snow wrote: >> verify_platform will check an explicit whitelist and blacklist instead. >> The default will now be assumed to be allowed to run anywhere. >> >> For tests that do not specify their platforms explicitly, this has the >> effect of >> enabling these tests on non-linux platforms. For tests that always specified >> linux explicitly, there is no change. >> >> For Python tests on FreeBSD at least; only seven python tests fail: >> 045 147 149 169 194 199 211 >> >> 045 and 149 appear to be misconfigurations, >> 147 and 194 are the AF_UNIX path too long error, >> 169 and 199 are bitmap migration bugs, and >> 211 is a bug that shows up on Linux platforms, too. >> >> This is at least good evidence that these tests are not Linux-only. If >> they aren't suitable for other platforms, they should be disabled on a >> per-platform basis as appropriate. >> >> Therefore, let's switch these on and deal with the failures. > > What exactly do you mean by “deal with the failures”? Do you have a > reference to patches that deal with them, or are you or is someone else > working on them...? > > Apart from that, I am rather hesitant to take a patch through my tree > that not only may cause test failures on platforms that I will not or > actually cannot run tests on (like MacOS or Windows), but that actually > does introduce new failures as you describe. > > Well, at least it doesn’t introduce build failures because it appears > there is no Python test that’s in the auto group, so I suppose “rather > hesitant” is not an “I won’t”. > > Max > This is why I didn't want this to be part of the logging series. There's basically no way to win and this series is egregiously beyond the five minutes I devoted to it. I'd rather we just merge the last version if we're not ready to enable testing on other platforms. It's wrong, but it was wrong anyway. --js
