From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from wes1-so1.wedos.net (wes1-so1.wedos.net [46.28.106.15]) by dpdk.org (Postfix) with ESMTP id 75D676CC5 for ; Tue, 14 Jun 2016 11:38:50 +0200 (CEST) Received: from pcviktorin.fit.vutbr.cz (pcviktorin.fit.vutbr.cz [147.229.13.147]) by wes1-so1.wedos.net (Postfix) with ESMTPSA id 3rTPjQ1ZT2zBqn; Tue, 14 Jun 2016 11:38:50 +0200 (CEST) Date: Tue, 14 Jun 2016 11:33:43 +0200 From: Jan Viktorin To: Thomas Monjalon Cc: David Hunt , dev@dpdk.org Message-ID: <20160614113343.2d6fd116@pcviktorin.fit.vutbr.cz> In-Reply-To: <1465894789-20733-1-git-send-email-thomas.monjalon@6wind.com> References: <575FBF1A.5080005@intel.com> <1465894789-20733-1-git-send-email-thomas.monjalon@6wind.com> Organization: RehiveTech MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH] config: make libarchive optional X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 14 Jun 2016 09:38:50 -0000 Hello Thomas, On Tue, 14 Jun 2016 10:59:49 +0200 Thomas Monjalon 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 > --- > 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 > #include > -#include > -#include > #include > #include > > @@ -97,6 +95,10 @@ int resource_fwrite_file(const struct resource *r, const char *fname) > return ret; > } > > +#ifdef RTE_APP_TEST_RESOURCE_TAR > +#include > +#include > + > 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 () # > 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