Re: [PATCH v4 1/9] KVM: selftest: Create KVM selftest runner

Vipin Sharma <[email protected]> writes:

>
> [...snip...]
>
>>
>> I was expecting something like if the output is piped to some file or
>> like "not TTY" then don't print colors.
>>
>> Would you consider not printing in color at all? Or adding some kind of
>> "turn off all display magic" flag?
>
> Good point about color on/off. I can make following
> changes in following priority, starting with the highest:
>
> 1. If NO_COLOR enivornment variable is set then don't print color at
>    all.
> 2. If FORCE_COLOR environment variable is set then print color
>    (default).
> 3. If output is not TTY then don't print color.
> 4. Print color.
>

I think we can take reference from what grep does.

+ --color=auto (default), if output is piped or redirected, no color.
+ --color=always
+ --color=never

>>
>> I also saw code for a sticky footer in another patch in this series,
>> that's probably not nice for other stuff wrapping this runner.
>>
> For sticky update, I can hide it if there is no TTY.
>
>> > +if __name__ == "__main__":
>> > +    PYTHON_VERSION = (3, 6)
>> > +    if sys.version_info < PYTHON_VERSION:
>> > +        print(f"Minimum required python version {PYTHON_VERSION}, found {sys.version}")
>> > +        sys.exit(1)
>> > +
>> > +    sys.exit(main())
>>
>> This shouldn't block merge: why not align with the kernel's official
>> required python version?
>>
>
> When I first proposed official version was 3.5, it got upgraded to 3.9
> in commit 5e25b972a22b ("docs: changes: update Python minimal version")
>
> I was using some APIs which were not present in 3.5 and 3.6 was the
> minimum version I was able to run all the features I needed.
>
> It just stayed that version and I never checked to keep it in sync with
> the latest python version.
>

I see, no worries, not a huge issue.

>> > +
>> > +        run_args = {
>> > +            "universal_newlines": True,
>> > +            "shell": True,
>> > +            "stdout": subprocess.PIPE,
>> > +            "stderr": subprocess.PIPE
>> > +        }
>> > +        proc = subprocess.run(self.command, **run_args)
>> > +
>> > +        out, err = proc.stdout, proc.stderr
>> > +        self.stdout = out.decode("utf-8", "replace") if isinstance(out, bytes) else (out or "")
>> > +        self.stderr = err.decode("utf-8", "replace") if isinstance(err, bytes) else (err or "")
>>
>> I think it would be useful to capture stdout and stderr in order that it
>> was output, so that the output being saved shows everything
>> interleaved. I think the order could be useful in debugging.
>>
>> I can see benefits in knowing which was on stdout and which was on
>> stderr too, so is there some way of having both?
>>
>
> Both approaches are useful, I am not sure how to achieve what you are
> asking reliably.
>
> However, I think separate stdout and stderr is more useful in automation/CI
> tools. Considering, runner is for the human use I am more inclined on
> having combined output of stdout and stderr as you are suggsting.
>
> I will change it to combined output in next version, new filename will
> be "out" consisting of both stdout and stderr. If there is ever
> need for separate dumps we can revisit it.
>
> One thing we will lose is in current approach, non-zero stderr is signal
> of finding errored out runs. Not a big loss as same information can be
> grepped using master log file.
>

Here's an idea to have both:

        import selectors

        process = subprocess.Popen(
            command,
            shell=True,
            stdout=subprocess.PIPE,
            stderr=subprocess.PIPE,
            text=True,
            bufsize=1,
        )

        sel = selectors.DefaultSelector()
        sel.register(process.stdout, selectors.EVENT_READ)
        sel.register(process.stderr, selectors.EVENT_READ)

        output_lines = []
        stdout_lines = []
        stderr_lines = []

        while True:
            events = sel.select()
            for key, mask in events:
                line = key.fileobj.readline()
                if not line:
                    sel.unregister(key.fileobj)
                    continue

                if key.fileobj is process.stdout:
                    # append to stdout_lines... Might need to deal
with newlines?
                    # perhaps even print it from the runner's stdout, so
                    #   the runner gives faster feedback for a human
                elif key.fileobj is process.stderr:
                    # do something equivalent for stderr_lines

                output_lines.append(line)

            if not sel.get_map():
                break

        process.wait()

I'm also okay if it's first combined, and this can be an extension if
it's really needed.

>> > +
>> > +        if proc.returncode == 0:
>> > +            self.status = SelftestStatus.PASSED
>> > +        elif proc.returncode == 4:
>> > +            self.status = SelftestStatus.SKIPPED
>> > +        else:
>> > +            self.status = SelftestStatus.FAILED
>>
>> Using this class-based pattern requires us to do
>> Selftest(test_path).run(). Would you consider using a function-based
>> pattern, like run_selftest(test_path) instead?
>>
>
> I find current class model provides an easy way to group data in
> hierarchy. For example, test_runner has list of selftests which it needs to
> execute. Selftests have their own test command, output data, result
> status, etc.
>
> Both are suitable to implement runner. But I am little hesitant to
> change it now considering it is already written. If you can provide
> benefits of using functions approach here then I am open to
> rewrite.
>

I think the classes approach introduces more boilerplate which isn't
necessary. I don't think there's any state stored in the classes and so
this is a good fit for using functions. After removing the boilerplate,
we might actually be able to put everything nicely into 1 file! That's
not to say classes can't be in 1 file, I think we're more used to
classes being in separate files.

__main__.py is already mostly in a non-class style, which makes the rest
being in the class style more jarring.

Most of the kernel is C which is mostly procedural where you need to
implement your own OOP, so I wish the accompanying python would follow a
more procedural/function-based style rather than directly reach for OO.

I'm also heavily influenced by Jack Diederich's talk [1], which is a fun
talk but may be too long to watch, so [2] is a text overview.

[1] https://youtu.be/o9pEzgHorH0
[2] https://szuckerman.github.io/stop_writing_classes.html

I guess this might be more of a philosophy/preference thing, feel free
to go ahead if others express preference for the class style.

>> >
>> > [...snip...]
>> >

-- 
kvm-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kvm-riscv