kconfig: diagnostics cleanups

kconfig: diagnostics cleanups

[Boris Kolpackov]

I am preparing a set of patches that clean up kconfig diagnostics and
make it more consistent both internally and with respect to other
tools (like compilers). However, a couple of changes that I would like
to make could be controversial so I want to discuss them before wasting
everyone's time with patches:

1. Add 'warning' word to $(warning-if) output:

-  fprintf(stderr, "%s:%d: %s\n", ...);
+  fprintf(stderr, "%s:%d: warning: %s\n", ...);

   This makes it consistent with the rest of the warnings printed by
   kconfig.

2. Print $(info) output to stderr instead of stdout.

I realize the current behavior is consistent with GNU make (on which
it is based) but at the same time it's inconsistent with the rest of
kconfig (#1) or does not seem to make much sense (#2), at least to
me.

To elaborate on #2, $(info) is still diagnostics, just a different
level compared to $(warning-if) and $(error-if). It's not clear to
me why it should go to stdout.

If we needed the ability to print something to stdout, we could add
another function, such as $(print). However, I can't think of a good
reason why we would need to; this, for example, has the potential to
mess up with the terminal-based UI (which is written to stdout).

I've done a search and as far as I can see, neither $(warning) nor
$(info) is currently used anywhere in the kernel outside the kconfig
testsuite. So these changes shouldn't have any backwards-compatibility
issues.

Thoughts?

Re: kconfig: diagnostics cleanups

[Masahiro Yamada]

On Wed, Nov 25, 2020 at 11:38 PM Boris Kolpackov
<boris@codesynthesis.com> wrote:
>
> I am preparing a set of patches that clean up kconfig diagnostics and
> make it more consistent both internally and with respect to other
> tools (like compilers). However, a couple of changes that I would like
> to make could be controversial so I want to discuss them before wasting
> everyone's time with patches:
>
> 1. Add 'warning' word to $(warning-if) output:
>
> -  fprintf(stderr, "%s:%d: %s\n", ...);
> +  fprintf(stderr, "%s:%d: warning: %s\n", ...);
>
>    This makes it consistent with the rest of the warnings printed by
>    kconfig.
>
> 2. Print $(info) output to stderr instead of stdout.
>
> I realize the current behavior is consistent with GNU make (on which
> it is based) but at the same time it's inconsistent with the rest of
> kconfig (#1) or does not seem to make much sense (#2), at least to
> me.
>
> To elaborate on #2, $(info) is still diagnostics, just a different
> level compared to $(warning-if) and $(error-if). It's not clear to
> me why it should go to stdout.
>
> If we needed the ability to print something to stdout, we could add
> another function, such as $(print). However, I can't think of a good
> reason why we would need to; this, for example, has the potential to
> mess up with the terminal-based UI (which is written to stdout).
>
> I've done a search and as far as I can see, neither $(warning) nor
> $(info) is currently used anywhere in the kernel outside the kconfig
> testsuite. So these changes shouldn't have any backwards-compatibility
> issues.
>
> Thoughts?


$(warning-if ...) and $(info ...) mimic
$(warning ...) and $(info ...) because
the design of kconfig macros was inspired by GNU Make.

So, I implemented them in the same way as GNU Make did
unless I had a good reason to do otherwise.

I expected they would be useful for debugging for something,
but there is no actual user.

We can change them if there is a reason,
but I cannot see it in your description.


-- 
Best Regards
Masahiro Yamada

Re: kconfig: diagnostics cleanups

[Boris Kolpackov]

Masahiro Yamada <masahiroy@kernel.org> writes:

> We can change them if there is a reason, but I cannot see it in your
> description.

> Boris Kolpackov <boris@codesynthesis.com> wrote:
>
> > 1. Add 'warning' word to $(warning-if) output:

This will make the diagnostics consistent with other places in kconfig
where warnings are issued (see conf_warrning() in confdata.c).


> > 2. Print $(info) output to stderr instead of stdout.

There are two reasons:

1. Error, warning, and info are different diagnostics levels. It was
   surprising to me that the first two go to stderr while info goes
   to stdout. For example, as a user, if I redirect stderr, I would
   naturally expect all the diagnostics to go there.

2. More importantly, stdout is used by terminal-based UI configurators.
   So depending on exactly when $(info) is issued, its output could either
   be clobbered by UI (so the user won't notice it) or it can clobber UI
   (so the user will see broken UI).

Re: kconfig: diagnostics cleanups

[Masahiro Yamada]

On Wed, Dec 2, 2020 at 5:06 PM Boris Kolpackov <boris@codesynthesis.com> wrote:
>
> Masahiro Yamada <masahiroy@kernel.org> writes:
>
> > We can change them if there is a reason, but I cannot see it in your
> > description.
>
> > Boris Kolpackov <boris@codesynthesis.com> wrote:
> >
> > > 1. Add 'warning' word to $(warning-if) output:
>
> This will make the diagnostics consistent with other places in kconfig
> where warnings are issued (see conf_warrning() in confdata.c).


'warning:' from C code and $(warning-if) are implemented
in different layers. So, I do not think it is necessary
to prepend 'warning:'.



More importantly, I cannot find a good way to print
multiple lines of error messages when $(error ...) is hit.
I prefer wrapping a long message in 80-columns.


After all, the best way I came up with for GNU Make is to use
$(error ) for the last line, and $(warning ) for the rest.

$(warning This is the first line)
$(warning This is the second line)
$(error This is the last line)


masahiro@grover:~$ make
Makefile:1: This is the first line
Makefile:2: This is the second line
Makefile:3: *** This is the last line.  Stop.



This works, except the small flaw, "***".



What if GNU Make prepended 'warning:' and 'error:'
as you suggest?


Makefile:1: warning: This is the first line
Makefile:2: warning: This is the second line
Makefile:3: error: *** This is the last line.  Stop.

This is even odd since I just want to split the message
into multiple lines.

If you want, you can include 'warning: ' in the message,
but you would not be able to get rid of it if it were
printed by default.

So, I do not want to add unwanted prefixes.


In a little more thought, I'd rather go opposite;
make $(warning-if) and $(error-if) as simple as
just printing the given message without any prefix.

https://patchwork.kernel.org/project/linux-kbuild/patch/20201221094650.283511-1-masahiroy@kernel.org/



>
> > > 2. Print $(info) output to stderr instead of stdout.
>
> There are two reasons:
>
> 1. Error, warning, and info are different diagnostics levels. It was
>    surprising to me that the first two go to stderr while info goes
>    to stdout. For example, as a user, if I redirect stderr, I would
>    naturally expect all the diagnostics to go there.
>
> 2. More importantly, stdout is used by terminal-based UI configurators.
>    So depending on exactly when $(info) is issued, its output could either
>    be clobbered by UI (so the user won't notice it) or it can clobber UI
>    (so the user will see broken UI).


I do not think this is overly problematic
because Kconfig enters the GUI mode after
parsing all Kconfig files.

Also, if my new patch lands, the format from $(info) and $(warning-if )
will be the same.
If we directed the $(info ) to stderr,
$(info ) and $(warning-if ) will be completely equivalent, and
we will lose the reason to have $(info ).



--
Best Regards
Masahiro Yamada

Re: kconfig: diagnostics cleanups

[Boris Kolpackov]

Masahiro Yamada <masahiroy@kernel.org> writes:

> On Wed, Dec 2, 2020 at 5:06 PM Boris Kolpackov <boris@codesynthesis.com> wrote:
>
> > > Boris Kolpackov <boris@codesynthesis.com> wrote:
> > >
> > > > 1. Add 'warning' word to $(warning-if) output:
> >
> > This will make the diagnostics consistent with other places in kconfig
> > where warnings are issued (see conf_warrning() in confdata.c).
> 
> 
> 'warning:' from C code and $(warning-if) are implemented
> in different layers. So, I do not think it is necessary
> to prepend 'warning:'.

What does it matter to the user, who sees the inconsistent
diagnostics, that they are implemented in different layers?


> More importantly, I cannot find a good way to print
> multiple lines of error messages when $(error ...) is hit.
> I prefer wrapping a long message in 80-columns.
> 
> 
> After all, the best way I came up with for GNU Make is to use
> $(error ) for the last line, and $(warning ) for the rest.
> 
> $(warning This is the first line)
> $(warning This is the second line)
> $(error This is the last line)
> 
> 
> masahiro@grover:~$ make
> Makefile:1: This is the first line
> Makefile:2: This is the second line
> Makefile:3: *** This is the last line.  Stop.
>  
> 
> This works, except the small flaw, "***".

IMO, there is a much bigger flaw: there is no way for the user
to know that these three lines are about the same error.

If you want this ability, then let's find a way do it properly
rather than spreading further hacks. For example, in the build
system I am working on, we have suport for multi-line diagnostics
records that to the user look like this:

Makefile:3: error: This is the first line
  This is the second line
  This is the last line

 
> If you want, you can include 'warning: ' in the message,
> but you would not be able to get rid of it if it were
> printed by default.

You can get rid of it by using $(info).

 
> In a little more thought, I'd rather go opposite;
> make $(warning-if) and $(error-if) as simple as
> just printing the given message without any prefix.
> 
> https://patchwork.kernel.org/project/linux-kbuild/patch/20201221094650.283511-1-masahiroy@kernel.org/

Wouldn't showing the position in the Kconfig file where
the error/warning has originated be much, much more useful
than the occasional need to print multi-line messages?


> > > > 2. Print $(info) output to stderr instead of stdout.
> >
> > There are two reasons:
> >
> > 1. Error, warning, and info are different diagnostics levels. It was
> >    surprising to me that the first two go to stderr while info goes
> >    to stdout. For example, as a user, if I redirect stderr, I would
> >    naturally expect all the diagnostics to go there.
> >
> > 2. More importantly, stdout is used by terminal-based UI configurators.
> >    So depending on exactly when $(info) is issued, its output could either
> >    be clobbered by UI (so the user won't notice it) or it can clobber UI
> >    (so the user will see broken UI).
> 
> 
> I do not think this is overly problematic
> because Kconfig enters the GUI mode after
> parsing all Kconfig files.

How is me (as a user) redirecting stderr to a location of
my choosing and only ending up with half of the diagnostics
there (with the other half silently overridden by the GUI)
not problematic?

 
> Also, if my new patch lands, [...]

I hope it does not.

In one of your emails you've said that you believe the kconfig
implementation is very immature (which I wholly agree with) and
you would like to clean it up. To me one of the biggest signs
of this immaturity is various inconsistencies and the changes I
am proposing were to address some of the more user-visible ones.
It's baffling to me that you not only find my changes unnecessary
but actually propose changes which in my view would only exacerbate
the problem. It seems we understand very differently what exactly
is immature about kconfig.

Re: kconfig: diagnostics cleanups

[Masahiro Yamada]

On Mon, Dec 21, 2020 at 11:05 PM Boris Kolpackov
<boris@codesynthesis.com> wrote:
>
> Masahiro Yamada <masahiroy@kernel.org> writes:
>
> > On Wed, Dec 2, 2020 at 5:06 PM Boris Kolpackov <boris@codesynthesis.com> wrote:
> >
> > > > Boris Kolpackov <boris@codesynthesis.com> wrote:
> > > >
> > > > > 1. Add 'warning' word to $(warning-if) output:
> > >
> > > This will make the diagnostics consistent with other places in kconfig
> > > where warnings are issued (see conf_warrning() in confdata.c).
> >
> >
> > 'warning:' from C code and $(warning-if) are implemented
> > in different layers. So, I do not think it is necessary
> > to prepend 'warning:'.
>
> What does it matter to the user, who sees the inconsistent
> diagnostics, that they are implemented in different layers?
>
>
> > More importantly, I cannot find a good way to print
> > multiple lines of error messages when $(error ...) is hit.
> > I prefer wrapping a long message in 80-columns.
> >
> >
> > After all, the best way I came up with for GNU Make is to use
> > $(error ) for the last line, and $(warning ) for the rest.
> >
> > $(warning This is the first line)
> > $(warning This is the second line)
> > $(error This is the last line)
> >
> >
> > masahiro@grover:~$ make
> > Makefile:1: This is the first line
> > Makefile:2: This is the second line
> > Makefile:3: *** This is the last line.  Stop.
> >
> >
> > This works, except the small flaw, "***".
>
> IMO, there is a much bigger flaw: there is no way for the user
> to know that these three lines are about the same error.


It is clear from the context.

For example, this case.

https://github.com/torvalds/linux/blob/v5.3/Makefile#L213


masahiro@grover:~/ref/linux$ make SUBDIRS=drivers  -j24
Makefile:213: ================= WARNING ================
Makefile:214: 'SUBDIRS' will be removed after Linux 5.3
Makefile:215:
Makefile:216: If you are building an individual subdirectory
Makefile:217: in the kernel tree, you can do like this:
Makefile:218: $ make path/to/dir/you/want/to/build/
Makefile:219: (Do not forget the trailing slash)
Makefile:220:
Makefile:221: If you are building an external module,
Makefile:222: Please use 'M=' or 'KBUILD_EXTMOD' instead
Makefile:223: ==========================================

  ERROR: Kernel configuration is invalid.
         include/generated/autoconf.h or include/config/auto.conf are missing.
         Run 'make oldconfig && make prepare' on kernel src to fix it.

make: *** [Makefile:686: include/config/auto.conf] Error 1



The filename:lineno prefixes are noisy, but it is clear enough
for me to understand.




> If you want this ability, then let's find a way do it properly
> rather than spreading further hacks. For example, in the build
> system I am working on, we have suport for multi-line diagnostics
> records that to the user look like this:
>
> Makefile:3: error: This is the first line
>   This is the second line
>   This is the last line

I also thought about this possibility.

define newline


endef


$(warning This is first line$(newline) \
  This is the second line$(newline) \
  This is the last line\
)


masahiro@grover:~$ make
Makefile:7: This is first line
 This is the second line
 This is the last line


But, I do not like this format.

I do not want to have a complex macro
to fix the indentation, either.



>
> > If you want, you can include 'warning: ' in the message,
> > but you would not be able to get rid of it if it were
> > printed by default.
>
> You can get rid of it by using $(info).
>
>
> > In a little more thought, I'd rather go opposite;
> > make $(warning-if) and $(error-if) as simple as
> > just printing the given message without any prefix.
> >
> > https://patchwork.kernel.org/project/linux-kbuild/patch/20201221094650.283511-1-masahiroy@kernel.org/
>
> Wouldn't showing the position in the Kconfig file where
> the error/warning has originated be much, much more useful
> than the occasional need to print multi-line messages?


The idea in my mind is to give each build-in function
as small functionality as possible.

Since Kconfig already supports $(filename) and $(lineno).

The current functionality can be achieved by a macro.




>
> > > > > 2. Print $(info) output to stderr instead of stdout.
> > >
> > > There are two reasons:
> > >
> > > 1. Error, warning, and info are different diagnostics levels. It was
> > >    surprising to me that the first two go to stderr while info goes
> > >    to stdout. For example, as a user, if I redirect stderr, I would
> > >    naturally expect all the diagnostics to go there.
> > >
> > > 2. More importantly, stdout is used by terminal-based UI configurators.
> > >    So depending on exactly when $(info) is issued, its output could either
> > >    be clobbered by UI (so the user won't notice it) or it can clobber UI
> > >    (so the user will see broken UI).
> >
> >
> > I do not think this is overly problematic
> > because Kconfig enters the GUI mode after
> > parsing all Kconfig files.
>
> How is me (as a user) redirecting stderr to a location of
> my choosing and only ending up with half of the diagnostics
> there (with the other half silently overridden by the GUI)
> not problematic?

This is a hypothetical argument since we have no user of $(info).
$(info) is not a warning.
The change is unneeded.



>
> > Also, if my new patch lands, [...]
>
> I hope it does not.
>
> In one of your emails you've said that you believe the kconfig
> implementation is very immature (which I wholly agree with) and
> you would like to clean it up. To me one of the biggest signs
> of this immaturity is various inconsistencies and the changes I
> am proposing were to address some of the more user-visible ones.
> It's baffling to me that you not only find my changes unnecessary
> but actually propose changes which in my view would only exacerbate
> the problem. It seems we understand very differently what exactly
> is immature about kconfig.

Probably so.


-- 
Best Regards
Masahiro Yamada

Re: kconfig: diagnostics cleanups

[Boris Kolpackov]

Masahiro Yamada <masahiroy@kernel.org> writes:

> On Mon, Dec 21, 2020 at 11:05 PM Boris Kolpackov <boris@codesynthesis.com> wrote:
> 
> > If you want this ability, then let's find a way do it properly
> > rather than spreading further hacks. For example, in the build
> > system I am working on, we have suport for multi-line diagnostics
> > records that to the user look like this:
> >
> > Makefile:3: error: This is the first line
> >   This is the second line
> >   This is the last line
> 
> I also thought about this possibility.
> 
> define newline
> 
> 
> endef
> 
> 
> $(warning This is first line$(newline) \
>   This is the second line$(newline) \
>   This is the last line\
> )

Or we could extend these functions to accept additional lines of
diagnostics as additional arguments:

$(warning This is first line,\
This is the second line,\
This is the last line)


> > How is me (as a user) redirecting stderr to a location of
> > my choosing and only ending up with half of the diagnostics
> > there (with the other half silently overridden by the GUI)
> > not problematic?
> 
> This is a hypothetical argument since we have no user of $(info).
> $(info) is not a warning.
> The change is unneeded.

There is no use in the Linux kernel but there could be in other
projects that re-use the kconfig machinery. By narrowly focusing
only on the kernel's needs to the point of ignoring bugs that are
not triggered, you leave such projects no choice but to fork. While
this attitude means less work for you in the short term, it also
makes authors of such projects less interested in contributing
anything back. For example, I have a fix for SIGSEGV sitting in
my branch that I have very little incentive to try to upstream
since I will now be maintaining other fixes out of tree and one
more doesn't really make any difference.