Re: Running 297 from GitLab CI
On Fri, Oct 01, 2021 at 10:21:36AM +0200, Kevin Wolf wrote: > Am 30.09.2021 um 23:28 hat John Snow geschrieben: > > Hiya, I was talking this over with Hanna in review to '[PATCH v3 00/16] > > python/iotests: Run iotest linters during Python CI' [1] and I have some > > doubt about what you'd personally like to see happen, here. > > > > In a nutshell, I split out 'linters.py' from 297 and keep all of the > > iotest-bits in 297 and all of the generic "run the linters" bits in > > linters.py, then I run linters.py from the GitLab python CI jobs. > > > > I did this so that iotest #297 would continue to work exactly as it had, > > but trying to serve "two masters" in the form of two test suites means some > > non-beautiful design decisions. Hanna suggested we just outright drop test > > 297 to possibly improve the factoring of the tests. > > > > I don't want to do that unless you give it the go-ahead, though. I wanted > > to hear your feelings on if we still want to keep 297 around or not. > > My basic requirement is that the checks are run somewhere in my local > testing before I prepare a pull request. This means it could be done by > iotests in any test that runs for -raw or -qcow2, or in 'make check'. > > So if you have a replacement somewhere in 'make check', dropping 297 is > fine with me. If I have to run something entirely different, you may > need to invest a bit more effort to convince me. ;-) I'll echo that sentiment - if it's easy to automate, we can run it under 'make check', and then we don't need the duplication of also running it under iotests (especially since it isn't "really" an iotest, so much as a test that makes the rest of the iotests more consistent). If it's harder to automate, or requires me to run one more thing, then keeping it in iotests for the short term is not too drastic of a request, so that I don't accidentally skip it. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: Running 297 from GitLab CI
On Fri, Oct 1, 2021 at 4:21 AM Kevin Wolf wrote: > Am 30.09.2021 um 23:28 hat John Snow geschrieben: > > Hiya, I was talking this over with Hanna in review to '[PATCH v3 00/16] > > python/iotests: Run iotest linters during Python CI' [1] and I have some > > doubt about what you'd personally like to see happen, here. > > > > In a nutshell, I split out 'linters.py' from 297 and keep all of the > > iotest-bits in 297 and all of the generic "run the linters" bits in > > linters.py, then I run linters.py from the GitLab python CI jobs. > > > > I did this so that iotest #297 would continue to work exactly as it had, > > but trying to serve "two masters" in the form of two test suites means > some > > non-beautiful design decisions. Hanna suggested we just outright drop > test > > 297 to possibly improve the factoring of the tests. > > > > I don't want to do that unless you give it the go-ahead, though. I wanted > > to hear your feelings on if we still want to keep 297 around or not. > > My basic requirement is that the checks are run somewhere in my local > testing before I prepare a pull request. This means it could be done by > iotests in any test that runs for -raw or -qcow2, or in 'make check'. > > So if you have a replacement somewhere in 'make check', dropping 297 is > fine with me. If I have to run something entirely different, you may > need to invest a bit more effort to convince me. ;-) > > I love a good set of solid criteria ;-) Understood! At the moment it *would* require a separate invocation. The Python tests that run under CI generally set up their own environments to ensure they'll run with minimum fuss or intervention from humans, though there is an invocation in that Makefile that performs no environment setup whatsoever -- but since no automated test uses it, it's not really a big problem if your environment is "wrong" for that one. (But that also makes it useless for make check!) It's similar to how iotest 297 does not really check to see what version of pylint or mypy you might have, so sometimes that test fails if your environment isn't suitable. But that one isn't part of 'make check' either, so this feels like a bridge we've never crossed anywhere in the whole source tree. I have so far abstained from introducing such a step into 'make check' because it might require some additional engineering to ensure that I get the temporary directories all correct, tolerate the different types of builds we do, uses the correct Python interpreter for all steps, etc. So for now I'll propose leaving 297 as-is for convenience, but I will start working towards finding the right way to include those tests from 'make check'. I think there's no barrier (other than subjectively, beauty) to leaving both avenues in for running the test for the time being. Maybe I will find a way to convince Hanna to accept the interim solution as "well, not THAT ugly." Thanks for your input! --js
Re: [PATCH 09/11] qdev: Avoid QemuOpts in QMP device_add
On Fri, Sep 24, 2021 at 11:04:25 +0200, Kevin Wolf wrote: > Directly call qdev_device_add_from_qdict() for QMP device_add instead of > first going through QemuOpts and converting back to QDict. > > Note that this changes the behaviour of device_add, though in ways that > should be considered bug fixes: > > QemuOpts ignores differences between data types, so you could > successfully pass a string "123" for an integer property, or a string > "on" for a boolean property (and vice versa). After this change, the > correct data type for the property must be used in the JSON input. > > qemu_opts_from_qdict() also silently ignores any options whose value is > a QDict, QList or QNull. Sorry for chiming in a bit late, but preferrably this commit should be postponed to at least the next release so that we decrease the amount of libvirt users broken by this. Granted users are supposed to use new libvirt with new qemu but that's not always the case. Anyways, libvirt is currently mangling all the types to strings with device_add. I'm currently working on fixing it and it will hopefully be done until next-month's release, but preferrably we increase the window of working combinations by postponing this until the next release.
Re: Running 297 from GitLab CI
Am 30.09.2021 um 23:28 hat John Snow geschrieben: > Hiya, I was talking this over with Hanna in review to '[PATCH v3 00/16] > python/iotests: Run iotest linters during Python CI' [1] and I have some > doubt about what you'd personally like to see happen, here. > > In a nutshell, I split out 'linters.py' from 297 and keep all of the > iotest-bits in 297 and all of the generic "run the linters" bits in > linters.py, then I run linters.py from the GitLab python CI jobs. > > I did this so that iotest #297 would continue to work exactly as it had, > but trying to serve "two masters" in the form of two test suites means some > non-beautiful design decisions. Hanna suggested we just outright drop test > 297 to possibly improve the factoring of the tests. > > I don't want to do that unless you give it the go-ahead, though. I wanted > to hear your feelings on if we still want to keep 297 around or not. My basic requirement is that the checks are run somewhere in my local testing before I prepare a pull request. This means it could be done by iotests in any test that runs for -raw or -qcow2, or in 'make check'. So if you have a replacement somewhere in 'make check', dropping 297 is fine with me. If I have to run something entirely different, you may need to invest a bit more effort to convince me. ;-) Kevin
