Re: [ptxdist] [PATCH 7/7] ptxdist: add 'init' commands to create new configs

[Michael Olbrich <m.olbrich@pengutronix.de>]

On Wed, Sep 23, 2020 at 02:22:59PM +0200, Roland Hieber wrote:
> On Tue, Sep 22, 2020 at 11:46:22AM +0200, Michael Olbrich wrote:
> > On Tue, Sep 22, 2020 at 12:17:28AM +0200, Roland Hieber wrote:
> > > The goal here is to give users a way to create a working BSP or platform
> > > from scratch by asking only a minimal set of relevant questions and
> > > letting the user press Enter about 15 times, instead of asking every
> > > question in the whole platform and ptxconfig menu, which would require
> > > pressing Enter about 428 times.
> > > 
> > > For that, we create two separate Kconfig menus, rules/Kconfig-init for
> > > the ptxconfig, and platforms/Kconfig-init for the platformconfig, which
> > > include only the relevant .in files from the full menus. Still, some
> > > options from these .in files can be set to reasonable defaults for new
> > > BSPs or platforms, which is done with two new defconfig files instead of
> > > further outsourcing the relevant config options into single .in files.
> > > 
> > > With the 'init' subcommand, PTXdist first creates a standard directory
> > > structure under configs/ if necessary, then creates a minimal config
> > > file from the respective defconfig, and finally calls kconfig on the
> > > respective Kconfig-init menu, asking the user for the remaining config
> > > options. A silenced oldconfig run on the full menu file then takes care
> > > to set all other config options to their defaults.
> > > 
> > > Signed-off-by: Roland Hieber <rhi@pengutronix.de>
> > > ---
> > >  bin/ptxdist                     | 46 ++++++++++++++++
> > >  doc/ref_parameter.rst           |  5 ++
> > >  platforms/Kconfig-init          | 13 +++++
> > >  platforms/init_defconfig        | 18 +++++++
> > >  rules/Kconfig-init              | 62 +++++++++++++++++++++
> > >  rules/init_defconfig            | 21 ++++++++
> > >  rules/initmethod.in             |  3 ++
> > >  scripts/lib/ptxd_lib_init.sh    | 96 +++++++++++++++++++++++++++++++++
> > >  scripts/lib/ptxd_lib_kconfig.sh | 10 ++++
> > >  9 files changed, 274 insertions(+)
> > >  create mode 100644 platforms/Kconfig-init
> > >  create mode 100644 platforms/init_defconfig
> > >  create mode 100644 rules/Kconfig-init
> > >  create mode 100644 rules/init_defconfig
> > >  create mode 100644 scripts/lib/ptxd_lib_init.sh
> > 
> > Hmmm, this name seems a bit too generic. Maybe ptxd_lib_bsp_init.sh
> 
> Yeah, seems reasonable to me.
> 
> > > diff --git a/bin/ptxdist b/bin/ptxdist
> > > index c2cf744f70a8..a3d2f0deef56 100755
> > > --- a/bin/ptxdist
> > > +++ b/bin/ptxdist
> > > @@ -687,6 +687,9 @@ PTXdist $(printf "%-24s" ${PTXDIST_VERSION_FULL}) Build System for Embedded Linu
> > >  
> > >  Setup and Project Actions:
> > >  
> > > +  init [<platformname>]		initialise a new BSP in the current directory
> > > +  init platform [<name>]	initialise a new platform in the current BSP
> > > +
> > >    menu				enter main control menu
> > >  
> > >    setup				setup per-user preferences
> > > @@ -1803,6 +1806,49 @@ EOF
> > >  			do_${cmd}
> > >  			exit
> > >  			;;
> > > +		init)
> > > +			local do_ptxconfig=1
> > > +			if [ "$1" = "platform" ]; then
> > > +				shift
> > > +				do_ptxconfig=
> > > +			fi
> > > +
> > > +			local platform="$1"
> > > +			if [ -z "$platform" ]; then
> > > +				echo
> > > +				echo "${PTXDIST_LOG_PROMPT}error: Please specify a platform name."
> > > +				echo
> > > +				exit 1
> > > +			fi
> > > +
> > > +			PTXDIST_PLATFORMCONFIG="configs/platform-${platform}/platformconfig"
> > > +			PTXDIST_PTXCONFIG="configs/ptxconfig"
> > > +
> > > +			# abort early
> > > +			ptxd_lib_init_platform_check &&
> > > +			if [ -n "$do_ptxconfig" ]; then
> > > +				ptxd_lib_init_check
> > > +			fi &&
> > > +
> > > +			ptxd_lib_init_platform "${platform}" &&
> > > +			if [ -n "$do_ptxconfig" ]; then
> > > +				ptxd_lib_init
> > > +			fi &&
> > > +
> > > +			if [ -n "$do_ptxconfig" ]; then
> > > +				ptxd_dialog_msgbox \
> > > +					"Adapt the new BSP to your needs by running:\n\n" \
> > > +					"    ptxdist menuconfig\n" \
> > > +					"    ptxdist menuconfig platform\n" \
> > > +					"    ptxdist menuconfig kernel"
> > > +			else
> > > +				ptxd_dialog_msgbox \
> > > +					"Adapt the new platform to your needs by running:\n\n" \
> > > +					"    ptxdist menuconfig platform\n" \
> > > +					"    ptxdist menuconfig kernel"
> > > +			fi
> > 
> > Put this stuff in a function too. And in scripts/lib/ as well. I try to
> > avoid adding new stuff in the ptxdist script because that part cannot be
> > overwritten in a BSP.
> 
> Ah, good idea, I forgot that rationale.
> 
> > > +			exit
> > > +			;;
> > >  		image)
> > >  			PTXDIST_OPTIMIZE_IO=true
> > >  			if [ ${#} -eq 0 ]; then
> > > diff --git a/doc/ref_parameter.rst b/doc/ref_parameter.rst
> > > index ac1e06c5335e..edd489af831f 100644
> > > --- a/doc/ref_parameter.rst
> > > +++ b/doc/ref_parameter.rst
> > > @@ -12,6 +12,11 @@ PTXdist is a command line tool, which is basically called as:
> > >  Setup and Project Actions
> > >  ~~~~~~~~~~~~~~~~~~~~~~~~~
> > >  
> > > +``init <platformname>``, ``init platform <platformname>``
> > > +  initialise a new BSP in the current directory, or add a new platform to the
> > > +  current BSP. This action creates all required config files, and then calls
> > > +  *menuconfig* on them, and can be used to start a new BSP from scratch.
> > > +
> > >  ``menu``
> > >    this starts a dialog based frontend for those who do not like typing
> > >    commands. It will gain us access to the most common parameters to
> > > diff --git a/platforms/Kconfig-init b/platforms/Kconfig-init
> > > new file mode 100644
> > > index 000000000000..9e42556d77d3
> > > --- /dev/null
> > > +++ b/platforms/Kconfig-init
> > 
> > I think this (and the defconfig) should be somewhere in config/.
> 
> Not sure. I put it next to the already existing platform/Kconfig, which
> is used as the file_kconfig in ptxd_kconfig(), so I think it fits well
> here…

Maybe, but the default config really doesn't fit here. And I'd like to keep
them together.
Also, this is a rather special use-case and I'd like to keep rules/ and
platforms/ clean. There are enough files there already.


> > > @@ -0,0 +1,13 @@
> > > +#
> > > +# a kconfig menu structure that asks only a minimal amount of questions to
> > > +# quickly get started with a new platform
> > > +#
> > > +source "generated/platform_version.in"
> > > +source "platforms/architecture.in"
> > > +source "platforms/toolchain.in"
> > > +
> > > +# dependencies for kernel rule:
> > > +source "generated/hosttools_noprompt.in"
> > > +source "generated/hosttools_platform.in"
> > > +
> > > +source "platforms/kernel.in"
> > > diff --git a/platforms/init_defconfig b/platforms/init_defconfig
> > > new file mode 100644
> > > index 000000000000..5e44c536f314
> > > --- /dev/null
> > > +++ b/platforms/init_defconfig
> > > @@ -0,0 +1,18 @@
> > > +PTXCONF_PLATFORM_VERSION="-${PTXDIST_BSP_AUTOVERSION}"
> > > +PTXCONF_COMPILER_PREFIX_KERNEL="${PTXCONF_COMPILER_PREFIX}"
> > > +PTXCONF_COMPILER_PREFIX_BOOTLOADER="${PTXCONF_COMPILER_PREFIX}"
> > > +PTXCONF_LIBC_GLIBC=y
> > > +# PTXCONF_LIBC_UCLIBC is not set
> > > +PTXCONF_KERNEL=y
> > > +# PTXCONF_KERNEL_MODULES is not set
> > 
> > No modules by default? This seems wrong.
> 
> Modules don't seem to be frequently used in the embedded world. But I
> guess we can leave it out of the defconfig and ask for it instead.

In my experience, even in the embedded world most devices use modules. And
any kernel defconfig enables modules as well.

> > > +# PTXCONF_KERNEL_ZSTD is not set
> > > +# PTXCONF_KERNEL_XZ is not set
> > > +# PTXCONF_KERNEL_LZOP is not set
> > > +# PTXCONF_KERNEL_LZ4 is not set
> > > +# PTXCONF_KERNEL_OPENSSL is not set
> > > +# PTXCONF_KERNEL_LIBELF is not set
> > 
> > Keeping all of those off is just asking for trouble.
> > e.g. PTXCONF_KERNEL_LIBELF is needed by default on x86
> 
> OK, I'll have to look into this. Since the platformconfig is added
> before the kernel is configured, I'm not really sure what to do here if
> users configure their kernel e.g. to produce signed images, but
> KERNEL_OPENSSL is disabled in platformconfig. They would need to enable
> that afterwards anyways. Unless we just enable all those KERNEL_* host
> tools options unconditionally, which will always do the right thing,
> although lead to a longer build time.

Just ask the user. This really must be configured correctly. These options
exist for a reason.

> > > +# PTXCONF_KERNEL_GCC_PLUGINS is not set
> > 
> > This seems wrong. If you don't care about plugins, then they should be
> > disabled. Otherwise the question should be explicit.
> 
> OK.
> 
> > > +# PTXCONF_KERNEL_CONFIG_BASE_VERSION is not set
> > > +PTXCONF_KERNEL_SERIES="series"
> > > +PTXCONF_KERNEL_CONFIG="kernelconfig-${PTXCONF_KERNEL_VERSION}"
> > 
> > I thought I changed that default. Please change this and the default value
> > to just "kernelconfig". The version just causes problems in most cases.
> 
> Oh, right. I remember a discussion about it.
> 
> > > +PTXCONF_KERNEL_EXTRA_MAKEVARS=""
> > > diff --git a/rules/Kconfig-init b/rules/Kconfig-init
> > > new file mode 100644
> > > index 000000000000..2355d070de9b
> > > --- /dev/null
> > > +++ b/rules/Kconfig-init
> > 
> > Also in config/...
> 
> Right, after having a look at ptxd_kconfig() again, this makes sense to
> me now.
> 
> > > @@ -0,0 +1,62 @@
> > > +#
> > > +# a kconfig menu structure that asks only a minimal amount of questions to
> > > +# quickly get started with a new BSP
> > > +#
> > > +source "rules/ptxdist-dgen.in"
> > > +source "rules/ptxdist-version.in"
> > > +source "generated/ptxdist_options.in"
> > > +
> > > +source "rules/project-name.in"
> > > +
> > > +#
> > > +# stubs for options that we want to ask for or enable by default, but not ask
> > > +# for all their suboptions
> > > +#
> > > +
> > > +config BUSYBOX
> > > +	default y
> > > +	bool
> > > +
> > > +# busybox installs init, poweroff, halt etc. by default. This conflicts with
> > > +# systemd, but since busybox brings its own kconfig menu in-tree, we cannot
> > > +# easily influence its defaults. Therefore we first disable those options in
> > > +# init_defconfig, and explicitely select them when needed.
> > > +# Note: these option need to have a prompt, otherwise kconfig doesn't write them
> > > +# to the config file if they are disabled, and busybox's default will kick in.
> > > +config BUSYBOX_HALT
> > > +	bool "busybox /usr/sbin/halt"
> > > +config BUSYBOX_INIT
> > > +	bool "busybox /usr/sbin/init"
> > > +config BUSYBOX_POWEROFF
> > > +	bool "busybox /usr/sbin/poweroff"
> > > +config BUSYBOX_REBOOT
> > > +	bool "busybox /usr/sbin/reboot"
> > > +config BUSYBOX_RUNLEVEL
> > > +	bool "busybox /usr/sbin/runlevel"
> > 
> > I think we should look at my experiments to patch the busybox Kconfig files
> > during import. We cannot change the dependencies, but changing the defaults
> > is ok.
> > But I'm not sure if my sed scripting is enough for this.
> 
> Are there patches anywhere I can look at it? :-)

Hmmm, it's been a while. I need to find that branch again...

> > > +choice
> > > +	prompt "init method"
> > > +	default INITMETHOD_SYSTEMD
> > > +
> > > +	config INITMETHOD_BBINIT
> > > +		bool
> > > +		prompt "busybox init    "
> > > +		select BUSYBOX_HALT
> > > +		select BUSYBOX_INIT
> > > +		select BUSYBOX_POWEROFF
> > > +		select BUSYBOX_REBOOT
> > > +		select BUSYBOX_RUNLEVEL
> > > +
> > > +	config INITMETHOD_SYSTEMD
> > > +		bool
> > > +		prompt "systemd         "
> > > +
> > > +	config INITMETHOD_INITRAMFS
> > > +		bool
> > > +		prompt "initramfs init  "
> > > +		select BUSYBOX_HALT
> > > +		select BUSYBOX_INIT
> > > +		select BUSYBOX_POWEROFF
> > > +		select BUSYBOX_REBOOT
> > > +		select BUSYBOX_RUNLEVEL
> > 
> > I don't think the dependencies make sense here.
> 
> I have no experience with initramfs-tools to be honest, but who will
> supply /sbin/halt, /sbin/poweroff etc. in this case if not busybox?

initramfs-tools provide /init and that is used anyways when starting with
an initramfs.

> > > +endchoice
> > > diff --git a/rules/init_defconfig b/rules/init_defconfig
> > > new file mode 100644
> > > index 000000000000..9fa9ab00a60b
> > > --- /dev/null
> > > +++ b/rules/init_defconfig
> > > @@ -0,0 +1,21 @@
> > > +# PTXCONF_ALLYES is not set
> > > +# PTXCONF_BROKEN is not set
> > > +PTXCONF_FIX_PERMISSIONS=y
> > > +PTXCONF_PROJECT_VERSION="-${PTXDIST_BSP_AUTOVERSION}"
> > > +# PTXCONF_PROJECT_CREATE_DEVPKGS is not set
> > > +# PTXCONF_PROJECT_USE_DEVPKGS is not set
> > > +PTXCONF_PROJECT_DEVPKGDIR=""
> > > +PTXCONF_PROJECT_DEVMIRROR=""
> > > +# PTXCONF_PROJECT_CHECK_LICENSES is not set
> > > +PTXCONF_REPRODUCIBLE_TIMESTAMP_PTXDIST=y
> > > +# PTXCONF_REPRODUCIBLE_TIMESTAMP_TOOLCHAIN is not set
> > > +# PTXCONF_REPRODUCIBLE_TIMESTAMP_CUSTOM is not set
> > > +# PTXCONF_REPRODUCIBLE_TIMESTAMP_STRING is not set
> > > +PTXCONF_DEBUG_PACKAGES=y
> > > +# initmethod will enable the following busybox tools if needed:
> > > +# PTXCONF_BUSYBOX_HALT is not set
> > > +# PTXCONF_BUSYBOX_INIT is not set
> > > +# PTXCONF_BUSYBOX_POWEROFF is not set
> > > +# PTXCONF_BUSYBOX_REBOOT is not set
> > > +# PTXCONF_BUSYBOX_RUNLEVEL is not set
> > > +# PTXCONF_STAGING is not set
> > 
> > How can we keep these files up-to-date?
> 
> I guess like for everything else: patches or bug reports if someone
> notices that it's broken. And the only way it can break will just lead
> to more unnecessary questions being asked, which is a bit annoying but
> not a tragedy, or defaults from the Kconfig options being used, which
> should be fine.

This just helps for new options, not old ones. Those just pile up.

> Alternatively I think kconfig has a "save_defconfig" function which
> could help us.

The problem is that we want to provide defaults only for some of the
options.

> > > diff --git a/rules/initmethod.in b/rules/initmethod.in
> > > index b8c0b9f97b30..604fae3b4f7b 100644
> > > --- a/rules/initmethod.in
> > > +++ b/rules/initmethod.in
> > > @@ -10,6 +10,9 @@ menuconfig INITMETHOD
> > >  if INITMETHOD
> > >  
> > >  choice
> > > +	#
> > > +	# NOTE: if you add options here, also add them to rules/init_defconfig
> > > +	#
> 
> Or should I add more comments like this…?

I don't think that will help.

> > >  	prompt "init method"
> > >  	default INITMETHOD_SYSTEMD
> > >  
> > > diff --git a/scripts/lib/ptxd_lib_init.sh b/scripts/lib/ptxd_lib_init.sh
> > > new file mode 100644
> > > index 000000000000..54fcb4d27eee
> > > --- /dev/null
> > > +++ b/scripts/lib/ptxd_lib_init.sh
> > > @@ -0,0 +1,96 @@
> > > +#!/bin/bash
> > > +
> > > +ptxd_lib_init_check() {
> > > +	if [ -z "${PTXDIST_PTXCONFIG}" ]; then
> > > +		echo
> > > +		echo "${PTXDIST_LOG_PROMPT}error: cannot create empty ptxconfig file."
> > > +		echo
> > > +		return 1
> > > +	fi
> > > +
> > > +	if [ -z "${PTXDIST_FORCE}" ] && [ -e "${PTXDIST_PTXCONFIG}" ]; then
> > > +		echo
> > > +		echo "${PTXDIST_LOG_PROMPT}error: the file '${PTXDIST_PTXCONFIG}' already exists,"
> > > +		echo "${PTXDIST_LOG_PROMPT}       use '--force' to overwrite it."
> > > +		echo
> > > +		return 1
> > > +	fi
> > > +
> > > +	if [ -z "${PTXDIST_FORCE}" ] && [ -e "${PTXDIST_PTXCONFIG_DEFAULT}" ]; then
> > > +		echo
> > > +		echo "${PTXDIST_LOG_PROMPT}error: the file '${PTXDIST_PTXCONFIG_DEFAULT}' already exists,"
> > > +		echo "${PTXDIST_LOG_PROMPT}       use '--force' to overwrite it."
> > > +		echo
> > > +		return 1
> > > +	fi
> > > +}
> > > +export -f ptxd_lib_init_check
> > > +
> > > +ptxd_lib_init() {
> > > +	ptxd_lib_init_check &&
> > > +
> > > +	mkdir -p "$(dirname "${PTXDIST_PTXCONFIG}")" &&
> > > +	cat "${PTXDIST_TOPDIR}/rules/init_defconfig" > "${PTXDIST_PTXCONFIG}" &&
> > > +
> > > +	PTXDIST_FORCE=true do_select ptxconfig "${PTXDIST_PTXCONFIG}" &&
> > > +
> > > +	# only ask the important questions first, then do silentdefconfig for
> > > +	# all other options
> > > +	PTXDIST_DEP_TARGET="all" ptxd_kconfig oldconfig "init-ptx" &&
> > > +
> > > +	# ... then do a silent defconfig for all other defaults
> > > +	PTXDIST_DEP_TARGET="all" PTXDIST_FORCE=true \
> > > +		yes '' | ptxd_kconfig oldconfig ptx > /dev/null
> > 
> > I think we can use alldefconfig here.
> 
> I had this in my first draft, but alldefconfig will reset all previously
> pre-primed options from the init_defconfig to their respective defaults
> as defined in Kconfig, and we don't want that here.

You need to set:

KCONFIG_ALLCONFIG=/full/path/to/initial/config

Michael

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
ptxdist mailing list
ptxdist@pengutronix.de
To unsubscribe, send a mail with subject "unsubscribe" to ptxdist-request@pengutronix.de