[Buildroot] [PATCH 1/1] utils/test-pkg: add option to enable NLS support

[Buildroot] [PATCH 1/1] utils/test-pkg: add option to enable NLS support

[Vadim Kochan]

Sometimes the package or specific configuration should be build with and
without NLS support which requires additionally append NLS related
config options or to have separate config file or use additional shell
script.

So add helper command line option which appends config file with NLS
options to the final config file which is now always stored in temporary
file before merge it with the default configuration (even only -c was
specified).

Signed-off-by: Vadim Kochan <vadim4j@gmail.com>
---
 utils/test-pkg | 34 ++++++++++++++++++++++++++--------
 1 file changed, 26 insertions(+), 8 deletions(-)

diff --git a/utils/test-pkg b/utils/test-pkg
index f3b34d5d0d..6a48e039e0 100755
--- a/utils/test-pkg
+++ b/utils/test-pkg
@@ -16,9 +16,10 @@ main() {
     local ret nb nb_skip nb_fail nb_legal nb_tc build_dir keep
     local -a toolchains
     local pkg_br_name
+    local with_nls
 
-    o='hakc:d:n:p:r:t:'
-    O='help,all,keep,config-snippet:,build-dir:,number:,package:,random:,toolchains-csv:'
+    o='hakNc:d:n:p:r:t:'
+    O='help,all,keep,with-nls,config-snippet:,build-dir:,number:,package:,random:,toolchains-csv:'
     opts="$(getopt -n "${my_name}" -o "${o}" -l "${O}" -- "${@}")"
     eval set -- "${opts}"
 
@@ -27,6 +28,7 @@ main() {
     keep=0
     number=0
     mode=0
+    with_nls=0
     toolchains_csv="${TOOLCHAINS_CSV}"
     while [ ${#} -gt 0 ]; do
         case "${1}" in
@@ -41,6 +43,10 @@ main() {
             ;;
         (-c|--config-snippet)
             cfg="${2}"; shift 2
+
+            if [ ! -e "${cfg}" ]; then
+                printf "error: %s: no such file\n" "${cfg}" >&2; exit 1
+            fi
             ;;
         (-d|--build-dir)
             dir="${2}"; shift 2
@@ -57,6 +63,9 @@ main() {
         (-t|--toolchains-csv)
             toolchains_csv="${2}"; shift 2
             ;;
+        (-N|--with-nls)
+            with_nls=1; shift 1
+            ;;
         (--)
             shift; break
             ;;
@@ -65,16 +74,21 @@ main() {
 
     trap do_clean INT TERM HUP EXIT
 
+    TEMP_CONF=$(mktemp /tmp/test-${pkg}-config.XXXXXX)
+
     if [ -z "${cfg}" ]; then
         pkg_br_name="${pkg//-/_}"
         pkg_br_name="BR2_PACKAGE_${pkg_br_name^^}"
-        TEMP_CONF=$(mktemp /tmp/test-${pkg}-config.XXXXXX)
-        echo "${pkg_br_name}=y" > ${TEMP_CONF}
-        cfg="${TEMP_CONF}"
+        echo "${pkg_br_name}=y" >> ${TEMP_CONF}
+    else
+        cat "${cfg}" >> ${TEMP_CONF}
     fi
-    if [ ! -e "${cfg}" ]; then
-        printf "error: %s: no such file\n" "${cfg}" >&2; exit 1
+
+    if [ ${with_nls} -eq 1 ]; then
+        echo "BR2_USE_WCHAR=y" >> ${TEMP_CONF}
+        echo "BR2_SYSTEM_ENABLE_NLS=y" >> ${TEMP_CONF}
     fi
+
     if [ -z "${dir}" ]; then
         dir="${HOME}/br-test-pkg"
     fi
@@ -127,7 +141,7 @@ main() {
         toolchain="$(basename "${toolchainconfig}" .config)"
         build_dir="${dir}/${toolchain}"
         printf "%40s [%*d/%d]: " "${toolchain}" ${#nb_tc} ${nb} ${nb_tc}
-        build_one "${build_dir}" "${toolchainconfig}" "${cfg}" "${pkg}" && ret=0 || ret=${?}
+        build_one "${build_dir}" "${toolchainconfig}" "${TEMP_CONF}" "${pkg}" && ret=0 || ret=${?}
         case ${ret} in
         (0) printf "OK\n";;
         (1) : $((nb_skip++)); printf "SKIPPED\n";;
@@ -251,6 +265,10 @@ Options:
         Note: the logfile and configuration is always retained, even without
         this option.
 
+    -N, --with-nls
+        Build with NLS support enabled. Required config options
+        for NLS support will be automatically enabled.
+
 Example:
 
     Testing libcec would require a config snippet that contains:
-- 
2.17.1

[Buildroot] [PATCH 1/1] utils/test-pkg: add option to enable NLS support

[Thomas Petazzoni]

Hello Vadim,

On Tue, 23 Jul 2019 20:23:36 +0300
Vadim Kochan <vadim4j@gmail.com> wrote:

> Sometimes the package or specific configuration should be build with and
> without NLS support which requires additionally append NLS related
> config options or to have separate config file or use additional shell
> script.
> 
> So add helper command line option which appends config file with NLS
> options to the final config file which is now always stored in temporary
> file before merge it with the default configuration (even only -c was
> specified).
> 
> Signed-off-by: Vadim Kochan <vadim4j@gmail.com>

Thanks for the patch. It seems like a good idea to test NLS-enabled
configurations in test-pkg indeed. However, I'm not sure it's a good
idea to make it conditional/optional. The idea of test-pkg is that
people run test-pkg, and it tells them if their new package seems to be
OK or not. Most people will not know which knobs/options to pass to
test-pkg to test all possible situations.

So I think we should instead ensure that test-pkg by default tests the
different combinations that we think are useful to test.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 1/1] utils/test-pkg: add option to enable NLS support

[Vadim Kochan]

Hi Thomas,

On Wed, Jul 24, 2019 at 1:14 AM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
>
> Hello Vadim,
>
> On Tue, 23 Jul 2019 20:23:36 +0300
> Vadim Kochan <vadim4j@gmail.com> wrote:
>
> > Sometimes the package or specific configuration should be build with and
> > without NLS support which requires additionally append NLS related
> > config options or to have separate config file or use additional shell
> > script.
> >
> > So add helper command line option which appends config file with NLS
> > options to the final config file which is now always stored in temporary
> > file before merge it with the default configuration (even only -c was
> > specified).
> >
> > Signed-off-by: Vadim Kochan <vadim4j@gmail.com>
>
> Thanks for the patch. It seems like a good idea to test NLS-enabled
> configurations in test-pkg indeed. However, I'm not sure it's a good
> idea to make it conditional/optional. The idea of test-pkg is that
> people run test-pkg, and it tells them if their new package seems to be
> OK or not. Most people will not know which knobs/options to pass to
> test-pkg to test all possible situations.
>
> So I think we should instead ensure that test-pkg by default tests the
> different combinations that we think are useful to test.
>

OK, I see, I can only say that usually I use test-pkg for version
bumping or checking
the package compilation issue reported in 'Buildroot results' mailing
if it pass, for *.mk changing and of course
for the new packages. So I was thinking that others also use it in
such way, and my
assumption was that --with-nls option would be useful :)

Thanks,
Vadim Kochan

[Buildroot] [PATCH 1/1] utils/test-pkg: add option to enable NLS support

[Thomas Petazzoni]

On Wed, 24 Jul 2019 10:03:24 +0300
Vadim Kochan <vadim4j@gmail.com> wrote:

> > So I think we should instead ensure that test-pkg by default tests the
> > different combinations that we think are useful to test.
> >  
> 
> OK, I see, I can only say that usually I use test-pkg for version
> bumping or checking
> the package compilation issue reported in 'Buildroot results' mailing
> if it pass, for *.mk changing and of course
> for the new packages. So I was thinking that others also use it in
> such way, and my
> assumption was that --with-nls option would be useful :)

Yes, others also use it for the same reason. What I meant is only that
others will most likely not remember they should test with and without
the --with-nls option. So we should test both NLS enabled and NLS
disabled configurations in the default test-pkg run perhaps.

But of course then, should we test sysvinit/systemd configurations, to
make sure the init script installation logic works ? Should we test
hardening options ? etc. We can't test everything in test-pkg as it
would make the test too long, and making the test too long will reduce
the incentive to use this tool.

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 1/1] utils/test-pkg: add option to enable NLS support

[Vadim Kochan]

On Wed, Jul 24, 2019 at 10:17 AM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
>
> On Wed, 24 Jul 2019 10:03:24 +0300
> Vadim Kochan <vadim4j@gmail.com> wrote:
>
> > > So I think we should instead ensure that test-pkg by default tests the
> > > different combinations that we think are useful to test.
> > >
> >
> > OK, I see, I can only say that usually I use test-pkg for version
> > bumping or checking
> > the package compilation issue reported in 'Buildroot results' mailing
> > if it pass, for *.mk changing and of course
> > for the new packages. So I was thinking that others also use it in
> > such way, and my
> > assumption was that --with-nls option would be useful :)
>
> Yes, others also use it for the same reason. What I meant is only that
> others will most likely not remember they should test with and without
> the --with-nls option. So we should test both NLS enabled and NLS
> disabled configurations in the default test-pkg run perhaps.
>
> But of course then, should we test sysvinit/systemd configurations, to
> make sure the init script installation logic works ? Should we test
> hardening options ? etc. We can't test everything in test-pkg as it
> would make the test too long, and making the test too long will reduce
> the incentive to use this tool.
>

Of course it is better to left it on developers decision what to test
by providing
just config snippet. The goal of this option is just to simplify of
build testing the same package/config
with NLS w/o manually adding NLS related options to the snippet,
because usually it needs to
build the package twice - with and without NLS. If you think it is
useless, I will
reset the patch as rejected, thats ok)

Regards,
Vadim Kochan

[Buildroot] [PATCH 1/1] utils/test-pkg: add option to enable NLS support

[Thomas Petazzoni]

Hello,

On Tue, 23 Jul 2019 20:23:36 +0300
Vadim Kochan <vadim4j@gmail.com> wrote:

> Sometimes the package or specific configuration should be build with and
> without NLS support which requires additionally append NLS related
> config options or to have separate config file or use additional shell
> script.
> 
> So add helper command line option which appends config file with NLS
> options to the final config file which is now always stored in temporary
> file before merge it with the default configuration (even only -c was
> specified).
> 
> Signed-off-by: Vadim Kochan <vadim4j@gmail.com>
> ---
>  utils/test-pkg | 34 ++++++++++++++++++++++++++--------
>  1 file changed, 26 insertions(+), 8 deletions(-)

So, I discussed this patch with Yann and Peter, and we all agree that
we don't want to go down this route. Indeed, after checking NLS, we'll
have to check hardening options, debugging options, optimization
options, and more. This makes it unpractical.

The role of test-pkg is not to ensure that absolutely no failure will
be found by the autobuilders. By default, test-pkg only tests 6-7
toolchains, and not the whole set of toolchains/configurations that the
autobuilders are testing. The point of test-pkg is just to do some
basic sanity checking, and iron out the most basic issues. For
everything else, we rely on the autobuilders to detect the remaining
problems, including NLS enabled issues.

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com