Re: [dpdk-dev] [PATCH v2] net/mlx: add meson build support

["Nélio Laranjeiro" <nelio.laranjeiro@6wind.com>]

Hi Bruce,

Thanks for your comments I have address almost all of them in the v3 by
doing what you suggest, I still have some comments, please see below,

On Tue, Aug 28, 2018 at 04:45:00PM +0100, Bruce Richardson wrote:
> Thanks for this, comments inline below.
> 
> /Bruce
> 
> On Mon, Aug 27, 2018 at 02:42:25PM +0200, Nelio Laranjeiro wrote:
> > Mellanox drivers remains un-compiled by default due to third party
> > libraries dependencies.  They can be enabled through:
> > - enable_driver_mlx{4,5}=true or
> > - enable_driver_mlx{4,5}_glue=true
> > depending on the needs.
> 
> The big reason why we wanted a new build system was to move away from this
> sort of static configuration. Instead, detect if the requirements as
> present and build the driver if you can.

Ok, I am letting only the glue option for both drivers as suggested at
the end of your answer.

> > To avoid modifying the whole sources and keep the compatibility with
> > current build systems (e.g. make), the mlx{4,5}_autoconf.h is still
> > generated by invoking DPDK scripts though meson's run_command() instead
> > of using has_types, has_members, ... commands.
> > 
> > Meson will try to find the required external libraries.  When they are
> > not installed system wide, they can be provided though CFLAGS, LDFLAGS
> > and LD_LIBRARY_PATH environment variables, example (considering
> > RDMA-Core is installed in /tmp/rdma-core):
> > 
> >  # CLFAGS=-I/tmp/rdma-core/build/include \
> >    LDFLAGS=-L/tmp/rdma-core/build/lib \
> >    LD_LIBRARY_PATH=/tmp/rdma-core/build/lib \
> >    meson -Denable_driver_mlx4=true output
> > 
> >  # CLFAGS=-I/tmp/rdma-core/build/include \
> >    LDFLAGS=-L/tmp/rdma-core/build/lib \
> >    LD_LIBRARY_PATH=/tmp/rdma-core/build/lib \
> >    ninja -C output install
> 
> Once the CFLAGS/LDFLAGS are passed to meson, they should not be needed for
> ninja. The LD_LIBRARY_PATH might be - I'm not sure about that one! :-)

CFLAGS/LDFLAGS are correctly evaluated and inserted in the build.ninja
file, for the LD_LIBRARY_PATH, it is necessary for the run_command stuff
generating the mlx*_autoconf.h

>[...] 
> Rather than having your own separate debug option flag, why not set these
> based on the "buildtype" option e.g. if buildtype is set to "debug".
> 
> > +        # To maintain the compatibility with the make build system
> > +        # mlx4_autoconf.h file is still generated.
> > +        r = run_command('sh', '../../../buildtools/auto-config-h.sh',
> > +                        'mlx4_autoconf.h',
> > +                        'HAVE_IBV_MLX4_WQE_LSO_SEG',
> > +                        'infiniband/mlx4dv.h',
> > +                        'type', 'struct mlx4_wqe_lso_seg')
> > +        if r.returncode() != 0
> > +                error('autoconfiguration fail')
> > +        endif
> 
> Just to check that you are ok with this only being run at configure time?
> If any changes are made to the inputs, ninja won't pick them up. To have it
> tracked for input changes, "custom_target" should be used instead of
> run_command.

It seems to not be possible to have several custom_target on the same
output file has this last is used as the target identifier in ninja.

This limitation is acceptable for now, when meson will be the default
build system, then such autoconf can be removed to use meson built-in
functions.

> > +endif
> > +# Build Glue Library
> > +if pmd_dlopen
> > +        dlopen_name = 'mlx4_glue'
> > +        dlopen_lib_name = driver_name_fmt.format(dlopen_name)
> > +        dlopen_so_version = LIB_GLUE_VERSION
> > +        dlopen_sources = files('mlx4_glue.c')
> > +        dlopen_install_dir = [ eal_pmd_path + '-glue' ]
> > +        shared_lib = shared_library(
> > +               dlopen_lib_name,
> > +               dlopen_sources,
> > +               include_directories: global_inc,
> > +               c_args: cflags,
> > +               link_args: [
> > +                       '-Wl,-export-dynamic',
> > +                       '-Wl,-h,@0@'.format(LIB_GLUE),
> > +                       '-lmlx4',
> > +                       '-libverbs',
> 
> While this works, the recommended approach is to save the return value from
> cc.find_library() above, and pass that as a dependency directly, rather
> than as a linker flag.

I tried it, but:

 drivers/net/mlx5/meson.build:216:8: ERROR:  Link_args arguments must be
 strings.

find_library returns a compiler object, I did not found anyway to use
directly the output of the find_library which works in places.

Thanks,

-- 
Nélio Laranjeiro
6WIND