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:52 AM, Richardson, Bruce wrote:
> 
> 
>> -----Original Message-----
>> From: Thomas Monjalon [mailto:thomas@monjalon.net]
>> Sent: Wednesday, March 6, 2019 11:36 AM
>> To: Richardson, Bruce <bruce.richardson@intel.com>
>> Cc: dev@dpdk.org; Rawat, Anand <anand.rawat@intel.com>; Kadam, Pallavi
>> <pallavi.kadam@intel.com>; Menon, Ranjit <ranjit.menon@intel.com>; Shaw,
>> Jeffrey B <jeffrey.b.shaw@intel.com>
>> Subject: Re: [dpdk-dev] [PATCH v2 1/6] eal: eal stub to add windows
>> support
>>
>> 06/03/2019 12:20, Bruce Richardson:
>>> On Wed, Mar 06, 2019 at 11:03:24AM +0100, Thomas Monjalon wrote:
>>>> 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?
>>>>
>>>
>>> I think these come as part of libc, it's just on Linux they are not in
>>> the main libc library but need to be linked in separately.
>>>
>>> https://en.wikipedia.org/wiki/C_mathematical_functions#libm
>>>
>>> Therefore, this looks the best way of dealing with this.
>>
>> If it's the only solution, at least it deserves a comment.
> 
> Yes. I'd suggest changing the existing comment rather than adding a new one. Update it to point out that on some OS's the maths functions are in a separate library.

Bruce is right, both pthreads and math lib are not required for windows 
as the functionalities associated with them are a part of the standard 
library. Not having the check for pthread cause a warning at link time. 
I will update the comment in v3 as suggested.

> 
>>
>>>>> +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.
>>>>
>>>
>>> I believe that is exactly what this patch is trying to do - it's
>>> skipping the files unneeded to get helloworld working, and the
>>> intention is to fix them one by one and add them back in later.
>>> Perhaps this sort of change should be a separate (precursor) patch
>>> where the cover letter can call this out explicitly?
>>>
>>>>> -deps += 'kvargs'
>>>>> +if host_machine.system() != 'windows'
>>>>> +       deps += 'kvargs'
>>>>> +endif
>>>>
>>>> Why kvargs is removed?
>>>
>>> Again, I believe these actions are to disable the parts of DPDK that
>>> are not needed to enable helloworld, allowing later patches to come in
>>> and fix them.
>>
>> They are workarounds to build helloworld.
>> It is good to have progress in the draft tree, but I see no point in
>> merging this in master.
>> I think we should separate patches which are doing definitive changes from
>> temporary workaround patches disabling some files.
>> It is not an issue to merge some patches for Windows which are not
>> compiling.
>>
> 

Bruce is right, we only compile required header and source files in 
order to avoid compatibility errors on windows. Without these
change helloworld on windows would fail to compile. Adding windows 
specific implementations of the common headers and sources would
bloat up individual patches as well the number of patches. kvargs is 
removed as a dependency to have minimum viable product for helloworld.
If required for lcore mask, it'll added back in v3.

-- 
Anand Rawat