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

[Emil Velikov]

On 8 November 2016 at 15:48, Kyriazis, George <george.kyriazis at intel.com> wrote:
> 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:
>
The "unused" comment was meant for the "import distutils.version"
line. Which seemingly got manged somewhere along the way.

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

"By moving the auto generation to SConscript ..., however it splits
the build logic into two files..." did you mean "one file" here ?
I'm proposing to fold the two SConscripts, which effectively moves the
build logic into _one_ file :-)

Scons was never my thing, so I'm failing to see the "source code
cleanliness" that you're thinking about :-(
The following isn't that messy is it ?

build loader
generate sources
build avx - (uses above sources + avx compile flags)
build avx2 - (uses above sources + avx2 compile flags)


This is now I folded/cleaned up the autoconf build with commit
bb949e262cb5c4fffe991debc605447e15322666. A similar solution here
would be great/possible.

>> > +# 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?
>
Yes, that's correct. Just expand the existing infra to manage .hpp
alongside .h files.

Thanks
Emil