All of lore.kernel.org
 help / color / mirror / Atom feed
From: Thomas Petazzoni via buildroot <buildroot@buildroot.org>
To: Michael Trimarchi <michael@amarulasolutions.com>
Cc: "Yann E . MORIN" <yann.morin.1998@free.fr>, buildroot@buildroot.org
Subject: Re: [Buildroot] [PATCH V2] package: gdb: Add helping kernel gdb functions compilation
Date: Wed, 13 Dec 2023 21:08:04 +0100	[thread overview]
Message-ID: <20231213210804.39fe194f@windsurf> (raw)
In-Reply-To: <20231209214918.2071047-1-michael@amarulasolutions.com>

Hello Michael,

I think the commit title is not very good/clear, and could be improved.
However, I have some more fundamental concern. See below.

On Sat,  9 Dec 2023 22:49:18 +0100
Michael Trimarchi <michael@amarulasolutions.com> wrote:

> diff --git a/package/gdb/gdb.mk b/package/gdb/gdb.mk
> index 070598b385..0ebeb08dad 100644
> --- a/package/gdb/gdb.mk
> +++ b/package/gdb/gdb.mk
> @@ -304,5 +304,9 @@ HOST_GDB_POST_INSTALL_HOOKS += HOST_GDB_ADD_SYMLINK
>  
>  HOST_GDB_POST_INSTALL_HOOKS += gen_gdbinit_file
>  
> +define HOST_GDB_LINUX_CONFIG_FIXUPS
> +	$(call KCONFIG_ENABLE_OPT,CONFIG_GDB_SCRIPTS)
> +endef

I'm not sure I like this. <pkg>_LINUX_CONFIG_FIXUPS is really meant to
be used to enable kernel options that are really/absolutely needed for
the package to work. I understand that CONFIG_GDB_SCRIPTS is not really
adding anything to the kernel image itself, but rather just to the
kernel build process, but still I believe it falls outside of the goal
of <pkg>_LINUX_CONFIG_FIXUPS.

In addition:

- Many people will want to use gdb and build a Linux kernel, but not
  necessarily want to debug the kernel. So "enabling gdb" does not
  imply "I want to debug the kernel"

- This patch does not account for the fairly common case of host gdb
  being provided by an external toolchain. In this situation, the user
  still has to enable CONFIG_GDB_SCRIPTS=y "manually" in his kernel
  configuration.

For all those reasons, and even though the change is simple and does no
harm, I believe it's not really useful and aligned with the intention
of <pkg>_LINUX_CONFIG_FIXUPS.

So I would personally be in favor of not accepting this patch.

Best regards,

Thomas Petazzoni
-- 
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

  reply	other threads:[~2023-12-13 20:08 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-12-09 21:49 [Buildroot] [PATCH V2] package: gdb: Add helping kernel gdb functions compilation Michael Trimarchi
2023-12-13 20:08 ` Thomas Petazzoni via buildroot [this message]
2023-12-13 20:17   ` Michael Nazzareno Trimarchi
2023-12-13 20:24     ` Thomas Petazzoni via buildroot
2023-12-13 20:32       ` Michael Nazzareno Trimarchi
2023-12-13 20:41         ` Yann E. MORIN

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20231213210804.39fe194f@windsurf \
    --to=buildroot@buildroot.org \
    --cc=michael@amarulasolutions.com \
    --cc=thomas.petazzoni@bootlin.com \
    --cc=yann.morin.1998@free.fr \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.