[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-build-dev
Subject:    Re: RFR(M): 8057538: Build the freetype library during configure on Windows
From:       Erik Joelsson <erik.joelsson () oracle ! com>
Date:       2014-09-10 8:04:42
Message-ID: 5410061A.8040509 () oracle ! com
[Download RAW message or body]

Corrected the spelling errors and pushed it just now.

/Erik

On 2014-09-10 10:03, Volker Simonis wrote:
> Great. Thanks a lot for your support.
> Volker
>
> On Wed, Sep 10, 2014 at 9:39 AM, Erik Joelsson <erik.joelsson@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@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
>>

[prev in list] [next in list] [prev in thread] [next in thread]