> -----Original Message----- > From: Stefan Stanacar [mailto:[email protected]] > Sent: Saturday, June 07, 2014 5:09 PM > To: Stoicescu, CorneliuX > Cc: [email protected] > Subject: Re: [OE-core] [PATCH V2 7/7] scripts/test-remote-image: Add script > for running runtime tests on remotely built images > > Hi Corneliu, > > I haven't carefully reviewed this series so if I didn't quite got the point of > some things, sorry... > At a first look, here are my thoughts (this is actually a reply to the entire > series, I seem to have lost the cover letter): > > - It seems wierd that you instantiate the target controllers classes here... > You > do that so you can get the extra files, but that has nothing to do with > testimage anyway. Let bitbake/testimage do it's part and it's configuration. > Similar for image fs types (see my other > reply)
- I believe it's a good idea to use the target controllers to get information from them using class methods. This way we use the same source to get the information we need both internally and externally. > - I wouldn't mix the download stuff with the test/deploy stuff (bitbake's > job). > You already have command line configuration knobs, I'm sure you could add > one for extra files... Surely the user know it's testing BBB and core-image- > minimal and it needs to download the dtbs (because you need the dtbs only > for minimal). - This feature required keeping the user configuration to a minimum(we did not want to introduce any more TEST_ variables and have as few required command line options for the script as possible). The way it is, all target controllers use predictable image files and the missing piece was the so called "extra files" that some special target controllers need. This method, along with the other new class methods, can be used by any kind of external script needing information on what testimage needs/uses. > - the support for other fs types should be separate from the "test external > built image". E.g bbb uses tar.bz2, so even at this point you can still test > external images only with tar.gz (adding IMAGE_FSTYPES > += "tar.gz" to the Autobuilder config is a easy task). What I'm > trying to say is that this is a user/build config option and this script > shouldn't > deal with every little thing - We wanted to use the current default IMAGE_FSTYPES as much as possible(this is why the bug was opened). For this we had to enable the master image to decompress tar.bz2 and make the target controllers use them. Having IMAGE_FTYPES += "tar.gz" for all test images was not desired. - We wanted to make the way the rootfs image fstype was chosen need as little maintenance as possible. For this we just define what image_fstypes a target controller can handle and the choose one from the available IMAGE_FSTYPES. Also having a classmethod decide this makes it easy to get this information from external sources, with higher accuracy. > - What's with the postconfig stuff? QA_MACHINE = MACHINE? Same for > distro.. I really don't get that part... Bitbake does not export the MACHINE variable so in order to get this information we had to use this trick (the runqemu script also uses it, if I remember right). We do QA_MACHINE = MACHINE and then the QA_MACHINE variable is exported by bitbake and we can get it from 'bitbake -e'. :D > - I'm missing the bigger picture here with all the profiles stuff (plus some > classes are unnecessarily abstract ). The same way we have a BaseTarget class for the target controllers, we also have base target/repo profiles. These classes are abstract and define the bits and pieces that the script will actually use. We don't care how they are implemented. You can see them as interfaces to the target and images repo. Also we wanted classes so they can be extended/modified easily by new ones. For example we have an AutoTargetProfile class that auto determines the images needed, but say a target controller cannot be predicted at all with the information we can access. We just create a new target profile extending the base target profile and add new functionality and information sources but still use the 'interfaces' defined. > > So the whole point of this was to replace (assume default local.conf and > MACHINE="beaglebone" here) > this: > --- > $ vi conf/local.conf > INHERIT += "testimage" > TEST_TARGET_IP = "10.11.12.2" > TEST_TARGET = "BeagleBoneTarget" > TEST_SERIALCONTROL_CMD = "picocom /dev/ttyUSB0 -b 115200" > $ bitbake core-image-minimal # (or sato / sato-sdk) $ bitbake <image> -c > testimage > --- > with this (except the local.conf part which should already be there, it is not > the job of this script to add that): > ---- > $ bitbake rpm psplash # rpm/smart tests require these, else skip $ bitbake > package-index $ wget -r -l0 -nd -np -P "tmp/deploy/images" > http://autobuilder.yoctoproject.org/pub/nightly/20140606- > 3/machines/beaglebone/ > $ bitbake <image> -c testimage > --- > #yes, I cheated there, getting all the files, but you get the point.. > alternatively: > wget -r -l0 -nd -np -P "tmp/deploy/images" -A "*boneblack*.dtb" > http://autobuilder.yoctoproject.org/pub/nightly/20140606- > 3/machines/beaglebone/ > wget -P "tmp/deploy/images" > http://autobuilder.yoctoproject.org/pub/nightly/20140606- > 3/machines/beaglebone/core-image-minimal-beaglebone.manifest > wget -P "tmp/deploy/images" > http://autobuilder.yoctoproject.org/pub/nightly/20140606- > 3/machines/beaglebone/core-image-minimal-beaglebone.tar.gz > wget -P "tmp/deploy/images" > http://autobuilder.yoctoproject.org/pub/nightly/20140606- > 3/machines/beaglebone/uImage > > It seems to me that you are doing a lot more with this series and adding > unnecessary complications. I guess I'm missing some pieces here... > THB the first version of this script did mainly what you pointed out it should do. But after a few talks among the team (here in RO QA and Paul), we decided we need to automate some configuration (like deciding what images we need, downloading them, checking if they are different than upstream, etc.) and also we need to make this easily extendable and thus the abstract classes and interfaces. This series, apart from adding this script, addresses other issues too. Not everything is for the script, you can see them as issues found along the way (like hard coded image fstypes, multiple kernel files naming). These should have been fixed independently but I added them here because of lack of time(see below) and they are related somehow(especially to the script). The only change introduced that only helps the script (and other outside script that will want this information) is the get_extra_files method. If the only use case was to build the images locally and run the tests locally, this would have been easily avoided but unfortunately we need to use the AB images on remote hardware testing racks. Finally, why this series of patches was needed instead of standalone small patches. These bugs have been marked for 1.7 M1 and even though we tried to finish earlier some changes affected the others and there was no point to send something and then repair/change it. I will be OOO this week and the cutoff for M1 is on the 13-th. I am sorry for the large set and I hope, through the cover letter and these answers, I managed to point out what we intend to add and change. I will not be able to answer any more mails until the 17-th but I hope Paul can help with any more questions/issues that appear. If we did extend too much with this and some changes proposed are not indeed needed, I will revise the script and make it work without them when I get back, though the features should be moved to M2. Regards, Corneliu -- _______________________________________________ Openembedded-core mailing list [email protected] http://lists.openembedded.org/mailman/listinfo/openembedded-core
