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

["Richardson, Bruce" <bruce.richardson@intel.com>]

> -----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.

> 
> > > > +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.
>