[dpdk-dev,v2,1/2] mk: allow use of environment var for make config

Message ID 1495788764-37652-2-git-send-email-david.hunt@intel.com (mailing list archive)
State Superseded, archived
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Hunt, David May 26, 2017, 8:52 a.m. UTC
  Users can now set RTE_TARGET in their environment and use
'make config' without T=template.

If RTE_TARGET is set in the user's environment, and if T=
is not used, 'make config' will use $RTE_TARGET.

Signed-off-by: David Hunt <david.hunt@intel.com>
---
 mk/rte.sdkroot.mk | 2 ++
 1 file changed, 2 insertions(+)
  

Comments

Hunt, David June 7, 2017, 8:39 a.m. UTC | #1
Shreyansh,

     I found an issue (or two) with this part of the patch, and have a 
proposed solution.

1. RTE_TARGET originally had a different meaning. It was used for making 
examples, specifying the target directory of where the SDK was built. 
It's not good to re-purpose this for something else, as I'm doing in 
this patch. (even though I'm not sure that variable is suitably named in 
the first place, but that's a different issue).
2. If we set RTE_TARGET on the environment, we will break the 'make -C 
examples/<app>', unless we set RTE_TARGET to be something else (i.e. 
'make -C examples/<app> RTE_TARGET=build'). One value for making DPDK, 
and another for building examples. It's confusing to the user.

An alternative patch would be as follows:

  RTE_CONFIG_TEMPLATE :=
  ifdef T
*-ifeq ("$(origin T)", "command line")*
  RTE_CONFIG_TEMPLATE := $(RTE_SRCDIR)/config/defconfig_$(T)
*-endif**
*endif
  export RTE_CONFIG_TEMPLATE

So instead of setting 'RTE_TARGET' on in the environment, we set 'T' 
instead. This allows 'T' to come from the command line OR an environment 
variable. It resolves the 'make examples' issue, and everything else 
works as it did before, 'make install', etc. It seems to me to be a 
cleaner solution for this. What do you think? If it's OK with you, I'll 
submit a v3 (the 'make defconfig' part of the patchset will remain the 
same as v2).

Rgds,
Dave.



On 26/5/2017 9:52 AM, David Hunt wrote:
> Users can now set RTE_TARGET in their environment and use
> 'make config' without T=template.
>
> If RTE_TARGET is set in the user's environment, and if T=
> is not used, 'make config' will use $RTE_TARGET.
>
> Signed-off-by: David Hunt <david.hunt@intel.com>
> ---
>   mk/rte.sdkroot.mk | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/mk/rte.sdkroot.mk b/mk/rte.sdkroot.mk
> index 2843b7d..9bdaf20 100644
> --- a/mk/rte.sdkroot.mk
> +++ b/mk/rte.sdkroot.mk
> @@ -63,6 +63,8 @@ ifdef T
>   ifeq ("$(origin T)", "command line")
>   RTE_CONFIG_TEMPLATE := $(RTE_SRCDIR)/config/defconfig_$(T)
>   endif
> +else ifdef RTE_TARGET
> +RTE_CONFIG_TEMPLATE := $(RTE_SRCDIR)/config/defconfig_$(RTE_TARGET)
>   endif
>   export RTE_CONFIG_TEMPLATE
>
  
Shreyansh Jain June 7, 2017, 9:36 a.m. UTC | #2
Hello David,

On Wednesday 07 June 2017 02:09 PM, Hunt, David wrote:
> Shreyansh,
> 
>      I found an issue (or two) with this part of the patch, and have a 
> proposed solution.
> 
> 1. RTE_TARGET originally had a different meaning. It was used for making 
> examples, specifying the target directory of where the SDK was built. 
> It's not good to re-purpose this for something else, as I'm doing in 
> this patch. (even though I'm not sure that variable is suitably named in 
> the first place, but that's a different issue).

Even I didn't realize this until you highlighted here.

> 2. If we set RTE_TARGET on the environment, we will break the 'make -C 
> examples/<app>', unless we set RTE_TARGET to be something else (i.e. 
> 'make -C examples/<app> RTE_TARGET=build'). One value for making DPDK, 
> and another for building examples. It's confusing to the user.

Agree about re-using RTE_TARGET is breaking existing assumption about
its use.

> 
> An alternative patch would be as follows:
> 
>   RTE_CONFIG_TEMPLATE :=
>   ifdef T
> *-ifeq ("$(origin T)", "command line")*
>   RTE_CONFIG_TEMPLATE := $(RTE_SRCDIR)/config/defconfig_$(T)
> *-endif**
> *endif
>   export RTE_CONFIG_TEMPLATE
So, that would mean, user would do either of the following:

make T=<template> config

or

export T=<template>
make config

Is that correct? (I tried it and it seems to be working fine)
First method is same as today. For the second, I am just skeptical
whether we should use such a small identifier ("T") or we have a new
RTE_TEMPLATE.

Either way, I am OK. [export T=<template>] looks fine to me - in fact,
on a second though, IMO, if T=<template> is provided as command line, it 
should also be acceptable as env variable.

> 
> So instead of setting 'RTE_TARGET' on in the environment, we set 'T' 
> instead. This allows 'T' to come from the command line OR an environment 
> variable. It resolves the 'make examples' issue, and everything else 
> works as it did before, 'make install', etc. It seems to me to be a 
> cleaner solution for this. What do you think? If it's OK with you, I'll 
> submit a v3 (the 'make defconfig' part of the patchset will remain the 
> same as v2).
> 
> Rgds,
> Dave.
> 
> 
> 
> On 26/5/2017 9:52 AM, David Hunt wrote:
>> Users can now set RTE_TARGET in their environment and use
>> 'make config' without T=template.
>>
>> If RTE_TARGET is set in the user's environment, and if T=
>> is not used, 'make config' will use $RTE_TARGET.
>>
>> Signed-off-by: David Hunt <david.hunt@intel.com>
>> ---
>>   mk/rte.sdkroot.mk | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/mk/rte.sdkroot.mk b/mk/rte.sdkroot.mk
>> index 2843b7d..9bdaf20 100644
>> --- a/mk/rte.sdkroot.mk
>> +++ b/mk/rte.sdkroot.mk
>> @@ -63,6 +63,8 @@ ifdef T
>>   ifeq ("$(origin T)", "command line")
>>   RTE_CONFIG_TEMPLATE := $(RTE_SRCDIR)/config/defconfig_$(T)
>>   endif
>> +else ifdef RTE_TARGET
>> +RTE_CONFIG_TEMPLATE := $(RTE_SRCDIR)/config/defconfig_$(RTE_TARGET)
>>   endif
>>   export RTE_CONFIG_TEMPLATE
> 
>
  
Hunt, David June 7, 2017, 2:37 p.m. UTC | #3
This patch series is a couple of small patches to make the 'make config'
of the build a bit easier for users.

Users can now 'make defconfig' which will pick a sensible default based on
some 'uname' queries of the system.

Users can also set RTE_TEMPLATE in their environment which will get picked
users type 'make config' without T=template.

The changes are documented in docs/build-sdk-quick.txt.

v2 changes:
  Added better handling for non-IA platforms. The list is now as follows:
  uname -m  Output Target
  --------  ------------------
  aarch64   arm64-armv8a-...
  armv7l    arm-armv7a-...
  ppc64     ppc_64-power8-...
  x86_64    x86_64-native-...
  i686      i686-native-...

v3 changes:
  * Changed the use of RTE_TARGET back to it's original purpose and added new
    environmental variable RTE_TEMPLATE. Fully backward compatible if this is
    not set.
  * Added documentation changes to build-sdk-quick.txt to describe additions.

[1/3] mk: add sensible default target with defconfig
[2/3] mk: allow use of environment var for template
[3/3] doc: update build-sdk-quick txt file
  

Patch

diff --git a/mk/rte.sdkroot.mk b/mk/rte.sdkroot.mk
index 2843b7d..9bdaf20 100644
--- a/mk/rte.sdkroot.mk
+++ b/mk/rte.sdkroot.mk
@@ -63,6 +63,8 @@  ifdef T
 ifeq ("$(origin T)", "command line")
 RTE_CONFIG_TEMPLATE := $(RTE_SRCDIR)/config/defconfig_$(T)
 endif
+else ifdef RTE_TARGET
+RTE_CONFIG_TEMPLATE := $(RTE_SRCDIR)/config/defconfig_$(RTE_TARGET)
 endif
 export RTE_CONFIG_TEMPLATE