[dpdk-dev] [PATCH v2 1/6] eal: eal stub to add windows support

Thomas Monjalon thomas at monjalon.net
Wed Mar 6 11:03:24 CET 2019


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?




More information about the dev mailing list