Re: [dpdk-dev] [PATCH v2] devtools: fix 32-bits build

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 11/9/2020 5:01 PM, Thomas Monjalon wrote:
> 09/11/2020 17:48, Ferruh Yigit:
>> On 11/9/2020 4:19 PM, Bruce Richardson wrote:
>>> On Mon, Nov 09, 2020 at 05:14:24PM +0100, Thomas Monjalon wrote:
>>>> 09/11/2020 16:44, Bruce Richardson:
>>>>> On Mon, Nov 09, 2020 at 04:28:16PM +0100, Thomas Monjalon wrote:
>>>>>> 09/11/2020 15:55, Ferruh Yigit:
>>>>>>> If the 'PKG_CONFIG_PATH' is set in the environment before script run,
>>>>>>> 32 bit still uses that value for 64-bits libraries.
>>>>>>>
>>>>>>>   From the build log:
>>>>>>>
>>>>>>> "
>>>>>>> Using DPDK_TARGET i386-pc-linux-gnu
>>>>>>> meson  -Dexamples=l3fwd --buildtype=debugoptimized --werror
>>>>>>> -Dc_args=-m32 -Dc_link_args=-m32
>>>>>>> /tmp/dpdk_maintain/self/dpdk/devtools/.. ./build-32b
>>>>>>> ...
>>>>>>> Using 'PKG_CONFIG_PATH' from environment with value:
>>>>>>> '/usr/local/lib64/pkgconfig/
>>>>>>> "
>>>>>>>
>>>>>>> This causes build error when linking with the found libraries.
>>>>>>>
>>>>>>> Reproduced with 'librte_bpf' which only has 64 bit installed but still
>>>>>>> enables building 'af_xdp' and link fails.
>>>>>>>
>>>>>>> To fix this, using default 'PKG_CONFIG_PATH' variable unless
>>>>>>> 'CUSTOM_PKG_CONFIG_PATH' set, and set the 'CUSTOM_PKG_CONFIG_PATH' for
>>>>>>> 32 bit build.
>>>>>>>
>>>>>>> Fixes: 9b83106d8784 ("devtools: test 32-bit build")
>>>>>>>
>>>>>>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
>>>>>>> ---
>>>>>>> Cc: Bruce Richardson <bruce.richardson@intel.com>
>>>>>>> Cc: Thomas Monjalon <thomas@monjalon.net>
>>>>>>>
>>>>>>> v2:
>>>>>>> * Enable overwriting default 'PKG_CONFIG_PATH' value
>>>>>>
>>>>>> It was not my conclusion.
>>>>>> I think we can just reset all env vars.
>>>>>>
>>>>>
>>>>> Perhaps, but I think that may cause more problems for people who want
>>>>> custom CFLAGS and LDFLAGS for building with extra 3rd-party packages.
>>>>> While this can be done using the config script that is sourced in, I
>>>>> suspect most of us do not use such a script.
>>>>>
>>>>>    Therefore I'd suggest rather than clearing the env vars globally, we just
>>>>> override them temporarily for the 32-bit, using CFLAGS32, LDFLAGS32,
>>>>> PKG_CONFIG_PATH32 instead. That allows someone to have a 32-bit and 64-bit
>>>>> version of e.g. libbpf installed, with a PKG_CONFIG_PATH pointing to each.
>>>>
>>>> So you want to duplicate all vars just to avoid writing them cleanly in a file?
>>>> And what happens for other targets?
>>>>
>>>> I'm tending to nack this approach.
>>>> There is no problem in using a clean config file.
>>>>
>>> Well, when you put it that way :-)
>>> However, I was only thinking of the 3 variables above, though really it's
>>> only the PKG_CONFIG_PATH that is going to be the most likely culprit, so
>>> supporting just one extra var "PKG_CONFIG_PATH_32" seems reasonable.
>>>
>>> I'm also ok with your solution of just clearing the environment and relying
>>> on a config file too, though.
>>>
>>
>> I was worried for any side affects to reset the var, this is smaller change to
>> change only 'CUSTOM_PKG_CONFIG_PATH' for 'build-32b'
>>
>> I can send a new version to reset 'CUSTOM_PKG_CONFIG_PATH',
>> what is the intention/plan with the config file?
> 
> You should use a config file to set PKG_CONFIG_PATH, PATH,
> DPDK_MESON_OPTIONS, etc accordingly to build with all
> your non-installed dependencies.
> CUSTOM_PKG_CONFIG_PATH is a way to avoid using the config file
> in a single case.
> 

Either use 'PKG_CONFIG_PATH' environment variable or a config file, both has the 
same problem that we need two different pkg config path, one for 64bit and one 
for 32bit.

v2 of this patch provides a way to use correct one when needed.

Moving the default values from system environment or script hardcoded values to 
a config file is something else and can be done later, what do you think?


> Don't be afraid breaking the assumption of theoritical users
> of this script. If we want to avoid PATH reset,
> we can add such line at the beginning of the script:
> default_path=${PATH:-/bin:/usr/bin}
> 
>