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

[Erik Joelsson]

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