Message-ID: <62ea7097-256c-4331-b937-778444125a06@leemhuis.info>
Date: Sat, 17 Feb 2024 16:46:15 +0100
From: Thorsten Leemhuis <linux@...mhuis.info>
To: Petr Tesařík <petr@...arici.cz>
Cc: Jonathan Corbet <corbet@....net>, regressions@...ts.linux.dev,
 linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
 Bagas Sanjaya <bagasdotme@...il.com>, Nathan Chancellor <nathan@...nel.org>
Subject: Re: [PATCH v1] docs: new text on bisecting which also covers bug
 validation

On 16.02.24 20:41, Petr Tesařík wrote:
> On Fri, 16 Feb 2024 09:54:46 +0100
> Thorsten Leemhuis <linux@...mhuis.info> wrote:

First off a big "thank you" for your feedback, very much appreciated; it
was really helpful.

I followed your suggestions most of the time, sometimes with a few small
changes. I only mentions things below where I didn't do that or when
there was some other reason to reply.

>> [...]
>> Style and structure of the text match the one
>> Documentation/admin-guide/quickly-build-trimmed-linux.rst uses. Quite a
>> few paragraphs are even copied from there and not changed at all or only
>> slightly. This will complicate maintenance, as some future changes to
>> one of these documents will have to be replicated in the other. But this
>> is the lesser evil: solutions like "sending readers from one document
>> over to the other" or "extracting the common parts into a separate
>> document" might work in other cases, but would be too confusing here
>> given the topic and the target audience.
> 
> Is this because you want to keep it readable if the target audience
> reads the source text of the documentation? Otherwise, the .. include
> directive does not make a difference after rendering to HTML. AFAIK.

It less that I want that, it's more that I got the impression that both
Jonathan and most of the kernel development community wants the source
text to be readable; not totally sure, but I think that's the right
thing to do, too.

>> [...]
>> * The text as of now does not really describe what a bisection is --
>> neither in general nor in the scope of Git. Maybe that should be
>> added. Having a few nice graphical diagrams might also be good, as the
>> text is meant to be read in rendered form anyway. But I think it's
>> useful like this already.
> 
> But here you expect it to be read in rendered form.

It's just a suggestion (like the note in the test), but readers are
still free to ignore it if they want an won't run into major trouble.

> So, are you afraid
> of making things confusing for potential later editors of this text?

Not something I have thought about much about yet, but yes, keeping
things simple is likely in everybody's interest.

>> [...]
>> diff --git a/Documentation/admin-guide/verify-bugs-and-bisect-regressions.rst b/Documentation/admin-guide/verify-bugs-and-bisect-regressions.rst
>> new file mode 100644
>> index 00000000000000..0a6a1a082d867c
>> --- /dev/null
>> +++ b/Documentation/admin-guide/verify-bugs-and-bisect-regressions.rst
>> @@ -0,0 +1,1925 @@
>> +.. SPDX-License-Identifier: (GPL-2.0+ OR CC-BY-4.0)
>> +.. [see the bottom of this file for redistribution information]
>> +
>> +=========================================
>> +How to verify bugs and bisect regressions
>> +=========================================
>> +
>> +This document describes how to check if some Linux kernel problem occurs in the
>> +code developers currently support
> 
> I got puzzled for a moment as the subject of the sentence changed. What about:
> 
>   code currently supported by developers

As you have noticed here and everywhere: It seems I have overdone the
"avoid passive voice" approach. I followed your advice here and in other
places.

>> -- to then explain how to locate the change
>> +causing the issue, if it did not happen with earlier versions.
> 
> I believe this part of the sentence could be also improved. I would not
> be afraid of introducing the word "regression" here, e.g.:
> 
>   It also explains how to locate the change which introduced the issue if
>   it did not happen with an earlier version. Such issues are commonly
>   called regressions.

Hmmm. That to me feels a bit to dry for the first sentence of the
document, especially that "Such issues are commonly called regressions."
part. I reworked my version slightly, but not much:

 This document describes how to check if some Linux kernel problem
 occurs in code currently supported by developers -- to then explain how
 to locate the change causing the issue, if it is a regression (e.g. did
 not happen with earlier versions).

Not sure if the word regression really needs to be used and explained
here, as it's already in the headline. But whatever.


>> +    # * Hint: at this point you might want to adjust the build configuration;
>> +    #   you'll have to, if you are running Debian.
> 
> Ugh... this step is necessary if you are running Debian ?

It definitely was and it's still in the Debian docs[1], but I haven't
recently checked.

[1] search for "Missing debian/certs/debian-uefi-certs.pem" in
https://debian-handbook.info/browse/stable/sect.kernel-compilation.html

>> +    one. Note, you in this case nevertheless want to compile a mainline kernel
>> +    as explained in segment 1, as that will determine if that is a bug that the
>> +    regular developers or the stable team will have to handle.
> 
> Ugh again. Probably:
> 
>   Note, in this case you still want to compile a mainline kernel as
>   explained in segment 1. It will be used to decide if your issue will
>   be handled by regular developers or by the stable team.

I went with this instead:

 Note, in this case you still want to compile and test a mainline kernel
 as explained in segment 1: the outcome will determine if you need to
 report your issue to the regular developers or the stable team.

>> +* Retrieve the mainline Linux sources; then change into the directory holding
>> +  them, as all further commands in this guide are meant to be executed from
>> +  there.
>> +
>> +  *Note, the following describe how to retrieve the sources using a full
>> +  mainline clone, which downloads about 2,75 GByte as of early 2024. The*
>> +  :ref:`reference section describes two alternatives <sources_bisref>` *:
>> +  one downloads less than 500 MByte, the other works better with unreliable
>> +  internet connections.*
>> +
>> +  Execute the following command to retrieve a fresh mainline codebase::
>> +
>> +    git clone -o mainline --no-checkout \
>> +      git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git ~/linux/
>> +    cd ~/linux/
> 
> This is not very nice to git-daemon. Plus, it uses the insecure git
> protocol.

Argh, the latter was definitely unintended (it likely sneaked in due to
some unobservant cut'n'paste...). :-/ Fixed!

> Is it too much to show cloning from a bundle instead?
> 
>   https://www.kernel.org/cloning-linux-from-a-bundle.html

I know, but it's much more commands, that's why I decided against this.
And from what I heard from Konstantin when that came up during the
submission of the "quickly build trimmed kernel" guide it's not that bad
if a few users do it like that; CI systems are the big problem here.

> I stop here and call it a week. I may read the rest later. Hope even
> this much helps .

It helped a great deal, thx again!

Ciao, Thorsten

Powered by blists - more mailing lists