From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by dpdk.org (Postfix) with ESMTP id B27B14C9C for ; Wed, 29 Aug 2018 12:01:20 +0200 (CEST) X-Amp-Result: UNKNOWN X-Amp-Original-Verdict: FILE UNKNOWN X-Amp-File-Uploaded: False Received: from orsmga008.jf.intel.com ([10.7.209.65]) by fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 29 Aug 2018 03:01:19 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.53,301,1531810800"; d="scan'208";a="68977716" Received: from bricha3-mobl.ger.corp.intel.com ([10.237.221.107]) by orsmga008.jf.intel.com with SMTP; 29 Aug 2018 03:01:17 -0700 Received: by (sSMTP sendmail emulation); Wed, 29 Aug 2018 11:01:16 +0100 Date: Wed, 29 Aug 2018 11:01:15 +0100 From: Bruce Richardson To: =?iso-8859-1?Q?N=E9lio?= Laranjeiro Cc: dev@dpdk.org, Yongseok Koh , Shahaf Shuler , Matan Azrad Message-ID: <20180829100115.GA27208@bricha3-MOBL.ger.corp.intel.com> References: <7812af2267017898332783e934bef9478814ae96.1535361299.git.nelio.laranjeiro@6wind.com> <85a2b398fecdf917dc22810ea0974a91ccca1d13.1535373640.git.nelio.laranjeiro@6wind.com> <20180828154500.GA4208@bricha3-MOBL.ger.corp.intel.com> <20180829093410.3epizbi3pmx5ses2@laranjeiro-vm.dev.6wind.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20180829093410.3epizbi3pmx5ses2@laranjeiro-vm.dev.6wind.com> Organization: Intel Research and Development Ireland Ltd. User-Agent: Mutt/1.10.1 (2018-07-13) Subject: Re: [dpdk-dev] [PATCH v2] net/mlx: add meson build support X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 29 Aug 2018 10:01:21 -0000 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: > > 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 > 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. > >[...] > > 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. > 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