Re: [PATCH v2 2/5] kconfig: refactor Qt package checks for building qconf

From: Sam Ravnborg
Date: Sun May 20 2018 - 04:50:59 EST

Next message: kbuild test robot: "Re: [PATCH V3 2/5] X86: Hyper-V: Enable IPI enlightenments"
Previous message: Jonathan Cameron: "Re: [PATCH] iio: tsl2583: correct values in integration_time_available"
In reply to: Masahiro Yamada: "[PATCH v2 2/5] kconfig: refactor Qt package checks for building qconf"
Next in thread: Masahiro Yamada: "[PATCH v2 4/5] kconfig: refactor ncurses package checks for building mconf"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Masahiro

This commit (and the rest of the series) do wonders for the
readability of the Makefile - nice work.

Some nits below.

On Sun, May 20, 2018 at 05:16:50PM +0900, Masahiro Yamada wrote:
> Currently, the necessary package checks for building qconf is
> surrounded by ifeq ($(MAKECMDGOALS),xconfig) ... endif.
> Then, Make will restart when .tmp_qtcheck is generated.
>
> To simplify the Makefile, move the scripting to a separate file,
> and use filechk. The shell script is executed everytime xconfig
> is run, but it is not a costly script.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@xxxxxxxxxxxxx>
> Tested-by: Randy Dunlap <rdunlap@xxxxxxxxxxxxx>
> Acked-by: Randy Dunlap <rdunlap@xxxxxxxxxxxxx>
> ---
>
> Changes in v2: None
>
> scripts/kconfig/Makefile | 73 +++++++++++++++++---------------------------
> scripts/kconfig/qconf-cfg.sh | 25 +++++++++++++++
> 2 files changed, 53 insertions(+), 45 deletions(-)
> create mode 100755 scripts/kconfig/qconf-cfg.sh
>
> diff --git a/scripts/kconfig/Makefile b/scripts/kconfig/Makefile
> index 5def877..e9a87bf 100644
> --- a/scripts/kconfig/Makefile
> +++ b/scripts/kconfig/Makefile
> @@ -188,8 +188,6 @@ HOST_EXTRACFLAGS += $(shell $(CONFIG_SHELL) $(check-lxdialog) -ccflags) \
> # Utilizes ncurses
> # mconf: Used for the menuconfig target
> # Utilizes the lxdialog package
> -# qconf: Used for the xconfig target
> -# Based on Qt which needs to be installed to compile it
> # gconf: Used for the gconfig target
> # Based on GTK+ which needs to be installed to compile it
> # object files used by all kconfig flavours
> @@ -201,14 +199,12 @@ conf-objs := conf.o zconf.tab.o
> mconf-objs := mconf.o zconf.tab.o $(lxdialog)
> nconf-objs := nconf.o zconf.tab.o nconf.gui.o
> kxgettext-objs := kxgettext.o zconf.tab.o
> -qconf-cxxobjs := qconf.o
> -qconf-objs := zconf.tab.o
> gconf-objs := gconf.o zconf.tab.o
>
> -hostprogs-y := conf nconf mconf kxgettext qconf gconf
> +hostprogs-y := conf nconf mconf kxgettext gconf
>
> targets += zconf.lex.c
> -clean-files := qconf.moc .tmp_qtcheck .tmp_gtkcheck
> +clean-files := .tmp_gtkcheck
> clean-files += gconf.glade.h
> clean-files += config.pot linux.pot
>
> @@ -228,9 +224,6 @@ HOST_EXTRACXXFLAGS += $(shell $(CONFIG_SHELL) $(srctree)/$(src)/check.sh $(HOSTC
> HOSTCFLAGS_zconf.lex.o := -I$(src)
> HOSTCFLAGS_zconf.tab.o := -I$(src)
>
> -HOSTLOADLIBES_qconf = $(KC_QT_LIBS)
> -HOSTCXXFLAGS_qconf.o = $(KC_QT_CFLAGS)
> -
> HOSTLOADLIBES_gconf = `pkg-config --libs gtk+-2.0 gmodule-2.0 libglade-2.0`
> HOSTCFLAGS_gconf.o = `pkg-config --cflags gtk+-2.0 gmodule-2.0 libglade-2.0` \
> -Wno-missing-prototypes
> @@ -241,34 +234,22 @@ HOSTLOADLIBES_nconf = $(shell \
> pkg-config --libs menuw panelw ncursesw 2>/dev/null \
> || pkg-config --libs menu panel ncurses 2>/dev/null \
> || echo "-lmenu -lpanel -lncurses" )
> -$(obj)/qconf.o: $(obj)/.tmp_qtcheck
> -
> -ifeq ($(MAKECMDGOALS),xconfig)
> -$(obj)/.tmp_qtcheck: $(src)/Makefile
> --include $(obj)/.tmp_qtcheck
> -
> -# Qt needs some extra effort...
> -$(obj)/.tmp_qtcheck:
> - @set -e; $(kecho) " CHECK qt"; \
> - if pkg-config --exists Qt5Core; then \
> - cflags="-std=c++11 -fPIC `pkg-config --cflags Qt5Core Qt5Gui Qt5Widgets`"; \
> - libs=`pkg-config --libs Qt5Core Qt5Gui Qt5Widgets`; \
> - moc=`pkg-config --variable=host_bins Qt5Core`/moc; \
> - elif pkg-config --exists QtCore; then \
> - cflags=`pkg-config --cflags QtCore QtGui`; \
> - libs=`pkg-config --libs QtCore QtGui`; \
> - moc=`pkg-config --variable=moc_location QtCore`; \
> - else \
> - echo >&2 "*"; \
> - echo >&2 "* Could not find Qt via pkg-config."; \
> - echo >&2 "* Please install either Qt 4.8 or 5.x. and make sure it's in PKG_CONFIG_PATH"; \
> - echo >&2 "*"; \
> - exit 1; \
> - fi; \
> - echo "KC_QT_CFLAGS=$$cflags" > $@; \
> - echo "KC_QT_LIBS=$$libs" >> $@; \
> - echo "KC_QT_MOC=$$moc" >> $@
> -endif
> +
> +# qconf: Used for the xconfig target based on Qt
> +hostprogs-y += qconf
> +qconf-cxxobjs := qconf.o
> +qconf-objs := zconf.tab.o
> +
> +HOSTLOADLIBES_qconf = $(shell . $(obj)/.qconf-cfg && echo $$libs)
> +HOSTCXXFLAGS_qconf.o = $(shell . $(obj)/.qconf-cfg && echo $$cflags)
Nice way to access the variables generated earlier.

> +
> +$(obj)/qconf.o: $(obj)/.qconf-cfg $(obj)/qconf.moc

qconf.moc has a dependency on qconf-cfg so the first
dependency could be dropped here I think.

> +
> +quiet_cmd_moc = MOC $@
> + cmd_moc = $(shell . $(obj)/.qconf-cfg && echo $$moc) -i $< -o $@
> +
> +$(obj)/%.moc: $(src)/%.h $(obj)/.qconf-cfg
> + $(call cmd,moc)
We will in this makefile only support qconf.h => qconf.moc

So it may be simpler to read the makefile if we in the above
uses explicit filenames.

> -
> -quiet_cmd_moc = MOC $@
> - cmd_moc = $(KC_QT_MOC) -i $< -o $@
> -
> -$(obj)/%.moc: $(src)/%.h $(obj)/.tmp_qtcheck
> - $(call cmd,moc)
> -
> # Extract gconf menu items for i18n support
> $(obj)/gconf.glade.h: $(obj)/gconf.glade
> $(Q)intltool-extract --type=gettext/glade --srcdir=$(srctree) \
> $(obj)/gconf.glade

quiet_cmd_intl = INTL @$
cmd_intl = $(Q)intltool-extract --type=gettext/glade --srcdir=$(srctree) $<

$(obj)/gconf.glade.h: $(obj)/gconf.glade
$(cmd,intl)

To make this step visible in normal build

> +# check if necessary packages are available, and configure build flags
> +define filechk_conf_cfg
> + $(CONFIG_SHELL) $<
> +endef

I cannot remember the particulars, but I think we could use $(shell ...)
in the above. If it has any positive effect is doubtful.

> +
> +$(obj)/.%conf-cfg: $(src)/%conf-cfg.sh FORCE
> + $(call filechk,conf_cfg)
> +
> +clean-files += .*conf-cfg
> diff --git a/scripts/kconfig/qconf-cfg.sh b/scripts/kconfig/qconf-cfg.sh
> new file mode 100755
> index 0000000..0862e15
> --- /dev/null
> +++ b/scripts/kconfig/qconf-cfg.sh
> @@ -0,0 +1,25 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: GPL-2.0
> +
> +PKG="Qt5Core Qt5Gui Qt5Widgets"
> +PKG2="QtCore QtGui"
> +
> +if pkg-config --exists $PKG; then
> + echo cflags=\"-std=c++11 -fPIC $(pkg-config --cflags Qt5Core Qt5Gui Qt5Widgets)\"
> + echo libs=\"$(pkg-config --libs $PKG)\"
> + echo moc=\"$(pkg-config --variable=host_bins Qt5Core)/moc\"
> + exit 0
> +fi
> +
> +if pkg-config --exists $PKG2; then
> + echo cflags=\"$(pkg-config --cflags $PKG2)\"
> + echo libs=\"$(pkg-config --libs $PKG2)\"
> + echo moc=\"$(pkg-config --variable=moc_location QtCore)\"
> + exit 0
> +fi

The old code only checked ionly for Qt5Core / QtCore - so what you do here is
likely an extra improvement.
This change is not included in your changelog.

Sam

Next message: kbuild test robot: "Re: [PATCH V3 2/5] X86: Hyper-V: Enable IPI enlightenments"
Previous message: Jonathan Cameron: "Re: [PATCH] iio: tsl2583: correct values in integration_time_available"
In reply to: Masahiro Yamada: "[PATCH v2 2/5] kconfig: refactor Qt package checks for building qconf"
Next in thread: Masahiro Yamada: "[PATCH v2 4/5] kconfig: refactor ncurses package checks for building mconf"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]