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

[Thomas Monjalon <thomas@monjalon.net>]

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.

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