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

[Volker Simonis]

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