Re: [dpdk-dev] [PATCH v4 1/7] mk: Add rule for installing headers

["Arevalo, Mario Alfredo C" <mario.alfredo.c.arevalo@intel.com>]

Hi Olivier, thank you for your feedback , more comments below :) 

Thanks.
Mario.
________________________________________
From: Olivier MATZ [olivier.matz@6wind.com]
Sent: Friday, October 16, 2015 12:29 PM
To: Arevalo, Mario Alfredo C; dev@dpdk.org
Cc: Panu Matilainen
Subject: Re: [PATCH v4 1/7] mk: Add rule for installing headers

Hi Mario,

Thank you for this patch series, and thank you Panu for the
good comments on this series.

Please see some comments below.

On 10/05/2015 10:20 PM, Mario Carrillo wrote:
> Add hierarchy-file support to the DPDK headers.
>
> When invoking "make install-headers" headers will
> be installed in: $(DESTDIR)/$(INCLUDE_DIR)
> where INCLUDE_DIR=/usr/include/dpdk by default.
>
> You can override INCLUDE_DIR var.
> This hierarchy is based on:
> http://www.freedesktop.org/software/systemd/man/file-hierarchy.html
>
> Signed-off-by: Mario Carrillo <mario.alfredo.c.arevalo@intel.com>
> ---
>  mk/rte.sdkinstall.mk | 18 +++++++++++++++++-
>  mk/rte.sdkroot.mk    |  4 ++--
>  2 files changed, 19 insertions(+), 3 deletions(-)
>
> diff --git a/mk/rte.sdkinstall.mk b/mk/rte.sdkinstall.mk
> index 86c98a5..f016171 100644
> --- a/mk/rte.sdkinstall.mk
> +++ b/mk/rte.sdkinstall.mk
> @@ -41,6 +41,10 @@ endif
>  # x86_64-native-*-gcc
>  ifndef T
>  T=*
> +ifneq (,$(wildcard $(BUILD_DIR)/build/.config))

What about using $(RTE_OUTPUT) here instead of $(BUILD_DIR)/build ?

With you patch, the following commands work:

  make config T=x86_64-native-linuxapp-gcc
  make -j32
  make install-headers DESTDIR=install

Replacing all the occurences of $(BUILD_DIR)/build by $(RTE_OUTPUT)
would also make the following commands work:

  make config T=x86_64-native-linuxapp-gcc O=build2
  make -j32 O=build2
  make install-headers O=build2 DESTDIR=install2

Note: the default value of RTE_OUTPUT is set to $(RTE_SRCDIR)/build
in mk/rte.sdkroot.mk

We also need to to replace other occurences of $(BUILD_DIR), please
see my comment on patch 2/7.


[Mario]: Yes, you right, I can replace it :) 

> +INCLUDE_DIR ?= /usr/include/dpdk
> +HSLINKS := $(wildcard $(RTE_OUTPUT)/include/*)
> +endif
>  endif
>
>  #
> @@ -72,7 +76,19 @@ install: $(INSTALL_TARGETS)
>               echo "Using local configuration"; \
>       fi
>       $(Q)$(MAKE) all O=$(BUILD_DIR)/$*
> -
> +#
> +# install headers in /usr/include/dpdk by default
> +# INCLUDE_DIR can be overridden.
> +#
> +.PHONY: install-headers
> +install-headers:
> +     @echo ================== Installing headers;
> +     @[ -d $(DESTDIR)/$(INCLUDE_DIR) ] || mkdir -p $(DESTDIR)/$(INCLUDE_DIR)

I think it's useless to do the [ -d $(DESTDIR)/$(INCLUDE_DIR) ] as the
'mkdir -p' will do it as well.

But maybe it could be useful to check:
  [ "$${HSLINKS}" != "" ]
This would solve the issue described by Panu about the directories
created even if they are empty.

[Mario]: Sounds good, that can solve the problem. 

> +     @for HSLINK in ${HSLINKS}; do \

Not sure to understand what is the meaning of HSLINK?
HS = headers?

[Mario]: HSLINKS= headers links, in dpdk installation, it creates links to headers, however for installation I would like
to get the headers paths and copy them to installation path, not create links  :) .

Regards,
Olivier