[Mesa-dev] [PATCH 3/3] swr: Support windows builds

[Kyriazis, George]

Comments inline..

> -----Original Message-----
> From: Emil Velikov [mailto:emil.l.velikov at gmail.com]
> Sent: Tuesday, November 8, 2016 8:25 AM
> To: Kyriazis, George <george.kyriazis at intel.com>
> Cc: ML mesa-dev <mesa-dev at lists.freedesktop.org>
> Subject: Re: [Mesa-dev] [PATCH 3/3] swr: Support windows builds
> 
> On 7 November 2016 at 22:32, George Kyriazis <george.kyriazis at intel.com>
> wrote:
> > - Added SConscript files
> > - better handling of NOMINMAX for <windows.h> inclusion
> > - Reorder header files in swr_context.cpp to handle NOMINMAX better,
> since
> >   mesa header files include windows.h before we get a chance to #define
> >   NOMINMAX
> > - cleaner support for .dll and .so prefix/suffix across OSes
> > - added PUBLIC for some protos
> > - added swr_gdi_swap() which is call from libgl_gdi.c
> > ---
> >  src/gallium/drivers/swr/Makefile.am            |   8 ++
> >  src/gallium/drivers/swr/SConscript             |  46 +++++++
> >  src/gallium/drivers/swr/SConscript-arch        | 175
> +++++++++++++++++++++++++
> >  src/gallium/drivers/swr/rasterizer/common/os.h |   5 +-
> >  src/gallium/drivers/swr/swr_context.cpp        |  16 +--
> >  src/gallium/drivers/swr/swr_context.h          |   2 +
> >  src/gallium/drivers/swr/swr_loader.cpp         |  37 +++++-
> >  src/gallium/drivers/swr/swr_public.h           |  11 +-
> >  src/gallium/drivers/swr/swr_screen.cpp         |  25 +---
> >  9 files changed, 291 insertions(+), 34 deletions(-)  create mode
> > 100644 src/gallium/drivers/swr/SConscript
> >  create mode 100644 src/gallium/drivers/swr/SConscript-arch
> >
> Similar to 1/3 this patch does too many things. Please _don't_  do that.
> 
> Some ideas based on the above:
>  - source code fixes - one or multiple patches, depending on details.
>  - automake fixes - ^^
>  - introduce scons build (+ the EXTRA_DIST hunk)
> 
As stated in review of patch 1/3, I will send v2 of patches with different breakdown.


> Some misc comments below.
> 
> 
> > +++ b/src/gallium/drivers/swr/SConscript
> > @@ -0,0 +1,46 @@
> > +Import('*')
> > +
> > +from sys import executable as python_cmd import distutils.version
> Seems unused. Maybe it was aimed for the llvm 3.9 requirement/check
> mentioned in 1/3 ?
> 
Scons build fails without the Import('*'), because env is undefined:

NameError: name 'env' is not defined:
	
> > +import os.path
> > +
> > +if not 'swr' in COMMAND_LINE_TARGETS:
> > +    Return()
> > +
> > +if not env['llvm']:
> > +    print 'warning: LLVM disabled: not building swr'
> > +    Return()
> > +
> > +env.MSVC2013Compat()
> > +
> 
> > +swr_arch = 'avx'
> > +VariantDir('avx', '.', duplicate=0)
> > +SConscript('avx/SConscript-arch', exports='swr_arch')
> > +
> > +swr_arch = 'avx2'
> > +VariantDir('avx2', '.', duplicate=0)
> > +SConscript('avx2/SConscript-arch', exports='swr_arch')
> > +
> Afaict one can just fold the SConscript-arch here. Thus one won't need to
> bother with the above nor the Depends hunk below.
> Additionally with current approach one is generating [the] identical source
> files twice. Far from ideal...
> 
The AVX and AVX2 builds build differently (with different compiler flags).  At runtime, we load the appropriate dll, based on the underlying architecture.  We do the same thing on the linux build.  Also, since duplicate=0, source is not duplicated.  Yes, generated files are generated twice, however currently SConscript is just a shell around SConscript-arch; all the logic that generates the files and source lists is in SConscript-arch.  By moving the auto generation to SConscript will generate only one copy of the gen files, however it splits the build logic into two files, which is more messy.  I can certainly move the generation code in SConscript, however, I think that it's cleaner to strive for source code cleanliness, as opposed to generate code cleanliness.
		
> > +env = env.Clone()
> > +
> > +source = env.ParseSourceList('Makefile.sources', [
> > +    'LOADER_SOURCES'
> > +])
> > +
> > +env.Prepend(CPPPATH = [
> > +    'rasterizer/scripts'
> > +    ])
> > +
> > +swr = env.ConvenienceLibrary(
> > +       target = 'swr',
> > +       source = source,
> > +       )
> Keep the indentation to 4 spaces here and throughout the SConscripts.
> That's a python requirement.
Ok, will correct that.

> In general I'd encourage using .editorconfig and updating the section for swr,
> if needed.
> 
> 
> > +# remove headers, as scons thinks they are static objects for the .so
> > +source = [x for x in source if not x.endswith(tuple(['.h','.hpp']))]
> > +
> Should be handled already. Otherwise please do so in scons/* Quick grep
> suggests scons/custom.py
> 
ParseSourceList() will filter out .h files, however it won't filter out .hpp files.  Are you saying add the .hpp filter in custom.py?

> 
> > +#ifdef _WIN32
> > +   prefix = "";
> > +   postfix = ".dll";
> > +#else
> > +   prefix = "lib";
> > +   postfix = ".so";
> > +#endif
> > +
> Quick grep suggests:
> 
> UTIL_DL_EXT
> UTIL_DL_PREFIX
> 
Ah.  Thank you!  I'll fix this and include the change in the next rev.

> Regards,
> Emil


Thank you!

George