| Message ID | 20190723172336.24010-1-vadim4j@gmail.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [1/1] utils/test-pkg: add option to enable NLS support | expand |
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
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
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
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
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
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:
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(-)