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

[Anand Rawat <anand.rawat@intel.com>]

On 3/6/2019 3:20 AM, Richardson, Bruce wrote:
> On Wed, Mar 06, 2019 at 11:03:24AM +0100, Thomas Monjalon wrote:
> 
>>>   # 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.
>>
> 
> I'm not aware of any better way to implement this in meson right now,
> sadly. Through the compiler object we can get information about linker
> flags, but not about the specific linker itself, since it is only called
> through the compiler.

Since on windows we only support clang, this check cause an error.
'ERROR:  Program or command 'ld' not found or not executable'
It is for this reason we excluded this check from the build flow.
Clang uses lld as linker from the LLVM project.

> 
>>> --- /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?
>>
> Since we now mandate meson 0.47 we can use the "subdir_done()" function to
> this.
> 
> /Bruce
> 

Will be done as a part of v3

-- 
Anand Rawat