Hello Thomas,

On Tue, 14 Jun 2016 10:59:49 +0200
Thomas Monjalon <thomas.monjalon at 6wind.com> wrote:

> The commit 66819e6 has introduced a dependency on libarchive to be able
> to use some tar resources in the unit tests.
> It is now an optional dependency because some systems do not have it
> installed.

I am surprised how big deal is this. So, let's live with this fact.
Thank you, Thomas, for proposing a solution.

> 
> If CONFIG_RTE_APP_TEST_RESOURCE_TAR is disabled, the PCI test will not
> be run. When a "configure" script will be integrated, the libarchive
> availability could be checked to automatically enable the option.
> 
> Signed-off-by: Thomas Monjalon <thomas.monjalon at 6wind.com>
> ---
>  app/test/Makefile                 | 11 +++++++----
>  app/test/resource.c               |  8 ++++++--
>  app/test/test_mp_secondary.c      |  4 ++++
>  app/test/test_resource.c          |  5 +++++
>  config/common_base                |  1 +
>  doc/guides/linux_gsg/sys_reqs.rst |  3 +++
>  scripts/test-build.sh             |  4 ++++
>  7 files changed, 30 insertions(+), 6 deletions(-)
> 
> diff --git a/app/test/Makefile b/app/test/Makefile
> index 7e4d484..5ca5c0b 100644
> --- a/app/test/Makefile
> +++ b/app/test/Makefile
> @@ -71,9 +71,6 @@ SRCS-y += test.c
>  SRCS-y += resource.c
>  SRCS-y += test_resource.c
>  $(eval $(call linked_resource,test_resource_c,resource.c))
> -$(eval $(call linked_tar_resource,test_resource_tar,test_resource.c))
> -SRCS-y += test_pci.c
> -$(eval $(call linked_tar_resource,test_pci_sysfs,test_pci_sysfs))
>  SRCS-y += test_prefetch.c
>  SRCS-y += test_byteorder.c
>  SRCS-y += test_per_lcore.c
> @@ -88,6 +85,13 @@ SRCS-y += test_ring.c
>  SRCS-y += test_ring_perf.c
>  SRCS-y += test_pmd_perf.c
>  
> +ifeq ($(CONFIG_RTE_APP_TEST_RESOURCE_TAR),y)
> +$(eval $(call linked_tar_resource,test_resource_tar,test_resource.c))
> +SRCS-y += test_pci.c
> +$(eval $(call linked_tar_resource,test_pci_sysfs,test_pci_sysfs))
> +LDLIBS += -larchive
> +endif
> +

I don't like this very much. I think, the linked_tar_resource can be
disabled at the place of its definition. What about:

ifeq ($(CONFIG_RTE_APP_TEST_RESOURCE_TAR),y)
define linked_tar_resource
...
endef
else
linked_tar_resource =
endif

...

SRCS-$(CONFIG_RTE_APP_TEST_RESOURCE_TAR) += test_pci.c

...

ifeq ($(CONFIG_RTE_APP_TEST_RESOURCE_TAR),y)
LDLIBS += -larchive
endif

>  ifeq ($(CONFIG_RTE_LIBRTE_TABLE),y)
>  SRCS-y += test_table.c
>  SRCS-$(CONFIG_RTE_LIBRTE_PIPELINE) += test_table_pipeline.c
> @@ -194,7 +198,6 @@ CFLAGS += $(WERROR_FLAGS)
>  CFLAGS += -D_GNU_SOURCE
>  
>  LDLIBS += -lm
> -LDLIBS += -larchive
>  
>  # Disable VTA for memcpy test
>  ifeq ($(CONFIG_RTE_TOOLCHAIN_GCC),y)
> diff --git a/app/test/resource.c b/app/test/resource.c
> index 8c42eea..0e2b62c 100644
> --- a/app/test/resource.c
> +++ b/app/test/resource.c
> @@ -33,8 +33,6 @@
>  
>  #include <stdio.h>
>  #include <string.h>
> -#include <archive.h>
> -#include <archive_entry.h>
>  #include <errno.h>
>  #include <sys/queue.h>
>  
> @@ -97,6 +95,10 @@ int resource_fwrite_file(const struct resource *r, const 
> char *fname)
>       return ret;
>  }
>  
> +#ifdef RTE_APP_TEST_RESOURCE_TAR
> +#include <archive.h>
> +#include <archive_entry.h>
> +
>  static int do_copy(struct archive *r, struct archive *w)
>  {
>       const void *buf;
> @@ -295,6 +297,8 @@ fail:
>       return -1;
>  }
>  
> +#endif /* RTE_APP_TEST_RESOURCE_TAR */
> +

This looks OK.

>  void resource_register(struct resource *r)
>  {
>       TAILQ_INSERT_TAIL(&resource_list, r, next);
> diff --git a/app/test/test_mp_secondary.c b/app/test/test_mp_secondary.c
> index 4dfe418..f66b68f 100644
> --- a/app/test/test_mp_secondary.c
> +++ b/app/test/test_mp_secondary.c
> @@ -245,6 +245,7 @@ run_object_creation_tests(void)
>       printf("# Checked rte_lpm_create() OK\n");
>  #endif
>  
> +#ifdef RTE_APP_TEST_RESOURCE_TAR
>       /* Run a test_pci call */
>       if (test_pci() != 0) {
>               printf("PCI scan failed in secondary\n");
> @@ -252,6 +253,7 @@ run_object_creation_tests(void)
>                       return -1;
>       } else
>               printf("PCI scan succeeded in secondary\n");
> +#endif

Is it right to call a test from another test? I think this is
wrong... A user should first test the PCI and then the mp_seconday...
Or?

>  
>       return 0;
>  }
> @@ -266,9 +268,11 @@ test_mp_secondary(void)
>  {
>       if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
>               if (!test_pci_run) {
> +#ifdef RTE_APP_TEST_RESOURCE_TAR
>                       printf("=== Running pre-requisite test of test_pci\n");
>                       test_pci();
>                       printf("=== Requisite test done\n");
> +#endif

Similar here.

>               }
>               return run_secondary_instances();
>       }
> diff --git a/app/test/test_resource.c b/app/test/test_resource.c
> index 1e85040..39a6468 100644
> --- a/app/test/test_resource.c
> +++ b/app/test/test_resource.c
> @@ -85,6 +85,7 @@ static int test_resource_c(void)
>       return 0;
>  }
>  
> +#ifdef RTE_APP_TEST_RESOURCE_TAR
>  REGISTER_LINKED_RESOURCE(test_resource_tar);
>  
>  static int test_resource_tar(void)
> @@ -111,6 +112,8 @@ static int test_resource_tar(void)
>       return 0;
>  }
>  
> +#endif /* RTE_APP_TEST_RESOURCE_TAR */
> +

This looks OK.

>  static int test_resource(void)
>  {
>       if (test_resource_dpdk())
> @@ -119,8 +122,10 @@ static int test_resource(void)
>       if (test_resource_c())
>               return -1;
>  
> +#ifdef RTE_APP_TEST_RESOURCE_TAR
>       if (test_resource_tar())
>               return -1;
> +#endif /* RTE_APP_TEST_RESOURCE_TAR */

This looks OK.

>  
>       return 0;
>  }
> diff --git a/config/common_base b/config/common_base
> index 47c26f6..b9ba405 100644
> --- a/config/common_base
> +++ b/config/common_base
> @@ -546,6 +546,7 @@ CONFIG_RTE_INSECURE_FUNCTION_WARNING=n
>  # Compile the test application
>  #
>  CONFIG_RTE_APP_TEST=y
> +CONFIG_RTE_APP_TEST_RESOURCE_TAR=n
>  
>  #
>  # Compile the PMD test application
> diff --git a/doc/guides/linux_gsg/sys_reqs.rst 
> b/doc/guides/linux_gsg/sys_reqs.rst
> index 959709e..b321544 100644
> --- a/doc/guides/linux_gsg/sys_reqs.rst
> +++ b/doc/guides/linux_gsg/sys_reqs.rst
> @@ -101,6 +101,9 @@ Compilation of the DPDK
>  *   libpcap headers and libraries (libpcap-devel) to compile and use the 
> libpcap-based poll-mode driver.
>      This driver is disabled by default and can be enabled by setting 
> ``CONFIG_RTE_LIBRTE_PMD_PCAP=y`` in the build time config file.
>  
> +*   libarchive headers and library are needed for some unit tests using tar 
> to get their resources.
> +
> +
>  Running DPDK Applications
>  -------------------------
>  
> diff --git a/scripts/test-build.sh b/scripts/test-build.sh
> index 48539c1..9a11f94 100755
> --- a/scripts/test-build.sh
> +++ b/scripts/test-build.sh
> @@ -35,6 +35,7 @@ default_path=$PATH
>  # Load config options:
>  # - AESNI_MULTI_BUFFER_LIB_PATH
>  # - DPDK_BUILD_TEST_CONFIGS (defconfig1+option1+option2 defconfig2)
> +# - DPDK_DEP_ARCHIVE
>  # - DPDK_DEP_CFLAGS
>  # - DPDK_DEP_LDFLAGS
>  # - DPDK_DEP_MOFED (y/[n])
> @@ -111,6 +112,7 @@ reset_env ()
>  {
>       export PATH=$default_path
>       unset CROSS
> +     unset DPDK_DEP_ARCHIVE
>       unset DPDK_DEP_CFLAGS
>       unset DPDK_DEP_LDFLAGS
>       unset DPDK_DEP_MOFED
> @@ -149,6 +151,8 @@ config () # <directory> <target> <options>
>               sed -ri         's,(PCI_CONFIG=)n,\1y,' $1/.config
>               sed -ri    's,(LIBRTE_IEEE1588=)n,\1y,' $1/.config
>               sed -ri             's,(BYPASS=)n,\1y,' $1/.config
> +             test "$DPDK_DEP_ARCHIVE" != y || \
> +             sed -ri       's,(RESOURCE_TAR=)n,\1y,' $1/.config
>               test "$DPDK_DEP_MOFED" != y || \
>               sed -ri           's,(MLX._PMD=)n,\1y,' $1/.config
>               test "$DPDK_DEP_SZE" != y || \

This look OK.

Regards
Jan

Reply via email to