From mboxrd@z Thu Jan  1 00:00:00 1970
From: Thomas Monjalon <thomas@monjalon.net>
Subject: Re: [PATCH v2 1/6] eal: eal stub to add windows support
Date: Wed, 06 Mar 2019 11:03:24 +0100
Message-ID: <37871127.gZq9nUeyVu@xps>
References: <20190306041634.12976-1-anand.rawat@intel.com>
 <20190306041634.12976-2-anand.rawat@intel.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7Bit
Cc: dev@dpdk.org, pallavi.kadam@intel.com, ranjit.menon@intel.com,
 jeffrey.b.shaw@intel.com
To: Anand Rawat <anand.rawat@intel.com>
Return-path: <dev-bounces@dpdk.org>
Received: from out2-smtp.messagingengine.com (out2-smtp.messagingengine.com
 [66.111.4.26]) by dpdk.org (Postfix) with ESMTP id 1E1894CA6
 for <dev@dpdk.org>; Wed,  6 Mar 2019 12:08:23 +0100 (CET)
In-Reply-To: <20190306041634.12976-2-anand.rawat@intel.com>
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

Hi,

06/03/2019 05:16, Anand Rawat:
> -# some libs depend on maths lib
> -add_project_link_arguments('-lm', language: 'c')
> -dpdk_extra_ldflags += '-lm'
> +if cc.find_library('lm', required : false).found()
> +	# some libs depend on maths lib
> +	add_project_link_arguments('-lm', language: 'c')
> +	dpdk_extra_ldflags += '-lm'
> +endif

Either libmath is required or not.
I don't think it can be optional.
Why is it changed?

>  # get binutils version for the workaround of Bug 97
> -ldver = run_command('ld', '-v').stdout().strip()
> -if ldver.contains('2.30')
> -	if cc.has_argument('-mno-avx512f')
> -		march_opt += '-mno-avx512f'
> -		message('Binutils 2.30 detected, disabling AVX512 support as workaround for bug #97')
> +if host_machine.system() != 'windows'
> +	ldver = run_command('ld', '-v').stdout().strip()
> +	if ldver.contains('2.30')
> +		if cc.has_argument('-mno-avx512f')
> +			march_opt += '-mno-avx512f'
> +			message('Binutils 2.30 detected, disabling AVX512 support as workaround for bug #97')
> +		endif
>  	endif
>  endif

Same comment as in v1, you should check which linker is used.
This check is only for GNU ld 2.30.

> +if host_machine.system() != 'windows'
> +	common_sources = files(

The definitive solution should be to compile all common EAL files.
Please explain what are the issues in the common files.
I think we should not remove them and fix them one by one.
You could provide a separate patch to skip some files for
making helloworld working.

> -subdir(join_paths('arch', arch_subdir))
> +
> +common_headers += files('include/rte_common.h')
> +if host_machine.system() != 'windows'
> +	subdir(join_paths('arch', arch_subdir))
> +endif

Same comment as above, it should not be changed.

> -common_headers = files(
> +common_headers += files(
>  	'include/rte_alarm.h',
>  	'include/rte_branch_prediction.h',
>  	'include/rte_bus.h',
>  	'include/rte_bitmap.h',
>  	'include/rte_class.h',
> -	'include/rte_common.h',

Why rte_common.h is moved first in the list?

> -deps += 'kvargs'
> +if host_machine.system() != 'windows'
> +	deps += 'kvargs'
> +endif

Why kvargs is removed?

> --- /dev/null
> +++ b/lib/librte_eal/windows/eal/eal_lcore.c
> + /* Get the cpu core id value */
> +unsigned int
> +eal_cpu_core_id(unsigned int lcore_id)
> +{
> +	return lcore_id;
> +}

For this function and the others in this file,
please add a comment explaining this is a stub, not the expected result.
You can add a TODO or FIXME tag.

> --- a/lib/meson.build
> +++ b/lib/meson.build
> +if host_machine.system() == 'windows'
> +	libraries = ['eal'] # override libraries for windows
> +endif

Instead of simply "override", you could explain that the other libraries
are not yet supported on Windows.

> --- a/meson.build
> +++ b/meson.build
> -# build libs and drivers
> +# build libs
>  subdir('lib')
> -subdir('buildtools')
> -subdir('drivers')
>  
> -# build binaries and installable tools
> -subdir('usertools')
> -subdir('app')
> +if host_machine.system() != 'windows'
> +	# build buildtools and drivers
> +	subdir('buildtools')
> +	subdir('drivers')
>  
> -# build docs
> -subdir('doc')
> +	# build binaries and installable tools
> +	subdir('usertools')
> +	subdir('app')
> +	subdir('test')
> +
> +	# build kernel modules if enabled
> +	if get_option('enable_kmods')
> +		subdir('kernel')
> +	endif
> +
> +	# build docs
> +	subdir('doc')
> +endif

I don't like modifying this file.
Can we skip not supported directories inside the sub meson files?