On Mon, Sep 8, 2014 at 10:05 AM, Erik Joelsson <[email protected]> wrote: > Hello Volker, > > help.m4 > build->built >
Fixed. > libraries.m4 > It seems to me like the warning about freetype src only working on windows > will always be printed on other platforms? The conditions need to be moved > to separate if statements. > Yes, you're right. I slightly reworked the logic: - only enter the branch where we check for and build the sources if both --with-freetype-src was given and we're on Windows (i.e "test \( "x$with_freetype_src" != x -a "x$OPENJDK_TARGET_OS" = xwindows \)"). This saves us checks for windows inside that branch. - print the warning about --with-freetype-src being not supported in the else branch if it was used. > platform.m4 > I can see the point adding the code, even if commented out, as documentation > for how it could be solved. I think the comment around msbuild detection is > probably enough though. > I cut the comments down in toolchain.m4 and referenced the more detailed explanation in platfrom.m4 Here's the new version: http://cr.openjdk.java.net/~simonis/webrevs/8057538.v3/ I think I need a sponsor because of generated-configure.sh and the closed part :( Thank you and best regards, Volker > /Erik > > > On 2014-09-05 21:11, Volker Simonis wrote: >> >> On Fri, Sep 5, 2014 at 3:35 PM, Erik Joelsson <[email protected]> >> wrote: >>> >>> On 2014-09-05 14:59, Volker Simonis wrote: >>>> >>>> On Fri, Sep 5, 2014 at 12:31 PM, Erik Joelsson >>>> <[email protected]> >>>> wrote: >>>>> >>>>> Hello Volker, >>>>> >>>>> This is certainly an interesting proposition. >>>>> >>>>> The grep for "SDKs" as test wasn't enough for Visual Studio 2010 as we >>>>> have >>>>> an SDK directory then too, containing "v7.0A". I changed it to the >>>>> below >>>>> to >>>>> make it work for me, but I'm not sure it's a good enough test. Magnus >>>>> will >>>>> likely have an opinion on this. >>>>> >>>>> if $ECHO "$VS_PATH" | grep -q 'Microsoft Visual Studio 10.0'; then >>>>> freetype_platform_toolset="" >>>>> elif $ECHO "$VS_PATH" | grep -q 'SDKs'; then >>>>> freetype_platform_toolset="/p:PlatformToolset=Windows7.1SDK" >>>>> fi >>>>> >>>> I see. This seems to be much trickier than I thought. Unfortunately >>>> 'Microsoft Visual Studio 10.0' is also in the path if we have a >>>> SDK-only installation:) >>>> >>>> So if Magnus doesn't come up with a better idea maybe we have to >>>> remember the toolset in a variable right during the detection process? >>>> >>>> But apart from that - did it work for you? >>>> >>>> Thank you and best regards, >>>> Volker >>>> >>> It's probably better to get the value from our toolchain detecting code. >>> Parsing the path will not cover cases where it's installed in non >>> standard >>> places. Not sure what kind of strings are acceptable for that parameter >>> to >>> msbuild or how you could extract what you have? Also, instead of guessing >>> where msbuild is, I think you will find it if you have VS_PATH as your >>> path. >>> Maybe just add a AC_PATH_PROG(msbuild) (or whichever variant we use) >>> during >>> the toolchain setup? >>> >> That's a good point! I've updated my webrev accordingly: >> >> http://cr.openjdk.java.net/~simonis/webrevs/8057538.v2/ >> >> I've also moved the detection of the platform toolset into the >> compiler detection step. This should be more robust and extensible. >> And it should for now cover at least VS 2010 and SDK 7.1. >> >> I've also updated the help message in help.m4 as suggested by Magnus. >> >> Any other comments? >> >> Thank you and best regards, >> Volker >> >>> Other than that it seemed to work, but it did uncover another bug related >>> to >>> freetype. In Copy-java.desktop.gmk, an explicit $(CP) is used to copy the >>> file instead of the standard macro $(install-file). The build failed for >>> me >>> because the target directory didn't exist yet. >>> >> Strange, but I didn't saw this problem. I could build the 'images' >> step for both 32- and 64-bit with both, Cygwin and MSYS. >> >>> /Erik > >
