From: Luis Chamberlain <mcgrof@kernel.org>
To: Fan Ni <fan.ni@gmx.us>
Cc: nmtadam.samsung@gmail.com, fan.ni@samsung.com,
kdevops@lists.linux.dev, jlayton@kernel.org
Subject: Re: [PATCH v2 1/1] qemu-build: Add a qemu build option for enabling debug
Date: Fri, 25 Aug 2023 13:46:37 -0700 [thread overview]
Message-ID: <ZOkTLVh4uLwY87La@bombadil.infradead.org> (raw)
In-Reply-To: <ZOkOIhm9JfiGhSLu@debian>
On Fri, Aug 25, 2023 at 01:25:06PM -0700, Fan Ni wrote:
> > Note this is a bool, please rename it and add something like:
> >
> > qemu_build_debug_enable: False
> > qemu_build_debug_enable_string: "--enable-debug"
> > qemu_build_extra_strings: ""
>
> Do we really need to add two variables for that? It seems we add an
> unnecessary layer of check with qemu_build_debug_enable.
It is not needed.
The bool was more of a suggestion to minimize the effort required later
once we get kconfig to support outputting extra_vars.yaml for us.
If you do the string in the yaml default but not in Kconfig, it means we
have to keep around more clutter in the Makefiles. Where as if you do
it as I suggested, once we add support to kconfig to ouput both .config
and extra_vars.yaml automatically for us, then the entire set of Makefile
changes you add could be removed.
But on is just trading off Makefile edits to an ansible tasks so that is
also not as optimal.
Come to think of it, from an optimization point of view I think we would want
this to be in Kconfig as a string *if* the bool is set, because an ansible
task task that is exucted at runtime which means we're slowing things down,
whereas a Kconfig would be just at initial 'make' time.
So you could just add something like:
config FSTESTS_FSTYP
string
default "" if !DEBUG_BUILD_STUFF
default "--foo" if DEBUG_BUILD_STUFF
The Makefile then would set the string variable if its not empty.
Then later once kconfig gets "extra_vars.yaml" output support we just
remove the Makefile check as kconfig would do the setting for us. The
variable names just need to match for Kconfig and ansible.
So this is just *forward* thinking about the future of minizing changes
like these.
Luis
prev parent reply other threads:[~2023-08-25 20:46 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-08-25 18:17 [PATCH v2] Add configuration option to allow build qemu binary with fan.ni
2023-08-25 18:17 ` [PATCH v2 1/1] qemu-build: Add a qemu build option for enabling debug fan.ni
2023-08-25 19:11 ` Luis Chamberlain
2023-08-25 20:25 ` Fan Ni
2023-08-25 20:46 ` Luis Chamberlain [this message]
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=ZOkTLVh4uLwY87La@bombadil.infradead.org \
--to=mcgrof@kernel.org \
--cc=fan.ni@gmx.us \
--cc=fan.ni@samsung.com \
--cc=jlayton@kernel.org \
--cc=kdevops@lists.linux.dev \
--cc=nmtadam.samsung@gmail.com \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).