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

[Emil Velikov]

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)

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 ?

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

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


> +#ifdef _WIN32
> +   prefix = "";
> +   postfix = ".dll";
> +#else
> +   prefix = "lib";
> +   postfix = ".so";
> +#endif
> +
Quick grep suggests:

UTIL_DL_EXT
UTIL_DL_PREFIX

Regards,
Emil