On Sat, Apr 12, 2025 at 06:45:02AM -0600, Simon Glass wrote: > Hi Tom, > > On Fri, 11 Apr 2025 at 13:13, Tom Rini <tr...@konsulko.com> wrote: > > > > On Wed, Apr 02, 2025 at 06:55:02AM +1300, Simon Glass wrote: > > > Hi Tom, > > > > > > On Wed, 2 Apr 2025 at 05:49, Tom Rini <tr...@konsulko.com> wrote: > > > > > > > > On Wed, Apr 02, 2025 at 04:49:18AM +1300, Simon Glass wrote: > > > > > Hi Tom, > > > > > > > > > > On Mon, 31 Mar 2025 at 03:45, Tom Rini <tr...@konsulko.com> wrote: > > > > > > > > > > > > On Thu, Mar 20, 2025 at 03:41:33AM +0000, Simon Glass wrote: > > > > > > > Hi Tom, > > > > > > > > > > > > > > On Wed, 19 Mar 2025 at 16:35, Tom Rini <tr...@konsulko.com> wrote: > > > > > > > > > > > > > > > > On Wed, Mar 19, 2025 at 03:04:28PM +0000, Simon Glass wrote: > > > > > > > > > Hi Tom, > > > > > > > > > > > > > > > > > > On Wed, 19 Mar 2025 at 15:52, Tom Rini <tr...@konsulko.com> > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > On Wed, Mar 19, 2025 at 03:38:03PM +0100, Simon Glass wrote: > > > > > > > > > > > > > > > > > > > > > Add an extlinux image that contains a few Ubuntu entries. > > > > > > > > > > > > > > > > > > > > > > Increase the number of sandbox-USB-hub ports to permit > > > > > > > > > > > this. > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Simon Glass <s...@chromium.org> > > > > > > > > > > > > > > > > > > > > I don't understand what this test adds. In neither the > > > > > > > > > > current Fedora > > > > > > > > > > test nor in this new test are we actually booting > > > > > > > > > > something, we're just > > > > > > > > > > taking a sample extlinux.conf and making sure it doesn't > > > > > > > > > > fail. Is it > > > > > > > > > > that we're testing in a useful fashion now having two > > > > > > > > > > labels? > > > > > > > > > > > > > > > > > > I didn't think so either, which is why I never did this > > > > > > > > > before. But it > > > > > > > > > turns out that there were some bugs, too. > > > > > > > > > > > > > > > > I don't understand you, sorry. You don't think so either to > > > > > > > > what? > > > > > > > > > > > > > > I didn't think we needed two extlinux examples on different boot > > > > > > > devices, but we do. > > > > > > > > > > > > I'm not sure it's particularly clear what you're doing here then, or > > > > > > why, but I'll find some time to read it all more deeply. > > > > > > > > > > OK. This test case is how I found the bugs/problems in bootstd that > > > > > are fixed in this series. > > > > > > > > It should be (a) better explained and likely (b) instead of dropping in > > > > a seemingly verbatim installed file a specially crafted bit of content. > > > > We aren't testing "Ubuntu". We are testing multiple labels. > > > > > > > > > > > > > > We should probably be clear about what we're doing in the > > > > > > > > > > tests and > > > > > > > > > > instead of adding seemingly arbitrary distributions add an > > > > > > > > > > extlinux test > > > > > > > > > > and testcases. > > > > > > > > > > > > > > > > > > This is not actually a test case. It is simply creating a new > > > > > > > > > image. > > > > > > > > > The test cases are in the other patches, so please take a > > > > > > > > > look there. > > > > > > > > > > > > > > > > Nothing in this series quickly reads as adding tests and fixing > > > > > > > > problems > > > > > > > > with extlinux parsing, it's all bootmeth stuff? > > > > > > > > > > > > > > It isn't about the actual parsing of the .conf file, although I > > > > > > > would > > > > > > > like to add tests there as we have none apart from what I have > > > > > > > added > > > > > > > in my PXE series. It's more about having multiple devices with > > > > > > > bootable OSes on them. This series tidies up and fixes this. We > > > > > > > need > > > > > > > to have an image available on more than one device to spot these > > > > > > > problems. > > > > > > > > > > > > And the existing tests for pxelinux that we have in mainline > > > > > > already, > > > > > > don't forget those. > > > > > > > > > > Yes. But those tests actually don't use bootstd, do they? > > > > > > > > No, they're testing pxelinux, the thing you said we don't have any tests > > > > for. > > > > > > > > > > > Currently we have two accessible to sandbox, one extlinux and one > > > > > > > EFI*. I decided to add a third, using extlinux. > > > > > > > > > > > > > > Again, this is not a test case, but provides an image for the test > > > > > > > cases in this series. > > > > > > > > > > > > Adding mocked up things for use in sandbox is adding test cases. > > > > > > > > > > One is a test image for use by tests; the other is a test. Perhaps you > > > > > are just saying that there is no point in having one without the > > > > > other? Otherwise, I'm not sure what to do with this feedback. > > > > > > > > Maybe I clarified better above now. You're not testing Ubuntu (or Fedora > > > > or Armbian or ...) you're seeing if various pxelinux files parse > > > > correctly. Making the tests be clearer about what's being tested is > > > > what's missing at least in part. > > > > > > You want me to remove the word 'Ubuntu' from the test files? This is > > > what I get when I install u-boot-tools so I am trying to use that, > > > rather than invent my own thing. This is the same approach I've taken > > > with Fedora and Armbian and I think it makes the most sense. > > > > I seem to be unable to explain that if you're constructing tests, I'd > > like to see it clear that you're constructing test files. You're not > > testing "Ubuntu" or "Fedora" or "Armbian" here. I'm not nak'ing this > > patch as at the end of the day, it's adding some test, even if it's also > > I believe being done in confusing ways. > > I actually do understand what you are saying. Indeed I am not testing > the OS booting, just that we can parse the files that they use to > describe the OS. I think it is better to use real files rather than > invent things that are not used in the wild. > > Perhaps the commit subject is bad. 'Add an extlinux file as used by Ubuntu' ?
However you would like to expand the subject / body to clarify further is fine, thanks. -- Tom
signature.asc
Description: PGP signature
