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

[Volker Simonis]

On Mon, Sep 8, 2014 at 10:05 AM, Erik Joelsson <erik.joelsson at oracle.com> 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 <erik.joelsson at oracle.com>
>> wrote:
>>>
>>> On 2014-09-05 14:59, Volker Simonis wrote:
>>>>
>>>> On Fri, Sep 5, 2014 at 12:31 PM, Erik Joelsson
>>>> <erik.joelsson at oracle.com>
>>>> 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
>
>