From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by dpdk.org (Postfix) with ESMTP id 1B73927D for ; Thu, 7 Mar 2019 02:04:26 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by fmsmga106.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 06 Mar 2019 17:04:23 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.58,450,1544515200"; d="scan'208";a="305042219" Received: from anandraw-mobl.amr.corp.intel.com (HELO [10.121.163.141]) ([10.121.163.141]) by orsmga005.jf.intel.com with ESMTP; 06 Mar 2019 17:04:22 -0800 To: "Richardson, Bruce" , Thomas Monjalon Cc: "dev@dpdk.org" , "Kadam, Pallavi" , "Menon, Ranjit" , "Shaw, Jeffrey B" References: <20190306041634.12976-1-anand.rawat@intel.com> <37871127.gZq9nUeyVu@xps> <20190306112006.GA134252@bricha3-MOBL.ger.corp.intel.com> <1806536.Q1iGtk73nC@xps> <59AF69C657FD0841A61C55336867B5B072749EA4@IRSMSX103.ger.corp.intel.com> From: Anand Rawat Message-ID: <56df497b-5854-6693-8577-87413b647e5a@intel.com> Date: Wed, 6 Mar 2019 17:04:22 -0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101 Thunderbird/60.5.2 MIME-Version: 1.0 In-Reply-To: <59AF69C657FD0841A61C55336867B5B072749EA4@IRSMSX103.ger.corp.intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v2 1/6] eal: eal stub to add windows 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: Thu, 07 Mar 2019 01:04:27 -0000 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 >> Cc: dev@dpdk.org; Rawat, Anand ; Kadam, Pallavi >> ; Menon, Ranjit ; Shaw, >> Jeffrey B >> 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