On Wed, Aug 29, 2018 at 11:34:10AM +0200, Nélio Laranjeiro wrote:

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,

Thanks.

On Tue, Aug 28, 2018 at 04:45:00PM +0100, Bruce Richardson wrote:

quoted

Thanks for this, comments inline below.

/Bruce

On Mon, Aug 27, 2018 at 02:42:25PM +0200, Nelio Laranjeiro wrote:

quoted

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.

quoted

quoted

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

Just realised there is another issue which you should address. The
mlx*_autoconf.h files are being written into a source folder rather than
into the destination folder. This could cause problems if we are enabling
mlx for cross-compile builds. Perhaps inside the auto-config-h.sh script
you can check for $MESON_BUILD_ROOT value, and use that (and possibly
$MESON_SUBDIR) to put the header file in the build directory.

quoted

[...] 
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".

quoted

+        # 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.

quoted

quoted

+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.

It needs to be passed using "dependencies" rather than as a link_arg. [See
list of possible arguments to library and executable objects here:
http://mesonbuild.com/Reference-manual.html#executable]

/Bruce