| Message ID | CAK7LNASGqfMkTuzP28qydpYCC0ct3cAgMpbPpmgHuQHZbtLhbA@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [GIT,PULL] Kbuild fixes for v6.8-rc3 | expand |
On Thu, 1 Feb 2024 at 05:40, Masahiro Yamada <masahiroy@kernel.org> wrote: > > - Replace tabs with spaces when followed by conditionals for > future GNU Make versions This is horrid. Now, the whole "whitespace type matters" is broken in Make anyway, so clearly this is a fundamental make problem, but this commit makes things worse by making the tab replacement use eight spaces, so it really visually is entirely indistinguishable. Don't make a 'make' problem worse by not visually distinguishing tabs from spaces. IOW, those "that can't be a tab" cases should have used pretty much _anything_ but 8 spaces. Yes on indentation of nested 'if' statements, but no on then using something that visually makes no sense. IOW, those nested if-statements should use perhaps just 2-4 spaces instead. That tends to match what we sometimes see in C files too, and it is visually very clearly not a tab with the kernel coding rules (yes, yes, some people set tabstops to smaller values, that's _their_ problem). I've pulled this, but please fix it, and don't make an insane Makefile whitespace situation worse. Linus
The pull request you sent on Thu, 1 Feb 2024 22:39:45 +0900:
> git://git.kernel.org/pub/scm/linux/kernel/git/masahiroy/linux-kbuild.git tags/kbuild-fixes-v6.8
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/a412682659b3832ac6f1a301e1c147027926f605
Thank you!
Hi Linus, On Fri, Feb 2, 2024 at 5:06 AM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > On Thu, 1 Feb 2024 at 05:40, Masahiro Yamada <masahiroy@kernel.org> wrote: > > > > - Replace tabs with spaces when followed by conditionals for > > future GNU Make versions > > This is horrid. > > Now, the whole "whitespace type matters" is broken in Make anyway, so > clearly this is a fundamental make problem, but this commit makes > things worse by making the tab replacement use eight spaces, so it > really visually is entirely indistinguishable. > > Don't make a 'make' problem worse by not visually distinguishing tabs > from spaces. > > IOW, those "that can't be a tab" cases should have used pretty much > _anything_ but 8 spaces. Yes on indentation of nested 'if' statements, > but no on then using something that visually makes no sense. > > IOW, those nested if-statements should use perhaps just 2-4 spaces > instead. That tends to match what we sometimes see in C files too, and > it is visually very clearly not a tab with the kernel coding rules > (yes, yes, some people set tabstops to smaller values, that's _their_ > problem). > > I've pulled this, but please fix it, and don't make an insane Makefile > whitespace situation worse. > > Linus Personally, I find 4 spaces more comfortable than 2, as the increased indentation enhances readability. When we have a build rule inside an if-block, we cannot indent the code. In such cases, we can add a comment after the closing 'endif' if it helps improve the readability, just like we often do for preprocessor conditionals in C files. So, the best consistency we can achieve is a combination of "4-space indentation" and "no indentation at all". I attached a patch to replace tab/8-space indentation. Probably, there are still unconverted conditionals, but I fixed all the code blocks touched by commit 82175d1f9430. Is this your expectation?
On Thu, 1 Feb 2024 at 15:57, Masahiro Yamada <masahiroy@kernel.org> wrote: > > Is this your expectation? Commit 82175d1f9430 touched *only* the nested 'if' indentations. Your attached changed other indentations too, which I am not sure makes any sense. But honestly, that whole make rule wrt whitespace makes no sense to begin with, and I don't know why the conditional statement is so special to begin with, and why GNU make would then suddenly start messing with an insane rule with bad historical reasons. End result: all of this just reinforces how bad the Make rules for whitespace is, but I would suggest doing the *minimal* changes to make it work. Which commit 82175d1f9430 did, but your attached patch then does not. IOW, if the whole crazy makefile whitespace change was only about conditionals, let's keep all the stupid whitespace fixups as purely about conditionals too. Linus
Hello Linus, On Fri, Feb 2, 2024 at 9:43 AM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > On Thu, 1 Feb 2024 at 15:57, Masahiro Yamada <masahiroy@kernel.org> wrote: > > > > Is this your expectation? > > Commit 82175d1f9430 touched *only* the nested 'if' indentations. > > Your attached changed other indentations too, which I am not sure > makes any sense. > > But honestly, that whole make rule wrt whitespace makes no sense to > begin with, and I don't know why the conditional statement is so > special to begin with, and why GNU make would then suddenly start > messing with an insane rule with bad historical reasons. In my understanding, the GNU Make parser is confused with shell's 'else' keyword. So, GNU Make determined that 'else' indented with a tab is not the Make's conditional directive. > > End result: all of this just reinforces how bad the Make rules for > whitespace is, but I would suggest doing the *minimal* changes to make > it work. > > Which commit 82175d1f9430 did, but your attached patch then does not. > > IOW, if the whole crazy makefile whitespace change was only about > conditionals, let's keep all the stupid whitespace fixups as purely > about conditionals too. > > Linus > I attached a new patch. I only changed the lines touch by 82175d1f9430
Hello Linus, On Fri, Feb 2, 2024 at 11:15 AM Masahiro Yamada <masahiroy@kernel.org> wrote: > > Hello Linus, > > > On Fri, Feb 2, 2024 at 9:43 AM Linus Torvalds > <torvalds@linux-foundation.org> wrote: > > > > On Thu, 1 Feb 2024 at 15:57, Masahiro Yamada <masahiroy@kernel.org> wrote: > > > > > > Is this your expectation? > > > > Commit 82175d1f9430 touched *only* the nested 'if' indentations. > > > > Your attached changed other indentations too, which I am not sure > > makes any sense. > > > > But honestly, that whole make rule wrt whitespace makes no sense to > > begin with, and I don't know why the conditional statement is so > > special to begin with, and why GNU make would then suddenly start > > messing with an insane rule with bad historical reasons. > > > In my understanding, the GNU Make parser is confused with > shell's 'else' keyword. > > So, GNU Make determined that 'else' indented with a tab > is not the Make's conditional directive. > > > > > > End result: all of this just reinforces how bad the Make rules for > > whitespace is, but I would suggest doing the *minimal* changes to make > > it work. > > > > Which commit 82175d1f9430 did, but your attached patch then does not. > > > > IOW, if the whole crazy makefile whitespace change was only about > > conditionals, let's keep all the stupid whitespace fixups as purely > > about conditionals too. > > > > Linus > > > > > I attached a new patch. > I only changed the lines touch by 82175d1f9430 Is the second patch fine with you? If so, will you pick it up, or do you want me to include it in the next pull request?
On Sun, 4 Feb 2024 at 00:00, Masahiro Yamada <masahiroy@kernel.org> wrote: > > Is the second patch fine with you? Yes. > If so, will you pick it up, or do you want me > to include it in the next pull request? This is not critical enough to bypass the regular pull chain, so just put it in your queue, and I'll get it with the next pull. Linus
Hello Linus, Please pull Kbuild fixes for v6.8-rc3. Thanks. The following changes since commit 6613476e225e090cc9aad49be7fa504e290dd33d: Linux 6.8-rc1 (2024-01-21 14:11:32 -0800) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/masahiroy/linux-kbuild.git tags/kbuild-fixes-v6.8 for you to fetch changes up to bfef491df67022c56aab3b831044f8d259f9441f: kconfig: initialize sym->curr.tri to 'no' for all symbol types again (2024-01-31 23:59:42 +0900) ---------------------------------------------------------------- Kbuild fixes for v6.8 - Fix UML build with clang-18 and newer - Avoid using the alias attribute in host programs - Replace tabs with spaces when followed by conditionals for future GNU Make versions - Fix rpm-pkg for the systemd-provided kernel-install tool - Fix the undefined behavior in Kconfig for a 'int' symbol used in a conditional ---------------------------------------------------------------- Dmitry Goncharov (1): kbuild: Replace tabs with spaces when followed by conditionals Jose Ignacio Tornos Martinez (1): kbuild: rpm-pkg: simplify installkernel %post Masahiro Yamada (3): kbuild: fix W= flags in the help message modpost: avoid using the alias attribute kconfig: initialize sym->curr.tri to 'no' for all symbol types again Nathan Chancellor (2): um: Fix adding '-no-pie' for clang modpost: Add '.ltext' and '.ltext.*' to TEXT_SECTIONS Zhang Bingwu (1): kbuild: defconf: use SRCARCH to find merged configs Makefile | 14 +++++++------- arch/m68k/Makefile | 4 ++-- arch/parisc/Makefile | 4 ++-- arch/um/Makefile | 4 +++- arch/x86/Makefile | 10 +++++----- scripts/Makefile.defconf | 8 ++++---- scripts/kconfig/symbol.c | 4 +++- scripts/mod/modpost.c | 15 +++------------ scripts/mod/modpost.h | 6 +----- scripts/package/kernel.spec | 22 +++++++++++----------- 10 files changed, 41 insertions(+), 50 deletions(-)