Great. Thanks a lot for your support. Volker
On Wed, Sep 10, 2014 at 9:39 AM, Erik Joelsson <[email protected]> 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 >> <[email protected]> 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 > >
