RFR(M): 8057538: Build the freetype library during configure on Windows

[Volker Simonis]

Great. Thanks a lot for your support.
Volker

On Wed, Sep 10, 2014 at 9:39 AM, Erik Joelsson <erik.joelsson at oracle.com> wrote:
> Hello Volker,
>
> New version looks good to me, just a couple of spelling errors in comments.
> No need for new review for those. I can push this once Magnus is happy.
>
> * libraries.m4
> 266 configre-> configure
> 275 direcoties-> directories
>
> /Erik
>
>
> On 2014-09-09 19:21, Volker Simonis wrote:
>>
>> On Tue, Sep 9, 2014 at 2:38 PM, Magnus Ihse Bursie
>> <magnus.ihse.bursie at oracle.com> wrote:
>>>
>>> Hi Volker,
>>>
>>> Sorry for not responding to this earlier.
>>>
>> No problem..
>>
>>> As I said in the bug, I really like this idea!
>>>
>>> I have a few comments.
>>>
>>> I understand why you would have liked to add the ac_executable_extensions
>>> stuff, but since you can't due to performance, and that is not likely to
>>> change in the future either, the extra commented-out code in platform.m4
>>> serves no purpose and should be removed.
>>>
>> It was just because it took me so long to find this out because
>> 'ac_executable_extensions' doesn't seem to be documented anywhere. But
>> your're right, it's probably not the right place to document it. So I
>> removed:
>>
>> +  # Setting 'ac_executable_extensions' on Windows may be necessary if
>> there's a directory with the same name like
>> +  # an executable without it's extension (i.e. the .NET framework
>> directory contains the subdirectory 'msbuild' and
>> +  # the executable 'msbuild.exe'. The AC_CHECK_PROG macros won't find
>> 'msbuild' without setting 'ac_executable_extensions'.
>> +  # The drawback of setting 'ac_executable_extensions' is that it
>> will slow down all the program checks because the
>> +  # AC_CHECK_PROG macros will always for all the extensions. Therefor
>> we leave the code below commented out for now
>> +  # and simply check for 'msbuild.exe' in
>> "TOOLCHAIN_DETECT_TOOLCHAIN_EXTRA" in toolchain.m4.
>> +  #
>> +  #case "${build_os}" in
>> +  #  cygwin|msys)
>> +  #    ac_executable_extensions=".exe .com .bat .cmd"
>> +  #  ;;
>> +  #  *)
>> +  #  ;;
>> +  #esac
>> +
>>
>> ..and only repasted it here such that it can be easily googled in the
>> future and forever :)
>>
>>> The description when locating msbuild is enough I think, maybe rewritten
>>> like this:
>>> # We need to check for 'msbuild.exe' because at the place where we expect
>>> to
>>> # find 'msbuild.exe' there's also a directory called 'msbuild' and
>>> configure
>>> # won't find the 'msbuild.exe' executable in that case (and
>>> # 'ac_executable_extensions' is unusable due to performance reasons).
>>>
>> Done.
>>
>>> However, I'm thinking if maybe the check for msbuild.exe should move to
>>> toolchain_windows.m4, maybe at the end of
>>> TOOLCHAIN_SETUP_VISUAL_STUDIO_ENV.
>>> It's a bit extra messy (like you don't normalize the path), and it's part
>>> of
>>> the microsoft build tool chain mess. :-) If so, you can use the fact that
>>> the msbuild.exe you want to use is located in VS_PATH, and use that as
>>> search path to AC_CHECK_PROG. Or maybe that's not useful, I dunno. I'm
>>> not
>>> 100% sure about this though, so if you think it's better were it is,
>>> leave
>>> it.
>>>
>> As you wrote, TOOLCHAIN_SETUP_VISUAL_STUDIO_ENV is already quite big
>> and complex. I'd prefer to leave it where it is.
>>
>> Conceptually you're right that it would belong to toolchain_windows.m4
>> and that's where I wanted to add it first. But then I checked where
>> other Windows tools are detected and saw that we already have this
>> "x$TOOLCHAIN_TYPE" = xmicrosoft; section in the generic toolchain.m4
>> file anyway. So I think it's a good place to leave it there.
>>
>>> Also, I'm not entirely happy on the complexity of the freetype detection
>>> code. It's a beast. :-/ It's not your fault, and I'm not sure what do to
>>> counter that, but at least I think it would help if you could extract the
>>> relevant part of your new compilation code into a separate function so we
>>> at
>>> least try to keep it from growing further (remember that it need to be
>>> AC_DEFUN and not AC_DEFUN_ONCE in that case).
>>>
>> You're right! I extracted most of the build logic into its own
>> function "LIB_BUILD_FREETYPE". I think it's much cleaner and more
>> robust now.
>>
>>> I must compliment you on the well-designed feedback to the user about
>>> possible errors and what to do about them! I really like that.
>>>
>> Thanks.
>>
>>> Apart from this minor issues, your code looks good. (For the records, I
>>> have
>>> read the code but not tested it.)
>>>
>> I've tested on Linux and on Windows both 32- and 64-bit builds with
>> Cygwin and MinGW/MSYS
>>
>> Here's  the new version:
>>
>> http://cr.openjdk.java.net/~simonis/webrevs/8057538.v4/
>>
>> And as I wrote, I need a sponsor because of generated-configure.sh...
>>
>> Thank you and best regards,
>> Volker
>>
>>> /Magnus
>
>